Re: [PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

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

Hi Tomi,

On Mon, Nov 30, 2020 at 01:53:25PM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 12:50, Laurent Pinchart wrote:
> > On Tue, Nov 03, 2020 at 10:03:10AM +0200, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha@ti.com>
> >>
> >> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> >> omap_plane.c and dispc.c. The supported encodings and ranges are
> >> presets are:
> >>
> >> For COLOR_ENCODING:
> >> - YCbCr BT.601 (default)
> >> - YCbCr BT.709
> >>
> >> For COLOR_RANGE:
> >> - YCbCr limited range
> >> - YCbCr full range (default)
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
> >>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
> >>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
> >>  3 files changed, 97 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> index 48593932bddf..bf0c9d293077 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -874,50 +874,67 @@ 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))
> >> +/* YUV -> RGB, ITU-R BT.601, full range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> >> +	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
> >> +	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
> >> +	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
> >> +	true,			/* full range */
> >> +};
> >>  
> >> -	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));
> >> +/* YUV -> RGB, ITU-R BT.601, limited range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> +	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
> >> +	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
> >> +	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
> >> +	false,			/* limited range */
> >> +};
> >>  
> >> -	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> >> +/* YUV -> RGB, ITU-R BT.709, full range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> >> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
> >> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> >> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
> >> +	true,                   /* full range */
> >> +};
> >>  
> >> -#undef CVAL
> >> -}
> >> +/* YUV -> RGB, ITU-R BT.709, limited range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
> >> +	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
> >> +	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
> >> +	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
> >> +	false,			/* limited range */
> >> +};
> >>  
> >> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> >> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> >> +			     enum omap_plane_id plane,
> >> +			     enum drm_color_encoding color_encoding,
> >> +			     enum drm_color_range color_range)
> >>  {
> >> -	int i;
> >> -	int num_ovl = dispc_get_num_ovls(dispc);
> >> -
> >> -	/* 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 csc_coef_yuv2rgb *csc;
> >>  
> >> -	/* 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 */
> >> -	};
> >> +	switch (color_encoding) {
> >> +	case DRM_COLOR_YCBCR_BT601:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt601_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt601_lim;
> >> +		break;
> >> +	case DRM_COLOR_YCBCR_BT709:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt709_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt709_lim;
> >> +		break;
> >> +	default:
> >> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
> >> +		       color_encoding, plane);
> >> +		return -EINVAL;
> > 
> > Can this happen, given that the properties are created with only the
> > above four combinations being allowed ?
> 
> No, it shouldn't happen. Are you just asking, or are you suggesting that we shouldn't check if
> color_encoding is valid here?
> 
> I don't usually check parameters which we can expect to be correct, but with we use switch/if with
> the parameter, we have to deal with the "else" case too.

I use a default in that case, especially if it can avoid turning the
function from void to int.

> >> +	}
> >>  
> >> -	for (i = 1; i < num_ovl; i++)
> >> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> >> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
> >>  
> >> -	if (dispc->feat->has_writeback)
> >> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> > 
> > Unless I'm mistaken, the writeback plane doesn't have the CSC matrix
> > configured anymore. Is that intentional ?
> 
> It's intentional. I should add it to the description.
> 
> The driver doesn't support writeback, even if we have bits and pieces of writeback code in the
> dispc.c. I've been hoping to add WB support, so I haven't just removed them. However, here I didn't
> want to add new code for WB that I can't test, so I decided to just drop the WB case.

Sounds fair.

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

-- 
Regards,

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