| Message ID | 20250311155120.442633-4-tzimmermann@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/dumb-buffers: Fix and improve buffer-size calculation | expand |
Hi, On 11/03/2025 17:47, 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. > > Push the current calculation into the only direct caller imx. Imx's > hardware requires the framebuffer width to be aligned to 8. The > driver's current approach is actually incorrect, as it only guarantees > this implicitly and requires bpp to be a multiple of 8 already. A > later commit will fix this problem by aligning the scanline pitch > such that an aligned width still fits into each scanline's memory. > > A number of other drivers are build on top of gem-dma helpers and > implement their own dumb-buffer allocation. These drivers invoke > drm_gem_dma_dumb_create_internal(), which is not affected by this > commit. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_gem_dma_helper.c | 7 +++++-- > drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c > index b7f033d4352a..49be9b033610 100644 > --- a/drivers/gpu/drm/drm_gem_dma_helper.c > +++ b/drivers/gpu/drm/drm_gem_dma_helper.c > @@ -20,6 +20,7 @@ > #include <drm/drm.h> > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > +#include <drm/drm_dumb_buffers.h> > #include <drm/drm_gem_dma_helper.h> > #include <drm/drm_vma_manager.h> > > @@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv, > struct drm_mode_create_dumb *args) > { > struct drm_gem_dma_object *dma_obj; > + int ret; > > - args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > - args->size = args->pitch * args->height; > + ret = drm_mode_size_dumb(drm, args, SZ_8, 0); > + if (ret) > + return ret; > > dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size, > &args->handle); > diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c > index ec5fd9a01f1e..e7025df7b978 100644 > --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c > @@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv, > int ret; > > args->width = ALIGN(width, 8); > + args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > + args->size = args->pitch * args->height; > > ret = drm_gem_dma_dumb_create(file_priv, drm, args); > if (ret) Won't the pitch and size just be overwritten by the drm_gem_dma_dumb_create() call? Tomi
Hi Am 12.06.25 um 10:43 schrieb Tomi Valkeinen: > Hi, > > On 11/03/2025 17:47, 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. >> >> Push the current calculation into the only direct caller imx. Imx's >> hardware requires the framebuffer width to be aligned to 8. The >> driver's current approach is actually incorrect, as it only guarantees >> this implicitly and requires bpp to be a multiple of 8 already. A >> later commit will fix this problem by aligning the scanline pitch >> such that an aligned width still fits into each scanline's memory. >> >> A number of other drivers are build on top of gem-dma helpers and >> implement their own dumb-buffer allocation. These drivers invoke >> drm_gem_dma_dumb_create_internal(), which is not affected by this >> commit. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_gem_dma_helper.c | 7 +++++-- >> drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++ >> 2 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c >> index b7f033d4352a..49be9b033610 100644 >> --- a/drivers/gpu/drm/drm_gem_dma_helper.c >> +++ b/drivers/gpu/drm/drm_gem_dma_helper.c >> @@ -20,6 +20,7 @@ >> #include <drm/drm.h> >> #include <drm/drm_device.h> >> #include <drm/drm_drv.h> >> +#include <drm/drm_dumb_buffers.h> >> #include <drm/drm_gem_dma_helper.h> >> #include <drm/drm_vma_manager.h> >> >> @@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv, >> struct drm_mode_create_dumb *args) >> { >> struct drm_gem_dma_object *dma_obj; >> + int ret; >> >> - args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); >> - args->size = args->pitch * args->height; >> + ret = drm_mode_size_dumb(drm, args, SZ_8, 0); >> + if (ret) >> + return ret; >> >> dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size, >> &args->handle); >> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c >> index ec5fd9a01f1e..e7025df7b978 100644 >> --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c >> +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c >> @@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv, >> int ret; >> >> args->width = ALIGN(width, 8); >> + args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); >> + args->size = args->pitch * args->height; >> >> ret = drm_gem_dma_dumb_create(file_priv, drm, args); >> if (ret) > Won't the pitch and size just be overwritten by the > drm_gem_dma_dumb_create() call? Right, looks like it. Thanks for looking over this. The call to drm_gem_dma_dumb_create() needs to be changed drm_gem_dma_dumb_create_internal(). The latter doesn't modify the arguments besides some sanity checks. BTW patch 10 cleans up imx entirely. Best regards Thomas > > Tomi >
diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/drm_gem_dma_helper.c index b7f033d4352a..49be9b033610 100644 --- a/drivers/gpu/drm/drm_gem_dma_helper.c +++ b/drivers/gpu/drm/drm_gem_dma_helper.c @@ -20,6 +20,7 @@ #include <drm/drm.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> +#include <drm/drm_dumb_buffers.h> #include <drm/drm_gem_dma_helper.h> #include <drm/drm_vma_manager.h> @@ -304,9 +305,11 @@ int drm_gem_dma_dumb_create(struct drm_file *file_priv, struct drm_mode_create_dumb *args) { struct drm_gem_dma_object *dma_obj; + int ret; - args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); - args->size = args->pitch * args->height; + ret = drm_mode_size_dumb(drm, args, SZ_8, 0); + if (ret) + return ret; dma_obj = drm_gem_dma_create_with_handle(file_priv, drm, args->size, &args->handle); diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c index ec5fd9a01f1e..e7025df7b978 100644 --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c @@ -145,6 +145,8 @@ static int imx_drm_dumb_create(struct drm_file *file_priv, int ret; args->width = ALIGN(width, 8); + args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); + args->size = args->pitch * args->height; ret = drm_gem_dma_dumb_create(file_priv, drm, args); if (ret)
Call drm_mode_size_dumb() to compute dumb-buffer scanline pitch and buffer size. Align the pitch to a multiple of 8. Push the current calculation into the only direct caller imx. Imx's hardware requires the framebuffer width to be aligned to 8. The driver's current approach is actually incorrect, as it only guarantees this implicitly and requires bpp to be a multiple of 8 already. A later commit will fix this problem by aligning the scanline pitch such that an aligned width still fits into each scanline's memory. A number of other drivers are build on top of gem-dma helpers and implement their own dumb-buffer allocation. These drivers invoke drm_gem_dma_dumb_create_internal(), which is not affected by this commit. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_gem_dma_helper.c | 7 +++++-- drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)