[PATCH v2 2/5] drm/rockchip: vop: fix yuv plane support

On Thu, Jul 2, 2015 at 3:53 PM, Mark yao [off-list ref] wrote:

Hi Tomasz
    Thanks for your review, I will fix it soon.

On 2015?07?02? 14:00, Tomasz Figa wrote:

quoted

Hi Mark,

Please see my comments inline.

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao [off-list ref] wrote:

quoted

vop support yuv with NV12, NV16 and NV24, only 2 plane yuv.

Signed-off-by: Mark Yao <redacted>

Changes in v2:
- Uv buffer not support odd offset, align it.
- Fix error display when move yuv image.

---
  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   63
++++++++++++++++++++++++---
  1 file changed, 57 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 3c9f4f3..6ca08f8 100644

--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c

@@ -373,6 +373,18 @@ static enum vop_data_format

vop_convert_format(uint32_t format)
         }
  }

+static bool is_yuv_support(uint32_t format)
+{
+       switch (format) {
+       case DRM_FORMAT_NV12:
+       case DRM_FORMAT_NV16:
+       case DRM_FORMAT_NV24:
+               return true;
+       default:
+               return false;
+       }
+}
+
  static bool is_alpha_support(uint32_t format)
  {
         switch (format) {

@@ -577,16 +589,21 @@ static int vop_update_plane_event(struct drm_plane

*plane,
         struct vop *vop = to_vop(crtc);
         struct drm_gem_object *obj;
         struct rockchip_gem_object *rk_obj;
+       struct drm_gem_object *uv_obj;
+       struct rockchip_gem_object *rk_uv_obj;
         unsigned long offset;
         unsigned int actual_w;
         unsigned int actual_h;
         unsigned int dsp_stx;
         unsigned int dsp_sty;
         unsigned int y_vir_stride;
+       unsigned int uv_vir_stride;
         dma_addr_t yrgb_mst;
+       dma_addr_t uv_mst;
         enum vop_data_format format;
         uint32_t val;
         bool is_alpha;
+       bool is_yuv;
         bool visible;
         int ret;
         struct drm_rect dest = {

@@ -608,6 +625,12 @@ static int vop_update_plane_event(struct drm_plane

*plane,
         };
         bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;

+       if (drm_format_num_planes(fb->pixel_format) > 2) {
+               DRM_ERROR("unsupport more than 2 plane format[%08x]\n",
+                         fb->pixel_format);
+               return -EINVAL;
+       }

Hmm, do you need to check this? Doesn't the core guarantee that with
given pixel_format you always get the right plane count? (Possibly at
fb creation time, but I haven't checked that.)

I just want to point out that update_plane can't handle buffer number > 2
case.

But since all windows can't support 3 buffer count format, this check can
remove.

Even if some windows could support 3 planes, I think this check is
unnecessary, because before calling .update_plane(), the DRM core
actually checks if given format is supported by particular plane, so
inside the callback you can be sure that fb->pixel_format is supported
by given plane. Now, plane count is implied by fourcc, so again it is
impossible to have .update_plane() called with, for example, NV12 and
1 or 3 planes.

quoted

quoted

+       if (is_yuv) {
+               src.x1 &= (~1) << 16;
+               src.y1 &= (~1) << 16;

Hmm, if you align x1 and y1, shouldn't you also offset x2 and y2, so
the width and height of the rectangle are preserved? Also I couldn't
find any details on this, but what are the semantics of
.update_plane(), should it really align the values or maybe just fail?

for yuv format, the buffer start point need align, can't be odd.

OK, I will fix the x2 and y2 offset.

I'd actually wait for someone else to comment on this, because I'm not
sure what's the correct handling of such rounding in DRM. Dave,
Daniel, Rob?

Best regards,
Tomasz