Nouveau - Aug 2024 - [PATCH v2 1/9] drm: Do delayed switcheroo in drm_lastclose()

Hi

Am 12.08.24 um 12:18 schrieb Daniel Vetter:
> On Mon, Aug 12, 2024 at 11:23:44AM +0200, Daniel Vetter wrote:
>> On Mon, Aug 12, 2024 at 10:28:22AM +0200, Thomas Zimmermann wrote:
>>> Amdgpu and nouveau call vga_switcheroo_process_delayed_switch()
from
>>> their lastclose callbacks. Call it from drm_lastclose(), so that
the
>>> driver functions can finally be removed. Only PCI devices with
enabled
>>> switcheroo do the delayed switching. The call has no effect on
other
>>> hardware.
>>>
>>> v2:
>>> - move change to drm_lastclose() (Sima)
>>> - update docs for vga_switcheroo_process_delayed_switch()
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> A bit an aside: The entire vgaswitcheroo code is still a midlayer mess,
>> where the locking is at the wrong layers resulting in the can_switch
check
>> potentially being racy. But that's a different can of worms.
> Ok I got a bit annoyed about this mess again, and I think I have a
> reasonable idea for how to address it. Not sure why this took a decade,
> and definitely only pick this up if you're really bored.
No, definitely not. :) I don't think I have hardware for testing 
vga_switcheroo. Does this still exist? It seemed to be a thing of the 2000s.

Best regards
Thomas
>
> - We add a new vga_switcheroo_client_tryget, which checks the current
>    state, and if it's on, increments a newly added refcount (which vgw
>    switheroo maintains). Otherwise it fails. Drivers call this from their
>    drm_driver->open hook. This check also allows us to drop the
>    layer-violating checks in drm_open_helper for
drm_dev->dev_power_state.
>
> - That refcount is dropped with vga_switcheroo_client_put, called from
>    drm_driver->close. If the refcount drops to 0 this function also does
>    delayed switch processing.
>
> - All the can_switch callbacks get removed and instead the vgwswr code
>    directly consults its own refount.
>
> With this we don't have locking inversions anymore, and the old vgw
> switcheroo code works a lot more like the new mode based on runtime pm and
> power domains.
>
> With a bit more shuffling I think we can also ditch
> drm_driver->dev_power_state:
>
> - There's one in the intel backlight code, which is annoying, since
it's
>    wants to know whether the current callchain is from a vga switcheroo
>    state change. But doable with a little helper.
>
> - Most others just want a vga_switcheroo_client_is_off() helper, which
>    should be easy. Some are even entirely redundant, at least from a cursor
>    callchain check. There's no races for these because they only matter
>    during system suspend, since you should not mix both runtime and classic
>    vgaswitcheroo logic. We might want some checks for that in that new
>    helper ...
>
> - The one in the fbdev code is annoying, because it's another race.
>    Ideally instead of that check it needs a call to
>    vga_switcheroo_client_tryget/put just around the call to restore modes
>    (we do not want fbdev to block state switches), but that probably means
>    wiring a new callback through drm_client to drivers.
>
> - Might have missed a special case ...
>
> Anyway, I got nerdsniped, had an idea, figured best to type it up. Maybe
> we want to add a link to this to todo.rst, I think we have a vgaswitcheroo
> entry already.
>
> Cheers, Sima
>
>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>>> ---
>>>   drivers/gpu/drm/drm_file.c       | 4 ++++
>>>   drivers/gpu/vga/vga_switcheroo.c | 3 +--
>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c
b/drivers/gpu/drm/drm_file.c
>>> index 714e42b05108..513bef816ae9 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -38,6 +38,7 @@
>>>   #include <linux/pci.h>
>>>   #include <linux/poll.h>
>>>   #include <linux/slab.h>
>>> +#include <linux/vga_switcheroo.h>
>>>   
>>>   #include <drm/drm_client.h>
>>>   #include <drm/drm_drv.h>
>>> @@ -404,6 +405,9 @@ void drm_lastclose(struct drm_device * dev)
>>>   	drm_dbg_core(dev, "driver lastclose completed\n");
>>>   
>>>   	drm_client_dev_restore(dev);
>>> +
>>> +	if (dev_is_pci(dev->dev))
>>> +		vga_switcheroo_process_delayed_switch();
>>>   }
>>>   
>>>   /**
>>> diff --git a/drivers/gpu/vga/vga_switcheroo.c
b/drivers/gpu/vga/vga_switcheroo.c
>>> index 365e6ddbe90f..18f2c92beff8 100644
>>> --- a/drivers/gpu/vga/vga_switcheroo.c
>>> +++ b/drivers/gpu/vga/vga_switcheroo.c
>>> @@ -926,8 +926,7 @@ static void vga_switcheroo_debugfs_init(struct
vgasr_priv *priv)
>>>   /**
>>>    * vga_switcheroo_process_delayed_switch() - helper for delayed
switching
>>>    *
>>> - * Process a delayed switch if one is pending. DRM drivers should
call this
>>> - * from their ->lastclose callback.
>>> + * Process a delayed switch if one is pending.
>>>    *
>>>    * Return: 0 on success. -EINVAL if no delayed switch is pending,
if the client
>>>    * has unregistered in the meantime or if there are other clients
blocking the
>>> -- 
>>> 2.46.0
>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Mon, Aug 12, 2024 at 12:41:39PM +0200, Thomas Zimmermann
wrote:
> Hi
> 
> Am 12.08.24 um 12:18 schrieb Daniel Vetter:
> > On Mon, Aug 12, 2024 at 11:23:44AM +0200, Daniel Vetter wrote:
> > > On Mon, Aug 12, 2024 at 10:28:22AM +0200, Thomas Zimmermann
wrote:
> > > > Amdgpu and nouveau call
vga_switcheroo_process_delayed_switch() from
> > > > their lastclose callbacks. Call it from drm_lastclose(), so
that the
> > > > driver functions can finally be removed. Only PCI devices
with enabled
> > > > switcheroo do the delayed switching. The call has no effect
on other
> > > > hardware.
> > > > 
> > > > v2:
> > > > - move change to drm_lastclose() (Sima)
> > > > - update docs for vga_switcheroo_process_delayed_switch()
> > > > 
> > > > Signed-off-by: Thomas Zimmermann <tzimmermann at
suse.de>
> > > A bit an aside: The entire vgaswitcheroo code is still a midlayer
mess,
> > > where the locking is at the wrong layers resulting in the
can_switch check
> > > potentially being racy. But that's a different can of worms.
> > Ok I got a bit annoyed about this mess again, and I think I have a
> > reasonable idea for how to address it. Not sure why this took a
decade,
> > and definitely only pick this up if you're really bored.
> 
> No, definitely not. :) I don't think I have hardware for testing
> vga_switcheroo. Does this still exist? It seemed to be a thing of the
2000s.
Yeah good chances the old style switcheroo doesn't have many, if any users
left ...
-Sima
> 
> Best regards
> Thomas
> 
> > 
> > - We add a new vga_switcheroo_client_tryget, which checks the current
> >    state, and if it's on, increments a newly added refcount (which
vgw
> >    switheroo maintains). Otherwise it fails. Drivers call this from
their
> >    drm_driver->open hook. This check also allows us to drop the
> >    layer-violating checks in drm_open_helper for
drm_dev->dev_power_state.
> > 
> > - That refcount is dropped with vga_switcheroo_client_put, called from
> >    drm_driver->close. If the refcount drops to 0 this function also
does
> >    delayed switch processing.
> > 
> > - All the can_switch callbacks get removed and instead the vgwswr code
> >    directly consults its own refount.
> > 
> > With this we don't have locking inversions anymore, and the old
vgw
> > switcheroo code works a lot more like the new mode based on runtime pm
and
> > power domains.
> > 
> > With a bit more shuffling I think we can also ditch
> > drm_driver->dev_power_state:
> > 
> > - There's one in the intel backlight code, which is annoying,
since it's
> >    wants to know whether the current callchain is from a vga
switcheroo
> >    state change. But doable with a little helper.
> > 
> > - Most others just want a vga_switcheroo_client_is_off() helper, which
> >    should be easy. Some are even entirely redundant, at least from a
cursor
> >    callchain check. There's no races for these because they only
matter
> >    during system suspend, since you should not mix both runtime and
classic
> >    vgaswitcheroo logic. We might want some checks for that in that new
> >    helper ...
> > 
> > - The one in the fbdev code is annoying, because it's another
race.
> >    Ideally instead of that check it needs a call to
> >    vga_switcheroo_client_tryget/put just around the call to restore
modes
> >    (we do not want fbdev to block state switches), but that probably
means
> >    wiring a new callback through drm_client to drivers.
> > 
> > - Might have missed a special case ...
> > 
> > Anyway, I got nerdsniped, had an idea, figured best to type it up.
Maybe
> > we want to add a link to this to todo.rst, I think we have a
vgaswitcheroo
> > entry already.
> > 
> > Cheers, Sima
> > 
> > 
> > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > 
> > > > ---
> > > >   drivers/gpu/drm/drm_file.c       | 4 ++++
> > > >   drivers/gpu/vga/vga_switcheroo.c | 3 +--
> > > >   2 files changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_file.c
b/drivers/gpu/drm/drm_file.c
> > > > index 714e42b05108..513bef816ae9 100644
> > > > --- a/drivers/gpu/drm/drm_file.c
> > > > +++ b/drivers/gpu/drm/drm_file.c
> > > > @@ -38,6 +38,7 @@
> > > >   #include <linux/pci.h>
> > > >   #include <linux/poll.h>
> > > >   #include <linux/slab.h>
> > > > +#include <linux/vga_switcheroo.h>
> > > >   #include <drm/drm_client.h>
> > > >   #include <drm/drm_drv.h>
> > > > @@ -404,6 +405,9 @@ void drm_lastclose(struct drm_device *
dev)
> > > >   	drm_dbg_core(dev, "driver lastclose
completed\n");
> > > >   	drm_client_dev_restore(dev);
> > > > +
> > > > +	if (dev_is_pci(dev->dev))
> > > > +		vga_switcheroo_process_delayed_switch();
> > > >   }
> > > >   /**
> > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c
b/drivers/gpu/vga/vga_switcheroo.c
> > > > index 365e6ddbe90f..18f2c92beff8 100644
> > > > --- a/drivers/gpu/vga/vga_switcheroo.c
> > > > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > > > @@ -926,8 +926,7 @@ static void
vga_switcheroo_debugfs_init(struct vgasr_priv *priv)
> > > >   /**
> > > >    * vga_switcheroo_process_delayed_switch() - helper for
delayed switching
> > > >    *
> > > > - * Process a delayed switch if one is pending. DRM drivers
should call this
> > > > - * from their ->lastclose callback.
> > > > + * Process a delayed switch if one is pending.
> > > >    *
> > > >    * Return: 0 on success. -EINVAL if no delayed switch is
pending, if the client
> > > >    * has unregistered in the meantime or if there are other
clients blocking the
> > > > -- 
> > > > 2.46.0
> > > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch