Re: [PATCH 4/6] drm/msm: fbdev: Calculate buffer geometry with format helpers

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 22.04.26 um 02:07 schrieb Dmitry Baryshkov:
> On Tue, Apr 21, 2026 at 02:51:17PM +0200, Thomas Zimmermann wrote:
>> Replace the geometry and size calculation in msm's fbdev emulation
>> with DRM format helpers. This consists of a 4CC lookup from the fbdev
>> parameters, format lookup, pitch calculation and size calculation.
>> Then allocate the GEM buffer object for the framebuffer memory from
>> the calculated size.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/msm/msm_drv.h   |  7 -------
>>   drivers/gpu/drm/msm/msm_fbdev.c | 26 ++++++++++++--------------
>>   2 files changed, 12 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index b3173fa4d222..00c50d7437d6 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -509,13 +509,6 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
>>   #define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
>>   #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
>>   
>> -static inline int align_pitch(int width, int bpp)
>> -{
>> -	int bytespp = (bpp + 7) / 8;
>> -	/* adreno needs pitch aligned to 32 pixels: */
>> -	return bytespp * ALIGN(width, 32);
>> -}
>> -
>>   /* for the generated headers: */
>>   #define INVALID_IDX(idx) ({BUG(); 0;})
>>   #define fui(x)                ({BUG(); 0;})
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
>> index 1eb0368a6d07..5532c5779f17 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -95,23 +95,25 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>>   	struct fb_info *fbi = helper->info;
>>   	struct drm_mode_fb_cmd2 mode_cmd = { };
>>   	struct drm_framebuffer *fb = NULL;
>> +	const struct drm_format_info *format;
>> +	u32 fourcc, pitch;
>> +	u64 size;
>>   	struct drm_gem_object *bo;
>>   	uint64_t paddr;
>> -	uint32_t format;
>> -	int ret, pitch;
>> -	int size;
>> -
>> -	format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
>> +	int ret;
>>   
>>   	DBG("create fbdev: %dx%d@%d (%dx%d)", sizes->surface_width,
>>   			sizes->surface_height, sizes->surface_bpp,
>>   			sizes->fb_width, sizes->fb_height);
>>   
>> -	pitch = align_pitch(sizes->surface_width, sizes->surface_bpp);
>> +	fourcc = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth);
>> +	format = drm_get_format_info(dev, fourcc, DRM_FORMAT_MOD_LINEAR);
>> +	/* adreno needs pitch aligned to 32 pixels: */
>> +	pitch = drm_format_info_min_pitch(format, 0, ALIGN(sizes->surface_width, 32));
>> +	size = ALIGN(pitch * sizes->surface_height, PAGE_SIZE);
> Hmm, why do we need to align the size to the PAGE_SIZE? I might be
> missing a point, but it would be nice to mention it in the commit
> message.

This buffer will be mmap'ed to user space and fbdev's mmap of course 
requires page-aligned sizes.  Hence the alignment here. Msm's memory 
manager should take care of this, but it doesn't hurt to request it 
specifically.

On a more general note: while we use this GEM buffer object for backing 
the DRM framebuffer, it is better to think of it as fbdev framebuffer 
memory.

I'll update the commit description accordingly.

Best regards
Thomas


>
> Other than that, LGTM.
>

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