On Thu, Jan 05, 2017 at 02:26:30AM -0500, Sean Paul wrote:
> > +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.
>
> > + break;
> > + default:
> > + WARN_ONCE(1, "invalid pixel format %d\n", format);
> > + }
> > +
> > + return val;
> > +}
<snip>
> > +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.
>
> > + 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>
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
