[PATCH v2 0/5] drm/rockchip: support yuv overlay and plane scale

[PATCH v2 0/5] drm/rockchip: support yuv overlay and plane scale

[Mark Yao]

This series patches are used for yuv image overlay display.

Rockchip vop support NV11, NV16, NV24 yuv format,
and can scale the image scale 1/8 to 8.

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

Mark Yao (5):
  drm/rockchip: vop: optimize virtual stride calculate
  drm/rockchip: vop: fix yuv plane support
  drm/rockchip: vop: support plane scale
  drm/rockchip: vop: switch cursor plane to window 3
  drm/rockchip: default enable win2/3 area0 bit

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  467 ++++++++++++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   96 ++++++
 2 files changed, 551 insertions(+), 12 deletions(-)

-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/5] drm/rockchip: vop: optimize virtual stride calculate

[Mark Yao]

vir_stride need number words of the virtual width, and fb->pitches
save bytes_per_pixel, so just div 4 switch to stride.

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

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 6188221..3c9f4f3 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -644,7 +644,7 @@ static int vop_update_plane_event(struct drm_plane *plane,
 	offset += (src.y1 >> 16) * fb->pitches[0];
 	yrgb_mst = rk_obj->dma_addr + offset;
 
-	y_vir_stride = fb->pitches[0] / (fb->bits_per_pixel >> 3);
+	y_vir_stride = fb->pitches[0] >> 2;
 
 	/*
 	 * If this plane update changes the plane's framebuffer, (or more
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm/rockchip: vop: optimize virtual stride calculate

[Tomasz Figa]

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> vir_stride need number words of the virtual width, and fb->pitches
> save bytes_per_pixel, so just div 4 switch to stride.
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 6188221..3c9f4f3 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -644,7 +644,7 @@ static int vop_update_plane_event(struct drm_plane *plane,
>         offset += (src.y1 >> 16) * fb->pitches[0];
>         yrgb_mst = rk_obj->dma_addr + offset;
>
> -       y_vir_stride = fb->pitches[0] / (fb->bits_per_pixel >> 3);
> +       y_vir_stride = fb->pitches[0] >> 2;

If the unit of y_vir_stride is words then doesn't it mean that the
calculation before this patch was just invalid? If yes, shouldn't the
subject actually say "Fix virtual stride calculation"?

Best regards,
Tomasz

Re: [PATCH v2 1/5] drm/rockchip: vop: optimize virtual stride calculate

[Mark yao]

On 2015年07月02日 12:59, Tomasz Figa wrote:
> On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>> vir_stride need number words of the virtual width, and fb->pitches
>> save bytes_per_pixel, so just div 4 switch to stride.
>>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 6188221..3c9f4f3 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -644,7 +644,7 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>          offset += (src.y1 >> 16) * fb->pitches[0];
>>          yrgb_mst = rk_obj->dma_addr + offset;
>>
>> -       y_vir_stride = fb->pitches[0] / (fb->bits_per_pixel >> 3);
>> +       y_vir_stride = fb->pitches[0] >> 2;
> If the unit of y_vir_stride is words then doesn't it mean that the
> calculation before this patch was just invalid? If yes, shouldn't the
> subject actually say "Fix virtual stride calculation"?
>
> Best regards,
> Tomasz
>
>
>
Hi Tomasz
     the calculation thinking before is that:
        fb->pitches[0] = surface_width * bytes_per_pixel
        surface_width = fb->pitches[0] / bytes_per_pixel
                               = fb->pitches[0] / (fb->bits_per_pixel >> 3)

    means that "y_vir_stride = surface_width",

ARGB888 one pixel is word size, So that is right for ARGB888, but wrong for other formats.

You are right, subject say "Fix virtual stride calculation" is suitable.

Best regards,
-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mark Yao]

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;
+	}
+
 	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);
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tomasz Figa]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mark yao]

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


-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tomasz Figa]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Mark Yao]

Win_full support 1/8 to 8 scale down/up engine, support
all format scale.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
Changes in v2:
- Fix scale dest info. 

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  389 ++++++++++++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |   96 +++++++
 2 files changed, 483 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 6ca08f8..f6ef634 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -49,6 +49,8 @@
 
 #define VOP_WIN_SET(x, win, name, v) \
 		REG_SET(x, win->base, win->phy->name, v, RELAXED)
+#define VOP_SCL_SET(x, win, name, v) \
+		REG_SET(x, win->base, win->phy->scl->name, v, RELAXED)
 #define VOP_CTRL_SET(x, name, v) \
 		REG_SET(x, 0, (x)->data->ctrl->name, v, NORMAL)
 
@@ -163,7 +165,37 @@ struct vop_ctrl {
 	struct vop_reg vpost_st_end;
 };
 
+struct vop_scl_regs {
+	struct vop_reg cbr_vsd_mode;
+	struct vop_reg cbr_vsu_mode;
+	struct vop_reg cbr_hsd_mode;
+	struct vop_reg cbr_ver_scl_mode;
+	struct vop_reg cbr_hor_scl_mode;
+	struct vop_reg yrgb_vsd_mode;
+	struct vop_reg yrgb_vsu_mode;
+	struct vop_reg yrgb_hsd_mode;
+	struct vop_reg yrgb_ver_scl_mode;
+	struct vop_reg yrgb_hor_scl_mode;
+	struct vop_reg line_load_mode;
+	struct vop_reg cbr_axi_gather_num;
+	struct vop_reg yrgb_axi_gather_num;
+	struct vop_reg vsd_cbr_gt2;
+	struct vop_reg vsd_cbr_gt4;
+	struct vop_reg vsd_yrgb_gt2;
+	struct vop_reg vsd_yrgb_gt4;
+	struct vop_reg bic_coe_sel;
+	struct vop_reg cbr_axi_gather_en;
+	struct vop_reg yrgb_axi_gather_en;
+
+	struct vop_reg lb_mode;
+	struct vop_reg scale_yrgb_x;
+	struct vop_reg scale_yrgb_y;
+	struct vop_reg scale_cbcr_x;
+	struct vop_reg scale_cbcr_y;
+};
+
 struct vop_win_phy {
+	const struct vop_scl_regs *scl;
 	const uint32_t *data_formats;
 	uint32_t nformats;
 
@@ -212,7 +244,36 @@ static const uint32_t formats_234[] = {
 	DRM_FORMAT_RGB565,
 };
 
+static const struct vop_scl_regs win_full_scl = {
+	.cbr_vsd_mode = VOP_REG(WIN0_CTRL1, 0x1, 31),
+	.cbr_vsu_mode = VOP_REG(WIN0_CTRL1, 0x1, 30),
+	.cbr_hsd_mode = VOP_REG(WIN0_CTRL1, 0x3, 28),
+	.cbr_ver_scl_mode = VOP_REG(WIN0_CTRL1, 0x3, 26),
+	.cbr_hor_scl_mode = VOP_REG(WIN0_CTRL1, 0x3, 24),
+	.yrgb_vsd_mode = VOP_REG(WIN0_CTRL1, 0x1, 23),
+	.yrgb_vsu_mode = VOP_REG(WIN0_CTRL1, 0x1, 22),
+	.yrgb_hsd_mode = VOP_REG(WIN0_CTRL1, 0x3, 20),
+	.yrgb_ver_scl_mode = VOP_REG(WIN0_CTRL1, 0x3, 18),
+	.yrgb_hor_scl_mode = VOP_REG(WIN0_CTRL1, 0x3, 16),
+	.line_load_mode = VOP_REG(WIN0_CTRL1, 0x1, 15),
+	.cbr_axi_gather_num = VOP_REG(WIN0_CTRL1, 0x7, 12),
+	.yrgb_axi_gather_num = VOP_REG(WIN0_CTRL1, 0xf, 8),
+	.vsd_cbr_gt2 = VOP_REG(WIN0_CTRL1, 0x1, 7),
+	.vsd_cbr_gt4 = VOP_REG(WIN0_CTRL1, 0x1, 6),
+	.vsd_yrgb_gt2 = VOP_REG(WIN0_CTRL1, 0x1, 5),
+	.vsd_yrgb_gt4 = VOP_REG(WIN0_CTRL1, 0x1, 4),
+	.bic_coe_sel = VOP_REG(WIN0_CTRL1, 0x3, 2),
+	.cbr_axi_gather_en = VOP_REG(WIN0_CTRL1, 0x1, 1),
+	.yrgb_axi_gather_en = VOP_REG(WIN0_CTRL1, 0x1, 0),
+	.lb_mode = VOP_REG(WIN0_CTRL0, 0x7, 5),
+	.scale_yrgb_x = VOP_REG(WIN0_SCL_FACTOR_YRGB, 0xffff, 0x0),
+	.scale_yrgb_y = VOP_REG(WIN0_SCL_FACTOR_YRGB, 0xffff, 16),
+	.scale_cbcr_x = VOP_REG(WIN0_SCL_FACTOR_CBR, 0xffff, 0x0),
+	.scale_cbcr_y = VOP_REG(WIN0_SCL_FACTOR_CBR, 0xffff, 16),
+};
+
 static const struct vop_win_phy win01_data = {
+	.scl = &win_full_scl,
 	.data_formats = formats_01,
 	.nformats = ARRAY_SIZE(formats_01),
 	.enable = VOP_REG(WIN0_CTRL0, 0x1, 0),
@@ -351,6 +412,15 @@ static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t offset,
 	}
 }
 
+static inline int _get_vskiplines(uint32_t srch, uint32_t dsth)
+{
+	if (srch >= (uint32_t)(4 * dsth * MIN_SCL_FT_AFTER_VSKIP))
+		return 4;
+	else if (srch >= (uint32_t)(2 * dsth * MIN_SCL_FT_AFTER_VSKIP))
+		return 2;
+	return 1;
+}
+
 static enum vop_data_format vop_convert_format(uint32_t format)
 {
 	switch (format) {
@@ -538,6 +608,310 @@ static void vop_disable(struct drm_crtc *crtc)
 	pm_runtime_put(vop->dev);
 }
 
+static int _vop_cal_yrgb_lb_mode(int width)
+{
+	int lb_mode = LB_RGB_1920X5;
+
+	if (width > 2560)
+		lb_mode = LB_RGB_3840X2;
+	else if (width > 1920)
+		lb_mode = LB_RGB_2560X4;
+
+	return lb_mode;
+}
+
+static int _vop_cal_cbcr_lb_mode(int width)
+{
+	int lb_mode = LB_YUV_2560X8;
+
+	if (width > 2560)
+		lb_mode = LB_RGB_3840X2;
+	else if (width > 1920)
+		lb_mode = LB_RGB_2560X4;
+	else if (width > 1280)
+		lb_mode = LB_YUV_3840X5;
+
+	return lb_mode;
+}
+
+static void _vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
+			     uint32_t src_w, uint32_t src_h, uint32_t dst_w,
+			     uint32_t dst_h, uint32_t pixel_format)
+{
+	uint16_t yrgb_hor_scl_mode = SCALE_NONE;
+	uint16_t yrgb_ver_scl_mode = SCALE_NONE;
+	uint16_t cbr_hor_scl_mode = SCALE_NONE;
+	uint16_t cbr_ver_scl_mode = SCALE_NONE;
+	uint16_t yrgb_hsd_mode = SCALE_DOWN_BIL;
+	uint16_t cbr_hsd_mode = SCALE_DOWN_BIL;
+	uint16_t yrgb_vsd_mode = SCALE_DOWN_BIL;
+	uint16_t cbr_vsd_mode = SCALE_DOWN_BIL;
+	uint16_t yrgb_vsu_mode = SCALE_UP_BIL;
+	uint16_t cbr_vsu_mode = SCALE_UP_BIL;
+	uint16_t scale_yrgb_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
+	uint16_t scale_yrgb_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
+	uint16_t scale_cbcr_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
+	uint16_t scale_cbcr_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
+	int hsub = drm_format_horz_chroma_subsampling(pixel_format);
+	int vsub = drm_format_vert_chroma_subsampling(pixel_format);
+	bool is_yuv = is_yuv_support(pixel_format);
+	uint16_t vsd_yrgb_gt4 = 0;
+	uint16_t vsd_yrgb_gt2 = 0;
+	uint16_t vsd_cbr_gt4 = 0;
+	uint16_t vsd_cbr_gt2 = 0;
+	uint16_t yrgb_src_w = src_w;
+	uint16_t yrgb_src_h = src_h;
+	uint16_t yrgb_dst_w = dst_w;
+	uint16_t yrgb_dst_h = dst_h;
+	uint16_t cbcr_src_w;
+	uint16_t cbcr_src_h;
+	uint16_t cbcr_dst_w;
+	uint16_t cbcr_dst_h;
+	uint32_t vdmult;
+	uint16_t lb_mode;
+
+	if (((yrgb_dst_w << 3) <= yrgb_src_w) ||
+	    ((yrgb_dst_h << 3) <= yrgb_src_h) ||
+	    yrgb_dst_w > 3840) {
+		DRM_ERROR("yrgb scale exceed 8,src[%dx%d] dst[%dx%d]\n",
+			  yrgb_src_w, yrgb_src_h, yrgb_dst_w, yrgb_dst_h);
+		return;
+	}
+
+	if (yrgb_src_w < yrgb_dst_w)
+		yrgb_hor_scl_mode = SCALE_UP;
+	else if (yrgb_src_w > yrgb_dst_w)
+		yrgb_hor_scl_mode = SCALE_DOWN;
+	else
+		yrgb_hor_scl_mode = SCALE_NONE;
+
+	if (yrgb_src_h < yrgb_dst_h)
+		yrgb_ver_scl_mode = SCALE_UP;
+	else if (yrgb_src_h  > yrgb_dst_h)
+		yrgb_ver_scl_mode = SCALE_DOWN;
+	else
+		yrgb_ver_scl_mode = SCALE_NONE;
+
+	if (is_yuv) {
+		cbcr_src_w = src_w / hsub;
+		cbcr_src_h = src_h / vsub;
+		cbcr_dst_w = dst_w;
+		cbcr_dst_h = dst_h;
+		if ((cbcr_dst_w << 3) <= cbcr_src_w ||
+		    (cbcr_dst_h << 3) <= cbcr_src_h ||
+		    cbcr_src_w > 3840 ||
+		    cbcr_src_w == 0)
+			DRM_ERROR("cbcr scale failed,src[%dx%d] dst[%dx%d]\n",
+				  cbcr_src_w, cbcr_src_h,
+				  cbcr_dst_w, cbcr_dst_h);
+		if (cbcr_src_w < cbcr_dst_w)
+			cbr_hor_scl_mode = SCALE_UP;
+		else if (cbcr_src_w > cbcr_dst_w)
+			cbr_hor_scl_mode = SCALE_DOWN;
+
+		if (cbcr_src_h < cbcr_dst_h)
+			cbr_ver_scl_mode = SCALE_UP;
+		else if (cbcr_src_h > cbcr_dst_h)
+			cbr_ver_scl_mode = SCALE_DOWN;
+	}
+	/*
+	 * line buffer mode
+	 */
+	if (is_yuv) {
+		if (yrgb_hor_scl_mode == SCALE_DOWN && yrgb_dst_w > 2560)
+			lb_mode = LB_RGB_3840X2;
+		else if (cbr_hor_scl_mode == SCALE_DOWN)
+			lb_mode = _vop_cal_cbcr_lb_mode(cbcr_dst_w);
+		else
+			lb_mode = _vop_cal_cbcr_lb_mode(cbcr_src_w);
+	} else {
+		if (yrgb_hor_scl_mode == SCALE_DOWN)
+			lb_mode = _vop_cal_yrgb_lb_mode(yrgb_dst_w);
+		else
+			lb_mode = _vop_cal_yrgb_lb_mode(yrgb_src_w);
+	}
+
+	switch (lb_mode) {
+	case LB_YUV_3840X5:
+	case LB_YUV_2560X8:
+	case LB_RGB_1920X5:
+	case LB_RGB_1280X8:
+		yrgb_vsu_mode = SCALE_UP_BIC;
+		cbr_vsu_mode  = SCALE_UP_BIC;
+		break;
+	case LB_RGB_3840X2:
+		if (yrgb_ver_scl_mode != SCALE_NONE)
+			DRM_ERROR("ERROR : not allow yrgb ver scale\n");
+		if (cbr_ver_scl_mode != SCALE_NONE)
+			DRM_ERROR("ERROR : not allow cbcr ver scale\n");
+		break;
+	case LB_RGB_2560X4:
+		yrgb_vsu_mode = SCALE_UP_BIL;
+		cbr_vsu_mode  = SCALE_UP_BIL;
+		break;
+	default:
+		DRM_ERROR("unsupport lb_mode:%d\n", lb_mode);
+		break;
+	}
+	/*
+	 * (1.1)YRGB HOR SCALE FACTOR
+	 */
+	switch (yrgb_hor_scl_mode) {
+	case SCALE_UP:
+		scale_yrgb_x = GET_SCL_FT_BIC(yrgb_src_w, yrgb_dst_w);
+		break;
+	case SCALE_DOWN:
+		switch (yrgb_hsd_mode) {
+		case SCALE_DOWN_BIL:
+			scale_yrgb_x = GET_SCL_FT_BILI_DN(yrgb_src_w,
+							  yrgb_dst_w);
+			break;
+		case SCALE_DOWN_AVG:
+			scale_yrgb_x = GET_SCL_FT_AVRG(yrgb_src_w, yrgb_dst_w);
+			break;
+		default:
+			DRM_ERROR("unsupport yrgb_hsd_mode:%d\n",
+				  yrgb_hsd_mode);
+			break;
+		}
+		break;
+	}
+
+	/*
+	 * (1.2)YRGB VER SCALE FACTOR
+	 */
+	switch (yrgb_ver_scl_mode) {
+	case SCALE_UP:
+		switch (yrgb_vsu_mode) {
+		case SCALE_UP_BIL:
+			scale_yrgb_y = GET_SCL_FT_BILI_UP(yrgb_src_h,
+							  yrgb_dst_h);
+			break;
+		case SCALE_UP_BIC:
+			if (yrgb_src_h < 3)
+				DRM_ERROR("yrgb_src_h should greater than 3\n");
+			scale_yrgb_y = GET_SCL_FT_BIC(yrgb_src_h, yrgb_dst_h);
+			break;
+		default:
+			DRM_ERROR("unsupport yrgb_vsu_mode:%d\n",
+				  yrgb_vsu_mode);
+			break;
+		}
+		break;
+	case SCALE_DOWN:
+		switch (yrgb_vsd_mode) {
+		case SCALE_DOWN_BIL:
+			vdmult = _get_vskiplines(yrgb_src_h, yrgb_dst_h);
+			scale_yrgb_y = GET_SCL_FT_BILI_DN_VSKIP(yrgb_src_h,
+								yrgb_dst_h,
+								vdmult);
+			if (vdmult == 4) {
+				vsd_yrgb_gt4 = 1;
+				vsd_yrgb_gt2 = 0;
+			} else if (vdmult == 2) {
+				vsd_yrgb_gt4 = 0;
+				vsd_yrgb_gt2 = 1;
+			}
+			break;
+		case SCALE_DOWN_AVG:
+			scale_yrgb_y = GET_SCL_FT_AVRG(yrgb_src_h, yrgb_dst_h);
+			break;
+		default:
+			DRM_ERROR("unsupport yrgb_vsd_mode:%d\n",
+				  yrgb_vsd_mode);
+			break;
+		}
+		break;
+	}
+	/*
+	 * (2.1)CBCR HOR SCALE FACTOR
+	 */
+	switch (cbr_hor_scl_mode) {
+	case SCALE_UP:
+		scale_cbcr_x = GET_SCL_FT_BIC(cbcr_src_w, cbcr_dst_w);
+		break;
+	case SCALE_DOWN:
+		switch (cbr_hsd_mode) {
+		case SCALE_DOWN_BIL:
+			scale_cbcr_x = GET_SCL_FT_BILI_DN(cbcr_src_w,
+							  cbcr_dst_w);
+			break;
+		case SCALE_DOWN_AVG:
+			scale_cbcr_x = GET_SCL_FT_AVRG(cbcr_src_w, cbcr_dst_w);
+			break;
+		default:
+			DRM_ERROR("unsupport cbr_hsd_mode:%d\n", cbr_hsd_mode);
+			break;
+		}
+		break;
+	}
+
+	/*
+	 * (2.2)CBCR VER SCALE FACTOR
+	 */
+	switch (cbr_ver_scl_mode) {
+	case SCALE_UP:
+		switch (cbr_vsu_mode) {
+		case SCALE_UP_BIL:
+			scale_cbcr_y = GET_SCL_FT_BILI_UP(cbcr_src_h,
+							  cbcr_dst_h);
+			break;
+		case SCALE_UP_BIC:
+			if (cbcr_src_h < 3)
+				DRM_ERROR("cbcr_src_h need greater than 3 !\n");
+			scale_cbcr_y = GET_SCL_FT_BIC(cbcr_src_h, cbcr_dst_h);
+			break;
+		default:
+			DRM_ERROR("unsupport cbr_vsu_mode:%d\n", cbr_vsu_mode);
+			break;
+		}
+		break;
+	case SCALE_DOWN:
+		switch (cbr_vsd_mode) {
+		case SCALE_DOWN_BIL:
+			vdmult = _get_vskiplines(cbcr_src_h, cbcr_dst_h);
+			scale_cbcr_y = GET_SCL_FT_BILI_DN_VSKIP(cbcr_src_h,
+								cbcr_dst_h,
+								vdmult);
+			if (vdmult == 4) {
+				vsd_cbr_gt4 = 1;
+				vsd_cbr_gt2 = 0;
+			} else if (vdmult == 2) {
+				vsd_cbr_gt4 = 0;
+				vsd_cbr_gt2 = 1;
+			}
+			break;
+		case SCALE_DOWN_AVG:
+			scale_cbcr_y = GET_SCL_FT_AVRG(cbcr_src_h, cbcr_dst_h);
+			break;
+		default:
+			DRM_ERROR("unsupport cbr_vsd_mode:%d\n", cbr_vsd_mode);
+			break;
+		}
+		break;
+	}
+
+	VOP_SCL_SET(vop, win, yrgb_hor_scl_mode, yrgb_hor_scl_mode);
+	VOP_SCL_SET(vop, win, yrgb_ver_scl_mode, yrgb_ver_scl_mode);
+	VOP_SCL_SET(vop, win, cbr_hor_scl_mode, cbr_hor_scl_mode);
+	VOP_SCL_SET(vop, win, cbr_ver_scl_mode, cbr_ver_scl_mode);
+	VOP_SCL_SET(vop, win, lb_mode, lb_mode);
+	VOP_SCL_SET(vop, win, yrgb_hsd_mode, yrgb_hsd_mode);
+	VOP_SCL_SET(vop, win, cbr_hsd_mode, cbr_hsd_mode);
+	VOP_SCL_SET(vop, win, yrgb_vsd_mode, yrgb_vsd_mode);
+	VOP_SCL_SET(vop, win, cbr_vsd_mode, cbr_vsd_mode);
+	VOP_SCL_SET(vop, win, yrgb_vsu_mode, yrgb_vsu_mode);
+	VOP_SCL_SET(vop, win, cbr_vsu_mode, cbr_vsu_mode);
+	VOP_SCL_SET(vop, win, scale_yrgb_x, scale_yrgb_x);
+	VOP_SCL_SET(vop, win, scale_yrgb_y, scale_yrgb_y);
+	VOP_SCL_SET(vop, win, vsd_yrgb_gt4, vsd_yrgb_gt4);
+	VOP_SCL_SET(vop, win, vsd_yrgb_gt2, vsd_yrgb_gt2);
+	VOP_SCL_SET(vop, win, scale_cbcr_x, scale_cbcr_x);
+	VOP_SCL_SET(vop, win, scale_cbcr_y, scale_cbcr_y);
+	VOP_SCL_SET(vop, win, vsd_cbr_gt4, vsd_cbr_gt4);
+	VOP_SCL_SET(vop, win, vsd_cbr_gt2, vsd_cbr_gt2);
+}
+
 /*
  * Caller must hold vsync_mutex.
  */
@@ -624,6 +998,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
 		.y2 = crtc->mode.vdisplay,
 	};
 	bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
+	int min_scale = win->phy->scl ? 0x02000 : DRM_PLANE_HELPER_NO_SCALING;
+	int max_scale = win->phy->scl ? 0x80000 : DRM_PLANE_HELPER_NO_SCALING;
 
 	if (drm_format_num_planes(fb->pixel_format) > 2) {
 		DRM_ERROR("unsupport more than 2 plane format[%08x]\n",
@@ -633,8 +1009,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
 
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    &src, &dest, &clip,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
+					    min_scale,
+					    max_scale,
 					    can_position, false, &visible);
 	if (ret)
 		return ret;
@@ -732,9 +1108,18 @@ static int vop_update_plane_event(struct drm_plane *plane,
 		VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
 		VOP_WIN_SET(vop, win, uv_mst, uv_mst);
 	}
+
+	if (win->phy->scl)
+		_vop_cal_scl_fac(vop, win, actual_w, actual_h,
+				 dest.x2 - dest.x1, dest.y2 - dest.y1,
+				 fb->pixel_format);
+
 	val = (actual_h - 1) << 16;
 	val |= (actual_w - 1) & 0xffff;
 	VOP_WIN_SET(vop, win, act_info, val);
+
+	val = (dest.y2 - dest.y1 - 1) << 16;
+	val |= (dest.x2 - dest.x1 - 1) & 0xffff;
 	VOP_WIN_SET(vop, win, dsp_info, val);
 	val = (dsp_sty - 1) << 16;
 	val |= (dsp_stx - 1) & 0xffff;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 63e9b3a..edacdee 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -198,4 +198,100 @@ enum factor_mode {
 	ALPHA_SRC_GLOBAL,
 };
 
+enum scale_mode {
+	SCALE_NONE = 0x0,
+	SCALE_UP   = 0x1,
+	SCALE_DOWN = 0x2
+};
+
+enum lb_mode {
+	LB_YUV_3840X5 = 0x0,
+	LB_YUV_2560X8 = 0x1,
+	LB_RGB_3840X2 = 0x2,
+	LB_RGB_2560X4 = 0x3,
+	LB_RGB_1920X5 = 0x4,
+	LB_RGB_1280X8 = 0x5
+};
+
+enum sacle_up_mode {
+	SCALE_UP_BIL = 0x0,
+	SCALE_UP_BIC = 0x1
+};
+
+enum scale_down_mode {
+	SCALE_DOWN_BIL = 0x0,
+	SCALE_DOWN_AVG = 0x1
+};
+
+#define CUBIC_PRECISE  0
+#define CUBIC_SPLINE   1
+#define CUBIC_CATROM   2
+#define CUBIC_MITCHELL 3
+
+#define CUBIC_MODE_SELETION      CUBIC_PRECISE
+
+#define SCL_FT_BILI_DN_FIXPOINT_SHIFT	12
+#define SCL_FT_BILI_DN_FIXPOINT(x) \
+		((INT32)((x)*(1 << SCL_FT_BILI_DN_FIXPOINT_SHIFT)))
+
+#define SCL_FT_BILI_UP_FIXPOINT_SHIFT	16
+
+#define SCL_FT_AVRG_FIXPOINT_SHIFT	16
+#define SCL_FT_AVRG_FIXPOINT(x) \
+		((INT32)((x) * (1 << SCL_FT_AVRG_FIXPOINT_SHIFT)))
+
+#define SCL_FT_BIC_FIXPOINT_SHIFT	16
+#define SCL_FT_BIC_FIXPOINT(x) \
+		((INT32)((x)*(1 << SCL_FT_BIC_FIXPOINT_SHIFT)))
+
+#define SCL_FT_DEFAULT_FIXPOINT_SHIFT    12
+#define SCL_FT_VSDBIL_FIXPOINT_SHIFT     12
+
+#define SCL_CAL(src, dst, shift) \
+		((((src) * 2 - 3) << ((shift) - 1)) / ((dst) - 1))
+#define GET_SCL_FT_BILI_DN(src, dst) \
+		SCL_CAL(src, dst, SCL_FT_BILI_DN_FIXPOINT_SHIFT)
+#define GET_SCL_FT_BILI_UP(src, dst) \
+		SCL_CAL(src, dst, SCL_FT_BILI_UP_FIXPOINT_SHIFT)
+#define GET_SCL_FT_BIC(src, dst) \
+		SCL_CAL(src, dst, SCL_FT_BIC_FIXPOINT_SHIFT)
+
+#define GET_SCL_DN_ACT_HEIGHT(src_h, vdmult) \
+		(((src_h) + (vdmult) - 1) / (vdmult))
+
+#define MIN_SCL_FT_AFTER_VSKIP	1
+#define GET_SCL_FT_BILI_DN_VSKIP(src_h, dst_h, vdmult) \
+		((GET_SCL_DN_ACT_HEIGHT((src_h), (vdmult)) == (dst_h)) \
+			? (GET_SCL_FT_BILI_DN((src_h), (dst_h))/(vdmult)) \
+			: GET_SCL_FT_BILI_DN(GET_SCL_DN_ACT_HEIGHT((src_h), \
+					     (vdmult)), (dst_h)))
+
+#define GET_SCL_FT_AVRG(src, dst) \
+		(((dst) << ((SCL_FT_AVRG_FIXPOINT_SHIFT) + 1)) \
+		 / (2 * (src) - 1))
+
+#define SCL_COOR_ACC_FIXPOINT_SHIFT	16
+#define SCL_COOR_ACC_FIXPOINT_ONE	(1 << SCL_COOR_ACC_FIXPOINT_SHIFT)
+#define SCL_COOR_ACC_FIXPOINT(x) \
+		((INT32)((x)*(1 << SCL_COOR_ACC_FIXPOINT_SHIFT)))
+#define SCL_COOR_ACC_FIXPOINT_REVERT(x)	\
+		((((x) >> (SCL_COOR_ACC_FIXPOINT_SHIFT-1)) + 1) >> 1)
+
+#define SCL_GET_COOR_ACC_FIXPOINT(scale, shift)  \
+		((scale) << (SCL_COOR_ACC_FIXPOINT_SHIFT - (shift)))
+#define SCL_FILTER_FT_FIXPOINT_SHIFT	8
+#define SCL_FILTER_FT_FIXPOINT_ONE	(1 << SCL_FILTER_FT_FIXPOINT_SHIFT)
+#define SCL_FILTER_FT_FIXPOINT(x) \
+		((INT32)((x) * (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)))
+#define SCL_FILTER_FT_FIXPOINT_REVERT(x) \
+		((((x) >> (SCL_FILTER_FT_FIXPOINT_SHIFT - 1)) + 1) >> 1)
+
+#define SCL_GET_FILTER_FT_FIXPOINT(ca, shift) \
+		(((ca) >> ((shift)-SCL_FILTER_FT_FIXPOINT_SHIFT)) & \
+		 (SCL_FILTER_FT_FIXPOINT_ONE - 1))
+
+#define SCL_OFFSET_FIXPOINT_SHIFT	8
+#define SCL_OFFSET_FIXPOINT(x) \
+		((INT32)((x) * (1 << SCL_OFFSET_FIXPOINT_SHIFT)))
+
 #endif /* _ROCKCHIP_DRM_VOP_H */
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Tomasz Figa]

Hi Mark,

Please see my comments inline.

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> Win_full support 1/8 to 8 scale down/up engine, support
> all format scale.

[snip]

> @@ -351,6 +412,15 @@ static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t offset,
>         }
>  }
>
> +static inline int _get_vskiplines(uint32_t srch, uint32_t dsth)
> +{
> +       if (srch >= (uint32_t)(4 * dsth * MIN_SCL_FT_AFTER_VSKIP))
> +               return 4;
> +       else if (srch >= (uint32_t)(2 * dsth * MIN_SCL_FT_AFTER_VSKIP))
> +               return 2;
> +       return 1;

How about rewriting the above to:

#define SCL_MAX_VSKIPLINES 4

uint32_t vskiplines;

for (vskiplines = SCL_MAX_VSKIPLINES; vskiplines > 1; vskiplines /= 2)
        if (srch >= vskiplines * dsth * MIN_SCL_FT_AFTER_VSKIP)
                break;

return vskiplines;

nit: I believe it would be better for readability to move this
function to other scaler related functions below.
nit: I don't see any existing functions with names starting from _, so
to keep existing conventions, how about calling them scl_*, e.g.
scl_get_vskiplines().

> +}
> +
>  static enum vop_data_format vop_convert_format(uint32_t format)
>  {
>         switch (format) {
> @@ -538,6 +608,310 @@ static void vop_disable(struct drm_crtc *crtc)
>         pm_runtime_put(vop->dev);
>  }
>
> +static int _vop_cal_yrgb_lb_mode(int width)
> +{
> +       int lb_mode = LB_RGB_1920X5;
> +
> +       if (width > 2560)
> +               lb_mode = LB_RGB_3840X2;
> +       else if (width > 1920)
> +               lb_mode = LB_RGB_2560X4;

It would be more readable to add

else
        lb_mode = LB_RGB_1920X5;

instead of initializing the variable at declaration time.

> +
> +       return lb_mode;
> +}
> +
> +static int _vop_cal_cbcr_lb_mode(int width)
> +{
> +       int lb_mode = LB_YUV_2560X8;
> +
> +       if (width > 2560)
> +               lb_mode = LB_RGB_3840X2;
> +       else if (width > 1920)
> +               lb_mode = LB_RGB_2560X4;
> +       else if (width > 1280)
> +               lb_mode = LB_YUV_3840X5;
> +
> +       return lb_mode;
> +}
> +
> +static void _vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
> +                            uint32_t src_w, uint32_t src_h, uint32_t dst_w,
> +                            uint32_t dst_h, uint32_t pixel_format)
> +{
> +       uint16_t yrgb_hor_scl_mode = SCALE_NONE;
> +       uint16_t yrgb_ver_scl_mode = SCALE_NONE;
> +       uint16_t cbr_hor_scl_mode = SCALE_NONE;
> +       uint16_t cbr_ver_scl_mode = SCALE_NONE;
> +       uint16_t yrgb_hsd_mode = SCALE_DOWN_BIL;
> +       uint16_t cbr_hsd_mode = SCALE_DOWN_BIL;
> +       uint16_t yrgb_vsd_mode = SCALE_DOWN_BIL;
> +       uint16_t cbr_vsd_mode = SCALE_DOWN_BIL;

No code seems to be assigning the 4 variables above. Is some code
missing or they are simply constants and they (and code checking their
values) are not necessary?

> +       uint16_t yrgb_vsu_mode = SCALE_UP_BIL;
> +       uint16_t cbr_vsu_mode = SCALE_UP_BIL;
> +       uint16_t scale_yrgb_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
> +       uint16_t scale_yrgb_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
> +       uint16_t scale_cbcr_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
> +       uint16_t scale_cbcr_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
> +       int hsub = drm_format_horz_chroma_subsampling(pixel_format);
> +       int vsub = drm_format_vert_chroma_subsampling(pixel_format);
> +       bool is_yuv = is_yuv_support(pixel_format);
> +       uint16_t vsd_yrgb_gt4 = 0;
> +       uint16_t vsd_yrgb_gt2 = 0;
> +       uint16_t vsd_cbr_gt4 = 0;
> +       uint16_t vsd_cbr_gt2 = 0;
> +       uint16_t yrgb_src_w = src_w;
> +       uint16_t yrgb_src_h = src_h;
> +       uint16_t yrgb_dst_w = dst_w;
> +       uint16_t yrgb_dst_h = dst_h;
> +       uint16_t cbcr_src_w;
> +       uint16_t cbcr_src_h;
> +       uint16_t cbcr_dst_w;
> +       uint16_t cbcr_dst_h;
> +       uint32_t vdmult;
> +       uint16_t lb_mode;

The amount of local variables suggests that this function needs to be
split into several smaller ones.

By the way, do you need to initialize all of them? GCC will at least
warn (if not error out) if an unitialized variable is referenced, so
it's enough to make sure that the code correctly covers all branch
paths, which is actually better for the code than to rely on default
value.

> +
> +       if (((yrgb_dst_w << 3) <= yrgb_src_w) ||
> +           ((yrgb_dst_h << 3) <= yrgb_src_h) ||

Hmm, if this is enforcing the maximum downscaling factor, then
wouldn't it be more readable to write like this:

yrgb_src_w >= 8 * yrgb_dst_w

Also is >= correct here? Is the maximum factor less than 8?

> +           yrgb_dst_w > 3840) {

I'd suggest moving this to separate if with different error message.

> +               DRM_ERROR("yrgb scale exceed 8,src[%dx%d] dst[%dx%d]\n",
> +                         yrgb_src_w, yrgb_src_h, yrgb_dst_w, yrgb_dst_h);

Hmm, maybe "Maximum downscaling factor (8) exceeded" and then for the
destination width check "Maximum destination width (3840) exceeded"?

> +               return;
> +       }
> +
> +       if (yrgb_src_w < yrgb_dst_w)
> +               yrgb_hor_scl_mode = SCALE_UP;
> +       else if (yrgb_src_w > yrgb_dst_w)
> +               yrgb_hor_scl_mode = SCALE_DOWN;
> +       else
> +               yrgb_hor_scl_mode = SCALE_NONE;

This looks like a good candidate for a helper function:

enum scl_mode scl_get_scl_mode(uint32_t src, uint32_t dst)
{
        if (src < dst)
                return SCALE_UP;
        else if (src > dst)
                return SCALE_DOWN;
        else
                return SCALE_NONE;
}

> +
> +       if (yrgb_src_h < yrgb_dst_h)
> +               yrgb_ver_scl_mode = SCALE_UP;
> +       else if (yrgb_src_h  > yrgb_dst_h)
> +               yrgb_ver_scl_mode = SCALE_DOWN;
> +       else
> +               yrgb_ver_scl_mode = SCALE_NONE;

And now the helper function could be used here as well.

> +
> +       if (is_yuv) {
> +               cbcr_src_w = src_w / hsub;
> +               cbcr_src_h = src_h / vsub;
> +               cbcr_dst_w = dst_w;
> +               cbcr_dst_h = dst_h;
> +               if ((cbcr_dst_w << 3) <= cbcr_src_w ||
> +                   (cbcr_dst_h << 3) <= cbcr_src_h ||
> +                   cbcr_src_w > 3840 ||
> +                   cbcr_src_w == 0)
> +                       DRM_ERROR("cbcr scale failed,src[%dx%d] dst[%dx%d]\n",
> +                                 cbcr_src_w, cbcr_src_h,
> +                                 cbcr_dst_w, cbcr_dst_h);

I believe you should have those already enforced by Y plane. Also it
doesn't seem reasonable to ever get 0 src width as an argument for
this function.

> +               if (cbcr_src_w < cbcr_dst_w)
> +                       cbr_hor_scl_mode = SCALE_UP;
> +               else if (cbcr_src_w > cbcr_dst_w)
> +                       cbr_hor_scl_mode = SCALE_DOWN;
> +
> +               if (cbcr_src_h < cbcr_dst_h)
> +                       cbr_ver_scl_mode = SCALE_UP;
> +               else if (cbcr_src_h > cbcr_dst_h)
> +                       cbr_ver_scl_mode = SCALE_DOWN;

Aren't the scl_modes for CbCr planes always the same as for Y plane?

> +       }
> +       /*
> +        * line buffer mode
> +        */
> +       if (is_yuv) {
> +               if (yrgb_hor_scl_mode == SCALE_DOWN && yrgb_dst_w > 2560)
> +                       lb_mode = LB_RGB_3840X2;
> +               else if (cbr_hor_scl_mode == SCALE_DOWN)
> +                       lb_mode = _vop_cal_cbcr_lb_mode(cbcr_dst_w);
> +               else
> +                       lb_mode = _vop_cal_cbcr_lb_mode(cbcr_src_w);
> +       } else {
> +               if (yrgb_hor_scl_mode == SCALE_DOWN)
> +                       lb_mode = _vop_cal_yrgb_lb_mode(yrgb_dst_w);
> +               else
> +                       lb_mode = _vop_cal_yrgb_lb_mode(yrgb_src_w);
> +       }

I guess this could be moved into a helper function.

> +
> +       switch (lb_mode) {
> +       case LB_YUV_3840X5:
> +       case LB_YUV_2560X8:
> +       case LB_RGB_1920X5:
> +       case LB_RGB_1280X8:
> +               yrgb_vsu_mode = SCALE_UP_BIC;
> +               cbr_vsu_mode  = SCALE_UP_BIC;

I might be overlooking something, but don't  yrgb_vsu_mode and
cbr_vsu_mode always have the same values?

> +               break;
> +       case LB_RGB_3840X2:
> +               if (yrgb_ver_scl_mode != SCALE_NONE)
> +                       DRM_ERROR("ERROR : not allow yrgb ver scale\n");
> +               if (cbr_ver_scl_mode != SCALE_NONE)
> +                       DRM_ERROR("ERROR : not allow cbcr ver scale\n");

Shouldn't these return error? Also it would be nice to make the error
messages more helpful.

> +               break;
> +       case LB_RGB_2560X4:
> +               yrgb_vsu_mode = SCALE_UP_BIL;
> +               cbr_vsu_mode  = SCALE_UP_BIL;
> +               break;
> +       default:
> +               DRM_ERROR("unsupport lb_mode:%d\n", lb_mode);
> +               break;
> +       }

Anyway, this whole switch is a candidate for a helper function.

> +       /*
> +        * (1.1)YRGB HOR SCALE FACTOR
> +        */
> +       switch (yrgb_hor_scl_mode) {
> +       case SCALE_UP:
> +               scale_yrgb_x = GET_SCL_FT_BIC(yrgb_src_w, yrgb_dst_w);
> +               break;
> +       case SCALE_DOWN:
> +               switch (yrgb_hsd_mode) {
> +               case SCALE_DOWN_BIL:
> +                       scale_yrgb_x = GET_SCL_FT_BILI_DN(yrgb_src_w,
> +                                                         yrgb_dst_w);
> +                       break;
> +               case SCALE_DOWN_AVG:
> +                       scale_yrgb_x = GET_SCL_FT_AVRG(yrgb_src_w, yrgb_dst_w);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport yrgb_hsd_mode:%d\n",
> +                                 yrgb_hsd_mode);

Shouldn't this return an error?

> +                       break;
> +               }
> +               break;
> +       }

Ditto.

> +
> +       /*
> +        * (1.2)YRGB VER SCALE FACTOR
> +        */
> +       switch (yrgb_ver_scl_mode) {
> +       case SCALE_UP:
> +               switch (yrgb_vsu_mode) {
> +               case SCALE_UP_BIL:
> +                       scale_yrgb_y = GET_SCL_FT_BILI_UP(yrgb_src_h,
> +                                                         yrgb_dst_h);
> +                       break;
> +               case SCALE_UP_BIC:
> +                       if (yrgb_src_h < 3)
> +                               DRM_ERROR("yrgb_src_h should greater than 3\n");

Shouldn't this return an error?

> +                       scale_yrgb_y = GET_SCL_FT_BIC(yrgb_src_h, yrgb_dst_h);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport yrgb_vsu_mode:%d\n",
> +                                 yrgb_vsu_mode);

Shouldn't this return an error?

> +                       break;
> +               }
> +               break;
> +       case SCALE_DOWN:
> +               switch (yrgb_vsd_mode) {
> +               case SCALE_DOWN_BIL:
> +                       vdmult = _get_vskiplines(yrgb_src_h, yrgb_dst_h);
> +                       scale_yrgb_y = GET_SCL_FT_BILI_DN_VSKIP(yrgb_src_h,
> +                                                               yrgb_dst_h,
> +                                                               vdmult);
> +                       if (vdmult == 4) {
> +                               vsd_yrgb_gt4 = 1;
> +                               vsd_yrgb_gt2 = 0;
> +                       } else if (vdmult == 2) {
> +                               vsd_yrgb_gt4 = 0;
> +                               vsd_yrgb_gt2 = 1;
> +                       }

How about something like this:

vsd_yrgb_gt4 = (vdmult == 4);
vsd_yrgb_gt2 = (vdmult == 2);

> +                       break;
> +               case SCALE_DOWN_AVG:
> +                       scale_yrgb_y = GET_SCL_FT_AVRG(yrgb_src_h, yrgb_dst_h);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport yrgb_vsd_mode:%d\n",
> +                                 yrgb_vsd_mode);

Shouldn't this return an error?

> +                       break;
> +               }
> +               break;
> +       }

Another candidate for separate helper function.

> +       /*
> +        * (2.1)CBCR HOR SCALE FACTOR
> +        */
> +       switch (cbr_hor_scl_mode) {
> +       case SCALE_UP:
> +               scale_cbcr_x = GET_SCL_FT_BIC(cbcr_src_w, cbcr_dst_w);
> +               break;
> +       case SCALE_DOWN:
> +               switch (cbr_hsd_mode) {
> +               case SCALE_DOWN_BIL:
> +                       scale_cbcr_x = GET_SCL_FT_BILI_DN(cbcr_src_w,
> +                                                         cbcr_dst_w);
> +                       break;
> +               case SCALE_DOWN_AVG:
> +                       scale_cbcr_x = GET_SCL_FT_AVRG(cbcr_src_w, cbcr_dst_w);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport cbr_hsd_mode:%d\n", cbr_hsd_mode);

Error.

> +                       break;
> +               }
> +               break;
> +       }

Isn't this switch exactly the same as for Y plane just with different
widths used? Also, wouldn't the values for CbCr plane be the same as
for Y plane?

> +
> +       /*
> +        * (2.2)CBCR VER SCALE FACTOR
> +        */
> +       switch (cbr_ver_scl_mode) {
> +       case SCALE_UP:
> +               switch (cbr_vsu_mode) {
> +               case SCALE_UP_BIL:
> +                       scale_cbcr_y = GET_SCL_FT_BILI_UP(cbcr_src_h,
> +                                                         cbcr_dst_h);
> +                       break;
> +               case SCALE_UP_BIC:
> +                       if (cbcr_src_h < 3)
> +                               DRM_ERROR("cbcr_src_h need greater than 3 !\n");
> +                       scale_cbcr_y = GET_SCL_FT_BIC(cbcr_src_h, cbcr_dst_h);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport cbr_vsu_mode:%d\n", cbr_vsu_mode);

Error.

> +                       break;
> +               }
> +               break;
> +       case SCALE_DOWN:
> +               switch (cbr_vsd_mode) {
> +               case SCALE_DOWN_BIL:
> +                       vdmult = _get_vskiplines(cbcr_src_h, cbcr_dst_h);
> +                       scale_cbcr_y = GET_SCL_FT_BILI_DN_VSKIP(cbcr_src_h,
> +                                                               cbcr_dst_h,
> +                                                               vdmult);
> +                       if (vdmult == 4) {
> +                               vsd_cbr_gt4 = 1;
> +                               vsd_cbr_gt2 = 0;
> +                       } else if (vdmult == 2) {
> +                               vsd_cbr_gt4 = 0;
> +                               vsd_cbr_gt2 = 1;
> +                       }
> +                       break;
> +               case SCALE_DOWN_AVG:
> +                       scale_cbcr_y = GET_SCL_FT_AVRG(cbcr_src_h, cbcr_dst_h);
> +                       break;
> +               default:
> +                       DRM_ERROR("unsupport cbr_vsd_mode:%d\n", cbr_vsd_mode);
> +                       break;
> +               }
> +               break;
> +       }

Again, this looks like the values for CbCr would be the same as for Y.
Are there actually cases when they differ?

> +
> +       VOP_SCL_SET(vop, win, yrgb_hor_scl_mode, yrgb_hor_scl_mode);
> +       VOP_SCL_SET(vop, win, yrgb_ver_scl_mode, yrgb_ver_scl_mode);
> +       VOP_SCL_SET(vop, win, cbr_hor_scl_mode, cbr_hor_scl_mode);
> +       VOP_SCL_SET(vop, win, cbr_ver_scl_mode, cbr_ver_scl_mode);
> +       VOP_SCL_SET(vop, win, lb_mode, lb_mode);
> +       VOP_SCL_SET(vop, win, yrgb_hsd_mode, yrgb_hsd_mode);
> +       VOP_SCL_SET(vop, win, cbr_hsd_mode, cbr_hsd_mode);
> +       VOP_SCL_SET(vop, win, yrgb_vsd_mode, yrgb_vsd_mode);
> +       VOP_SCL_SET(vop, win, cbr_vsd_mode, cbr_vsd_mode);
> +       VOP_SCL_SET(vop, win, yrgb_vsu_mode, yrgb_vsu_mode);
> +       VOP_SCL_SET(vop, win, cbr_vsu_mode, cbr_vsu_mode);
> +       VOP_SCL_SET(vop, win, scale_yrgb_x, scale_yrgb_x);
> +       VOP_SCL_SET(vop, win, scale_yrgb_y, scale_yrgb_y);
> +       VOP_SCL_SET(vop, win, vsd_yrgb_gt4, vsd_yrgb_gt4);
> +       VOP_SCL_SET(vop, win, vsd_yrgb_gt2, vsd_yrgb_gt2);
> +       VOP_SCL_SET(vop, win, scale_cbcr_x, scale_cbcr_x);
> +       VOP_SCL_SET(vop, win, scale_cbcr_y, scale_cbcr_y);
> +       VOP_SCL_SET(vop, win, vsd_cbr_gt4, vsd_cbr_gt4);
> +       VOP_SCL_SET(vop, win, vsd_cbr_gt2, vsd_cbr_gt2);

If you split this function into smaller ones, you probably don't want
to keep all the writes like here in one place, but rather make the
smaller functions write particular registers after calculating their
values.

> +}
> +
>  /*
>   * Caller must hold vsync_mutex.
>   */
> @@ -624,6 +998,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>                 .y2 = crtc->mode.vdisplay,
>         };
>         bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
> +       int min_scale = win->phy->scl ? 0x02000 : DRM_PLANE_HELPER_NO_SCALING;
> +       int max_scale = win->phy->scl ? 0x80000 : DRM_PLANE_HELPER_NO_SCALING;

Hmm, I wonder if there aren't maybe some helpers to generate fixed
point values in the kernel in a readable way. If not, maybe something
like this would do:

#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))

and then you would just use

FRAC_16_16(1, 8) and FRAC_16_16(8, 1) for min and max scale respectively.

>
>         if (drm_format_num_planes(fb->pixel_format) > 2) {
>                 DRM_ERROR("unsupport more than 2 plane format[%08x]\n",
> @@ -633,8 +1009,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>
>         ret = drm_plane_helper_check_update(plane, crtc, fb,
>                                             &src, &dest, &clip,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> -                                           DRM_PLANE_HELPER_NO_SCALING,
> +                                           min_scale,
> +                                           max_scale,
>                                             can_position, false, &visible);
>         if (ret)
>                 return ret;
> @@ -732,9 +1108,18 @@ static int vop_update_plane_event(struct drm_plane *plane,
>                 VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
>                 VOP_WIN_SET(vop, win, uv_mst, uv_mst);
>         }
> +
> +       if (win->phy->scl)
> +               _vop_cal_scl_fac(vop, win, actual_w, actual_h,
> +                                dest.x2 - dest.x1, dest.y2 - dest.y1,
> +                                fb->pixel_format);

Maybe the function could be named "vop_init_scaler()".

> +
>         val = (actual_h - 1) << 16;
>         val |= (actual_w - 1) & 0xffff;
>         VOP_WIN_SET(vop, win, act_info, val);
> +
> +       val = (dest.y2 - dest.y1 - 1) << 16;
> +       val |= (dest.x2 - dest.x1 - 1) & 0xffff;
>         VOP_WIN_SET(vop, win, dsp_info, val);
>         val = (dsp_sty - 1) << 16;
>         val |= (dsp_stx - 1) & 0xffff;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 63e9b3a..edacdee 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -198,4 +198,100 @@ enum factor_mode {
>         ALPHA_SRC_GLOBAL,
>  };
>
> +enum scale_mode {
> +       SCALE_NONE = 0x0,
> +       SCALE_UP   = 0x1,
> +       SCALE_DOWN = 0x2
> +};
> +
> +enum lb_mode {
> +       LB_YUV_3840X5 = 0x0,
> +       LB_YUV_2560X8 = 0x1,
> +       LB_RGB_3840X2 = 0x2,
> +       LB_RGB_2560X4 = 0x3,
> +       LB_RGB_1920X5 = 0x4,
> +       LB_RGB_1280X8 = 0x5
> +};
> +
> +enum sacle_up_mode {
> +       SCALE_UP_BIL = 0x0,
> +       SCALE_UP_BIC = 0x1
> +};
> +
> +enum scale_down_mode {
> +       SCALE_DOWN_BIL = 0x0,
> +       SCALE_DOWN_AVG = 0x1
> +};
> +
> +#define CUBIC_PRECISE  0
> +#define CUBIC_SPLINE   1
> +#define CUBIC_CATROM   2
> +#define CUBIC_MITCHELL 3
> +
> +#define CUBIC_MODE_SELETION      CUBIC_PRECISE
> +
> +#define SCL_FT_BILI_DN_FIXPOINT_SHIFT  12
> +#define SCL_FT_BILI_DN_FIXPOINT(x) \
> +               ((INT32)((x)*(1 << SCL_FT_BILI_DN_FIXPOINT_SHIFT)))

static inline

> +
> +#define SCL_FT_BILI_UP_FIXPOINT_SHIFT  16
> +
> +#define SCL_FT_AVRG_FIXPOINT_SHIFT     16
> +#define SCL_FT_AVRG_FIXPOINT(x) \
> +               ((INT32)((x) * (1 << SCL_FT_AVRG_FIXPOINT_SHIFT)))

static inline

> +
> +#define SCL_FT_BIC_FIXPOINT_SHIFT      16
> +#define SCL_FT_BIC_FIXPOINT(x) \
> +               ((INT32)((x)*(1 << SCL_FT_BIC_FIXPOINT_SHIFT)))

static inline

> +
> +#define SCL_FT_DEFAULT_FIXPOINT_SHIFT    12
> +#define SCL_FT_VSDBIL_FIXPOINT_SHIFT     12
> +
> +#define SCL_CAL(src, dst, shift) \
> +               ((((src) * 2 - 3) << ((shift) - 1)) / ((dst) - 1))
> +#define GET_SCL_FT_BILI_DN(src, dst) \
> +               SCL_CAL(src, dst, SCL_FT_BILI_DN_FIXPOINT_SHIFT)
> +#define GET_SCL_FT_BILI_UP(src, dst) \
> +               SCL_CAL(src, dst, SCL_FT_BILI_UP_FIXPOINT_SHIFT)
> +#define GET_SCL_FT_BIC(src, dst) \
> +               SCL_CAL(src, dst, SCL_FT_BIC_FIXPOINT_SHIFT)
> +
> +#define GET_SCL_DN_ACT_HEIGHT(src_h, vdmult) \
> +               (((src_h) + (vdmult) - 1) / (vdmult))

All of the function macros above: static inline.

> +
> +#define MIN_SCL_FT_AFTER_VSKIP 1
> +#define GET_SCL_FT_BILI_DN_VSKIP(src_h, dst_h, vdmult) \
> +               ((GET_SCL_DN_ACT_HEIGHT((src_h), (vdmult)) == (dst_h)) \
> +                       ? (GET_SCL_FT_BILI_DN((src_h), (dst_h))/(vdmult)) \
> +                       : GET_SCL_FT_BILI_DN(GET_SCL_DN_ACT_HEIGHT((src_h), \
> +                                            (vdmult)), (dst_h)))

Static inline.

> +
> +#define GET_SCL_FT_AVRG(src, dst) \
> +               (((dst) << ((SCL_FT_AVRG_FIXPOINT_SHIFT) + 1)) \
> +                / (2 * (src) - 1))

Static inline.

> +
> +#define SCL_COOR_ACC_FIXPOINT_SHIFT    16
> +#define SCL_COOR_ACC_FIXPOINT_ONE      (1 << SCL_COOR_ACC_FIXPOINT_SHIFT)
> +#define SCL_COOR_ACC_FIXPOINT(x) \
> +               ((INT32)((x)*(1 << SCL_COOR_ACC_FIXPOINT_SHIFT)))
> +#define SCL_COOR_ACC_FIXPOINT_REVERT(x)        \
> +               ((((x) >> (SCL_COOR_ACC_FIXPOINT_SHIFT-1)) + 1) >> 1)
> +
> +#define SCL_GET_COOR_ACC_FIXPOINT(scale, shift)  \
> +               ((scale) << (SCL_COOR_ACC_FIXPOINT_SHIFT - (shift)))
> +#define SCL_FILTER_FT_FIXPOINT_SHIFT   8
> +#define SCL_FILTER_FT_FIXPOINT_ONE     (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)
> +#define SCL_FILTER_FT_FIXPOINT(x) \
> +               ((INT32)((x) * (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)))
> +#define SCL_FILTER_FT_FIXPOINT_REVERT(x) \
> +               ((((x) >> (SCL_FILTER_FT_FIXPOINT_SHIFT - 1)) + 1) >> 1)
> +
> +#define SCL_GET_FILTER_FT_FIXPOINT(ca, shift) \
> +               (((ca) >> ((shift)-SCL_FILTER_FT_FIXPOINT_SHIFT)) & \
> +                (SCL_FILTER_FT_FIXPOINT_ONE - 1))
> +
> +#define SCL_OFFSET_FIXPOINT_SHIFT      8
> +#define SCL_OFFSET_FIXPOINT(x) \
> +               ((INT32)((x) * (1 << SCL_OFFSET_FIXPOINT_SHIFT)))

All of the function macros above: static inline. This should also let
you remove the casts.

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Mark yao]

On 2015年07月03日 15:46, 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:
>> Win_full support 1/8 to 8 scale down/up engine, support
>> all format scale.
> [snip]
>
>> @@ -351,6 +412,15 @@ static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t offset,
>>          }
>>   }
>>
>> +static inline int _get_vskiplines(uint32_t srch, uint32_t dsth)
>> +{
>> +       if (srch >= (uint32_t)(4 * dsth * MIN_SCL_FT_AFTER_VSKIP))
>> +               return 4;
>> +       else if (srch >= (uint32_t)(2 * dsth * MIN_SCL_FT_AFTER_VSKIP))
>> +               return 2;
>> +       return 1;
> How about rewriting the above to:
>
> #define SCL_MAX_VSKIPLINES 4
>
> uint32_t vskiplines;
>
> for (vskiplines = SCL_MAX_VSKIPLINES; vskiplines > 1; vskiplines /= 2)
>          if (srch >= vskiplines * dsth * MIN_SCL_FT_AFTER_VSKIP)
>                  break;
>
> return vskiplines;
>
> nit: I believe it would be better for readability to move this
> function to other scaler related functions below.
> nit: I don't see any existing functions with names starting from _, so
> to keep existing conventions, how about calling them scl_*, e.g.
> scl_get_vskiplines().
OK
>> +}
>> +
>>   static enum vop_data_format vop_convert_format(uint32_t format)
>>   {
>>          switch (format) {
>> @@ -538,6 +608,310 @@ static void vop_disable(struct drm_crtc *crtc)
>>          pm_runtime_put(vop->dev);
>>   }
>>
>> +static int _vop_cal_yrgb_lb_mode(int width)
>> +{
>> +       int lb_mode = LB_RGB_1920X5;
>> +
>> +       if (width > 2560)
>> +               lb_mode = LB_RGB_3840X2;
>> +       else if (width > 1920)
>> +               lb_mode = LB_RGB_2560X4;
> It would be more readable to add
>
> else
>          lb_mode = LB_RGB_1920X5;
>
> instead of initializing the variable at declaration time.
>
OK
>> +
>> +       return lb_mode;
>> +}
>> +
>> +static int _vop_cal_cbcr_lb_mode(int width)
>> +{
>> +       int lb_mode = LB_YUV_2560X8;
>> +
>> +       if (width > 2560)
>> +               lb_mode = LB_RGB_3840X2;
>> +       else if (width > 1920)
>> +               lb_mode = LB_RGB_2560X4;
>> +       else if (width > 1280)
>> +               lb_mode = LB_YUV_3840X5;
>> +
>> +       return lb_mode;
>> +}
>> +
>> +static void _vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
>> +                            uint32_t src_w, uint32_t src_h, uint32_t dst_w,
>> +                            uint32_t dst_h, uint32_t pixel_format)
>> +{
>> +       uint16_t yrgb_hor_scl_mode = SCALE_NONE;
>> +       uint16_t yrgb_ver_scl_mode = SCALE_NONE;
>> +       uint16_t cbr_hor_scl_mode = SCALE_NONE;
>> +       uint16_t cbr_ver_scl_mode = SCALE_NONE;
>> +       uint16_t yrgb_hsd_mode = SCALE_DOWN_BIL;
>> +       uint16_t cbr_hsd_mode = SCALE_DOWN_BIL;
>> +       uint16_t yrgb_vsd_mode = SCALE_DOWN_BIL;
>> +       uint16_t cbr_vsd_mode = SCALE_DOWN_BIL;
> No code seems to be assigning the 4 variables above. Is some code
> missing or they are simply constants and they (and code checking their
> values) are not necessary?
those value directly write to the vop regs, it's necessary.
>> +       uint16_t yrgb_vsu_mode = SCALE_UP_BIL;
>> +       uint16_t cbr_vsu_mode = SCALE_UP_BIL;
>> +       uint16_t scale_yrgb_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> +       uint16_t scale_yrgb_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> +       uint16_t scale_cbcr_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> +       uint16_t scale_cbcr_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>> +       int hsub = drm_format_horz_chroma_subsampling(pixel_format);
>> +       int vsub = drm_format_vert_chroma_subsampling(pixel_format);
>> +       bool is_yuv = is_yuv_support(pixel_format);
>> +       uint16_t vsd_yrgb_gt4 = 0;
>> +       uint16_t vsd_yrgb_gt2 = 0;
>> +       uint16_t vsd_cbr_gt4 = 0;
>> +       uint16_t vsd_cbr_gt2 = 0;
>> +       uint16_t yrgb_src_w = src_w;
>> +       uint16_t yrgb_src_h = src_h;
>> +       uint16_t yrgb_dst_w = dst_w;
>> +       uint16_t yrgb_dst_h = dst_h;
>> +       uint16_t cbcr_src_w;
>> +       uint16_t cbcr_src_h;
>> +       uint16_t cbcr_dst_w;
>> +       uint16_t cbcr_dst_h;
>> +       uint32_t vdmult;
>> +       uint16_t lb_mode;
> The amount of local variables suggests that this function needs to be
> split into several smaller ones.
>
> By the way, do you need to initialize all of them? GCC will at least
> warn (if not error out) if an unitialized variable is referenced, so
> it's enough to make sure that the code correctly covers all branch
> paths, which is actually better for the code than to rely on default
> value.
Yeah, some value directly write to the vop, some value gcc report the 
warn, so initialize them.
>> +
>> +       if (((yrgb_dst_w << 3) <= yrgb_src_w) ||
>> +           ((yrgb_dst_h << 3) <= yrgb_src_h) ||
> Hmm, if this is enforcing the maximum downscaling factor, then
> wouldn't it be more readable to write like this:
>
> yrgb_src_w >= 8 * yrgb_dst_w
>
> Also is >= correct here? Is the maximum factor less than 8?
yrgb_src_w >= 8 * yrgb_dst_w means we can't scale down exceed 8,
means that (src_w)1920->(dst_w)240 is wrong.

>> +           yrgb_dst_w > 3840) {
> I'd suggest moving this to separate if with different error message.
>
>> +               DRM_ERROR("yrgb scale exceed 8,src[%dx%d] dst[%dx%d]\n",
>> +                         yrgb_src_w, yrgb_src_h, yrgb_dst_w, yrgb_dst_h);
> Hmm, maybe "Maximum downscaling factor (8) exceeded" and then for the
> destination width check "Maximum destination width (3840) exceeded"?
OK.
>> +               return;
>> +       }
>> +
>> +       if (yrgb_src_w < yrgb_dst_w)
>> +               yrgb_hor_scl_mode = SCALE_UP;
>> +       else if (yrgb_src_w > yrgb_dst_w)
>> +               yrgb_hor_scl_mode = SCALE_DOWN;
>> +       else
>> +               yrgb_hor_scl_mode = SCALE_NONE;
> This looks like a good candidate for a helper function:
>
> enum scl_mode scl_get_scl_mode(uint32_t src, uint32_t dst)
> {
>          if (src < dst)
>                  return SCALE_UP;
>          else if (src > dst)
>                  return SCALE_DOWN;
>          else
>                  return SCALE_NONE;
> }
>
OK
>> +
>> +       if (yrgb_src_h < yrgb_dst_h)
>> +               yrgb_ver_scl_mode = SCALE_UP;
>> +       else if (yrgb_src_h  > yrgb_dst_h)
>> +               yrgb_ver_scl_mode = SCALE_DOWN;
>> +       else
>> +               yrgb_ver_scl_mode = SCALE_NONE;
> And now the helper function could be used here as well.
>
>> +
>> +       if (is_yuv) {
>> +               cbcr_src_w = src_w / hsub;
>> +               cbcr_src_h = src_h / vsub;
>> +               cbcr_dst_w = dst_w;
>> +               cbcr_dst_h = dst_h;
>> +               if ((cbcr_dst_w << 3) <= cbcr_src_w ||
>> +                   (cbcr_dst_h << 3) <= cbcr_src_h ||
>> +                   cbcr_src_w > 3840 ||
>> +                   cbcr_src_w == 0)
>> +                       DRM_ERROR("cbcr scale failed,src[%dx%d] dst[%dx%d]\n",
>> +                                 cbcr_src_w, cbcr_src_h,
>> +                                 cbcr_dst_w, cbcr_dst_h);
> I believe you should have those already enforced by Y plane. Also it
> doesn't seem reasonable to ever get 0 src width as an argument for
> this function.
>
>> +               if (cbcr_src_w < cbcr_dst_w)
>> +                       cbr_hor_scl_mode = SCALE_UP;
>> +               else if (cbcr_src_w > cbcr_dst_w)
>> +                       cbr_hor_scl_mode = SCALE_DOWN;
>> +
>> +               if (cbcr_src_h < cbcr_dst_h)
>> +                       cbr_ver_scl_mode = SCALE_UP;
>> +               else if (cbcr_src_h > cbcr_dst_h)
>> +                       cbr_ver_scl_mode = SCALE_DOWN;
> Aren't the scl_modes for CbCr planes always the same as for Y plane?

No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
so Y plane horizontal and vertical is scale down.

but src_w = 1920 / 2 = 960 < 1280
       src_h = 1080 / 2 = 540 < 800.

So Cbcr horizontal and vertical is scale up.
>> +       }
>> +       /*
>> +        * line buffer mode
>> +        */
>> +       if (is_yuv) {
>> +               if (yrgb_hor_scl_mode == SCALE_DOWN && yrgb_dst_w > 2560)
>> +                       lb_mode = LB_RGB_3840X2;
>> +               else if (cbr_hor_scl_mode == SCALE_DOWN)
>> +                       lb_mode = _vop_cal_cbcr_lb_mode(cbcr_dst_w);
>> +               else
>> +                       lb_mode = _vop_cal_cbcr_lb_mode(cbcr_src_w);
>> +       } else {
>> +               if (yrgb_hor_scl_mode == SCALE_DOWN)
>> +                       lb_mode = _vop_cal_yrgb_lb_mode(yrgb_dst_w);
>> +               else
>> +                       lb_mode = _vop_cal_yrgb_lb_mode(yrgb_src_w);
>> +       }
> I guess this could be moved into a helper function.
>
>> +
>> +       switch (lb_mode) {
>> +       case LB_YUV_3840X5:
>> +       case LB_YUV_2560X8:
>> +       case LB_RGB_1920X5:
>> +       case LB_RGB_1280X8:
>> +               yrgb_vsu_mode = SCALE_UP_BIC;
>> +               cbr_vsu_mode  = SCALE_UP_BIC;
> I might be overlooking something, but don't  yrgb_vsu_mode and
> cbr_vsu_mode always have the same values?

Hmm, yrgb_vsu_mode and cbr_vsu_mode mean different vop config, OK, I 
think it can merge
together.

>> +               break;
>> +       case LB_RGB_3840X2:
>> +               if (yrgb_ver_scl_mode != SCALE_NONE)
>> +                       DRM_ERROR("ERROR : not allow yrgb ver scale\n");
>> +               if (cbr_ver_scl_mode != SCALE_NONE)
>> +                       DRM_ERROR("ERROR : not allow cbcr ver scale\n");
> Shouldn't these return error? Also it would be nice to make the error
> messages more helpful.

OK.

>> +               break;
>> +       case LB_RGB_2560X4:
>> +               yrgb_vsu_mode = SCALE_UP_BIL;
>> +               cbr_vsu_mode  = SCALE_UP_BIL;
>> +               break;
>> +       default:
>> +               DRM_ERROR("unsupport lb_mode:%d\n", lb_mode);
>> +               break;
>> +       }
> Anyway, this whole switch is a candidate for a helper function.

This only use one time, it's ok to be a helper function?

>> +       /*
>> +        * (1.1)YRGB HOR SCALE FACTOR
>> +        */
>> +       switch (yrgb_hor_scl_mode) {
>> +       case SCALE_UP:
>> +               scale_yrgb_x = GET_SCL_FT_BIC(yrgb_src_w, yrgb_dst_w);
>> +               break;
>> +       case SCALE_DOWN:
>> +               switch (yrgb_hsd_mode) {
>> +               case SCALE_DOWN_BIL:
>> +                       scale_yrgb_x = GET_SCL_FT_BILI_DN(yrgb_src_w,
>> +                                                         yrgb_dst_w);
>> +                       break;
>> +               case SCALE_DOWN_AVG:
>> +                       scale_yrgb_x = GET_SCL_FT_AVRG(yrgb_src_w, yrgb_dst_w);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport yrgb_hsd_mode:%d\n",
>> +                                 yrgb_hsd_mode);
> Shouldn't this return an error?
>
>> +                       break;
>> +               }
>> +               break;
>> +       }
> Ditto.
>
>> +
>> +       /*
>> +        * (1.2)YRGB VER SCALE FACTOR
>> +        */
>> +       switch (yrgb_ver_scl_mode) {
>> +       case SCALE_UP:
>> +               switch (yrgb_vsu_mode) {
>> +               case SCALE_UP_BIL:
>> +                       scale_yrgb_y = GET_SCL_FT_BILI_UP(yrgb_src_h,
>> +                                                         yrgb_dst_h);
>> +                       break;
>> +               case SCALE_UP_BIC:
>> +                       if (yrgb_src_h < 3)
>> +                               DRM_ERROR("yrgb_src_h should greater than 3\n");
> Shouldn't this return an error?
>
>> +                       scale_yrgb_y = GET_SCL_FT_BIC(yrgb_src_h, yrgb_dst_h);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport yrgb_vsu_mode:%d\n",
>> +                                 yrgb_vsu_mode);
> Shouldn't this return an error?
>
>> +                       break;
>> +               }
>> +               break;
>> +       case SCALE_DOWN:
>> +               switch (yrgb_vsd_mode) {
>> +               case SCALE_DOWN_BIL:
>> +                       vdmult = _get_vskiplines(yrgb_src_h, yrgb_dst_h);
>> +                       scale_yrgb_y = GET_SCL_FT_BILI_DN_VSKIP(yrgb_src_h,
>> +                                                               yrgb_dst_h,
>> +                                                               vdmult);
>> +                       if (vdmult == 4) {
>> +                               vsd_yrgb_gt4 = 1;
>> +                               vsd_yrgb_gt2 = 0;
>> +                       } else if (vdmult == 2) {
>> +                               vsd_yrgb_gt4 = 0;
>> +                               vsd_yrgb_gt2 = 1;
>> +                       }
> How about something like this:
>
> vsd_yrgb_gt4 = (vdmult == 4);
> vsd_yrgb_gt2 = (vdmult == 2);

But I think it's not easy to read.

>> +                       break;
>> +               case SCALE_DOWN_AVG:
>> +                       scale_yrgb_y = GET_SCL_FT_AVRG(yrgb_src_h, yrgb_dst_h);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport yrgb_vsd_mode:%d\n",
>> +                                 yrgb_vsd_mode);
> Shouldn't this return an error?
>
>> +                       break;
>> +               }
>> +               break;
>> +       }
> Another candidate for separate helper function.
>
>> +       /*
>> +        * (2.1)CBCR HOR SCALE FACTOR
>> +        */
>> +       switch (cbr_hor_scl_mode) {
>> +       case SCALE_UP:
>> +               scale_cbcr_x = GET_SCL_FT_BIC(cbcr_src_w, cbcr_dst_w);
>> +               break;
>> +       case SCALE_DOWN:
>> +               switch (cbr_hsd_mode) {
>> +               case SCALE_DOWN_BIL:
>> +                       scale_cbcr_x = GET_SCL_FT_BILI_DN(cbcr_src_w,
>> +                                                         cbcr_dst_w);
>> +                       break;
>> +               case SCALE_DOWN_AVG:
>> +                       scale_cbcr_x = GET_SCL_FT_AVRG(cbcr_src_w, cbcr_dst_w);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport cbr_hsd_mode:%d\n", cbr_hsd_mode);
> Error.
>
>> +                       break;
>> +               }
>> +               break;
>> +       }
> Isn't this switch exactly the same as for Y plane just with different
> widths used? Also, wouldn't the values for CbCr plane be the same as
> for Y plane?

But Cbcr case is not same as Y plane case,  how to merge?

>> +
>> +       /*
>> +        * (2.2)CBCR VER SCALE FACTOR
>> +        */
>> +       switch (cbr_ver_scl_mode) {
>> +       case SCALE_UP:
>> +               switch (cbr_vsu_mode) {
>> +               case SCALE_UP_BIL:
>> +                       scale_cbcr_y = GET_SCL_FT_BILI_UP(cbcr_src_h,
>> +                                                         cbcr_dst_h);
>> +                       break;
>> +               case SCALE_UP_BIC:
>> +                       if (cbcr_src_h < 3)
>> +                               DRM_ERROR("cbcr_src_h need greater than 3 !\n");
>> +                       scale_cbcr_y = GET_SCL_FT_BIC(cbcr_src_h, cbcr_dst_h);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport cbr_vsu_mode:%d\n", cbr_vsu_mode);
> Error.
>
>> +                       break;
>> +               }
>> +               break;
>> +       case SCALE_DOWN:
>> +               switch (cbr_vsd_mode) {
>> +               case SCALE_DOWN_BIL:
>> +                       vdmult = _get_vskiplines(cbcr_src_h, cbcr_dst_h);
>> +                       scale_cbcr_y = GET_SCL_FT_BILI_DN_VSKIP(cbcr_src_h,
>> +                                                               cbcr_dst_h,
>> +                                                               vdmult);
>> +                       if (vdmult == 4) {
>> +                               vsd_cbr_gt4 = 1;
>> +                               vsd_cbr_gt2 = 0;
>> +                       } else if (vdmult == 2) {
>> +                               vsd_cbr_gt4 = 0;
>> +                               vsd_cbr_gt2 = 1;
>> +                       }
>> +                       break;
>> +               case SCALE_DOWN_AVG:
>> +                       scale_cbcr_y = GET_SCL_FT_AVRG(cbcr_src_h, cbcr_dst_h);
>> +                       break;
>> +               default:
>> +                       DRM_ERROR("unsupport cbr_vsd_mode:%d\n", cbr_vsd_mode);
>> +                       break;
>> +               }
>> +               break;
>> +       }
> Again, this looks like the values for CbCr would be the same as for Y.
> Are there actually cases when they differ?

Hmm, Actually the cbcr calculations have some different with Y.

>> +
>> +       VOP_SCL_SET(vop, win, yrgb_hor_scl_mode, yrgb_hor_scl_mode);
>> +       VOP_SCL_SET(vop, win, yrgb_ver_scl_mode, yrgb_ver_scl_mode);
>> +       VOP_SCL_SET(vop, win, cbr_hor_scl_mode, cbr_hor_scl_mode);
>> +       VOP_SCL_SET(vop, win, cbr_ver_scl_mode, cbr_ver_scl_mode);
>> +       VOP_SCL_SET(vop, win, lb_mode, lb_mode);
>> +       VOP_SCL_SET(vop, win, yrgb_hsd_mode, yrgb_hsd_mode);
>> +       VOP_SCL_SET(vop, win, cbr_hsd_mode, cbr_hsd_mode);
>> +       VOP_SCL_SET(vop, win, yrgb_vsd_mode, yrgb_vsd_mode);
>> +       VOP_SCL_SET(vop, win, cbr_vsd_mode, cbr_vsd_mode);
>> +       VOP_SCL_SET(vop, win, yrgb_vsu_mode, yrgb_vsu_mode);
>> +       VOP_SCL_SET(vop, win, cbr_vsu_mode, cbr_vsu_mode);
>> +       VOP_SCL_SET(vop, win, scale_yrgb_x, scale_yrgb_x);
>> +       VOP_SCL_SET(vop, win, scale_yrgb_y, scale_yrgb_y);
>> +       VOP_SCL_SET(vop, win, vsd_yrgb_gt4, vsd_yrgb_gt4);
>> +       VOP_SCL_SET(vop, win, vsd_yrgb_gt2, vsd_yrgb_gt2);
>> +       VOP_SCL_SET(vop, win, scale_cbcr_x, scale_cbcr_x);
>> +       VOP_SCL_SET(vop, win, scale_cbcr_y, scale_cbcr_y);
>> +       VOP_SCL_SET(vop, win, vsd_cbr_gt4, vsd_cbr_gt4);
>> +       VOP_SCL_SET(vop, win, vsd_cbr_gt2, vsd_cbr_gt2);
> If you split this function into smaller ones, you probably don't want
> to keep all the writes like here in one place, but rather make the
> smaller functions write particular registers after calculating their
> values.

Hmm, I will try to do that.

>> +}
>> +
>>   /*
>>    * Caller must hold vsync_mutex.
>>    */
>> @@ -624,6 +998,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>                  .y2 = crtc->mode.vdisplay,
>>          };
>>          bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY;
>> +       int min_scale = win->phy->scl ? 0x02000 : DRM_PLANE_HELPER_NO_SCALING;
>> +       int max_scale = win->phy->scl ? 0x80000 : DRM_PLANE_HELPER_NO_SCALING;
> Hmm, I wonder if there aren't maybe some helpers to generate fixed
> point values in the kernel in a readable way. If not, maybe something
> like this would do:
>
> #define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
>
> and then you would just use
>
> FRAC_16_16(1, 8) and FRAC_16_16(8, 1) for min and max scale respectively.
OK.

>>          if (drm_format_num_planes(fb->pixel_format) > 2) {
>>                  DRM_ERROR("unsupport more than 2 plane format[%08x]\n",
>> @@ -633,8 +1009,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>
>>          ret = drm_plane_helper_check_update(plane, crtc, fb,
>>                                              &src, &dest, &clip,
>> -                                           DRM_PLANE_HELPER_NO_SCALING,
>> -                                           DRM_PLANE_HELPER_NO_SCALING,
>> +                                           min_scale,
>> +                                           max_scale,
>>                                              can_position, false, &visible);
>>          if (ret)
>>                  return ret;
>> @@ -732,9 +1108,18 @@ static int vop_update_plane_event(struct drm_plane *plane,
>>                  VOP_WIN_SET(vop, win, uv_vir, uv_vir_stride);
>>                  VOP_WIN_SET(vop, win, uv_mst, uv_mst);
>>          }
>> +
>> +       if (win->phy->scl)
>> +               _vop_cal_scl_fac(vop, win, actual_w, actual_h,
>> +                                dest.x2 - dest.x1, dest.y2 - dest.y1,
>> +                                fb->pixel_format);
> Maybe the function could be named "vop_init_scaler()".
>
OK
>> +
>>          val = (actual_h - 1) << 16;
>>          val |= (actual_w - 1) & 0xffff;
>>          VOP_WIN_SET(vop, win, act_info, val);
>> +
>> +       val = (dest.y2 - dest.y1 - 1) << 16;
>> +       val |= (dest.x2 - dest.x1 - 1) & 0xffff;
>>          VOP_WIN_SET(vop, win, dsp_info, val);
>>          val = (dsp_sty - 1) << 16;
>>          val |= (dsp_stx - 1) & 0xffff;
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 63e9b3a..edacdee 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -198,4 +198,100 @@ enum factor_mode {
>>          ALPHA_SRC_GLOBAL,
>>   };
>>
>> +enum scale_mode {
>> +       SCALE_NONE = 0x0,
>> +       SCALE_UP   = 0x1,
>> +       SCALE_DOWN = 0x2
>> +};
>> +
>> +enum lb_mode {
>> +       LB_YUV_3840X5 = 0x0,
>> +       LB_YUV_2560X8 = 0x1,
>> +       LB_RGB_3840X2 = 0x2,
>> +       LB_RGB_2560X4 = 0x3,
>> +       LB_RGB_1920X5 = 0x4,
>> +       LB_RGB_1280X8 = 0x5
>> +};
>> +
>> +enum sacle_up_mode {
>> +       SCALE_UP_BIL = 0x0,
>> +       SCALE_UP_BIC = 0x1
>> +};
>> +
>> +enum scale_down_mode {
>> +       SCALE_DOWN_BIL = 0x0,
>> +       SCALE_DOWN_AVG = 0x1
>> +};
>> +
>> +#define CUBIC_PRECISE  0
>> +#define CUBIC_SPLINE   1
>> +#define CUBIC_CATROM   2
>> +#define CUBIC_MITCHELL 3
>> +
>> +#define CUBIC_MODE_SELETION      CUBIC_PRECISE
>> +
>> +#define SCL_FT_BILI_DN_FIXPOINT_SHIFT  12
>> +#define SCL_FT_BILI_DN_FIXPOINT(x) \
>> +               ((INT32)((x)*(1 << SCL_FT_BILI_DN_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_BILI_UP_FIXPOINT_SHIFT  16
>> +
>> +#define SCL_FT_AVRG_FIXPOINT_SHIFT     16
>> +#define SCL_FT_AVRG_FIXPOINT(x) \
>> +               ((INT32)((x) * (1 << SCL_FT_AVRG_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_BIC_FIXPOINT_SHIFT      16
>> +#define SCL_FT_BIC_FIXPOINT(x) \
>> +               ((INT32)((x)*(1 << SCL_FT_BIC_FIXPOINT_SHIFT)))
> static inline
>
>> +
>> +#define SCL_FT_DEFAULT_FIXPOINT_SHIFT    12
>> +#define SCL_FT_VSDBIL_FIXPOINT_SHIFT     12
>> +
>> +#define SCL_CAL(src, dst, shift) \
>> +               ((((src) * 2 - 3) << ((shift) - 1)) / ((dst) - 1))
>> +#define GET_SCL_FT_BILI_DN(src, dst) \
>> +               SCL_CAL(src, dst, SCL_FT_BILI_DN_FIXPOINT_SHIFT)
>> +#define GET_SCL_FT_BILI_UP(src, dst) \
>> +               SCL_CAL(src, dst, SCL_FT_BILI_UP_FIXPOINT_SHIFT)
>> +#define GET_SCL_FT_BIC(src, dst) \
>> +               SCL_CAL(src, dst, SCL_FT_BIC_FIXPOINT_SHIFT)
>> +
>> +#define GET_SCL_DN_ACT_HEIGHT(src_h, vdmult) \
>> +               (((src_h) + (vdmult) - 1) / (vdmult))
> All of the function macros above: static inline.
>
>> +
>> +#define MIN_SCL_FT_AFTER_VSKIP 1
>> +#define GET_SCL_FT_BILI_DN_VSKIP(src_h, dst_h, vdmult) \
>> +               ((GET_SCL_DN_ACT_HEIGHT((src_h), (vdmult)) == (dst_h)) \
>> +                       ? (GET_SCL_FT_BILI_DN((src_h), (dst_h))/(vdmult)) \
>> +                       : GET_SCL_FT_BILI_DN(GET_SCL_DN_ACT_HEIGHT((src_h), \
>> +                                            (vdmult)), (dst_h)))
> Static inline.
>
>> +
>> +#define GET_SCL_FT_AVRG(src, dst) \
>> +               (((dst) << ((SCL_FT_AVRG_FIXPOINT_SHIFT) + 1)) \
>> +                / (2 * (src) - 1))
> Static inline.
>
>> +
>> +#define SCL_COOR_ACC_FIXPOINT_SHIFT    16
>> +#define SCL_COOR_ACC_FIXPOINT_ONE      (1 << SCL_COOR_ACC_FIXPOINT_SHIFT)
>> +#define SCL_COOR_ACC_FIXPOINT(x) \
>> +               ((INT32)((x)*(1 << SCL_COOR_ACC_FIXPOINT_SHIFT)))
>> +#define SCL_COOR_ACC_FIXPOINT_REVERT(x)        \
>> +               ((((x) >> (SCL_COOR_ACC_FIXPOINT_SHIFT-1)) + 1) >> 1)
>> +
>> +#define SCL_GET_COOR_ACC_FIXPOINT(scale, shift)  \
>> +               ((scale) << (SCL_COOR_ACC_FIXPOINT_SHIFT - (shift)))
>> +#define SCL_FILTER_FT_FIXPOINT_SHIFT   8
>> +#define SCL_FILTER_FT_FIXPOINT_ONE     (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)
>> +#define SCL_FILTER_FT_FIXPOINT(x) \
>> +               ((INT32)((x) * (1 << SCL_FILTER_FT_FIXPOINT_SHIFT)))
>> +#define SCL_FILTER_FT_FIXPOINT_REVERT(x) \
>> +               ((((x) >> (SCL_FILTER_FT_FIXPOINT_SHIFT - 1)) + 1) >> 1)
>> +
>> +#define SCL_GET_FILTER_FT_FIXPOINT(ca, shift) \
>> +               (((ca) >> ((shift)-SCL_FILTER_FT_FIXPOINT_SHIFT)) & \
>> +                (SCL_FILTER_FT_FIXPOINT_ONE - 1))
>> +
>> +#define SCL_OFFSET_FIXPOINT_SHIFT      8
>> +#define SCL_OFFSET_FIXPOINT(x) \
>> +               ((INT32)((x) * (1 << SCL_OFFSET_FIXPOINT_SHIFT)))
> All of the function macros above: static inline. This should also let
> you remove the casts.
what the "casts" mean? Why do you thinking that "static inline" is 
better than "#define"?

> Best regards,
> Tomasz
>
>
>


-- 
Ｍark


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Tomasz Figa]

On Fri, Jul 3, 2015 at 6:17 PM, Mark yao <mark.yao-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>> +static void _vop_cal_scl_fac(struct vop *vop, const struct vop_win_data
>>> *win,
>>> +                            uint32_t src_w, uint32_t src_h, uint32_t
>>> dst_w,
>>> +                            uint32_t dst_h, uint32_t pixel_format)
>>> +{
>>> +       uint16_t yrgb_hor_scl_mode = SCALE_NONE;
>>> +       uint16_t yrgb_ver_scl_mode = SCALE_NONE;
>>> +       uint16_t cbr_hor_scl_mode = SCALE_NONE;
>>> +       uint16_t cbr_ver_scl_mode = SCALE_NONE;
>>> +       uint16_t yrgb_hsd_mode = SCALE_DOWN_BIL;
>>> +       uint16_t cbr_hsd_mode = SCALE_DOWN_BIL;
>>> +       uint16_t yrgb_vsd_mode = SCALE_DOWN_BIL;
>>> +       uint16_t cbr_vsd_mode = SCALE_DOWN_BIL;
>>
>> No code seems to be assigning the 4 variables above. Is some code
>> missing or they are simply constants and they (and code checking their
>> values) are not necessary?
>
> those value directly write to the vop regs, it's necessary.
>

Couldn't the constants (SCALE_DOWN_BIL) be written directly without
introducing unnecessary variables?

>>> +       uint16_t yrgb_vsu_mode = SCALE_UP_BIL;
>>> +       uint16_t cbr_vsu_mode = SCALE_UP_BIL;
>>> +       uint16_t scale_yrgb_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>>> +       uint16_t scale_yrgb_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>>> +       uint16_t scale_cbcr_x = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>>> +       uint16_t scale_cbcr_y = 1 << SCL_FT_DEFAULT_FIXPOINT_SHIFT;
>>> +       int hsub = drm_format_horz_chroma_subsampling(pixel_format);
>>> +       int vsub = drm_format_vert_chroma_subsampling(pixel_format);
>>> +       bool is_yuv = is_yuv_support(pixel_format);
>>> +       uint16_t vsd_yrgb_gt4 = 0;
>>> +       uint16_t vsd_yrgb_gt2 = 0;
>>> +       uint16_t vsd_cbr_gt4 = 0;
>>> +       uint16_t vsd_cbr_gt2 = 0;
>>> +       uint16_t yrgb_src_w = src_w;
>>> +       uint16_t yrgb_src_h = src_h;
>>> +       uint16_t yrgb_dst_w = dst_w;
>>> +       uint16_t yrgb_dst_h = dst_h;
>>> +       uint16_t cbcr_src_w;
>>> +       uint16_t cbcr_src_h;
>>> +       uint16_t cbcr_dst_w;
>>> +       uint16_t cbcr_dst_h;
>>> +       uint32_t vdmult;
>>> +       uint16_t lb_mode;
>>
>> The amount of local variables suggests that this function needs to be
>> split into several smaller ones.
>>
>> By the way, do you need to initialize all of them? GCC will at least
>> warn (if not error out) if an unitialized variable is referenced, so
>> it's enough to make sure that the code correctly covers all branch
>> paths, which is actually better for the code than to rely on default
>> value.
>
> Yeah, some value directly write to the vop, some value gcc report the warn,
> so initialize them.

IMHO in many cases it's better to make sure that the code properly
assigns to them, because otherwise the default value might be used
inadvertently.

>>>
>>> +
>>> +       if (((yrgb_dst_w << 3) <= yrgb_src_w) ||
>>> +           ((yrgb_dst_h << 3) <= yrgb_src_h) ||
>>
>> Hmm, if this is enforcing the maximum downscaling factor, then
>> wouldn't it be more readable to write like this:
>>
>> yrgb_src_w >= 8 * yrgb_dst_w
>>
>> Also is >= correct here? Is the maximum factor less than 8?
>
> yrgb_src_w >= 8 * yrgb_dst_w means we can't scale down exceed 8,
> means that (src_w)1920->(dst_w)240 is wrong.

>= means that the factor exceeds _or_equals_ 8, which means that scale factor 8 is not allowed. However the value passed to drm_plane_helper_check_update() as max_scale is 8, which permits 8.

>>> +               if (cbcr_src_w < cbcr_dst_w)
>>> +                       cbr_hor_scl_mode = SCALE_UP;
>>> +               else if (cbcr_src_w > cbcr_dst_w)
>>> +                       cbr_hor_scl_mode = SCALE_DOWN;
>>> +
>>> +               if (cbcr_src_h < cbcr_dst_h)
>>> +                       cbr_ver_scl_mode = SCALE_UP;
>>> +               else if (cbcr_src_h > cbcr_dst_h)
>>> +                       cbr_ver_scl_mode = SCALE_DOWN;
>>
>> Aren't the scl_modes for CbCr planes always the same as for Y plane?
>
>
> No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
> so Y plane horizontal and vertical is scale down.
>
> but src_w = 1920 / 2 = 960 < 1280
>       src_h = 1080 / 2 = 540 < 800.
>
> So Cbcr horizontal and vertical is scale up.

Sorry, I don't follow.

If we scale down Y plane in NV12 from 1920x1080 to 1280x800, then
original CbCr plane will be 960x540 and destination CbCr plane will be
640x400 (because CbCr plane of NV12 is subsampled 2x2, which is half
the width and half the height of Y plane), so both planes are being
scaled down.

>>> +
>>> +       switch (lb_mode) {
>>> +       case LB_YUV_3840X5:
>>> +       case LB_YUV_2560X8:
>>> +       case LB_RGB_1920X5:
>>> +       case LB_RGB_1280X8:
>>> +               yrgb_vsu_mode = SCALE_UP_BIC;
>>> +               cbr_vsu_mode  = SCALE_UP_BIC;
>>
>> I might be overlooking something, but don't  yrgb_vsu_mode and
>> cbr_vsu_mode always have the same values?
>
>
> Hmm, yrgb_vsu_mode and cbr_vsu_mode mean different vop config, OK, I think
> it can merge
> together.

Even though they are different registers bitfields, if they always
have the same values, there is no need to create variables just
because of that. IMHO the cleanest way would be to just create a
single value called vsu_mode and use it for both planes.

>
>>> +               break;
>>> +       case LB_RGB_2560X4:
>>> +               yrgb_vsu_mode = SCALE_UP_BIL;
>>> +               cbr_vsu_mode  = SCALE_UP_BIL;
>>> +               break;
>>> +       default:
>>> +               DRM_ERROR("unsupport lb_mode:%d\n", lb_mode);
>>> +               break;
>>> +       }
>>
>> Anyway, this whole switch is a candidate for a helper function.
>
>
> This only use one time, it's ok to be a helper function?
>

I think so, because it will make this function stay at reasonable
length. (Please see Chapter 6: Functions of Documentation/CodingStyle
for more details.)

>>> +                       break;
>>> +               }
>>> +               break;
>>> +       case SCALE_DOWN:
>>> +               switch (yrgb_vsd_mode) {
>>> +               case SCALE_DOWN_BIL:
>>> +                       vdmult = _get_vskiplines(yrgb_src_h, yrgb_dst_h);
>>> +                       scale_yrgb_y =
>>> GET_SCL_FT_BILI_DN_VSKIP(yrgb_src_h,
>>> +
>>> yrgb_dst_h,
>>> +                                                               vdmult);
>>> +                       if (vdmult == 4) {
>>> +                               vsd_yrgb_gt4 = 1;
>>> +                               vsd_yrgb_gt2 = 0;
>>> +                       } else if (vdmult == 2) {
>>> +                               vsd_yrgb_gt4 = 0;
>>> +                               vsd_yrgb_gt2 = 1;
>>> +                       }
>>
>> How about something like this:
>>
>> vsd_yrgb_gt4 = (vdmult == 4);
>> vsd_yrgb_gt2 = (vdmult == 2);
>
>
> But I think it's not easy to read.
>

OK. Either is fine for me.

I thought this way of coding would represent more closely what the
hardware seems to expect (gt4 for vdmult == 2 and gt2 for vdmult ==
4).

>>> +       /*
>>> +        * (2.1)CBCR HOR SCALE FACTOR
>>> +        */
>>> +       switch (cbr_hor_scl_mode) {
>>> +       case SCALE_UP:
>>> +               scale_cbcr_x = GET_SCL_FT_BIC(cbcr_src_w, cbcr_dst_w);
>>> +               break;
>>> +       case SCALE_DOWN:
>>> +               switch (cbr_hsd_mode) {
>>> +               case SCALE_DOWN_BIL:
>>> +                       scale_cbcr_x = GET_SCL_FT_BILI_DN(cbcr_src_w,
>>> +                                                         cbcr_dst_w);
>>> +                       break;
>>> +               case SCALE_DOWN_AVG:
>>> +                       scale_cbcr_x = GET_SCL_FT_AVRG(cbcr_src_w,
>>> cbcr_dst_w);
>>> +                       break;
>>> +               default:
>>> +                       DRM_ERROR("unsupport cbr_hsd_mode:%d\n",
>>> cbr_hsd_mode);
>>
>> Error.
>>
>>> +                       break;
>>> +               }
>>> +               break;
>>> +       }
>>
>> Isn't this switch exactly the same as for Y plane just with different
>> widths used? Also, wouldn't the values for CbCr plane be the same as
>> for Y plane?
>
>
> But Cbcr case is not same as Y plane case,  how to merge?
>

Hmm, could you point the differences? I don't see anything else than
simple s/yrgb/cbcr/ in both parts of the code. Please correct me if
I'm missing something.

>
>>> +
>>> +       /*
>>> +        * (2.2)CBCR VER SCALE FACTOR
>>> +        */
>>> +       switch (cbr_ver_scl_mode) {
>>> +       case SCALE_UP:
>>> +               switch (cbr_vsu_mode) {
>>> +               case SCALE_UP_BIL:
>>> +                       scale_cbcr_y = GET_SCL_FT_BILI_UP(cbcr_src_h,
>>> +                                                         cbcr_dst_h);
>>> +                       break;
>>> +               case SCALE_UP_BIC:
>>> +                       if (cbcr_src_h < 3)
>>> +                               DRM_ERROR("cbcr_src_h need greater than 3
>>> !\n");
>>> +                       scale_cbcr_y = GET_SCL_FT_BIC(cbcr_src_h,
>>> cbcr_dst_h);
>>> +                       break;
>>> +               default:
>>> +                       DRM_ERROR("unsupport cbr_vsu_mode:%d\n",
>>> cbr_vsu_mode);
>>
>> Error.
>>
>>> +                       break;
>>> +               }
>>> +               break;
>>> +       case SCALE_DOWN:
>>> +               switch (cbr_vsd_mode) {
>>> +               case SCALE_DOWN_BIL:
>>> +                       vdmult = _get_vskiplines(cbcr_src_h, cbcr_dst_h);
>>> +                       scale_cbcr_y =
>>> GET_SCL_FT_BILI_DN_VSKIP(cbcr_src_h,
>>> +
>>> cbcr_dst_h,
>>> +                                                               vdmult);
>>> +                       if (vdmult == 4) {
>>> +                               vsd_cbr_gt4 = 1;
>>> +                               vsd_cbr_gt2 = 0;
>>> +                       } else if (vdmult == 2) {
>>> +                               vsd_cbr_gt4 = 0;
>>> +                               vsd_cbr_gt2 = 1;
>>> +                       }
>>> +                       break;
>>> +               case SCALE_DOWN_AVG:
>>> +                       scale_cbcr_y = GET_SCL_FT_AVRG(cbcr_src_h,
>>> cbcr_dst_h);
>>> +                       break;
>>> +               default:
>>> +                       DRM_ERROR("unsupport cbr_vsd_mode:%d\n",
>>> cbr_vsd_mode);
>>> +                       break;
>>> +               }
>>> +               break;
>>> +       }
>>
>> Again, this looks like the values for CbCr would be the same as for Y.
>> Are there actually cases when they differ?
>
>
> Hmm, Actually the cbcr calculations have some different with Y.
>

Again I don't see any differences between those two blocks of code
other than simple s/yrgb/cbcr/. Please correct me if I'm missing
something.

>>> +
>>> +#define SCL_OFFSET_FIXPOINT_SHIFT      8
>>> +#define SCL_OFFSET_FIXPOINT(x) \
>>> +               ((INT32)((x) * (1 << SCL_OFFSET_FIXPOINT_SHIFT)))
>>
>> All of the function macros above: static inline. This should also let
>> you remove the casts.
>
> what the "casts" mean? Why do you thinking that "static inline" is better
> than "#define"?

I mean typecasting, such as (INT32).

Static inline provides built in type checking by compiler. So you
specify types of arguments and return values and have this enforced at
compilation time. Moreover, inline functions are just functions so
have all the goodness of C parser, helping to avoid strange mistakes,
such as missing parentheses around macro argument and mixing operator
precedence because of that.

Of course sometimes you just can't get away without using macros (such
as the need of argument concatenation or stringification), but this
case doesn't seem to be such.

Best regards,
Tomasz

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Russell King - ARM Linux]

On Fri, Jul 03, 2015 at 06:58:44PM +0900, Tomasz Figa wrote:
> On Fri, Jul 3, 2015 at 6:17 PM, Mark yao <mark.yao@rock-chips.com> wrote:
> >> Aren't the scl_modes for CbCr planes always the same as for Y plane?
> >
> >
> > No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
> > so Y plane horizontal and vertical is scale down.
> >
> > but src_w = 1920 / 2 = 960 < 1280
> >       src_h = 1080 / 2 = 540 < 800.
> >
> > So Cbcr horizontal and vertical is scale up.
> 
> Sorry, I don't follow.
> 
> If we scale down Y plane in NV12 from 1920x1080 to 1280x800, then
> original CbCr plane will be 960x540 and destination CbCr plane will be
> 640x400 (because CbCr plane of NV12 is subsampled 2x2, which is half
> the width and half the height of Y plane), so both planes are being
> scaled down.

This is about displaying a NV12 image with arbitary scaling, right?
If so, think about what happens when you want to display such an image.
You don't display the raw NV12 image, the image gets converted to an
appropriate format first (be that RGB or YUV with no sub-sampling.)

What that means is that you need to scale each component to be the
same resolution.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Mark yao]

On 2015年07月03日 17:58, Tomasz Figa wrote:
>>> >>Aren't the scl_modes for CbCr planes always the same as for Y plane?
>> >
>> >
>> >No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
>> >so Y plane horizontal and vertical is scale down.
>> >
>> >but src_w = 1920 / 2 = 960 < 1280
>> >       src_h = 1080 / 2 = 540 < 800.
>> >
>> >So Cbcr horizontal and vertical is scale up.
> Sorry, I don't follow.
>
> If we scale down Y plane in NV12 from 1920x1080 to 1280x800, then
> original CbCr plane will be 960x540 and destination CbCr plane will be
> 640x400 (because CbCr plane of NV12 is subsampled 2x2, which is half
> the width and half the height of Y plane), so both planes are being
> scaled down.
>
destination CbCr plane is 1280x800, destination can't be subsample.

-- 
Ｍark


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm/rockchip: vop: support plane scale

[Tomasz Figa]

On Fri, Jul 3, 2015 at 7:22 PM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2015年07月03日 17:58, Tomasz Figa wrote:
>>>>
>>>> >>Aren't the scl_modes for CbCr planes always the same as for Y plane?
>>>
>>> >
>>> >
>>> >No, such as src(1920 x 1080) -> dst(1280x800), yuv format is NV12.
>>> >so Y plane horizontal and vertical is scale down.
>>> >
>>> >but src_w = 1920 / 2 = 960 < 1280
>>> >       src_h = 1080 / 2 = 540 < 800.
>>> >
>>> >So Cbcr horizontal and vertical is scale up.
>>
>> Sorry, I don't follow.
>>
>> If we scale down Y plane in NV12 from 1920x1080 to 1280x800, then
>> original CbCr plane will be 960x540 and destination CbCr plane will be
>> 640x400 (because CbCr plane of NV12 is subsampled 2x2, which is half
>> the width and half the height of Y plane), so both planes are being
>> scaled down.
>>
> destination CbCr plane is 1280x800, destination can't be subsample.

Ah, I see, so internally it's always scalling to destination plane
size. This means that calculation for both Y and CbCr must be done
indeed, but even then you could use helper functions for that, since
they differ only in parameters.

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/5] drm/rockchip: vop: switch cursor plane to window 3

[Mark Yao]

Window 1 support scale and yuv format, it's waste use it for a
cursor, use window 3 is enough.

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

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f6ef634..40107bb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -342,13 +342,14 @@ static const struct vop_reg_data vop_init_reg_table[] = {
 /*
  * Note: rk3288 has a dedicated 'cursor' window, however, that window requires
  * special support to get alpha blending working.  For now, just use overlay
- * window 1 for the drm cursor.
+ * window 3 for the drm cursor.
+ *
  */
 static const struct vop_win_data rk3288_vop_win_data[] = {
 	{ .base = 0x00, .phy = &win01_data, .type = DRM_PLANE_TYPE_PRIMARY },
-	{ .base = 0x40, .phy = &win01_data, .type = DRM_PLANE_TYPE_CURSOR },
+	{ .base = 0x40, .phy = &win01_data, .type = DRM_PLANE_TYPE_OVERLAY },
 	{ .base = 0x00, .phy = &win23_data, .type = DRM_PLANE_TYPE_OVERLAY },
-	{ .base = 0x50, .phy = &win23_data, .type = DRM_PLANE_TYPE_OVERLAY },
+	{ .base = 0x50, .phy = &win23_data, .type = DRM_PLANE_TYPE_CURSOR },
 	{ .base = 0x00, .phy = &cursor_data, .type = DRM_PLANE_TYPE_OVERLAY },
 };
 
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm/rockchip: vop: switch cursor plane to window 3

[Tomasz Figa]

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> Window 1 support scale and yuv format, it's waste use it for a
> cursor, use window 3 is enough.
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm/rockchip: vop: switch cursor plane to window 3

[Tomasz Figa]

I missed it originally, but if it's not too late yet...

On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> Window 1 support scale and yuv format, it's waste use it for a
> cursor, use window 3 is enough.
>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index f6ef634..40107bb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -342,13 +342,14 @@ static const struct vop_reg_data vop_init_reg_table[] = {
>  /*
>   * Note: rk3288 has a dedicated 'cursor' window, however, that window requires
>   * special support to get alpha blending working.  For now, just use overlay
> - * window 1 for the drm cursor.
> + * window 3 for the drm cursor.
> + *

I guess this blank line is not necessary, as pointed on chromium review.

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Mark Yao]

Win2/3 support 4 area display, but now havn't found a suitable
way to use it, and it enable by win gate and area gate,
so default enable area0 gate, so that its behaviour just like a
win.

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

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 40107bb..e001d26 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -337,6 +337,12 @@ static const struct vop_reg_data vop_init_reg_table[] = {
 	{DSP_CTRL0, 0x00000000},
 	{WIN0_CTRL0, 0x00000080},
 	{WIN1_CTRL0, 0x00000080},
+	/*
+	 * Todo: win2/3 support area func, but now havn't found a suitable
+	 * way to use it, so default enable area0 as a win display.
+	 */
+	{WIN2_CTRL0, 0x00000010},
+	{WIN3_CTRL0, 0x00000010},
 };
 
 /*
-- 
1.7.9.5


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Tomasz Figa]

Hi Mark,

Please see my comments inline.

On Fri, Jun 26, 2015 at 7:10 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
> Win2/3 support 4 area display, but now havn't found a suitable
> way to use it, and it enable by win gate and area gate,
> so default enable area0 gate, so that its behaviour just like a
> win.

So I assume this means that currently, without those bits set, win2
and win3 do not work? This would make this patch a fix maybe even with
a potential backportability.

>
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 40107bb..e001d26 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -337,6 +337,12 @@ static const struct vop_reg_data vop_init_reg_table[] = {
>         {DSP_CTRL0, 0x00000000},
>         {WIN0_CTRL0, 0x00000080},
>         {WIN1_CTRL0, 0x00000080},
> +       /*
> +        * Todo: win2/3 support area func, but now havn't found a suitable
> +        * way to use it, so default enable area0 as a win display.

TODO: Win2/3 support multiple area function, but we haven't found
a suitable way to use it yet, so let's just use them as other windows
with only area 0 enabled.

> +        */
> +       {WIN2_CTRL0, 0x00000010},
> +       {WIN3_CTRL0, 0x00000010},

Anyway, is it enough to program those registers one time in
vop_initial()? Won't they get cleared when VOP is power cycled, e.g.
in case of DPMS off and on? Maybe instead this could be done in
vop_update_plane_event() for windows that need it?

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Mark yao]

On 2015年07月03日 16:02, Tomasz Figa wrote:
> Hi Mark,
>
> Please see my comments inline.
>
> On Fri, Jun 26, 2015 at 7:10 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>> Win2/3 support 4 area display, but now havn't found a suitable
>> way to use it, and it enable by win gate and area gate,
>> so default enable area0 gate, so that its behaviour just like a
>> win.
> So I assume this means that currently, without those bits set, win2
> and win3 do not work? This would make this patch a fix maybe even with
> a potential backportability.

Yes, without this patch, all win2/3 area gate default disabled.
vop_update_plane_event call win enable only enable the win gate.

>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 40107bb..e001d26 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -337,6 +337,12 @@ static const struct vop_reg_data vop_init_reg_table[] = {
>>          {DSP_CTRL0, 0x00000000},
>>          {WIN0_CTRL0, 0x00000080},
>>          {WIN1_CTRL0, 0x00000080},
>> +       /*
>> +        * Todo: win2/3 support area func, but now havn't found a suitable
>> +        * way to use it, so default enable area0 as a win display.
> TODO: Win2/3 support multiple area function, but we haven't found
> a suitable way to use it yet, so let's just use them as other windows
> with only area 0 enabled.
>
>> +        */
>> +       {WIN2_CTRL0, 0x00000010},
>> +       {WIN3_CTRL0, 0x00000010},
> Anyway, is it enough to program those registers one time in
> vop_initial()? Won't they get cleared when VOP is power cycled, e.g.
> in case of DPMS off and on? Maybe instead this could be done in
> vop_update_plane_event() for windows that need it?
There are two gate for Win2/3,
at VOP_WIN3_CTRL0:
         bit[0], "win3_en"
             this gating all the area.

         bit[4], win3_mst0_en
         bit[5], win3_mst1_en
         bit[6], win3_mst2_en
         bit[7], win3_mst3_en
             those gate each area.

This patch default enable win3_mst0_en, so control bit[0]"win3_en" that 
cat power on/off this window.

vop_update_plane_event()/ vop_disable_plane() only can control bit[0]"win3_en".


So this patch is enough to enable window2/3 area 0.


> Best regards,
> Tomasz

-- 
Ｍark


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Tomasz Figa]

On Fri, Jul 3, 2015 at 5:19 PM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2015年07月03日 16:02, Tomasz Figa wrote:
>>
>> Hi Mark,
>>
>> Please see my comments inline.
>>
>> On Fri, Jun 26, 2015 at 7:10 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>>>
>>> Win2/3 support 4 area display, but now havn't found a suitable
>>> way to use it, and it enable by win gate and area gate,
>>> so default enable area0 gate, so that its behaviour just like a
>>> win.
>>
>> So I assume this means that currently, without those bits set, win2
>> and win3 do not work? This would make this patch a fix maybe even with
>> a potential backportability.
>
>
> Yes, without this patch, all win2/3 area gate default disabled.
> vop_update_plane_event call win enable only enable the win gate.
>
>
>>
>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>> ---
>>> Changes in v2: None
>>>
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index 40107bb..e001d26 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -337,6 +337,12 @@ static const struct vop_reg_data
>>> vop_init_reg_table[] = {
>>>          {DSP_CTRL0, 0x00000000},
>>>          {WIN0_CTRL0, 0x00000080},
>>>          {WIN1_CTRL0, 0x00000080},
>>> +       /*
>>> +        * Todo: win2/3 support area func, but now havn't found a
>>> suitable
>>> +        * way to use it, so default enable area0 as a win display.
>>
>> TODO: Win2/3 support multiple area function, but we haven't found
>> a suitable way to use it yet, so let's just use them as other windows
>> with only area 0 enabled.
>>
>>> +        */
>>> +       {WIN2_CTRL0, 0x00000010},
>>> +       {WIN3_CTRL0, 0x00000010},
>>
>> Anyway, is it enough to program those registers one time in
>> vop_initial()? Won't they get cleared when VOP is power cycled, e.g.
>> in case of DPMS off and on? Maybe instead this could be done in
>> vop_update_plane_event() for windows that need it?
>
> There are two gate for Win2/3,
> at VOP_WIN3_CTRL0:
>         bit[0], "win3_en"
>             this gating all the area.
>
>         bit[4], win3_mst0_en
>         bit[5], win3_mst1_en
>         bit[6], win3_mst2_en
>         bit[7], win3_mst3_en
>             those gate each area.
>
> This patch default enable win3_mst0_en, so control bit[0]"win3_en" that cat
> power on/off this window.
>
> vop_update_plane_event()/ vop_disable_plane() only can control
> bit[0]"win3_en".
>
>
> So this patch is enough to enable window2/3 area 0.

That's right. However, the vop_init_reg_table[] is only used at probe
time by vop_initial() and register settings listed there are not
applied any time later. If we call DPMS off, which will turn the VOP
off and in turn also the whole power domain off, won't the registers
be reset to default values (e.g. zeroed)?

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Mark yao]

On 2015年07月03日 17:24, Tomasz Figa wrote:
> On Fri, Jul 3, 2015 at 5:19 PM, Mark yao <mark.yao@rock-chips.com> wrote:
>> On 2015年07月03日 16:02, Tomasz Figa wrote:
>>> Hi Mark,
>>>
>>> Please see my comments inline.
>>>
>>> On Fri, Jun 26, 2015 at 7:10 PM, Mark Yao <mark.yao@rock-chips.com> wrote:
>>>> Win2/3 support 4 area display, but now havn't found a suitable
>>>> way to use it, and it enable by win gate and area gate,
>>>> so default enable area0 gate, so that its behaviour just like a
>>>> win.
>>> So I assume this means that currently, without those bits set, win2
>>> and win3 do not work? This would make this patch a fix maybe even with
>>> a potential backportability.
>>
>> Yes, without this patch, all win2/3 area gate default disabled.
>> vop_update_plane_event call win enable only enable the win gate.
>>
>>
>>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>>> ---
>>>> Changes in v2: None
>>>>
>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> index 40107bb..e001d26 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> @@ -337,6 +337,12 @@ static const struct vop_reg_data
>>>> vop_init_reg_table[] = {
>>>>           {DSP_CTRL0, 0x00000000},
>>>>           {WIN0_CTRL0, 0x00000080},
>>>>           {WIN1_CTRL0, 0x00000080},
>>>> +       /*
>>>> +        * Todo: win2/3 support area func, but now havn't found a
>>>> suitable
>>>> +        * way to use it, so default enable area0 as a win display.
>>> TODO: Win2/3 support multiple area function, but we haven't found
>>> a suitable way to use it yet, so let's just use them as other windows
>>> with only area 0 enabled.
>>>
>>>> +        */
>>>> +       {WIN2_CTRL0, 0x00000010},
>>>> +       {WIN3_CTRL0, 0x00000010},
>>> Anyway, is it enough to program those registers one time in
>>> vop_initial()? Won't they get cleared when VOP is power cycled, e.g.
>>> in case of DPMS off and on? Maybe instead this could be done in
>>> vop_update_plane_event() for windows that need it?
>> There are two gate for Win2/3,
>> at VOP_WIN3_CTRL0:
>>          bit[0], "win3_en"
>>              this gating all the area.
>>
>>          bit[4], win3_mst0_en
>>          bit[5], win3_mst1_en
>>          bit[6], win3_mst2_en
>>          bit[7], win3_mst3_en
>>              those gate each area.
>>
>> This patch default enable win3_mst0_en, so control bit[0]"win3_en" that cat
>> power on/off this window.
>>
>> vop_update_plane_event()/ vop_disable_plane() only can control
>> bit[0]"win3_en".
>>
>>
>> So this patch is enough to enable window2/3 area 0.
> That's right. However, the vop_init_reg_table[] is only used at probe
> time by vop_initial() and register settings listed there are not
> applied any time later. If we call DPMS off, which will turn the VOP
> off and in turn also the whole power domain off, won't the registers
> be reset to default values (e.g. zeroed)?
Right, the vop registers would be reset to default values when power 
domain off.

But the cursor can works after resume. because the initial value save to 
the regbak cache,
and cursor area gate, win gate are at the same regs, so it can be 
restore when do cursor enable.

But if we add other regs, this may cause bug, maybe no one restore them.
So I think we need do like under to force restore all the regs when resume:
     memcpy(vop->regs, vop->regsbak, vop->len);

> Best regards,
> Tomasz
>
>
>


-- 
Ｍark


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/rockchip: default enable win2/3 area0 bit

[Tomasz Figa]

On Fri, Jul 3, 2015 at 7:08 PM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2015年07月03日 17:24, Tomasz Figa wrote:
>>
>> On Fri, Jul 3, 2015 at 5:19 PM, Mark yao <mark.yao@rock-chips.com> wrote:
>>>
>>> On 2015年07月03日 16:02, Tomasz Figa wrote:
>>>>
>>>> Hi Mark,
>>>>
>>>> Please see my comments inline.
>>>>
>>>> On Fri, Jun 26, 2015 at 7:10 PM, Mark Yao <mark.yao@rock-chips.com>
>>>> wrote:
>>>>>
>>>>> Win2/3 support 4 area display, but now havn't found a suitable
>>>>> way to use it, and it enable by win gate and area gate,
>>>>> so default enable area0 gate, so that its behaviour just like a
>>>>> win.
>>>>
>>>> So I assume this means that currently, without those bits set, win2
>>>> and win3 do not work? This would make this patch a fix maybe even with
>>>> a potential backportability.
>>>
>>>
>>> Yes, without this patch, all win2/3 area gate default disabled.
>>> vop_update_plane_event call win enable only enable the win gate.
>>>
>>>
>>>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>>>>
>>>>> ---
>>>>> Changes in v2: None
>>>>>
>>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> index 40107bb..e001d26 100644
>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> @@ -337,6 +337,12 @@ static const struct vop_reg_data
>>>>> vop_init_reg_table[] = {
>>>>>           {DSP_CTRL0, 0x00000000},
>>>>>           {WIN0_CTRL0, 0x00000080},
>>>>>           {WIN1_CTRL0, 0x00000080},
>>>>> +       /*
>>>>> +        * Todo: win2/3 support area func, but now havn't found a
>>>>> suitable
>>>>> +        * way to use it, so default enable area0 as a win display.
>>>>
>>>> TODO: Win2/3 support multiple area function, but we haven't found
>>>> a suitable way to use it yet, so let's just use them as other windows
>>>> with only area 0 enabled.
>>>>
>>>>> +        */
>>>>> +       {WIN2_CTRL0, 0x00000010},
>>>>> +       {WIN3_CTRL0, 0x00000010},
>>>>
>>>> Anyway, is it enough to program those registers one time in
>>>> vop_initial()? Won't they get cleared when VOP is power cycled, e.g.
>>>> in case of DPMS off and on? Maybe instead this could be done in
>>>> vop_update_plane_event() for windows that need it?
>>>
>>> There are two gate for Win2/3,
>>> at VOP_WIN3_CTRL0:
>>>          bit[0], "win3_en"
>>>              this gating all the area.
>>>
>>>          bit[4], win3_mst0_en
>>>          bit[5], win3_mst1_en
>>>          bit[6], win3_mst2_en
>>>          bit[7], win3_mst3_en
>>>              those gate each area.
>>>
>>> This patch default enable win3_mst0_en, so control bit[0]"win3_en" that
>>> cat
>>> power on/off this window.
>>>
>>> vop_update_plane_event()/ vop_disable_plane() only can control
>>> bit[0]"win3_en".
>>>
>>>
>>> So this patch is enough to enable window2/3 area 0.
>>
>> That's right. However, the vop_init_reg_table[] is only used at probe
>> time by vop_initial() and register settings listed there are not
>> applied any time later. If we call DPMS off, which will turn the VOP
>> off and in turn also the whole power domain off, won't the registers
>> be reset to default values (e.g. zeroed)?
>
> Right, the vop registers would be reset to default values when power domain
> off.
>
> But the cursor can works after resume. because the initial value save to the
> regbak cache,
> and cursor area gate, win gate are at the same regs, so it can be restore
> when do cursor enable.
>
> But if we add other regs, this may cause bug, maybe no one restore them.
> So I think we need do like under to force restore all the regs when resume:
>     memcpy(vop->regs, vop->regsbak, vop->len);
>

Actually, you're right. The value will be restored on next write to
WINx_CTRL0 using the bit field accessors. So:

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel