Tomi Valkeinen
2025-Jan-15 10:13 UTC
[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
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: 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
Thomas Zimmermann
2025-Jan-15 10:26 UTC
[PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
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 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: > > 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.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] 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)
Apparently Analagous Threads
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 25/25] drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb()
- [PATCH v2 00/25] drm/dumb-buffers: Fix and improve buffer-size calculation
- [PATCH v2 20/21] drm/xlnx: Initialize DRM driver instance with CMA helper macro