Re: [PATCH 01/10] drm/rockchip: YUV overlays BT.601 color conversion.

[Sean Paul <seanpaul@chromium.org>]

On Thu, Feb 15, 2018 at 12:32:51AM -0500, Daniele Castagna wrote:
> Currently NV12 hardware overlays scheduled with atomic interface are
> converted to RGB using a color space conversion different than BT.601.
> 
> The result is that colors of NV12 buffers composited with Mali don't
> match colors of YUV hardware overlays.
> 
> Running modetest with an NV12 plane also shows colors are incorrect
> if compared to an RGB overlay that represents the same color pattern.
> 
> This CL adds support for YUV2YUV color space conversion (CSC) module on
> rockchip and enables the YUV2RGB part that allows to specify a 4x3
> matrix to convert the colors.
> 
> The matrix is set to BT.601 coefficients to align to Mali and to what
> modetest expects.

Hey Daniele,
Apologies for missing this set. 

> 
> TEST=modetest -M rockchip -s 34@30:2400x1600 -P 30:640x480+650+10@NV12 -P 30:640x480+10+10@XR24
> 
> Change-Id: Ia5a3b4793229e2c63f79f9f414d1cbe6ccc63fc1

It's usually good form to strip out the gerrit Change-Id from the commit
message.

> Tested-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 40 ++++++++++++++++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  9 +++++
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 39 ++++++++++++++++++--
>  3 files changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index ba7505292b786..ea43ab797f555 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -50,6 +50,10 @@
>  		vop_reg_set(vop, &win->phy->scl->ext->name, \
>  			    win->base, ~0, v, #name)
>  
> +#define VOP_YUV2YUV_SET(x, name, v) \
> +		if ((x)->data->yuv2yuv) \
> +			vop_reg_set(vop, &vop->data->yuv2yuv->name, 0, ~0, v, #name)
> +
>  #define VOP_INTR_SET_MASK(vop, name, mask, v) \
>  		vop_reg_set(vop, &vop->data->intr->name, 0, mask, v, #name)
>  
> @@ -79,6 +83,18 @@
>  #define to_vop(x) container_of(x, struct vop, crtc)
>  #define to_vop_win(x) container_of(x, struct vop_win, base)
>  
> +/*
> + * The coefficients of the following matrix are all fixed points.
> + * The format is S2.10 for the 3x3 part of the matrix, and S9.12 for the offsets.
> + * They are all represented in two's complement.
> + */
> +static const uint32_t bt601_yuv2rgb[] = {
> +	0x4A8, 0x0,    0x662,
> +	0x4A8, 0x1E6F, 0x1CBF,
> +	0x4A8, 0x812,  0x0,
> +	0x321168, 0x0877CF, 0x2EB127
> +};
> +
>  enum vop_pending {
>  	VOP_PENDING_FB_UNREF,
>  };
> @@ -722,6 +738,9 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>  	uint32_t val;
>  	bool rb_swap;
>  	int format;
> +	int is_yuv = is_yuv_support(fb->format->format);
> +	int win_index = VOP_WIN_TO_INDEX(vop_win);
> +	int i;
>  
>  	/*
>  	 * can't update plane when vop is disabled.
> @@ -762,7 +781,14 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>  	VOP_WIN_SET(vop, win, format, format);
>  	VOP_WIN_SET(vop, win, yrgb_vir, DIV_ROUND_UP(fb->pitches[0], 4));
>  	VOP_WIN_SET(vop, win, yrgb_mst, dma_addr);
> -	if (is_yuv_support(fb->format->format)) {
> +
> +	if (!win_index) {
> +		VOP_YUV2YUV_SET(vop, win0_y2r_en, is_yuv);
> +	} else if (win_index == 1) {
> +		VOP_YUV2YUV_SET(vop, win1_y2r_en, is_yuv);
> +	}
> +
> +	if (is_yuv) {
>  		int hsub = drm_format_horz_chroma_subsampling(fb->format->format);
>  		int vsub = drm_format_vert_chroma_subsampling(fb->format->format);
>  		int bpp = fb->format->cpp[1];
> @@ -776,6 +802,18 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
>  		dma_addr = rk_uv_obj->dma_addr + offset + fb->offsets[1];
>  		VOP_WIN_SET(vop, win, uv_vir, DIV_ROUND_UP(fb->pitches[1], 4));
>  		VOP_WIN_SET(vop, win, uv_mst, dma_addr);
> +
> +		for (i = 0; i < 12; i++) {
> +			if (!win_index) {
> +				VOP_YUV2YUV_SET(vop,
> +						win0_y2r_coefficients[i],
> +						bt601_yuv2rgb[i]);
> +			} else {
> +				VOP_YUV2YUV_SET(vop,
> +						win1_y2r_coefficients[i],
> +						bt601_yuv2rgb[i]);
> +			}
> +		}
>  	}
>  
>  	if (win->phy->scl)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 56bbd2e2a8efb..aa8a5d2690376 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -79,6 +79,14 @@ struct vop_misc {
>  	struct vop_reg global_regdone_en;
>  };
>  
> +struct vop_yuv2yuv {
> +	struct vop_reg win0_y2r_en;
> +	struct vop_reg win0_y2r_coefficients[12];
> +
> +	struct vop_reg win1_y2r_en;
> +	struct vop_reg win1_y2r_coefficients[12];
> +};

struct vop_yuv2yuv {
        struct {
                struct vop_reg y2r_en;
                struct vop_reg y2r_coefficients[12];
        } regs[2];
};

Then add win_index to the macro above and you can avoid the if (!win_index)
checks everywhere.

Bonus points if you #define the '2' and '12' as something more meaningful

> +
>  struct vop_intr {
>  	const int *intrs;
>  	uint32_t nintrs;
> @@ -157,6 +165,7 @@ struct vop_data {
>  	const struct vop_misc *misc;
>  	const struct vop_modeset *modeset;
>  	const struct vop_output *output;
> +	const struct vop_yuv2yuv *yuv2yuv;
>  	const struct vop_win_data *win;
>  	unsigned int win_size;
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> index 2e4eea3459fe6..be9c414e2e4bc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> @@ -422,6 +422,40 @@ static const struct vop_output rk3399_output = {
>  	.mipi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 15),
>  };
>  
> +static const struct vop_yuv2yuv rk3399_vop_yuv2yuv = {
> +	.win0_y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 1),
> +	.win0_y2r_coefficients = {
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 0, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 0, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 4, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 4, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 8, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 8, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 12, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 12, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 16, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 20, 0xffffffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 24, 0xffffffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 28, 0xffffffff, 0),
> +	},
> +
> +	.win1_y2r_en = VOP_REG(RK3399_YUV2YUV_WIN, 0x1, 9),
> +	.win1_y2r_coefficients = {
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 0, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 0, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 4, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 4, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 8, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 8, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 12, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 12, 0xffff, 16),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 16, 0xffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 20, 0xffffffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 24, 0xffffffff, 0),
> +		VOP_REG(RK3399_WIN1_YUV2YUV_Y2R + 28, 0xffffffff, 0),
> +	},
> +};
> +
>  static const struct vop_data rk3399_vop_big = {
>  	.version = VOP_VERSION(3, 5),
>  	.feature = VOP_FEATURE_OUTPUT_RGB10,
> @@ -430,8 +464,9 @@ static const struct vop_data rk3399_vop_big = {
>  	.modeset = &rk3288_modeset,
>  	.output = &rk3399_output,
>  	.misc = &rk3368_misc,
> -	.win = rk3368_vop_win_data,
> -	.win_size = ARRAY_SIZE(rk3368_vop_win_data),
> +	.yuv2yuv = &rk3399_vop_yuv2yuv,
> +	.win = rk3399_vop_win_data,
> +	.win_size = ARRAY_SIZE(rk3399_vop_win_data),
>  };
>  
>  static const struct vop_win_data rk3399_vop_lit_win_data[] = {
> -- 
> 2.16.1.291.g4437f3f132-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel