Re: [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

Hi!

On 09/01/2025 16:57, Thomas Zimmermann wrote:
> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and
> buffer size. Align the pitch according to hardware requirements.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c 
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index b47463473472..7ea0cd4f71d3 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -19,6 +19,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_device.h>
>   #include <drm/drm_drv.h>
> +#include <drm/drm_dumb_buffers.h>
>   #include <drm/drm_encoder.h>
>   #include <drm/drm_fbdev_dma.h>
>   #include <drm/drm_fourcc.h>
> @@ -363,10 +364,12 @@ static int zynqmp_dpsub_dumb_create(struct drm_file 
> *file_priv,
>                                     struct drm_mode_create_dumb *args)
>   {
>         struct zynqmp_dpsub *dpsub = to_zynqmp_dpsub(drm);
> -       unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +       int ret;
>   
>   	/* Enforce the alignment constraints of the DMA engine. */
> -       args->pitch = ALIGN(pitch, dpsub->dma_align);
> +       ret = drm_mode_size_dumb(drm, args, dpsub->dma_align, 0);
> +       if (ret)
> +               return ret;
>   
>   	return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
>   }
I have some trouble with this one.

I have sent a series to add some pixel formats:

https://lore.kernel.org/all/20250115-xilinx-formats-v2-0-160327ca652a@xxxxxxxxxxxxxxxx/

Let's look at XV15. It's similar to NV12, but 10 bits per component, and 
some packing and padding.
First plane: 3 pixels in a 32 bit group
Second plane: 3 pixels in a 64 bit group, 2x2 subsampled

So, on average, a pixel on the first plane takes 32 / 3 = 10.666... bits 
on a line. That's not a usable number for the DRM_IOCTL_MODE_CREATE_DUMB 
ioctl.
What I did was to use the pixel group size as "bpp" for 
DRM_IOCTL_MODE_CREATE_DUMB. So, e.g., for 720 x 576:
Stride for first plane: 720 * (32 / 3) / 8 = 960 bytes
Stride for second plane: 720 / 2 * (64 / 3) / 8 = 960 bytes

First plane: 720 / 3 = 240 pixel groups
Second plane: 720 / 2 / 3 = 120 pixel groups

So I allocated the two planes with:
240 x 576 with 32 bitspp
120 x 288 with 64 bitspp

This worked, and if I look at the DRM_IOCTL_MODE_CREATE_DUMB in the 
docs, I can't right away see anything there that says my tactic was not 
allowed.
The above doesn't work anymore with this patch, as the code calls 
drm_driver_color_mode_format(), which fails for 64 bitspp. It feels a 
bit odd that DRM_IOCTL_MODE_CREATE_DUMB will try to guess the RGB fourcc 
for a dumb buffer allocation.
So, what to do here? Am I doing something silly? What's the correct way 
to allocate the buffers for XV15? Should I just use 32 bitspp for the 
plane 2 too, and double the width (this works)?
Is DRM_IOCTL_MODE_CREATE_DUMB only meant for simple RGB formats? The 
xilinx driver can, of course, just not use drm_mode_size_dumb(). But if 
so, I guess the limitations of drm_mode_size_dumb() should be documented.
Do we need a new dumb-alloc ioctl that takes the format and plane number 
as parameters? Or alternatively a simpler dumb-alloc that doesn't have 
width and bpp, but instead takes a stride and height as parameters? I 
think those would be easier for the userspace to use, instead of trying 
to adjust the parameters to be suitable for the kernel.
 Tomi