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