[PATCH v3 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Framebuffer memory is allocated via vzalloc() 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.
v2:
- refer to vzalloc() in commit message (Javier)
Signed-off-by: Thomas Zimmermann
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # v6.4+
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Zack Rusin
Reviewed-by: Sui Jingfeng
Tested-by: Sui Jingfeng
Acked-by: Maxime Ripard
---
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 be357f926faec..97e579c33d84a 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
Re: [PATCH v2 01/43] drm/fbdev-generic: Do not set physical framebuffer address
On Wed, 10 Apr 2024 15:01:57 +0200, Thomas Zimmermann wrote: > Framebuffer memory is allocated via vzalloc() 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 > > [ ... ] Acked-by: Maxime Ripard Thanks! Maxime
[PATCH v2 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Framebuffer memory is allocated via vzalloc() 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.
v2:
- refer to vzalloc() in commit message (Javier)
Signed-off-by: Thomas Zimmermann
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # v6.4+
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Zack Rusin
Reviewed-by: Sui Jingfeng
Tested-by: Sui Jingfeng
---
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 be357f926faec..97e579c33d84a 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
Re: [01/43] drm/fbdev-generic: Do not set physical framebuffer address
Hi,
On 2024/3/12 23:44, Thomas Zimmermann 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
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # v6.4+
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Zack Rusin
Reviewed-by: Sui Jingfeng
Tested-by: Sui Jingfeng
--
Best regards,
Sui
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Maxime Ripard 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
>> > 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
>> > > Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
>> > > Cc: Thomas Zimmermann
>> > > Cc: Javier Martinez Canillas
>> > > Cc: Zack Rusin
>> > > Cc: Maarten Lankhorst
>> > > Cc: Maxime Ripard
>> > > Cc: # 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
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
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
> > 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
> > > Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> > > Cc: Thomas Zimmermann
> > > Cc: Javier Martinez Canillas
> > > Cc: Zack Rusin
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: # 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.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Hi
Am 17.03.24 um 13:43 schrieb Javier Martinez Canillas:
Thomas Zimmermann 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
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # 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
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)
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Hi
Am 18.03.24 um 03:35 schrieb Zack Rusin:
On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann 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
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # 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
z
--
--
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)
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
On Tue, Mar 12, 2024 at 11:48 AM Thomas Zimmermann 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
> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Zack Rusin
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: # 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...
Reviewed-by: Zack Rusin
z
Re: [PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
Thomas Zimmermann 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.
> 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 ?
> Signed-off-by: Thomas Zimmermann
> Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Zack Rusin
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: # 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
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.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
[PATCH 01/43] drm/fbdev-generic: Do not set physical framebuffer address
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
Fixes: a5b44c4adb16 ("drm/fbdev-generic: Always use shadow buffering")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Zack Rusin
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: # 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
