Re: [PATCHv2 17/17] drm/omap: cleanup color space conversion

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

Hi Tomi,

Thank you for the patch.

On Wednesday, 28 February 2018 13:26:14 EET Tomi Valkeinen wrote:
> 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.
> 
> Recalculation also showed that 'bcb' value in yuv-to-rgb conversion had
> been rounded wrongly, and it should be 516 instead of 517.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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 57960df1517a..f688f09b4eae
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -352,11 +352,6 @@ static const struct {
>  	},
>  };
> 
> -struct color_conv_coef {
> -	int ry, rcr, rcb, gy, gcr, gcb, by, bcr, bcb;
> -	int full_range;
> -};
> -
>  static unsigned long dispc_fclk_rate(struct dispc_device *dispc);
>  static unsigned long dispc_core_clk_rate(struct dispc_device *dispc);
>  static unsigned long dispc_mgr_lclk_rate(struct dispc_device *dispc,
> @@ -868,10 +863,19 @@ static void dispc_ovl_set_scale_coef(struct
> dispc_device *dispc, }
>  }
> 
> +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(struct dispc_device *dispc,
>  					    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))
> 
> @@ -886,25 +890,50 @@ static void dispc_ovl_write_color_conv_coef(struct
> dispc_device *dispc, #undef CVAL
>  }
> 
> +static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
> +					   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, DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg, 
> ct->yr));
> +	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr,
> ct->yb));
> +	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb,
> ct->crg));
> +	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg,
> ct->cbr));
> +	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> +
> +	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), !!ct->full_range, 11, 11);

I think I mentioned in my review of v1 that you can drop the !!. Apart from 
that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +#undef CVAL
> +}
> +
>  static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
>  {
>  	int i;
>  	int num_ovl = dispc_get_num_ovls(dispc);
> -	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 */
> +		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(dispc, i, &ctbl_bt601_5_ovl);
> +		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> 
>  	if (dispc->feat->has_writeback)
> -		dispc_ovl_write_color_conv_coef(dispc, OMAP_DSS_WB,
> -						&ctbl_bt601_5_wb);
> +		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
>  }
> 
>  static void dispc_ovl_set_ba0(struct dispc_device *dispc,

-- 
Regards,

Laurent Pinchart

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