| Message ID | 20171030153951.56269-16-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 30, 2017 at 04:39:51PM +0100, Noralf Trønnes wrote: > This driver can use drm_fb_helper_lastclose() as its .lastclose callback. > > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/staging/vboxvideo/vbox_drv.c | 2 +- > drivers/staging/vboxvideo/vbox_drv.h | 1 - > drivers/staging/vboxvideo/vbox_main.c | 12 ------------ > 3 files changed, 1 insertion(+), 14 deletions(-) > > diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c > index e18642e5027e..a4d8d7898e3d 100644 > --- a/drivers/staging/vboxvideo/vbox_drv.c > +++ b/drivers/staging/vboxvideo/vbox_drv.c > @@ -229,7 +229,7 @@ static struct drm_driver driver = { > > .load = vbox_driver_load, > .unload = vbox_driver_unload, > - .lastclose = vbox_driver_lastclose, > + .lastclose = drm_fb_helper_lastclose, > .master_set = vbox_master_set, > .master_drop = vbox_master_drop, > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h > index 4b9302703b36..7273d7e9bc9b 100644 > --- a/drivers/staging/vboxvideo/vbox_drv.h > +++ b/drivers/staging/vboxvideo/vbox_drv.h > @@ -128,7 +128,6 @@ struct vbox_private { > > int vbox_driver_load(struct drm_device *dev, unsigned long flags); > void vbox_driver_unload(struct drm_device *dev); > -void vbox_driver_lastclose(struct drm_device *dev); > > struct vbox_gem_object; > > diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c > index 80bd039fa08e..c3d756620fd5 100644 > --- a/drivers/staging/vboxvideo/vbox_main.c > +++ b/drivers/staging/vboxvideo/vbox_main.c > @@ -421,18 +421,6 @@ void vbox_driver_unload(struct drm_device *dev) > vbox_hw_fini(vbox); > } > > -/** > - * @note this is described in the DRM framework documentation. AST does not > - * have it, but we get an oops on driver unload if it is not present. > - */ > -void vbox_driver_lastclose(struct drm_device *dev) > -{ > - struct vbox_private *vbox = dev->dev_private; > - > - if (vbox->fbdev) Hm, except gma500 all the drivers have this NULL check here. I'm not sure whether that's just cargo-culted or whether that's needed, but I'm wary from slapping an Ack on the entire series as-is. I'm pretty sure we still need it for i915, but everyone else might just have copied too much. Or it predates the helper support for disabling the fbdev emulation. There's also the question that a bunch of drivers who set up fbdev forgot to add the lastclose call it seems, but that's a different story. -Daniel > - drm_fb_helper_restore_fbdev_mode_unlocked(&vbox->fbdev->helper); > -} > - > int vbox_gem_create(struct drm_device *dev, > u32 size, bool iskernel, struct drm_gem_object **obj) > { > -- > 2.14.2 >
Den 31.10.2017 11.32, skrev Daniel Vetter: > On Mon, Oct 30, 2017 at 04:39:51PM +0100, Noralf Trønnes wrote: >> This driver can use drm_fb_helper_lastclose() as its .lastclose callback. >> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/staging/vboxvideo/vbox_drv.c | 2 +- >> drivers/staging/vboxvideo/vbox_drv.h | 1 - >> drivers/staging/vboxvideo/vbox_main.c | 12 ------------ >> 3 files changed, 1 insertion(+), 14 deletions(-) >> >> diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c >> index e18642e5027e..a4d8d7898e3d 100644 >> --- a/drivers/staging/vboxvideo/vbox_drv.c >> +++ b/drivers/staging/vboxvideo/vbox_drv.c >> @@ -229,7 +229,7 @@ static struct drm_driver driver = { >> >> .load = vbox_driver_load, >> .unload = vbox_driver_unload, >> - .lastclose = vbox_driver_lastclose, >> + .lastclose = drm_fb_helper_lastclose, >> .master_set = vbox_master_set, >> .master_drop = vbox_master_drop, >> >> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h >> index 4b9302703b36..7273d7e9bc9b 100644 >> --- a/drivers/staging/vboxvideo/vbox_drv.h >> +++ b/drivers/staging/vboxvideo/vbox_drv.h >> @@ -128,7 +128,6 @@ struct vbox_private { >> >> int vbox_driver_load(struct drm_device *dev, unsigned long flags); >> void vbox_driver_unload(struct drm_device *dev); >> -void vbox_driver_lastclose(struct drm_device *dev); >> >> struct vbox_gem_object; >> >> diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c >> index 80bd039fa08e..c3d756620fd5 100644 >> --- a/drivers/staging/vboxvideo/vbox_main.c >> +++ b/drivers/staging/vboxvideo/vbox_main.c >> @@ -421,18 +421,6 @@ void vbox_driver_unload(struct drm_device *dev) >> vbox_hw_fini(vbox); >> } >> >> -/** >> - * @note this is described in the DRM framework documentation. AST does not >> - * have it, but we get an oops on driver unload if it is not present. >> - */ >> -void vbox_driver_lastclose(struct drm_device *dev) >> -{ >> - struct vbox_private *vbox = dev->dev_private; >> - >> - if (vbox->fbdev) > Hm, except gma500 all the drivers have this NULL check here. I'm not sure > whether that's just cargo-culted or whether that's needed, but I'm wary > from slapping an Ack on the entire series as-is. I think it's cargo-culted because IIRC almost every driver bails out if fbdev init fails. So the only time the pointer is NULL is when fbdev emulation is compiled out. And in that case the check is not needed. vboxvideo for instance fails probing if fbdev init fails, and if it succeeds vbox->fbdev is always set. But anyhow, yeah, I think it's better that the driver maintainers verify this. Noralf. > I'm pretty sure we still need it for i915, but everyone else might just > have copied too much. Or it predates the helper support for disabling the > fbdev emulation. > > There's also the question that a bunch of drivers who set up fbdev forgot > to add the lastclose call it seems, but that's a different story. > -Daniel > >> - drm_fb_helper_restore_fbdev_mode_unlocked(&vbox->fbdev->helper); >> -} >> - >> int vbox_gem_create(struct drm_device *dev, >> u32 size, bool iskernel, struct drm_gem_object **obj) >> { >> -- >> 2.14.2 >>
On Tue, Oct 31, 2017 at 03:40:40PM +0100, Noralf Trønnes wrote: > > Den 31.10.2017 11.32, skrev Daniel Vetter: > > On Mon, Oct 30, 2017 at 04:39:51PM +0100, Noralf Trønnes wrote: > > > This driver can use drm_fb_helper_lastclose() as its .lastclose callback. > > > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/staging/vboxvideo/vbox_drv.c | 2 +- > > > drivers/staging/vboxvideo/vbox_drv.h | 1 - > > > drivers/staging/vboxvideo/vbox_main.c | 12 ------------ > > > 3 files changed, 1 insertion(+), 14 deletions(-) > > > > > > diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c > > > index e18642e5027e..a4d8d7898e3d 100644 > > > --- a/drivers/staging/vboxvideo/vbox_drv.c > > > +++ b/drivers/staging/vboxvideo/vbox_drv.c > > > @@ -229,7 +229,7 @@ static struct drm_driver driver = { > > > .load = vbox_driver_load, > > > .unload = vbox_driver_unload, > > > - .lastclose = vbox_driver_lastclose, > > > + .lastclose = drm_fb_helper_lastclose, > > > .master_set = vbox_master_set, > > > .master_drop = vbox_master_drop, > > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h > > > index 4b9302703b36..7273d7e9bc9b 100644 > > > --- a/drivers/staging/vboxvideo/vbox_drv.h > > > +++ b/drivers/staging/vboxvideo/vbox_drv.h > > > @@ -128,7 +128,6 @@ struct vbox_private { > > > int vbox_driver_load(struct drm_device *dev, unsigned long flags); > > > void vbox_driver_unload(struct drm_device *dev); > > > -void vbox_driver_lastclose(struct drm_device *dev); > > > struct vbox_gem_object; > > > diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c > > > index 80bd039fa08e..c3d756620fd5 100644 > > > --- a/drivers/staging/vboxvideo/vbox_main.c > > > +++ b/drivers/staging/vboxvideo/vbox_main.c > > > @@ -421,18 +421,6 @@ void vbox_driver_unload(struct drm_device *dev) > > > vbox_hw_fini(vbox); > > > } > > > -/** > > > - * @note this is described in the DRM framework documentation. AST does not > > > - * have it, but we get an oops on driver unload if it is not present. > > > - */ > > > -void vbox_driver_lastclose(struct drm_device *dev) > > > -{ > > > - struct vbox_private *vbox = dev->dev_private; > > > - > > > - if (vbox->fbdev) > > Hm, except gma500 all the drivers have this NULL check here. I'm not sure > > whether that's just cargo-culted or whether that's needed, but I'm wary > > from slapping an Ack on the entire series as-is. > > I think it's cargo-culted because IIRC almost every driver bails out if > fbdev init fails. So the only time the pointer is NULL is when fbdev > emulation is compiled out. And in that case the check is not needed. > > vboxvideo for instance fails probing if fbdev init fails, and if it > succeeds vbox->fbdev is always set. > > But anyhow, yeah, I think it's better that the driver maintainers verify > this. See my other mail, I was blind, I think it's all good. -Daniel
diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c index e18642e5027e..a4d8d7898e3d 100644 --- a/drivers/staging/vboxvideo/vbox_drv.c +++ b/drivers/staging/vboxvideo/vbox_drv.c @@ -229,7 +229,7 @@ static struct drm_driver driver = { .load = vbox_driver_load, .unload = vbox_driver_unload, - .lastclose = vbox_driver_lastclose, + .lastclose = drm_fb_helper_lastclose, .master_set = vbox_master_set, .master_drop = vbox_master_drop, diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h index 4b9302703b36..7273d7e9bc9b 100644 --- a/drivers/staging/vboxvideo/vbox_drv.h +++ b/drivers/staging/vboxvideo/vbox_drv.h @@ -128,7 +128,6 @@ struct vbox_private { int vbox_driver_load(struct drm_device *dev, unsigned long flags); void vbox_driver_unload(struct drm_device *dev); -void vbox_driver_lastclose(struct drm_device *dev); struct vbox_gem_object; diff --git a/drivers/staging/vboxvideo/vbox_main.c b/drivers/staging/vboxvideo/vbox_main.c index 80bd039fa08e..c3d756620fd5 100644 --- a/drivers/staging/vboxvideo/vbox_main.c +++ b/drivers/staging/vboxvideo/vbox_main.c @@ -421,18 +421,6 @@ void vbox_driver_unload(struct drm_device *dev) vbox_hw_fini(vbox); } -/** - * @note this is described in the DRM framework documentation. AST does not - * have it, but we get an oops on driver unload if it is not present. - */ -void vbox_driver_lastclose(struct drm_device *dev) -{ - struct vbox_private *vbox = dev->dev_private; - - if (vbox->fbdev) - drm_fb_helper_restore_fbdev_mode_unlocked(&vbox->fbdev->helper); -} - int vbox_gem_create(struct drm_device *dev, u32 size, bool iskernel, struct drm_gem_object **obj) {