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

[Heiko Stuebner]

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.

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.


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


Heiko