| Message ID | 20250326-xilinx-formats-v4-9-322a300c6d72@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Add new pixel formats for Xilinx Zynqmp | expand |
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 ? > +}; > + > +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. > +}; > + > /** > * zynqmp_disp_blend_set_output_format - Set the output format of the blender > * @disp: Display controller > @@ -846,7 +866,11 @@ static void zynqmp_disp_blend_layer_enable(struct zynqmp_disp *disp, > ZYNQMP_DISP_V_BLEND_LAYER_CONTROL(layer->id), > val); > > - if (layer->drm_fmt->is_yuv) { > + if (layer->drm_fmt->format == DRM_FORMAT_Y8 || > + layer->drm_fmt->format == DRM_FORMAT_Y10_P32) { > + coeffs = csc_sdtv_to_rgb_yonly_matrix; > + offsets = csc_sdtv_to_rgb_yonly_offsets; > + } else if (layer->drm_fmt->is_yuv) { > coeffs = csc_sdtv_to_rgb_matrix; > offsets = csc_sdtv_to_rgb_offsets; > } else {
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
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, }, }; @@ -697,6 +707,16 @@ static const u32 csc_sdtv_to_rgb_offsets[] = { 0x0, 0x1800, 0x1800 }; +static const u16 csc_sdtv_to_rgb_yonly_matrix[] = { + 0x0, 0x0, 0x1000, + 0x0, 0x0, 0x1000, + 0x0, 0x0, 0x1000, +}; + +static const u32 csc_sdtv_to_rgb_yonly_offsets[] = { + 0x1800, 0x1800, 0x0 +}; + /** * zynqmp_disp_blend_set_output_format - Set the output format of the blender * @disp: Display controller @@ -846,7 +866,11 @@ static void zynqmp_disp_blend_layer_enable(struct zynqmp_disp *disp, ZYNQMP_DISP_V_BLEND_LAYER_CONTROL(layer->id), val); - if (layer->drm_fmt->is_yuv) { + if (layer->drm_fmt->format == DRM_FORMAT_Y8 || + layer->drm_fmt->format == DRM_FORMAT_Y10_P32) { + coeffs = csc_sdtv_to_rgb_yonly_matrix; + offsets = csc_sdtv_to_rgb_yonly_offsets; + } else if (layer->drm_fmt->is_yuv) { coeffs = csc_sdtv_to_rgb_matrix; offsets = csc_sdtv_to_rgb_offsets; } else {
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(-)