[PATCH 24/24] drm/omap: cleanup color space conversion

[Tomi Valkeinen]

On 28/02/18 12:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wednesday, 28 February 2018 12:09:35 EET Tomi Valkeinen wrote:
>> On 27/02/18 16:08, Laurent Pinchart wrote:
>>> On Monday, 12 February 2018 11:44:54 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.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at 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 ff09e2be470f..697274317f7c
>>>> 100644
>>>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>>>> @@ -345,11 +345,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(void);
>>>>  static unsigned long dispc_core_clk_rate(void);
>>>>  static unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
>>>> @@ -841,9 +836,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;
>>>
>>> If you made this a 3x3 matrix without explicit names for the fields I
>>> think you wouldn't need two structure and two functions.
>>
>> That is true, but I think it would also make the code more difficult to
>> follow. It's not exactly obvious which coefficients go where.
> 
> I don't expect this code to be often read, and I think that with appropriate 
> comments in the source code and proper formatting of the table, as you do 
> below, it should be clear enough, but I'll leave it up to you. We're really 
> programming a 3x3 matrix in the hardware, so I think that modeling it as a 3x3 
> matrix in the driver makes sense :-)

Yep. Well, another reason to not change this here is that Jyri has made
a patch to add full range and bt.709 conversions, based on this one.
Changing the structs here would break his patch, and verifying these
conversions is somewhat time consuming.

I'm fine with revisiting the struct after Jyri's patch is also merged.

>>>> +	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))
>>>>
>>>> @@ -853,7 +857,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);
>>>
>>> true and false should be equal to 1 and 0 respectively, so the !!
>>> shouldn't be needed.
>>
>> Yep.
>>
>>>> +
>>>> +#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
>>>>  }
>>>>
>>>> @@ -862,20 +883,28 @@ static void dispc_setup_color_conv_coef(void)
>>>>  {
>>>>  	int i;
>>>>  	int num_ovl = dispc_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 */
>>>
>>> The 517 turned into 516, was that intentional ?
>>
>> Good point. It's been a while, but I did recalculate these, and with
>> rounding, all the other values match except the 517, so I changed it
>> according to my calculations.
>>
>> That said, I'm not sure if the original calculations are correct, as
>> they seem to use 256 as multiplier, which would not result 0xff from 1.0
>> value. In any case, I changed the single value that seemed to be off
>> from the other ones.
> 
> I have no issue with that, but could you please mention it in the commit 
> message ?

Agreed, done.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki