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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi


Am 15.01.25 um 11:13 schrieb Tomi Valkeinen:
> 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.
These are all good points. Did you read my discussion with Andy on patch 
2? I think it resolves all the points you have. The current CREATE_DUMB 
ioctl is unsuited for anything but the simple RGB formats. The bpp 
parameter is not very precise. The solution would be a new ioctl call 
that receives the DRM format and returns a buffer for each individual plane.
I provided a workaround patch that uses the bpp value directly if 
drm_driver_color_mode_format() does not support the bpp value. 
User-space code has to allocate a large enough buffer via the current 
CREATE_DUMB and compute the individual planes itself. See [1] for an 
example. [1] 
https://gitlab.freedesktop.org/mesa/drm/-/blob/main/tests/modetest/buffers.c?ref_type=heads#L302 
Does this work for you? Otherwise, I guess we should be talking about a 
possible CREATE_DUMB2 that fixes these shortcomings. Best regards Thomas
>  Tomi
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)