| Message ID | 20240507120422.25492-9-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Provide common fbdev client helpers | expand |
On Tue, May 07, 2024 at 01:58:29PM +0200, Thomas Zimmermann wrote: > Implement struct drm_client_funcs.unregister with the helpers > drm_fbdev_helper_client_unregister() and remove the custom code > from the driver. The generic helper is equivalent in functionality. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/i915/display/intel_fbdev.c | 21 ++------------------- > 1 file changed, 2 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c > index bda702c2cab8e..f1b4543bffc02 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c > @@ -38,7 +38,6 @@ > #include <linux/vga_switcheroo.h> > > #include <drm/drm_crtc.h> > -#include <drm/drm_crtc_helper.h> > #include <drm/drm_fb_helper.h> > #include <drm/drm_fourcc.h> > #include <drm/drm_gem_framebuffer_helper.h> > @@ -580,25 +579,9 @@ static int intel_fbdev_restore_mode(struct drm_i915_private *dev_priv) > } > > /* > - * Fbdev client and struct drm_client_funcs > + * struct drm_client_funcs > */ > > -static void intel_fbdev_client_unregister(struct drm_client_dev *client) > -{ > - struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); > - struct drm_device *dev = fb_helper->dev; > - struct pci_dev *pdev = to_pci_dev(dev->dev); > - > - if (fb_helper->info) { > - vga_switcheroo_client_fb_set(pdev, NULL); > - drm_fb_helper_unregister_info(fb_helper); > - } else { > - drm_fb_helper_unprepare(fb_helper); > - drm_client_release(&fb_helper->client); The only real difference is that these 2 calls are inverted in the new drm_fbdev_helper_client_unregister. I feel that the releasing after the unprepare sounds more logical, but if there's no actual issue with that and it is working for everybody, let's do that. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (to get through drm-misc with everything else if you prefer) > - kfree(fb_helper); > - } > -} > - > static int intel_fbdev_client_restore(struct drm_client_dev *client) > { > struct drm_i915_private *dev_priv = to_i915(client->dev); > @@ -644,7 +627,7 @@ static int intel_fbdev_client_hotplug(struct drm_client_dev *client) > > static const struct drm_client_funcs intel_fbdev_client_funcs = { > .owner = THIS_MODULE, > - .unregister = intel_fbdev_client_unregister, > + .unregister = drm_fbdev_helper_client_unregister, > .restore = intel_fbdev_client_restore, > .hotplug = intel_fbdev_client_hotplug, > }; > -- > 2.44.0 >
Hi Am 07.05.24 um 14:25 schrieb Rodrigo Vivi: > On Tue, May 07, 2024 at 01:58:29PM +0200, Thomas Zimmermann wrote: >> Implement struct drm_client_funcs.unregister with the helpers >> drm_fbdev_helper_client_unregister() and remove the custom code >> from the driver. The generic helper is equivalent in functionality. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/i915/display/intel_fbdev.c | 21 ++------------------- >> 1 file changed, 2 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c >> index bda702c2cab8e..f1b4543bffc02 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c >> @@ -38,7 +38,6 @@ >> #include <linux/vga_switcheroo.h> >> >> #include <drm/drm_crtc.h> >> -#include <drm/drm_crtc_helper.h> >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> @@ -580,25 +579,9 @@ static int intel_fbdev_restore_mode(struct drm_i915_private *dev_priv) >> } >> >> /* >> - * Fbdev client and struct drm_client_funcs >> + * struct drm_client_funcs >> */ >> >> -static void intel_fbdev_client_unregister(struct drm_client_dev *client) >> -{ >> - struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); >> - struct drm_device *dev = fb_helper->dev; >> - struct pci_dev *pdev = to_pci_dev(dev->dev); >> - >> - if (fb_helper->info) { >> - vga_switcheroo_client_fb_set(pdev, NULL); >> - drm_fb_helper_unregister_info(fb_helper); >> - } else { >> - drm_fb_helper_unprepare(fb_helper); >> - drm_client_release(&fb_helper->client); > The only real difference is that these 2 calls are inverted in the new > drm_fbdev_helper_client_unregister. The condition in this if statement is different for some drivers, but not i915. The setup code first does a _prepare and then an _init+_register, hence the _release goes first and then comes the _unprepare. > > I feel that the releasing after the unprepare sounds more logical, > but if there's no actual issue with that and it is working for > everybody, let's do that. It should make no difference in practice, but doing the release first is the inverse of the setup; hence conceptually cleaner. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > (to get through drm-misc with everything else if you prefer) Thanks. Best regards Thomas > >> - kfree(fb_helper); >> - } >> -} >> - >> static int intel_fbdev_client_restore(struct drm_client_dev *client) >> { >> struct drm_i915_private *dev_priv = to_i915(client->dev); >> @@ -644,7 +627,7 @@ static int intel_fbdev_client_hotplug(struct drm_client_dev *client) >> >> static const struct drm_client_funcs intel_fbdev_client_funcs = { >> .owner = THIS_MODULE, >> - .unregister = intel_fbdev_client_unregister, >> + .unregister = drm_fbdev_helper_client_unregister, >> .restore = intel_fbdev_client_restore, >> .hotplug = intel_fbdev_client_hotplug, >> }; >> -- >> 2.44.0 >>
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c index bda702c2cab8e..f1b4543bffc02 100644 --- a/drivers/gpu/drm/i915/display/intel_fbdev.c +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c @@ -38,7 +38,6 @@ #include <linux/vga_switcheroo.h> #include <drm/drm_crtc.h> -#include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> @@ -580,25 +579,9 @@ static int intel_fbdev_restore_mode(struct drm_i915_private *dev_priv) } /* - * Fbdev client and struct drm_client_funcs + * struct drm_client_funcs */ -static void intel_fbdev_client_unregister(struct drm_client_dev *client) -{ - struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); - struct drm_device *dev = fb_helper->dev; - struct pci_dev *pdev = to_pci_dev(dev->dev); - - if (fb_helper->info) { - vga_switcheroo_client_fb_set(pdev, NULL); - drm_fb_helper_unregister_info(fb_helper); - } else { - drm_fb_helper_unprepare(fb_helper); - drm_client_release(&fb_helper->client); - kfree(fb_helper); - } -} - static int intel_fbdev_client_restore(struct drm_client_dev *client) { struct drm_i915_private *dev_priv = to_i915(client->dev); @@ -644,7 +627,7 @@ static int intel_fbdev_client_hotplug(struct drm_client_dev *client) static const struct drm_client_funcs intel_fbdev_client_funcs = { .owner = THIS_MODULE, - .unregister = intel_fbdev_client_unregister, + .unregister = drm_fbdev_helper_client_unregister, .restore = intel_fbdev_client_restore, .hotplug = intel_fbdev_client_hotplug, };
Implement struct drm_client_funcs.unregister with the helpers drm_fbdev_helper_client_unregister() and remove the custom code from the driver. The generic helper is equivalent in functionality. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/i915/display/intel_fbdev.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)