Re: [PATCH v5 00/11] drm: Add new pixel formats for Xilinx Zynqmp

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi All,

Any further comments on this series? Can we get this merged?

 Tomi

On 25/04/2025 14:01, Tomi Valkeinen wrote:
> Add new DRM pixel formats and add support for those in the Xilinx zynqmp
> display driver.
> 
> All other formats except XVUY2101010 are already supported in upstream
> gstreamer, but gstreamer's kmssink does not have the support yet, as it
> obviously cannot support the formats without kernel having the formats.
> 
> Xilinx has support for these formats in their BSP kernel, and Xilinx has
> a branch here, adding the support to gstreamer kmssink:
> 
> https://github.com/Xilinx/gst-plugins-bad.git xlnx-rebase-v1.18.5
> 
> New formats added:
> 
> DRM_FORMAT_Y8
> - 8-bit Y-only
> - fourcc: "GREY"
> - gstreamer: GRAY8
> 
> DRM_FORMAT_Y10_P32
> - 10-bit Y-only, three pixels packed into 32-bits
> - fourcc: "YPA4"
> - gstreamer: GRAY10_LE32
> 
> DRM_FORMAT_XV15
> - Like NV12, but with 10-bit components
> - fourcc: "XV15"
> - gstreamer: NV12_10LE32
> 
> DRM_FORMAT_XV20
> - Like NV16, but with 10-bit components
> - fourcc: "XV20"
> - gstreamer: NV16_10LE32
> 
> DRM_FORMAT_X403
> - 10-bit planar 4:4:4, with three samples packed into 32-bits
> - fourcc: "X403"
> - gstreamer: Y444_10LE32
> 
> XVUY2101010
> - 10-bit 4:4:4, one pixel in 32 bits
> - fourcc: "XY30"
> 
> Some notes:
> 
> I know the 8-bit greyscale format has been discussed before, and the
> guidance was to use DRM_FORMAT_R8. While I'm not totally against that, I
> would argue that adding DRM_FORMAT_Y8 makes sense, as:
> 
> 1) We can mark it as 'is_yuv' in the drm_format_info, and this can help
>    the drivers handle e.g. full/limited range. Probably some hardware
>    handles grayscale as a value used for all RGB components, in which case
>    R8 makes sense, but when the hardware handles the Y-only pixels as YCbCr,
>    where Cb and Cr are "neutral", it makes more sense to consider the
>    format as an YUV format rather than RGB.
> 
> 2) We can have the same fourcc as in v4l2. While not strictly necessary,
>    it's a constant source of confusion when the fourccs differ.
> 
> 3) It (possibly) makes more sense for the user to use Y8/GREY format
>    instead of R8, as, in my experience, the documentation usually refers
>    to gray(scale) format or Y-only format.
> 
> As we add new Y-only formats, it makes sense to have similar terms, so
> we need to adjust the Y10_P32 format name accordingly.
> 
> I have made some adjustments to the formats compared to the Xilinx's
> branch. E.g. The DRM_FORMAT_Y10_P32 format in Xilinx's kmssink uses
> fourcc "Y10 ", and DRM_FORMAT_Y10. I didn't like those, as the format is
> a packed format, three 10-bit pixels in a 32-bit container, and I think
> Y10 means a 10-bit pixel in a 16-bit container.
> 
> Generally speaking, if someone has good ideas for the format define
> names or fourccs, speak up, as it's not easy to invent good names =).
> That said, keeping them the same as in the Xilinx trees will, of course,
> be slightly easier for the users of Xilinx platforms.
> 
> I made WIP additions to modetest to support most of these formats,
> partially based on Xilinx's code:
> 
> https://github.com/tomba/libdrm.git xilinx
> 
> A few thoughts about that:
> 
> modetest uses bo_create_dumb(), and as highlighted in recent discussions
> in the kernel list [1], dumb buffers are only for RGB formats. They may
> work for non-RGB formats, but that's platform specific. None of the
> formats I add here are RGB formats. Do we want to go this way with
> modetest?
> 
> I also feel that the current structure of modetest is not well suited to
> more complicated formats. Both the buffer allocation is a bit more
> difficult (see "Add virtual_width and pixels_per_container"), and the
> drawing is complicated (see, e.g., "Add support for DRM_FORMAT_XV15 &
> DRM_FORMAT_XV20").
> 
> I have recently added support for these Xilinx formats to both kms++ [2] and
> pykms/pixutils [3][4] (WIP), and it's not been easy... But I have to say I
> think I like the template based version in kms++. That won't work in
> modetest, of course, but a non-templated version might be implementable,
> but probably much slower.
> 
> In any case, I slighly feel it's not worth merging the modetest patches
> I have for these formats: they complicate the code quite a bit, break
> the RGB-only rule, and I'm not sure if there really are (m)any users. If
> we want to add support to modetest, I think a bigger rewrite of the test
> pattern code might be in order.
> 
> [1] https://lore.kernel.org/all/20250109150310.219442-26-tzimmermann%40suse.de/
> [2] git@github.com:tomba/kmsxx.git xilinx
> [3] git@github.com:tomba/pykms.git xilinx
> [4] git@github.com:tomba/pixutils.git xilinx
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Changes in v5:
> - Add comment about Y-only formats, clarifying how the display pipeline
>   handles them (they're handled as YCbCr, with Cb and Cr as "neutral")
> - Clarify X403 format in the patch description
> - Set unused Y-only CSC offsets to 0 (instead of 0x1800).
> - Add R-bs
> - Link to v4: https://lore.kernel.org/r/20250326-xilinx-formats-v4-0-322a300c6d72@ideasonboard.com
> 
> Changes in v4:
> - Reformat the drm_format_info entries a bit
> - Calculate block size only once in drm_format_info_bpp()
> - Declare local variables in separate lines
> - Add review tags
> - Fix commit message referring to Y10_LE32 (should be Y10_P32)
> - Link to v3: https://lore.kernel.org/r/20250212-xilinx-formats-v3-0-90d0fe106995@ideasonboard.com
> 
> Changes in v3:
> - Drop "drm: xlnx: zynqmp: Fix max dma segment size". It is already
>   pushed.
> - Add XVUY2101010 format.
> - Rename DRM_FORMAT_Y10_LE32 to DRM_FORMAT_Y10_P32.
> - Link to v2: https://lore.kernel.org/r/20250115-xilinx-formats-v2-0-160327ca652a@ideasonboard.com
> 
> Changes in v2:
> - I noticed V4L2 already has fourcc Y10P, referring to MIPI-style packed
>   Y10 format. So I changed Y10_LE32 fourcc to YPA4. If logic has any
>   relevance here, P means packed, A means 10, 4 means "in 4 bytes".
> - Added tags to "Fix max dma segment size" patch
> - Updated description for "Add warning for bad bpp"
> - Link to v1: https://lore.kernel.org/r/20241204-xilinx-formats-v1-0-0bf2c5147db1@ideasonboard.com
> 
> ---
> Tomi Valkeinen (11):
>       drm/fourcc: Add warning for bad bpp
>       drm/fourcc: Add DRM_FORMAT_XV15/XV20
>       drm/fourcc: Add DRM_FORMAT_Y8
>       drm/fourcc: Add DRM_FORMAT_Y10_P32
>       drm/fourcc: Add DRM_FORMAT_X403
>       drm/fourcc: Add DRM_FORMAT_XVUY2101010
>       drm: xlnx: zynqmp: Use drm helpers when calculating buffer sizes
>       drm: xlnx: zynqmp: Add support for XV15 & XV20
>       drm: xlnx: zynqmp: Add support for Y8 and Y10_P32
>       drm: xlnx: zynqmp: Add support for X403
>       drm: xlnx: zynqmp: Add support for XVUY2101010
> 
>  drivers/gpu/drm/drm_fourcc.c       | 28 ++++++++++++++++---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c | 55 +++++++++++++++++++++++++++++++++++---
>  include/uapi/drm/drm_fourcc.h      | 29 ++++++++++++++++++++
>  3 files changed, 105 insertions(+), 7 deletions(-)
> ---
> base-commit: ef6517ac5cf971cfeaccea4238d9da7e2425b8b1
> change-id: 20241120-xilinx-formats-f71901621833
> 
> Best regards,