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

Tomi Valkeinen
2025-03-31T11:37:56Z

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@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