| Message ID | 20250109150310.219442-24-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/dumb-buffers: Fix and improve buffer-size calculation | expand |
On 09/01/2025 14:57, Thomas Zimmermann wrote: > Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch > and buffer size. Align the pitch to a multiple of 8. Align the > buffer size according to hardware requirements. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/xe/xe_bo.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index e6c896ad5602..d75e3c39ab14 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c > @@ -8,6 +8,7 @@ > #include <linux/dma-buf.h> > > #include <drm/drm_drv.h> > +#include <drm/drm_dumb_buffers.h> > #include <drm/drm_gem_ttm_helper.h> > #include <drm/drm_managed.h> > #include <drm/ttm/ttm_device.h> > @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file *file_priv, > struct xe_device *xe = to_xe_device(dev); > struct xe_bo *bo; > uint32_t handle; > - int cpp = DIV_ROUND_UP(args->bpp, 8); > int err; > u32 page_size = max_t(u32, PAGE_SIZE, > xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K); > > - args->pitch = ALIGN(args->width * cpp, 64); > - args->size = ALIGN(mul_u32_u32(args->pitch, args->height), > - page_size); > + err = drm_mode_size_dumb(dev, args, SZ_64, page_size); AFAICT this looks to change the behaviour, where u64 size was technically possible and was allowed given that args->size is u64, but this helper is limiting the size to u32. Is that intentional? If so, we should probably make that clear in the commit message. > + if (err) > + return err; > > bo = xe_bo_create_user(xe, NULL, NULL, args->size, > DRM_XE_GEM_CPU_CACHING_WC,
Hi Am 09.01.25 um 17:05 schrieb Matthew Auld: > On 09/01/2025 14:57, Thomas Zimmermann wrote: >> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch >> and buffer size. Align the pitch to a multiple of 8. Align the >> buffer size according to hardware requirements. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> --- >> drivers/gpu/drm/xe/xe_bo.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c >> index e6c896ad5602..d75e3c39ab14 100644 >> --- a/drivers/gpu/drm/xe/xe_bo.c >> +++ b/drivers/gpu/drm/xe/xe_bo.c >> @@ -8,6 +8,7 @@ >> #include <linux/dma-buf.h> >> #include <drm/drm_drv.h> >> +#include <drm/drm_dumb_buffers.h> >> #include <drm/drm_gem_ttm_helper.h> >> #include <drm/drm_managed.h> >> #include <drm/ttm/ttm_device.h> >> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file >> *file_priv, >> struct xe_device *xe = to_xe_device(dev); >> struct xe_bo *bo; >> uint32_t handle; >> - int cpp = DIV_ROUND_UP(args->bpp, 8); >> int err; >> u32 page_size = max_t(u32, PAGE_SIZE, >> xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K); >> - args->pitch = ALIGN(args->width * cpp, 64); >> - args->size = ALIGN(mul_u32_u32(args->pitch, args->height), >> - page_size); >> + err = drm_mode_size_dumb(dev, args, SZ_64, page_size); > > AFAICT this looks to change the behaviour, where u64 size was > technically possible and was allowed given that args->size is u64, but > this helper is limiting the size to u32. Is that intentional? If so, > we should probably make that clear in the commit message. That's an interesting observation; thanks. The ioctl's internal checks have always limited the size to 32 bit. [1] I think it is not supposed to be larger than that. We can change the helper to support 64-bit sizes as well. Having said that, is there any use case? Dumb buffers are for software rendering only. Allocating more than a few dozen MiB seems like a mistake. Maybe we should rather limit the allowed allocation size instead? Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/drm_dumb_buffers.c#L82 > >> + if (err) >> + return err; >> bo = xe_bo_create_user(xe, NULL, NULL, args->size, >> DRM_XE_GEM_CPU_CACHING_WC, >
On 09/01/2025 16:26, Thomas Zimmermann wrote: > Hi > > > Am 09.01.25 um 17:05 schrieb Matthew Auld: >> On 09/01/2025 14:57, Thomas Zimmermann wrote: >>> Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch >>> and buffer size. Align the pitch to a multiple of 8. Align the >>> buffer size according to hardware requirements. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> --- >>> drivers/gpu/drm/xe/xe_bo.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c >>> index e6c896ad5602..d75e3c39ab14 100644 >>> --- a/drivers/gpu/drm/xe/xe_bo.c >>> +++ b/drivers/gpu/drm/xe/xe_bo.c >>> @@ -8,6 +8,7 @@ >>> #include <linux/dma-buf.h> >>> #include <drm/drm_drv.h> >>> +#include <drm/drm_dumb_buffers.h> >>> #include <drm/drm_gem_ttm_helper.h> >>> #include <drm/drm_managed.h> >>> #include <drm/ttm/ttm_device.h> >>> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file >>> *file_priv, >>> struct xe_device *xe = to_xe_device(dev); >>> struct xe_bo *bo; >>> uint32_t handle; >>> - int cpp = DIV_ROUND_UP(args->bpp, 8); >>> int err; >>> u32 page_size = max_t(u32, PAGE_SIZE, >>> xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K); >>> - args->pitch = ALIGN(args->width * cpp, 64); >>> - args->size = ALIGN(mul_u32_u32(args->pitch, args->height), >>> - page_size); >>> + err = drm_mode_size_dumb(dev, args, SZ_64, page_size); >> >> AFAICT this looks to change the behaviour, where u64 size was >> technically possible and was allowed given that args->size is u64, but >> this helper is limiting the size to u32. Is that intentional? If so, >> we should probably make that clear in the commit message. > > That's an interesting observation; thanks. The ioctl's internal checks > have always limited the size to 32 bit. [1] I think it is not supposed > to be larger than that. We can change the helper to support 64-bit sizes > as well. Ah, I missed the internal check. > > Having said that, is there any use case? Dumb buffers are for software > rendering only. Allocating more than a few dozen MiB seems like a > mistake. Maybe we should rather limit the allowed allocation size instead? Yeah, I doubt there are any real users. Given the existing internal check, limiting to u32 makes sense to me. > > Best regards > Thomas > > [1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/ > drm_dumb_buffers.c#L82 > >> >>> + if (err) >>> + return err; >>> bo = xe_bo_create_user(xe, NULL, NULL, args->size, >>> DRM_XE_GEM_CPU_CACHING_WC, >> >
diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c index e6c896ad5602..d75e3c39ab14 100644 --- a/drivers/gpu/drm/xe/xe_bo.c +++ b/drivers/gpu/drm/xe/xe_bo.c @@ -8,6 +8,7 @@ #include <linux/dma-buf.h> #include <drm/drm_drv.h> +#include <drm/drm_dumb_buffers.h> #include <drm/drm_gem_ttm_helper.h> #include <drm/drm_managed.h> #include <drm/ttm/ttm_device.h> @@ -2535,14 +2536,13 @@ int xe_bo_dumb_create(struct drm_file *file_priv, struct xe_device *xe = to_xe_device(dev); struct xe_bo *bo; uint32_t handle; - int cpp = DIV_ROUND_UP(args->bpp, 8); int err; u32 page_size = max_t(u32, PAGE_SIZE, xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K); - args->pitch = ALIGN(args->width * cpp, 64); - args->size = ALIGN(mul_u32_u32(args->pitch, args->height), - page_size); + err = drm_mode_size_dumb(dev, args, SZ_64, page_size); + if (err) + return err; bo = xe_bo_create_user(xe, NULL, NULL, args->size, DRM_XE_GEM_CPU_CACHING_WC,
Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and buffer size. Align the pitch to a multiple of 8. Align the buffer size according to hardware requirements. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/xe/xe_bo.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)