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

[Noralf Trønnes]

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.

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
>>