[3/4] drm: Only lastclose on unload for legacy drivers
diff mbox

Message ID 20170802115604.12734-4-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Aug. 2, 2017, 11:56 a.m. UTC
The only thing modern drivers are supposed to do in lastclose is
restore the fb emulation state. Which is entirely optional, and
there's really no reason to do that. So restrict it to legacy drivers
(where the driver cleanup essentially happens in lastclose).

This will also allow us to share the unregister function with
drm_dev_unplug().

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Alex Deucher Aug. 2, 2017, 8:50 p.m. UTC | #1
On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The only thing modern drivers are supposed to do in lastclose is
> restore the fb emulation state. Which is entirely optional, and
> there's really no reason to do that. So restrict it to legacy drivers
> (where the driver cleanup essentially happens in lastclose).

vga_switcheroo_process_delayed_switch() gets called in lastclose.
Won't that need to get moved elsewhere for this to work?

Alex

>
> This will also allow us to share the unregister function with
> drm_dev_unplug().
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 39191e5c240e..694040a240af 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -860,7 +860,8 @@ void drm_dev_unregister(struct drm_device *dev)
>  {
>         struct drm_map_list *r_list, *list_temp;
>
> -       drm_lastclose(dev);
> +       if (drm_core_check_feature(dev, DRIVER_LEGACY))
> +               drm_lastclose(dev);
>
>         dev->registered = false;
>
> --
> 2.13.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Aug. 2, 2017, 11:17 p.m. UTC | #2
On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> The only thing modern drivers are supposed to do in lastclose is
>> restore the fb emulation state. Which is entirely optional, and
>> there's really no reason to do that. So restrict it to legacy drivers
>> (where the driver cleanup essentially happens in lastclose).
>
> vga_switcheroo_process_delayed_switch() gets called in lastclose.
> Won't that need to get moved elsewhere for this to work?

Hm right, I forgot the lazy way to do runtime pm by keeping the device
alive as long as anyone has an open fd for it ... This shouldn't be a
problem, since you need to unregister from vgaswitcheroo anyway on
unload. Maybe that blows up, I'll check the code and augment the patch
as needed.
-Daniel

>
> Alex
>
>>
>> This will also allow us to share the unregister function with
>> drm_dev_unplug().
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>  drivers/gpu/drm/drm_drv.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 39191e5c240e..694040a240af 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -860,7 +860,8 @@ void drm_dev_unregister(struct drm_device *dev)
>>  {
>>         struct drm_map_list *r_list, *list_temp;
>>
>> -       drm_lastclose(dev);
>> +       if (drm_core_check_feature(dev, DRIVER_LEGACY))
>> +               drm_lastclose(dev);
>>
>>         dev->registered = false;
>>
>> --
>> 2.13.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Aug. 3, 2017, 1:54 p.m. UTC | #3
On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> The only thing modern drivers are supposed to do in lastclose is
>>> restore the fb emulation state. Which is entirely optional, and
>>> there's really no reason to do that. So restrict it to legacy drivers
>>> (where the driver cleanup essentially happens in lastclose).
>>
>> vga_switcheroo_process_delayed_switch() gets called in lastclose.
>> Won't that need to get moved elsewhere for this to work?
>
> Hm right, I forgot the lazy way to do runtime pm by keeping the device
> alive as long as anyone has an open fd for it ... This shouldn't be a
> problem, since you need to unregister from vgaswitcheroo anyway on
> unload. Maybe that blows up, I'll check the code and augment the patch
> as needed.

So I think there's 3 cases:
- Trying to unload the module. You can't do that while anyone has the
fd still open, so lastclose is guaranteeed to run.
- Forcefully unbinding the driver through sysfs. Not any better, since
the can_switch stuff checks for the open count, and so will delay the
delayed switch no matter what.
- Actual hotremoval: a) not implemented since none of the drivers
taking part in vgaswitcheroo correctly unplug the drm device and b)
you can't hotremove a chip from a laptop.

So I think I'm not breaking this anymore than it probably already is :-)
-Daniel
Alex Deucher Aug. 3, 2017, 5:52 p.m. UTC | #4
On Thu, Aug 3, 2017 at 9:54 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>> The only thing modern drivers are supposed to do in lastclose is
>>>> restore the fb emulation state. Which is entirely optional, and
>>>> there's really no reason to do that. So restrict it to legacy drivers
>>>> (where the driver cleanup essentially happens in lastclose).
>>>
>>> vga_switcheroo_process_delayed_switch() gets called in lastclose.
>>> Won't that need to get moved elsewhere for this to work?
>>
>> Hm right, I forgot the lazy way to do runtime pm by keeping the device
>> alive as long as anyone has an open fd for it ... This shouldn't be a
>> problem, since you need to unregister from vgaswitcheroo anyway on
>> unload. Maybe that blows up, I'll check the code and augment the patch
>> as needed.
>
> So I think there's 3 cases:
> - Trying to unload the module. You can't do that while anyone has the
> fd still open, so lastclose is guaranteeed to run.
> - Forcefully unbinding the driver through sysfs. Not any better, since
> the can_switch stuff checks for the open count, and so will delay the
> delayed switch no matter what.
> - Actual hotremoval: a) not implemented since none of the drivers
> taking part in vgaswitcheroo correctly unplug the drm device and b)
> you can't hotremove a chip from a laptop.
>
> So I think I'm not breaking this anymore than it probably already is :-)

Thanks.  This series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Michel Dänzer Aug. 4, 2017, 4:12 a.m. UTC | #5
On 03/08/17 10:54 PM, Daniel Vetter wrote:
> On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>> The only thing modern drivers are supposed to do in lastclose is
>>>> restore the fb emulation state. Which is entirely optional, and
>>>> there's really no reason to do that. So restrict it to legacy drivers
>>>> (where the driver cleanup essentially happens in lastclose).
>>>
>>> vga_switcheroo_process_delayed_switch() gets called in lastclose.
>>> Won't that need to get moved elsewhere for this to work?
>>
>> Hm right, I forgot the lazy way to do runtime pm by keeping the device
>> alive as long as anyone has an open fd for it ... This shouldn't be a
>> problem, since you need to unregister from vgaswitcheroo anyway on
>> unload. Maybe that blows up, I'll check the code and augment the patch
>> as needed.
> 
> So I think there's 3 cases:
> - Trying to unload the module. You can't do that while anyone has the
> fd still open, so lastclose is guaranteeed to run.
> - Forcefully unbinding the driver through sysfs. Not any better, since
> the can_switch stuff checks for the open count, and so will delay the
> delayed switch no matter what.

Are you sure that this is working as intended?
https://bugs.freedesktop.org/show_bug.cgi?id=100399 sounds like
unbinding works even while Xorg is using the DRM device.
Daniel Vetter Aug. 4, 2017, 9:15 a.m. UTC | #6
On Fri, Aug 4, 2017 at 6:12 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 03/08/17 10:54 PM, Daniel Vetter wrote:
>> On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>>> The only thing modern drivers are supposed to do in lastclose is
>>>>> restore the fb emulation state. Which is entirely optional, and
>>>>> there's really no reason to do that. So restrict it to legacy drivers
>>>>> (where the driver cleanup essentially happens in lastclose).
>>>>
>>>> vga_switcheroo_process_delayed_switch() gets called in lastclose.
>>>> Won't that need to get moved elsewhere for this to work?
>>>
>>> Hm right, I forgot the lazy way to do runtime pm by keeping the device
>>> alive as long as anyone has an open fd for it ... This shouldn't be a
>>> problem, since you need to unregister from vgaswitcheroo anyway on
>>> unload. Maybe that blows up, I'll check the code and augment the patch
>>> as needed.
>>
>> So I think there's 3 cases:
>> - Trying to unload the module. You can't do that while anyone has the
>> fd still open, so lastclose is guaranteeed to run.
>> - Forcefully unbinding the driver through sysfs. Not any better, since
>> the can_switch stuff checks for the open count, and so will delay the
>> delayed switch no matter what.
>
> Are you sure that this is working as intended?
> https://bugs.freedesktop.org/show_bug.cgi?id=100399 sounds like
> unbinding works even while Xorg is using the DRM device.

Unbinding the gpu while someone has a file open (like X) is very much
possible, it's how it's designed. That's the forced unplug case. But:
a) amdgpu doesn't implement actual hotremoval support and blows up
b) even in the unload path we still check for can_switch, which will
notice that there's still open fds and not do the delayed switch, so
the code I've removed is already dead in that case.

But yeah, unbinding while X is running is very much possible, it's
exactly what will happen if you hotremove the gpu (from the driver
model pov) physically. The only thing I tried to show is that my patch
doesn't make anything worse :-)
-Daniel
Daniel Vetter Aug. 11, 2017, 8:49 a.m. UTC | #7
On Thu, Aug 03, 2017 at 01:52:55PM -0400, Alex Deucher wrote:
> On Thu, Aug 3, 2017 at 9:54 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Thu, Aug 3, 2017 at 1:17 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> On Wed, Aug 2, 2017 at 10:50 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> >>> On Wed, Aug 2, 2017 at 7:56 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >>>> The only thing modern drivers are supposed to do in lastclose is
> >>>> restore the fb emulation state. Which is entirely optional, and
> >>>> there's really no reason to do that. So restrict it to legacy drivers
> >>>> (where the driver cleanup essentially happens in lastclose).
> >>>
> >>> vga_switcheroo_process_delayed_switch() gets called in lastclose.
> >>> Won't that need to get moved elsewhere for this to work?
> >>
> >> Hm right, I forgot the lazy way to do runtime pm by keeping the device
> >> alive as long as anyone has an open fd for it ... This shouldn't be a
> >> problem, since you need to unregister from vgaswitcheroo anyway on
> >> unload. Maybe that blows up, I'll check the code and augment the patch
> >> as needed.
> >
> > So I think there's 3 cases:
> > - Trying to unload the module. You can't do that while anyone has the
> > fd still open, so lastclose is guaranteeed to run.
> > - Forcefully unbinding the driver through sysfs. Not any better, since
> > the can_switch stuff checks for the open count, and so will delay the
> > delayed switch no matter what.
> > - Actual hotremoval: a) not implemented since none of the drivers
> > taking part in vgaswitcheroo correctly unplug the drm device and b)
> > you can't hotremove a chip from a laptop.
> >
> > So I think I'm not breaking this anymore than it probably already is :-)

Added the above to the commit message ...
> 
> Thanks.  This series is:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

... and pulled the entire series in.

Thanks for your review.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 39191e5c240e..694040a240af 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -860,7 +860,8 @@  void drm_dev_unregister(struct drm_device *dev)
 {
 	struct drm_map_list *r_list, *list_temp;
 
-	drm_lastclose(dev);
+	if (drm_core_check_feature(dev, DRIVER_LEGACY))
+		drm_lastclose(dev);
 
 	dev->registered = false;