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

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

Hi,

On 15/01/2025 12:26, Thomas Zimmermann wrote:
> 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 

I had missed the discussion, and, indeed, the patch you attached fixes 
the problem on Xilinx.

> ioctl is unsuited for anything but the simple RGB formats. The bpp 

It's a bit difficult to use, but is it really unsuited? bitsperpixel, 
width and height do give an exact pitch and size, do they not? It does 
require the userspace to handle the subsampling and planes, though, so 
far from perfect.

So, I'm all for a new ioctl, but I don't right away see why the current 
ioctl couldn't be used. Which makes me wonder about the drm_warn() in 
your patch, and the "userspace throws in arbitrary values for bpp and 
relies on the kernel to figure it out". Maybe I'm missing something here.

> 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.

Yes, I think that makes sense. That's a long road, though =). So my 
question is, is CREATE_DUMB really unsuitable for other than simple RGB 
formats, or can it be suitable if we just define how the userspace 
should use it for multiplanar, subsampled formats?

  Tomi