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

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

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

  Tomi