Re: [PATCH v6 03/25] drm/gem-dma: Compute dumb-buffer sizes with drm_mode_size_dumb()

[<Ludovic.Desroches@xxxxxxxxxxxxx>]

On 11/25/25 16:03, Thomas Zimmermann wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> Hi
> 
> Am 25.11.25 um 15:39 schrieb Ludovic.Desroches@xxxxxxxxxxxxx:
>> On 8/21/25 10:17, 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.
> 
> I missed a chance to explain it here. :/

I was wondering if it was 8-bits or 8-bytes.

> 
>>>
>>> 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.
>>>
>>> v5:
>>> - avoid reset of arguments (Tomi)
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>> ---
>>>    drivers/gpu/drm/drm_gem_dma_helper.c     | 7 +++++--
>>>    drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 4 +++-
>>>    2 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_dma_helper.c b/drivers/gpu/drm/ 
>>> drm_gem_dma_helper.c
>>> index 4f0320df858f..ab1a70b1d6f1 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;
>> Hi,
>>
>> Was it intentional for this change to alter the pitch?
> 
> Most hardware does not need the pitch to be of a certain alignment. But
> these buffers are possibly shared with other hardware, which sometimes
> needs alignment to certain values. Using SZ_8 improves compatible with
> that hardware.
> 
>>
>> The alignment requirement has been updated—from 8-bit alignment to
>> 64-bit alignment. Since the pitch is expressed in bytes, we should pass
>> SZ_1 instead of SZ_8 for hw_patch_align.
>>
>> For example, for an 850×480 framebuffer at 16 bpp, the pitch should be
>> 1700 bytes. With the new alignment, the pitch becomes 1704 bytes.
> 
> Many display modes have an 8-byte alignment in their width. 850 pixels
> is somewhat of an exception.

Indeed, but overlay-type planes are often used to display windows that 
are not the same size as the screen, and therefore potentially not on 
8-bytes aligned.

> 
>>
>> Please let me know if you’d like me to submit a fix.
> 
> Do you see a bug with your hardware? Unless this creates a real problem,
> I'd like to keep it as it is now.

Yes, that’s how I noticed this change. I have an application that 
requests a framebuffer larger than my screen, in this case 850x480 in 
RGB565 for an 800x480 display. So I have a pitch of 1700 bytes. Now it’s 
using a pitch of 1704, which shifts my lines.

Regards,
Ludovic

> 
> Best regards
> Thomas
> 
>>
>> Regards,
>> Ludovic
>>
>>
>>>
>>>      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..af4a30311e18 100644
>>> --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
>>> +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
>>> @@ -145,8 +145,10 @@ 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);
>>> +    ret = drm_gem_dma_dumb_create_internal(file_priv, drm, args);
>>>      if (ret)
>>>              return ret;
>>>
> 
> -- 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG 
> Nürnberg)
> 
>