[PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address

[Thomas Zimmermann]

Hi

Am 17.03.24 um 13:43 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> writes:
>
> Hello Thomas,
>
>> Framebuffer memory is allocated via vmalloc() from non-contiguous
> It's vmalloc() true, but through vzmalloc() so I would mention that
> function instead in the commit message.

Ok.

>
>> physical pages. The physical framebuffer start address is therefore
>> meaningless. Do not set it.
>>
>> The value is not used within the kernel and only exported to userspace
>> on dedicated ARM configs. No functional change is expected.
>>
> How's that info used? Does user-space assumes that the whole memory range
> is contiguous in physical memory or just cares about the phyisical start
> address ?

I assume that it is supposed to refer to contiguous memory. There's the 
corresponding field smem_len, which refers to the full memory.

>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Javier Martinez Canillas <javierm at redhat.com>
>> Cc: Zack Rusin <zackr at vmware.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: <stable at vger.kernel.org> # v6.4+
>> ---
>>   drivers/gpu/drm/drm_fbdev_generic.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>> index d647d89764cb9..b4659cd6285ab 100644
>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>> @@ -113,7 +113,6 @@ static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
>>   	/* screen */
>>   	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>>   	info->screen_buffer = screen_buffer;
>> -	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
>>   	info->fix.smem_len = screen_size;
>>   
> Makes sense:
>
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>
> What about drivers/gpu/drm/drm_fb_helper.c btw? Since the memory range
> allocated may not be physically contiguous if a platform uses an IOMMU ?
>
> Asking because I don't really know how these exported values are used...
> I just know that is when the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM is enabled.

The value of smem_start is used in the fbdev code in only two places : 
deferred I/O [1] and devfile I/O [2]. For the former, patch 5 of this 
series adds an additional test. The latter case is only relevant for 
framebuffers in I/O memory space. But that does not affect 
fbdev-generic, which has the shadow buffer in main memory. Some 
driver-internal fbdev code sets smem_length, such as in gma500, but 
these drivers are special anyway.

For what userspace does with this value IDK. But it can't be much, as 
we've had smem_start cleared for years.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/video/fbdev/core/fb_defio.c#L34
[2] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/video/fbdev/core/fb_io_fops.c#L143

>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)