Re: [PATCH v2 23/25] drm/xe: Compute dumb-buffer sizes with drm_mode_size_dumb()

[Matthew Auld <matthew.auld@intel.com>]

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,
>>
>