| Message ID | 20250109150310.219442-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm/dumb-buffers: Fix and improve buffer-size calculation | expand |
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 Thu, Jan 9, 2025 at 10:03 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch > and buffer size. No alignment required. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Zack Rusin <zack.rusin@broadcom.com> > Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 21 ++++----------------- > 1 file changed, 4 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index 5721c74da3e0..a3fbd4148f73 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -34,6 +34,7 @@ > #include "vmw_surface_cache.h" > #include "device_include/svga3d_surfacedefs.h" > > +#include <drm/drm_dumb_buffers.h> > #include <drm/ttm/ttm_placement.h> > > #define SVGA3D_FLAGS_64(upper32, lower32) (((uint64_t)upper32 << 32) | lower32) > @@ -2291,23 +2292,9 @@ int vmw_dumb_create(struct drm_file *file_priv, > * contents is going to be rendered guest side. > */ > if (!dev_priv->has_mob || !vmw_supports_3d(dev_priv)) { > - int cpp = DIV_ROUND_UP(args->bpp, 8); > - > - switch (cpp) { > - case 1: /* DRM_FORMAT_C8 */ > - case 2: /* DRM_FORMAT_RGB565 */ > - case 4: /* DRM_FORMAT_XRGB8888 */ > - break; > - default: > - /* > - * Dumb buffers don't allow anything else. > - * This is tested via IGT's dumb_buffers > - */ > - return -EINVAL; > - } > - > - args->pitch = args->width * cpp; > - args->size = ALIGN(args->pitch * args->height, PAGE_SIZE); > + ret = drm_mode_size_dumb(dev, args, 0, 0); > + if (ret) > + return ret; > > ret = vmw_gem_object_create_with_handle(dev_priv, file_priv, > args->size, &args->handle, > -- > 2.47.1 > Ah, that's great. Thanks! Reviewed-by: Zack Rusin <zack.rusin@broadcom.com> z
Dumb-buffer pitch and size is specified by width, height, bits-per-pixel plus various hardware-specific alignments. The calculation of these values is inconsistent and duplicated among drivers. The results for formats with bpp < 8 are incorrect. This series fixes this for most drivers. Default scanline pitch and buffer size are now calculated with the existing 4CC helpers. There is a new helper drm_mode_size_dumb() that calculates scanline pitch and buffer size according to driver requirements. The series fixes the common GEM implementations for DMA, SHMEM and VRAM. It further changes most implementations of dumb_create to use the new helper. A small number of drivers has more complicated calculations and will be updated by a later patches. v2: - rewrite series - convert many individual drivers besides the shared GEM helpers Thomas Zimmermann (25): drm/dumb-buffers: Sanitize output on errors drm/dumb-buffers: Provide helper to set pitch and size drm/gem-dma: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/gem-shmem: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/gem-vram: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/armada: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/exynos: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/gma500: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/hibmc: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/imx/ipuv3: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/loongson: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/mediatek: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/msm: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/nouveau: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/omapdrm: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/qxl: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/renesas/rcar-du: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/renesas/rz-du: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/rockchip: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/tegra: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/virtio: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/vmwgfx: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/xen: Compute dumb-buffer sizes with drm_mode_size_dumb() drm/xlnx: Compute dumb-buffer sizes with drm_mode_size_dumb() drivers/gpu/drm/armada/armada_gem.c | 16 +-- drivers/gpu/drm/drm_dumb_buffers.c | 133 ++++++++++++++++-- drivers/gpu/drm/drm_gem_dma_helper.c | 7 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 16 +-- drivers/gpu/drm/drm_gem_vram_helper.c | 89 +++--------- drivers/gpu/drm/exynos/exynos_drm_gem.c | 8 +- drivers/gpu/drm/gma500/gem.c | 21 +-- .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 25 +++- drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 29 +++- drivers/gpu/drm/loongson/lsdc_gem.c | 29 ++-- drivers/gpu/drm/mediatek/mtk_gem.c | 13 +- drivers/gpu/drm/msm/msm_gem.c | 27 +++- drivers/gpu/drm/nouveau/nouveau_display.c | 7 +- drivers/gpu/drm/omapdrm/omap_gem.c | 15 +- drivers/gpu/drm/qxl/qxl_dumb.c | 17 ++- drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 7 +- drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c | 7 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 12 +- drivers/gpu/drm/tegra/gem.c | 8 +- drivers/gpu/drm/virtio/virtgpu_gem.c | 11 +- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 21 +-- drivers/gpu/drm/xe/xe_bo.c | 8 +- drivers/gpu/drm/xen/xen_drm_front.c | 7 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 7 +- include/drm/drm_dumb_buffers.h | 14 ++ include/drm/drm_gem_vram_helper.h | 6 - 26 files changed, 333 insertions(+), 227 deletions(-) create mode 100644 include/drm/drm_dumb_buffers.h base-commit: f06efdfad9d0e9f5cb74404ac98e1a5b3b246567 prerequisite-patch-id: 0aa359f6144c4015c140c8a6750be19099c676fb prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24 prerequisite-patch-id: cbc453ee02fae02af22fbfdce56ab732c7a88c36