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

[Thomas Zimmermann <tzimmermann@suse.de>]

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@suse.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   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@ideasonboard.com/ 
>
>
> 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)