| Message ID | 20170802115604.12734-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
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
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;
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(-)