| Message ID | 20250109150310.219442-26-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/dumb-buffers: Fix and improve buffer-size calculation | expand |
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. Tomi
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 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 >
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
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); }
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(-)