Hi

Am 17.03.24 um 13:43 schrieb Javier Martinez Canillas:

Thomas Zimmermann tzimmermann@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@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 zackr@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: stable@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@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_def... [2] https://elixir.bootlin.com/linux/v6.8/source/drivers/video/fbdev/core/fb_io_...

-- Best regards,

Javier Martinez Canillas Core Platforms Red Hat

Hi

Am 18.03.24 um 03:35 schrieb Zack Rusin:

On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann tzimmermann@suse.de wrote:

...

Framebuffer memory is allocated via vmalloc() from non-contiguous 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.

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 zackr@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: stable@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;
  
     /* deferred I/O */
  

-- 2.44.0

Good idea. I think given that drm_leak_fbdev_smem is off by default we could remove the setting of smem_start by all of the in-tree drm drivers (they all have open source userspace that won't mess around with fbdev fb) - it will be reset to 0 anyway. Actually, I wonder if we still need drm_leak_fbdev_smem at all...

All I know is that there's an embedded userspace driver that requires that setting. I don't even know which hardware.

Best regards Thomas

Reviewed-by: Zack Rusin zack.rusin@broadcom.com

z

Maxime Ripard mripard@kernel.org writes:

On Mon, Mar 18, 2024 at 08:59:01AM +0100, Thomas Zimmermann wrote:

...

Hi

Am 18.03.24 um 03:35 schrieb Zack Rusin:

...

On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann tzimmermann@suse.de wrote:

...

Framebuffer memory is allocated via vmalloc() from non-contiguous 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.

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 zackr@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: stable@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;
  
     /* deferred I/O */
  

-- 2.44.0

Good idea. I think given that drm_leak_fbdev_smem is off by default we could remove the setting of smem_start by all of the in-tree drm drivers (they all have open source userspace that won't mess around with fbdev fb) - it will be reset to 0 anyway. Actually, I wonder if we still need drm_leak_fbdev_smem at all...

All I know is that there's an embedded userspace driver that requires that setting. I don't even know which hardware.

The original Mali driver (ie, lima) used to require it, that's why we introduced it in the past.

I'm not sure if the newer versions of that driver, or if newer Mali generations (ie, panfrost and panthor) closed source driver would require it, so it might be worth removing if it's easy enough to revert.

Agreed. The DRM_FBDEV_LEAK_PHYS_SMEM symbol already depends on EXPERT and defaults to 'n', which implies that isn't enabled by most kernels AFAICT.

So dropping it and see if anyone complains sounds like a good idea to me.

Maxime