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

[Javier Martinez Canillas <javierm@redhat.com>]

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat