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,

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

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

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

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

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

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

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