[Intel-gfx] [PATCH v2 15/15] staging: vboxvideo: Use drm_fb_helper_lastclose()

[Daniel Vetter]

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 at redhat.com>
> > > Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> > > Reviewed-by: Hans de Goede <hdegoede at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch