| Message ID | 20240830084456.77630-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Provide client setup helper and convert drivers | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Call vga_switcheroo_client_fb_set() with the PCI device from the > instance of struct fb_info. All fbdev clients now run these calls. > For non-PCI devices or drivers without vga-switcheroo, this does > nothing. For i915 and radeon, it allows these drivers to use a > common fbdev client. > > The device is the same as the one stored in struct drm_client and > struct drm_fb_helper, so there is no difference in behavior. Some > NULL-pointer checks are being removed, where those pointers cannot > be NULL. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index af1fe79c701d..13095d38aa42 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info); > */ > void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper) > { > - if (fb_helper && fb_helper->info) > - unregister_framebuffer(fb_helper->info); I'm not sure if we can assume these won't be NULL... AFAICT some drivers still have their own struct drm_client_funcs vtable and could potentially pass a NULL struct drm_fb_helper ? If you think that's safe to do this and the function semantics should be changed, then I think that the kernel-doc needs to be updated: - * @fb_helper: driver-allocated fbdev helper, can be NULL + * @fb_helper: driver-allocated fbdev helper, must not be NULL Other than that, the patch looks good to me: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier Am 03.09.24 um 12:18 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > Hello Thomas, > >> Call vga_switcheroo_client_fb_set() with the PCI device from the >> instance of struct fb_info. All fbdev clients now run these calls. >> For non-PCI devices or drivers without vga-switcheroo, this does >> nothing. For i915 and radeon, it allows these drivers to use a >> common fbdev client. >> >> The device is the same as the one stored in struct drm_client and >> struct drm_fb_helper, so there is no difference in behavior. Some >> NULL-pointer checks are being removed, where those pointers cannot >> be NULL. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index af1fe79c701d..13095d38aa42 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info); >> */ >> void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper) >> { >> - if (fb_helper && fb_helper->info) >> - unregister_framebuffer(fb_helper->info); > I'm not sure if we can assume these won't be NULL... AFAICT some drivers > still have their own struct drm_client_funcs vtable and could potentially > pass a NULL struct drm_fb_helper ? I did a git grep -B4 drm_fb_helper_unregister_info on drm-tip and all callers, such as [1], test fb_helper->info before calling the function. So it's safe to remove the test. [1] https://elixir.bootlin.com/linux/v6.10/source/drivers/gpu/drm/drm_fbdev_dma.c#L162 > > If you think that's safe to do this and the function semantics should be > changed, then I think that the kernel-doc needs to be updated: > > - * @fb_helper: driver-allocated fbdev helper, can be NULL > + * @fb_helper: driver-allocated fbdev helper, must not be NULL Ok. > > Other than that, the patch looks good to me: > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thanks. Best regards Thomas >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi Javier > > Am 03.09.24 um 12:18 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >> Hello Thomas, >> >>> Call vga_switcheroo_client_fb_set() with the PCI device from the >>> instance of struct fb_info. All fbdev clients now run these calls. >>> For non-PCI devices or drivers without vga-switcheroo, this does >>> nothing. For i915 and radeon, it allows these drivers to use a >>> common fbdev client. >>> >>> The device is the same as the one stored in struct drm_client and >>> struct drm_fb_helper, so there is no difference in behavior. Some >>> NULL-pointer checks are being removed, where those pointers cannot >>> be NULL. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++---- >>> 1 file changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>> index af1fe79c701d..13095d38aa42 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info); >>> */ >>> void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper) >>> { >>> - if (fb_helper && fb_helper->info) >>> - unregister_framebuffer(fb_helper->info); >> I'm not sure if we can assume these won't be NULL... AFAICT some drivers >> still have their own struct drm_client_funcs vtable and could potentially >> pass a NULL struct drm_fb_helper ? > > I did a > > git grep -B4 drm_fb_helper_unregister_info > > on drm-tip and all callers, such as [1], test fb_helper->info before > calling the function. So it's safe to remove the test. > Yes, I also noticed that all callers were already checking if is not NULL but still is more robust for the function to check it in case there is a bug in a driver. But as a said, I'm OK with dropping it as long as it is mentioned in the kernel-doc that the parameter must not be NULL. > [1] > https://elixir.bootlin.com/linux/v6.10/source/drivers/gpu/drm/drm_fbdev_dma.c#L162 > >> >> If you think that's safe to do this and the function semantics should be >> changed, then I think that the kernel-doc needs to be updated: >> >> - * @fb_helper: driver-allocated fbdev helper, can be NULL >> + * @fb_helper: driver-allocated fbdev helper, must not be NULL > > Ok. > >> >> Other than that, the patch looks good to me: >> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > Thanks. > > Best regards > Thomas > >> > > -- > -- > 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) >
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index af1fe79c701d..13095d38aa42 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info); */ void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper) { - if (fb_helper && fb_helper->info) - unregister_framebuffer(fb_helper->info); + struct fb_info *info = fb_helper->info; + struct device *dev = info->device; + + if (dev_is_pci(dev)) + vga_switcheroo_client_fb_set(to_pci_dev(dev), NULL); + unregister_framebuffer(fb_helper->info); } EXPORT_SYMBOL(drm_fb_helper_unregister_info); @@ -1615,6 +1619,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper) struct drm_client_dev *client = &fb_helper->client; struct drm_device *dev = fb_helper->dev; struct drm_fb_helper_surface_size sizes; + struct fb_info *info; int ret; ret = drm_fb_helper_find_sizes(fb_helper, &sizes); @@ -1632,9 +1637,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper) strcpy(fb_helper->fb->comm, "[fbcon]"); + info = fb_helper->info; + /* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */ - if (dev_is_pci(dev->dev)) - vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), fb_helper->info); + if (dev_is_pci(info->device)) + vga_switcheroo_client_fb_set(to_pci_dev(info->device), info); return 0; }
Call vga_switcheroo_client_fb_set() with the PCI device from the instance of struct fb_info. All fbdev clients now run these calls. For non-PCI devices or drivers without vga-switcheroo, this does nothing. For i915 and radeon, it allows these drivers to use a common fbdev client. The device is the same as the one stored in struct drm_client and struct drm_fb_helper, so there is no difference in behavior. Some NULL-pointer checks are being removed, where those pointers cannot be NULL. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_fb_helper.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)