On Thu, Jan 05, 2017 at 02:26:30AM -0500, Sean Paul wrote:

quoted

+static u32 zx_vl_get_fmt(uint32_t format)
+{
+       u32 val = 0;
+
+       switch (format) {
+       case DRM_FORMAT_NV12:
+               val = VL_FMT_YUV420;
+               break;
+       case DRM_FORMAT_YUV420:
+               val = VL_YUV420_PLANAR | VL_FMT_YUV420;
+               break;
+       case DRM_FORMAT_YUYV:
+               val = VL_YUV422_YUYV | VL_FMT_YUV422;
+               break;
+       case DRM_FORMAT_YVYU:
+               val = VL_YUV422_YVYU | VL_FMT_YUV422;
+               break;
+       case DRM_FORMAT_UYVY:
+               val = VL_YUV422_UYVY | VL_FMT_YUV422;
+               break;
+       case DRM_FORMAT_VYUY:
+               val = VL_YUV422_VYUY | VL_FMT_YUV422;
+               break;
+       case DRM_FORMAT_YUV444:
+               val = VL_FMT_YUV444_8BIT;

Minor nit: You could have eliminated val and just returned directly
from all of the cases. Seems like there are a few other functions this
is also true for.

Okay.  I will change zx_vl_get_fmt() and zx_vl_rsz_get_fmt()
accordingly.

quoted

+               break;
+       default:
+               WARN_ONCE(1, "invalid pixel format %d\n", format);
+       }
+
+       return val;
+}

<snip>

quoted

+static void zx_vl_rsz_setup(struct zx_plane *zplane, uint32_t format,
+                           u32 src_w, u32 src_h, u32 dst_w, u32 dst_h)
+{
+       void __iomem *rsz = zplane->rsz;
+       u32 src_chroma_w = src_w;
+       u32 src_chroma_h = src_h;
+       u32 fmt;
+
+       /* Set up source and destination resolution */
+       zx_writel(rsz + RSZ_SRC_CFG, RSZ_VER(src_h - 1) | RSZ_HOR(src_w - 1));
+       zx_writel(rsz + RSZ_DEST_CFG, RSZ_VER(dst_h - 1) | RSZ_HOR(dst_w - 1));
+
+       /* Configure data format for VL RSZ */
+       fmt = zx_vl_rsz_get_fmt(format);
+       zx_writel_mask(rsz + RSZ_VL_CTRL_CFG, RSZ_VL_FMT_MASK, fmt);
+
+       /* Calculate Chroma heigth and width */

s/heigth/height/

Thanks for spotting it.

quoted

+       if (fmt == RSZ_VL_FMT_YCBCR420) {
+               src_chroma_w = src_w >> 1;
+               src_chroma_h = src_h >> 1;
+       } else if (fmt == RSZ_VL_FMT_YCBCR422) {
+               src_chroma_w = src_w >> 1;
+       }
+
+       /* Set up Luma and Chroma step registers */
+       zx_writel(rsz + RSZ_VL_LUMA_HOR, rsz_step_value(src_w, dst_w));
+       zx_writel(rsz + RSZ_VL_LUMA_VER, rsz_step_value(src_h, dst_h));
+       zx_writel(rsz + RSZ_VL_CHROMA_HOR, rsz_step_value(src_chroma_w, dst_w));
+       zx_writel(rsz + RSZ_VL_CHROMA_VER, rsz_step_value(src_chroma_h, dst_h));
+
+       zx_vl_rsz_set_update(zplane);
+}

<snip>

quoted

diff --git a/drivers/gpu/drm/zte/zx_vou.c b/drivers/gpu/drm/zte/zx_vou.c
index 3fb4fc04e693..e832c2ec3156 100644
--- a/drivers/gpu/drm/zte/zx_vou.c
+++ b/drivers/gpu/drm/zte/zx_vou.c

@@ -84,6 +84,8 @@ struct zx_crtc_bits {
 struct zx_crtc {
        struct drm_crtc crtc;
        struct drm_plane *primary;
+       struct drm_plane *overlay_active[VL_NUM];
+       unsigned int overlay_active_num;

I don't think this belongs here. You can instead add an active (or
enabled) bool to the zx_plane struct and keep track of it via
atomic_plane_update/disable. This allows you to call
zx_plane_set_update unconditionally in the vou irq handler and check
active/enabled in zx_plane_set_update.

It's a truly great suggestion.  How did I not think of it :)  The v4 is
coming for that.

Thanks a lot for the review effort, Sean.

Shawn