| 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
Hi Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: [...] >> 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. Great. Thanks for testing. > >> 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. The bpp value sets the number of bits per pixel; except for bpp==15 (XRGB1555), where it sets the color depth. OR bpp is the color depth; except for bpp==32 (XRGB8888), where it is the number of bits per pixel. It's therefore best to interpret it like a color-mode enum. > > 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. I was unsure about the drm_warn() as well. It's not really wrong to have odd bpp values, but handing in an unknown bpp value might point to a user-space error. At least there should be a drm_dbg(). > >> 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? That would duplicate format and hardware information in user-space. Some hardware might have odd per-plane limitations that only the driver knows about. For example, there's another discussion on dri-devel about pitch-alignment requirements of DRM_FORMAT_MOD_LINEAR on various hardware. That affects dumb buffers as well. I don't think that there's an immediate need for a CREATE_DUMB2, but it seems worth to keep in mind. Best regards Thomas > > Tomi >
Hi, On 15/01/2025 13:37, Thomas Zimmermann wrote: > Hi > > > Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: > [...] >>> 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. > > Great. Thanks for testing. > >> >>> 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. > > The bpp value sets the number of bits per pixel; except for bpp==15 > (XRGB1555), where it sets the color depth. OR bpp is the color depth; > except for bpp==32 (XRGB8888), where it is the number of bits per pixel. > It's therefore best to interpret it like a color-mode enum. Ah, right... That's horrible =). And I assume it's not really possible to define the bpp to mean bits per pixel, except for a few special cases like 15? Why do we even really care about color depth here? We're just allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for XRGB1555 too? >> 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. > > I was unsure about the drm_warn() as well. It's not really wrong to have > odd bpp values, but handing in an unknown bpp value might point to a > user-space error. At least there should be a drm_dbg(). > >> >>> 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? > > That would duplicate format and hardware information in user-space. Some But we already have that, don't we? We have drivers and userspace that support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we don't document how CREATE_DUMB has to be used to allocate multiplanar subsampled buffers, so the userspace devs have to "guess". > hardware might have odd per-plane limitations that only the driver knows > about. For example, there's another discussion on dri-devel about pitch- > alignment requirements of DRM_FORMAT_MOD_LINEAR on various hardware. > That affects dumb buffers as well. I don't think that there's an > immediate need for a CREATE_DUMB2, but it seems worth to keep in mind. Yes, the current CREATE_DUMB can't cover all the hardware. We do need CREATE_DUMB2, sooner or later. I just hope we can define and document a set of rules that allows using CREATE_DUMB for the cases where it sensibly works (and is already being used). Tomi
Hi Am 15.01.25 um 13:06 schrieb Tomi Valkeinen: > Hi, > > On 15/01/2025 13:37, Thomas Zimmermann wrote: >> Hi >> >> >> Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: >> [...] >>>> 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. >> >> Great. Thanks for testing. >> >>> >>>> 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. >> >> The bpp value sets the number of bits per pixel; except for bpp==15 >> (XRGB1555), where it sets the color depth. OR bpp is the color depth; >> except for bpp==32 (XRGB8888), where it is the number of bits per >> pixel. It's therefore best to interpret it like a color-mode enum. > > Ah, right... That's horrible =). > > And I assume it's not really possible to define the bpp to mean bits > per pixel, except for a few special cases like 15? > > Why do we even really care about color depth here? We're just > allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for > XRGB1555 too? Drivers always did that, but it does not work correctly for (bpp < 8). As we already have helpers to deal with bpp, it makes sense to use them. This also aligns dumb buffers with the kernel's video= parameter, which as the same odd semantics. The fallback that uses bpp directly will hopefully be the exception. > >>> 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. >> >> I was unsure about the drm_warn() as well. It's not really wrong to >> have odd bpp values, but handing in an unknown bpp value might point >> to a user-space error. At least there should be a drm_dbg(). >> >>> >>>> 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? >> >> That would duplicate format and hardware information in user-space. Some > > But we already have that, don't we? We have drivers and userspace that > support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we > don't document how CREATE_DUMB has to be used to allocate multiplanar > subsampled buffers, so the userspace devs have to "guess". Yeah, there are constrains in the scanline and buffer alignments and orientation. And if we say that bpp==12 means NV12, it will be a problem for all other cases where bpp==12 makes sense. Best regards Thomas > >> hardware might have odd per-plane limitations that only the driver >> knows about. For example, there's another discussion on dri-devel >> about pitch- alignment requirements of DRM_FORMAT_MOD_LINEAR on >> various hardware. That affects dumb buffers as well. I don't think >> that there's an immediate need for a CREATE_DUMB2, but it seems worth >> to keep in mind. > > Yes, the current CREATE_DUMB can't cover all the hardware. We do need > CREATE_DUMB2, sooner or later. I just hope we can define and document > a set of rules that allows using CREATE_DUMB for the cases where it > sensibly works (and is already being used). > > Tomi >
Hi, On 15/01/2025 14:34, Thomas Zimmermann wrote: > Hi > > > Am 15.01.25 um 13:06 schrieb Tomi Valkeinen: >> Hi, >> >> On 15/01/2025 13:37, Thomas Zimmermann wrote: >>> Hi >>> >>> >>> Am 15.01.25 um 11:58 schrieb Tomi Valkeinen: >>> [...] >>>>> 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. >>> >>> Great. Thanks for testing. >>> >>>> >>>>> 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. >>> >>> The bpp value sets the number of bits per pixel; except for bpp==15 >>> (XRGB1555), where it sets the color depth. OR bpp is the color depth; >>> except for bpp==32 (XRGB8888), where it is the number of bits per >>> pixel. It's therefore best to interpret it like a color-mode enum. >> >> Ah, right... That's horrible =). >> >> And I assume it's not really possible to define the bpp to mean bits >> per pixel, except for a few special cases like 15? >> >> Why do we even really care about color depth here? We're just >> allocating memory. Doesn't DIV_ROUND_UP(args->bpp, SZ_8) work fine for >> XRGB1555 too? > > Drivers always did that, but it does not work correctly for (bpp < 8). > As we already have helpers to deal with bpp, it makes sense to use > them. This also aligns dumb buffers with the kernel's video= parameter, > which as the same odd semantics. The fallback that uses bpp directly > will hopefully be the exception. Hmm, well... If we had a 64-bit RGB format in the list of "legacy fb formats", I wouldn't have noticed anything. And if I would just use 32 as the bpp, and adjust width accordingly, it would also have worked. So, I expect the fallback to be an exception, And by working I mean that I can run my apps fine, but the internal operation would sure be odd: allocating any YUV buffer will cause drm_mode_size_dumb() to get an RGB format from drm_driver_color_mode_format(), and get a drm_format_info_min_pitch() for an RGB format. >>>> 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. >>> >>> I was unsure about the drm_warn() as well. It's not really wrong to >>> have odd bpp values, but handing in an unknown bpp value might point >>> to a user-space error. At least there should be a drm_dbg(). >>> >>>> >>>>> 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? >>> >>> That would duplicate format and hardware information in user-space. Some >> >> But we already have that, don't we? We have drivers and userspace that >> support, say, NV12 via dumb buffers. But (correct me if I'm wrong) we >> don't document how CREATE_DUMB has to be used to allocate multiplanar >> subsampled buffers, so the userspace devs have to "guess". > > Yeah, there are constrains in the scanline and buffer alignments and > orientation. And if we say that bpp==12 means NV12, it will be a problem > for all other cases where bpp==12 makes sense. I feel I still don't quite understand. Can't we define and document CREATE_DUMB like this: If (bpp < 8 || is_power_of_two(bpp)) bpp means bitsperpixel pitch is args->width * args->bpp / 8, aligned up to driver-specific-align else bpp is a legacy parameter, and we deal with it case by case. list the cases and what they mean And describe that when allocating subsampled buffers, the caller must adjust the width and height accordingly. And that the bpp and width can also refer to pixel groups. Or if the currently existing code prevents the above for 16 and 32 bpps, how about defining that any non-RGB or not-simple buffer has to be allocated with bpp=8, and the userspace has to align the pitch correctly according to the format and platform's hw restrictions? Tomi
Hi Am 15.01.25 um 14:33 schrieb Tomi Valkeinen: [...] >> Yeah, there are constrains in the scanline and buffer alignments and >> orientation. And if we say that bpp==12 means NV12, it will be a >> problem for all other cases where bpp==12 makes sense. > > I feel I still don't quite understand. Can't we define and document > CREATE_DUMB like this: > > If (bpp < 8 || is_power_of_two(bpp)) > bpp means bitsperpixel > pitch is args->width * args->bpp / 8, aligned up to > driver-specific-align > else > bpp is a legacy parameter, and we deal with it case by case. > list the cases and what they mean > > And describe that when allocating subsampled buffers, the caller must > adjust the width and height accordingly. And that the bpp and width > can also refer to pixel groups. > > Or if the currently existing code prevents the above for 16 and 32 > bpps, how about defining that any non-RGB or not-simple buffer has to > be allocated with bpp=8, and the userspace has to align the pitch > correctly according to the format and platform's hw restrictions? What if a hardware requires certain per-format alignments? Or requires certain alignments for each plane? Or only supports tile modes? Or has strict limits on the maximum buffer size? It is not possible to encode all this in a simple 32-bit value. So user-space code has to be aware of all this and tweak bpp-based allocation to make it work. Obviously you can use the current UAPI for your use case. It's just not optimal or future proof. Best regards Thomas > > > Tomi >
On 15/01/2025 15:45, Thomas Zimmermann wrote: > Hi > > > Am 15.01.25 um 14:33 schrieb Tomi Valkeinen: > [...] >>> Yeah, there are constrains in the scanline and buffer alignments and >>> orientation. And if we say that bpp==12 means NV12, it will be a >>> problem for all other cases where bpp==12 makes sense. >> >> I feel I still don't quite understand. Can't we define and document >> CREATE_DUMB like this: >> >> If (bpp < 8 || is_power_of_two(bpp)) >> bpp means bitsperpixel >> pitch is args->width * args->bpp / 8, aligned up to driver- >> specific-align >> else >> bpp is a legacy parameter, and we deal with it case by case. >> list the cases and what they mean >> >> And describe that when allocating subsampled buffers, the caller must >> adjust the width and height accordingly. And that the bpp and width >> can also refer to pixel groups. >> >> Or if the currently existing code prevents the above for 16 and 32 >> bpps, how about defining that any non-RGB or not-simple buffer has to >> be allocated with bpp=8, and the userspace has to align the pitch >> correctly according to the format and platform's hw restrictions? > > What if a hardware requires certain per-format alignments? Or requires > certain alignments for each plane? Or only supports tile modes? Or has > strict limits on the maximum buffer size? > > It is not possible to encode all this in a simple 32-bit value. So user- > space code has to be aware of all this and tweak bpp-based allocation to > make it work. Obviously you can use the current UAPI for your use case. > It's just not optimal or future proof. No disagreement there, we need CREATE_DUMB2. My point is that we have the current UAPI, and we have userspace using it, but we don't have clear rules what the ioctl does with specific parameters, and we don't document how it has to be used. Perhaps the situation is bad, and all we can really say is that CREATE_DUMB only works for use with simple RGB formats, and the behavior for all other formats is platform specific. But I think even that would be valuable in the UAPI docs. Thinking about this, I wonder if this change is good for omapdrm or xilinx (probably other platforms too that support non-simple non-RGB formats via dumb buffers): without this patch, in both drivers, the pitch calculations just take the bpp as bit-per-pixels, align it up, and that's it. With this patch we end up using drm_driver_color_mode_format(), and aligning buffers according to RGB formats figured out via heuristics. It does happen to work, for the formats I tested, but it sounds like something that might easily not work, as it's doing adjustments based on wrong format. Should we have another version of drm_mode_size_dumb() which just calculates using the bpp, without the drm_driver_color_mode_format() path? Or does the drm_driver_color_mode_format() path provide some value for the drivers that do not currently do anything similar? Tomi
On Wed, 15 Jan 2025 at 14:20, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > No disagreement there, we need CREATE_DUMB2. > > My point is that we have the current UAPI, and we have userspace using > it, but we don't have clear rules what the ioctl does with specific > parameters, and we don't document how it has to be used. > > Perhaps the situation is bad, and all we can really say is that > CREATE_DUMB only works for use with simple RGB formats, and the behavior > for all other formats is platform specific. But I think even that would > be valuable in the UAPI docs. Yeah, CREATE_DUMB only works for use with simple RGB formats in a linear layout. Not monochrome or YUV or tiled or displayed rotated or whatever. If it happens to accidentally work for other uses, that's fine, but it's not generically reliable for anything other than simple linear RGB. It's intended to let you do splash screens, consoles, recovery password entries, and software-rendered compositors if you really want. Anything more than that isn't 'dumb'. Cheers, Daniel
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(-)