[PATCH v4 09/11] drm: xlnx: zynqmp: Add support for Y8 and Y10_P32

[Tomi Valkeinen]

Hi,

On 28/03/2025 00:52, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Mar 26, 2025 at 03:22:52PM +0200, Tomi Valkeinen wrote:
>> Add support for Y8 and Y10_P32 formats. We also need to add new csc
>> matrices for the y-only formats.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>> ---
>>   drivers/gpu/drm/xlnx/zynqmp_disp.c | 26 +++++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index 1dc77f2e4262..ae8b4073edf6 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -307,6 +307,16 @@ static const struct zynqmp_disp_format avbuf_vid_fmts[] = {
>>   		.buf_fmt	= ZYNQMP_DISP_AV_BUF_FMT_NL_VID_YV16CI_10,
>>   		.swap		= false,
>>   		.sf		= scaling_factors_101010,
>> +	}, {
>> +		.drm_fmt	= DRM_FORMAT_Y8,
>> +		.buf_fmt	= ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MONO,
>> +		.swap		= false,
>> +		.sf		= scaling_factors_888,
>> +	}, {
>> +		.drm_fmt	= DRM_FORMAT_Y10_P32,
>> +		.buf_fmt	= ZYNQMP_DISP_AV_BUF_FMT_NL_VID_YONLY_10,
>> +		.swap		= false,
>> +		.sf		= scaling_factors_101010,
> 
> Assuming the DRM format definitions get approved, this looks good to me.
> 
>>   	},
>>   };
>>   
>> @@ -697,6 +707,16 @@ static const u32 csc_sdtv_to_rgb_offsets[] = {
>>   	0x0, 0x1800, 0x1800
>>   };
>>   
>> +static const u16 csc_sdtv_to_rgb_yonly_matrix[] = {
> 
> TODO: Add support for colorspaces to the driver.
> 
>> +	0x0, 0x0, 0x1000,
>> +	0x0, 0x0, 0x1000,
>> +	0x0, 0x0, 0x1000,
> 
> This surprises me a bit, I was expecting 0x1000 to be in the first
> column. What am I missing ?

All this is undocumented (afaics). But my understanding is that as this 
is a single channel format, the Y data is in the "lowest" channel, which 
is handled by the rightmost column in the matrix.

>> +};
>> +
>> +static const u32 csc_sdtv_to_rgb_yonly_offsets[] = {
>> +	0x1800, 0x1800, 0x0
> 
> Why do you need offsets ? Those values correspond to -128 in a 8-bit
> range, and that's what would need to be applied to the chroma values.
> There's no chroma here. I think you could use csc_zero_offsets.

Indeed, the values are not needed. The only value in the offsets that 
matters is the last one (matching the rightmost column in the matrix), 
which needs to be 0. But I think it makes sense to have the array here, 
otherwise one needs to dig into the code to see what are the offsets for 
y-only.

  Tomi