Hi

Am 17.03.24 um 13:43 schrieb Javier Martinez Canillas:

Thomas Zimmermann [off-list ref] writes:

Hello Thomas,

quoted

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.

quoted

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.

quoted

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Zack Rusin <redacted>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: <redacted> # 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@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)