[v2,2/5] drm/rockchip: vop: fix yuv plane support
diff mbox

Message ID 1435313249-4549-3-git-send-email-mark.yao@rock-chips.com
State New
Headers show

Commit Message

yao mark June 26, 2015, 10:07 a.m. UTC
vop support yuv with NV12, NV16 and NV24, only 2 plane yuv.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>

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(-)

Comments

Tomasz Figa July 2, 2015, 6 a.m. UTC | #1
Hi Mark,

Please see my comments inline.

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> vop support yuv with NV12, NV16 and NV24, only 2 plane yuv.
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>
> 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.)

> +
>         ret = drm_plane_helper_check_update(plane, crtc, fb,
>                                             &src, &dest, &clip,
>                                             DRM_PLANE_HELPER_NO_SCALING,
> @@ -624,28 +647,52 @@ static int vop_update_plane_event(struct drm_plane *plane,
>         if (format < 0)
>                 return format;
>
> +       is_yuv = is_yuv_support(fb->pixel_format);

nit: Could you group this together with other is_* assignments, above
the call to vop_convert_format()?

> +
>         obj = rockchip_fb_get_gem_obj(fb, 0);
>         if (!obj) {
>                 DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
>                 return -EINVAL;
>         }
>
> +       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?

> +       }
> +
>         rk_obj = to_rockchip_obj(obj);
>
>         actual_w = (src.x2 - src.x1) >> 16;
>         actual_h = (src.y2 - src.y1) >> 16;
> -       crtc_x = max(0, crtc_x);
> -       crtc_y = max(0, crtc_y);
>
> -       dsp_stx = crtc_x + crtc->mode.htotal - crtc->mode.hsync_start;
> -       dsp_sty = crtc_y + crtc->mode.vtotal - crtc->mode.vsync_start;
> +       dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start;
> +       dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start;

This change could be split into separate patch, which actually fixes
the coordinates used for this calculation, because that's why we do
clipping with drm_plane_helper_check_update() first to use dest not
crtc_{x,y}.

>
> -       offset = (src.x1 >> 16) * (fb->bits_per_pixel >> 3);
> +       offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
>         offset += (src.y1 >> 16) * fb->pitches[0];
> -       yrgb_mst = rk_obj->dma_addr + offset;
>
> +       yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];

This (missing offsets[0] addition) should also be a separate patch,
because it was obviously incorrect before this patch.

Best regards,
Tomasz
yao mark July 2, 2015, 6:53 a.m. UTC | #2
Hi Tomasz
     Thanks for your review, I will fix it soon.
On 2015?07?02? 14:00, Tomasz Figa wrote:
> Hi Mark,
>
> Please see my comments inline.
>
> On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>> vop support yuv with NV12, NV16 and NV24, only 2 plane yuv.
>>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>
>> 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.

>> +
>>          ret = drm_plane_helper_check_update(plane, crtc, fb,
>>                                              &src, &dest, &clip,
>>                                              DRM_PLANE_HELPER_NO_SCALING,
>> @@ -624,28 +647,52 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>          if (format < 0)
>>                  return format;
>>
>> +       is_yuv = is_yuv_support(fb->pixel_format);
> nit: Could you group this together with other is_* assignments, above
> the call to vop_convert_format()?
OK.
>> +
>>          obj = rockchip_fb_get_gem_obj(fb, 0);
>>          if (!obj) {
>>                  DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
>>                  return -EINVAL;
>>          }
>>
>> +       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.

>> +       }
>> +
>>          rk_obj = to_rockchip_obj(obj);
>>
>>          actual_w = (src.x2 - src.x1) >> 16;
>>          actual_h = (src.y2 - src.y1) >> 16;
>> -       crtc_x = max(0, crtc_x);
>> -       crtc_y = max(0, crtc_y);
>>
>> -       dsp_stx = crtc_x + crtc->mode.htotal - crtc->mode.hsync_start;
>> -       dsp_sty = crtc_y + crtc->mode.vtotal - crtc->mode.vsync_start;
>> +       dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start;
>> +       dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
> This change could be split into separate patch, which actually fixes
> the coordinates used for this calculation, because that's why we do
> clipping with drm_plane_helper_check_update() first to use dest not
> crtc_{x,y}.
OK
>> -       offset = (src.x1 >> 16) * (fb->bits_per_pixel >> 3);
>> +       offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
>>          offset += (src.y1 >> 16) * fb->pitches[0];
>> -       yrgb_mst = rk_obj->dma_addr + offset;
>>
>> +       yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
> This (missing offsets[0] addition) should also be a separate patch,
> because it was obviously incorrect before this patch.
OK.
>
> Best regards,
> Tomasz
>
>
>
Tomasz Figa July 2, 2015, 7:07 a.m. UTC | #3
On Thu, Jul 2, 2015 at 3:53 PM, Mark yao <mark.yao@rock-chips.com> wrote:
> Hi Tomasz
>     Thanks for your review, I will fix it soon.
>
> On 2015?07?02? 14:00, Tomasz Figa wrote:
>>
>> Hi Mark,
>>
>> Please see my comments inline.
>>
>> On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>>>
>>> vop support yuv with NV12, NV16 and NV24, only 2 plane yuv.
>>>
>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>>
>>> 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.

>>>
>>> +       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

Patch
diff mbox

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;
+	}
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -624,28 +647,52 @@  static int vop_update_plane_event(struct drm_plane *plane,
 	if (format < 0)
 		return format;
 
+	is_yuv = is_yuv_support(fb->pixel_format);
+
 	obj = rockchip_fb_get_gem_obj(fb, 0);
 	if (!obj) {
 		DRM_ERROR("fail to get rockchip gem object from framebuffer\n");
 		return -EINVAL;
 	}
 
+	if (is_yuv) {
+		src.x1 &= (~1) << 16;
+		src.y1 &= (~1) << 16;
+	}
+
 	rk_obj = to_rockchip_obj(obj);
 
 	actual_w = (src.x2 - src.x1) >> 16;
 	actual_h = (src.y2 - src.y1) >> 16;
-	crtc_x = max(0, crtc_x);
-	crtc_y = max(0, crtc_y);
 
-	dsp_stx = crtc_x + crtc->mode.htotal - crtc->mode.hsync_start;
-	dsp_sty = crtc_y + crtc->mode.vtotal - crtc->mode.vsync_start;
+	dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start;
+	dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start;
 
-	offset = (src.x1 >> 16) * (fb->bits_per_pixel >> 3);
+	offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
 	offset += (src.y1 >> 16) * fb->pitches[0];
-	yrgb_mst = rk_obj->dma_addr + offset;
 
+	yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
 	y_vir_stride = fb->pitches[0] >> 2;
 
+	if (is_yuv) {
+		int hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
+		int vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
+		int bpp = drm_format_plane_cpp(fb->pixel_format, 1);
+
+		uv_obj = rockchip_fb_get_gem_obj(fb, 1);
+		if (!uv_obj) {
+			DRM_ERROR("fail to get uv object from framebuffer\n");
+			return -EINVAL;
+		}
+		rk_uv_obj = to_rockchip_obj(uv_obj);
+		uv_vir_stride = fb->pitches[1] >> 2;
+
+		offset = (src.x1 >> 16) * bpp / hsub;
+		offset += (src.y1 >> 16) * fb->pitches[1] / vsub;
+
+		uv_mst = rk_uv_obj->dma_addr + offset + fb->offsets[1];
+	}
+
 	/*
 	 * If this plane update changes the plane's framebuffer, (or more
 	 * precisely, if this update has a different framebuffer than the last
@@ -681,6 +728,10 @@  static int vop_update_plane_event(struct drm_plane *plane,
 	VOP_WIN_SET(vop, win, format, format);
 	VOP_WIN_SET(vop, win, yrgb_vir, y_vir_stride);
 	VOP_WIN_SET(vop, win, yrgb_mst, yrgb_mst);
+	if (is_yuv) {
+		VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
+		VOP_WIN_SET(vop, win, uv_mst, uv_mst);
+	}
 	val = (actual_h - 1) << 16;
 	val |= (actual_w - 1) & 0xffff;
 	VOP_WIN_SET(vop, win, act_info, val);