Re: [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@xxxxxxxxxxxxxxxx>
> > ---
> >   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