[PATCH v2] drm/rockchip: Fix YUV buffers color rendering

[Ezequiel Garcia]

On Mon, 2019-01-07 at 14:26 +0100, Heiko Stuebner wrote:
> Hi,
> 
> sorry, only now got to test this on actual hardware,
> 
> Am Freitag, 14. Dezember 2018, 17:29:20 CET schrieb Ezequiel Garcia:
> > From: Daniele Castagna <dcastagna at chromium.org>
> > 
> > Currently, YUV hardware overlays are converted to RGB using
> > a color space conversion different than BT.601.
> > 
> > The result is that colors of e.g. NV12 buffers don't match
> > colors of YUV hardware overlays.
> > 
> > In order to fix this, enable YUV2YUV and set appropriate coefficients
> > for formats such as NV12 to be displayed correctly.
> > 
> > This commit was tested using modetest, gstreamer and chromeos (hardware
> > accelerated video playback). Before the commit, tests rendering
> > with NV12 format resulted in colors not displayed correctly.
> > 
> > Test examples (RK3399 Ficus board connected to HDMI monitor):
> > 
> >   $ modetest 39 at 32:1920x1080 at NV12
> >   $ gst-launch-1.0 videotestrc ! video/x-raw,format=NV12 ! kmssink
> > 
> > Signed-off-by: Daniele Castagna <dcastagna at chromium.org>
> > [ezequiel: rebase on linux-next and massage commit log]
> > Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
> > ---
> > v2:
> >   * Addressed feedback from Sean Paul
> >   * Rebased on linux-next
> > 
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 ++++++++++++++++++++-
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 13 +++++++
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 36 ++++++++++++++++++
> >  3 files changed, 89 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index fb70fb486fbf..78c7f63a60c0 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -52,6 +52,18 @@
> >  		vop_reg_set(vop, &win->phy->scl->ext->name, \
> >  			    win->base, ~0, v, #name)
> >  
> > +#define VOP_WIN_YUV2YUV_SET(x, win_yuv2yuv, name, v) \
> > +	do { \
> > +		if (win_yuv2yuv->name.mask) \
> > +			vop_reg_set(vop, &win_yuv2yuv->name, 0, ~0, v, #name); \
> > +	} while (0)
> > +
> > +#define VOP_WIN_YUV2YUV_COEFFICIENT_SET(x, win_yuv2yuv, name, v) \
> > +	do { \
> > +		if (win_yuv2yuv->phy->name.mask) \
> > +			vop_reg_set(vop, &win_yuv2yuv->phy->name, win_yuv2yuv->base, ~0, v, #name); \
> > +	} while (0)
> > +
> 
> While this seems to work on rk3399, it hangs both my rk3328 (rock64)
> and rk3288 (google-pinky) during rockchip-drm probe.
> 

Oh, shame on me, I should've done that.

> Making this something like
> 
> 	if (win_yuv2yuv && win_yuv2yuv->phy->name.mask) \
> 
> aka testing for existence of win_yuv2yuv first, makes them boot again,
> so I guess I ran into a (for whatever reason) silent null-ptr-dereference.
> 

Sounds good. I'll post a v3.

> 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > index 08fc40af52c8..fe752df4e038 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> > @@ -637,6 +637,34 @@ static const struct vop_output rk3399_output = {
> >  	.mipi_dual_channel_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 3),
> >  };
> >  
> > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win01_data = {
> > +	.y2r_coefficients = {
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 0, 0xffff, 16),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 4, 0xffff, 16),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 8, 0xffff, 16),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 12, 0xffff, 16),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 16, 0xffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 20, 0xffffffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 24, 0xffffffff, 0),
> > +		VOP_REG(RK3399_WIN0_YUV2YUV_Y2R + 28, 0xffffffff, 0),
> > +	},
> > +};
> > +
> > +static const struct vop_yuv2yuv_phy rk3399_yuv2yuv_win23_data = { };
> 
> looking at the rk3399 TRM it seems that win2+3 also have yuv2rgb
> coefficient registers. I didn't check in depth but are they so different
> that they cannot be supported?
> 
> Aka what is the difference between win0/1 and win2/3 ?
> 
> 

I think Sandy is the best to answer this. I can't say I'm an expert
on the details of this hardware.

Thanks,
Eze