Re: [PATCH RFC 4/6] drm/omap: cleanup color space conversion

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jyri,

Thank you for the patch.

On Friday 21 Apr 2017 12:51:15 Jyri Sarha wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> The setup code for color space conversion is a bit messy. This patch
> cleans it up.
> 
> For some reason the TRM uses values in YCrCb order, which is also used
> in the current driver, whereas everywhere else it's YCbCr (which also
> matches YUV order). This patch changes the tables to use the common
> order to avoid confusion.
> 
> The tables are split into separate lines, and comments added for
> clarity.
> 
> WB color conversion registers are similar but different than non-WB, but
> the same function was used to write both. It worked fine because the
> coef table was adjusted accordingly, but that was rather confusing. This
> patch adds a separate function to write the WB values so that the coef
> table can be written in an understandable way.

That's quite a few changes for a single patch. I might have split the last one 
in a separate patch.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 59 +++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 5ac0145..b53e63d 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -293,11 +293,6 @@ struct dispc_gamma_desc {
>  	},
>  };
> 
> -struct color_conv_coef {
> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
> -	int full_range;
> -};
> -
>  static unsigned long dispc_fclk_rate(void);
>  static unsigned long dispc_core_clk_rate(void);
>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
> @@ -757,9 +752,18 @@ static void dispc_ovl_set_scale_coef(enum omap_plane_id
> plane, int fir_hinc, }
>  }
> 
> +struct csc_coef_yuv2rgb {
> +	int ry, rcb, rcr, gy, gcb, gcr, by, bcb, bcr;
> +	bool full_range;
> +};
> +
> +struct csc_coef_rgb2yuv {
> +	int yr, yg, yb, cbr, cbg, cbb, crr, crg, crb;
> +	bool full_range;
> +};
> 
>  static void dispc_ovl_write_color_conv_coef(enum omap_plane_id plane,
> -		const struct color_conv_coef *ct)
> +		const struct csc_coef_yuv2rgb *ct)
>  {
>  #define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> 
> @@ -769,7 +773,24 @@ static void dispc_ovl_write_color_conv_coef(enum
> omap_plane_id plane, dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3),
> CVAL(ct->bcr, ct->by)); dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4),
> CVAL(0, ct->bcb));
> 
> -	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> +
> +#undef CVAL
> +}
> +
> +static void dispc_wb_write_color_conv_coef(const struct csc_coef_rgb2yuv
> *ct) +{
> +	const enum omap_plane_id plane = OMAP_DSS_WB;
> +
> +#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> +
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct-
>crg));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct-
>cbr));
> +	dispc_write_reg(DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> +
> +	REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);
> 
>  #undef CVAL
>  }
> @@ -778,20 +799,28 @@ static void dispc_setup_color_conv_coef(void)
>  {
>  	int i;
>  	int num_ovl = dss_feat_get_num_ovls();
> -	const struct color_conv_coef ctbl_bt601_5_ovl = {
> -		/* YUV -> RGB */
> -		298, 409, 0, 298, -208, -100, 298, 0, 517, 0,
> +
> +	/* YUV -> RGB, ITU-R BT.601, limited range */
> +	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> +		298,    0,  409,	/* ry, rcb, rcr */
> +		298, -100, -208,	/* gy, gcb, gcr */
> +		298,  516,    0,	/* by, bcb, bcr */

You changed 517 to 516, was it intentional ? The commit message doesn't 
mention that modification.

> +		false,			/* limited range */
>  	};
> -	const struct color_conv_coef ctbl_bt601_5_wb = {
> -		/* RGB -> YUV */
> -		66, 129, 25, 112, -94, -18, -38, -74, 112, 0,
> +
> +	/* RGB -> YUV, ITU-R BT.601, limited range */
> +	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> +		 66, 129,  25,		/* yr,   yg,  yb */
> +		-38, -74, 112,		/* cbr, cbg, cbb */
> +		112, -94, -18,		/* crr, crg, crb */
> +		false,			/* limited range */
>  	};
> 
>  	for (i = 1; i < num_ovl; i++)
> -		dispc_ovl_write_color_conv_coef(i, &ctbl_bt601_5_ovl);
> +		dispc_ovl_write_color_conv_coef(i, &coefs_yuv2rgb_bt601_lim);
> 
>  	if (dispc.feat->has_writeback)
> -		dispc_ovl_write_color_conv_coef(OMAP_DSS_WB, 
&ctbl_bt601_5_wb);
> +		dispc_wb_write_color_conv_coef(&coefs_rgb2yuv_bt601_lim);
>  }
> 
>  static void dispc_ovl_set_ba0(enum omap_plane_id plane, u32 paddr)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel