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)
