diff mbox series

[v2,1/9] drm: Do delayed switcheroo in drm_lastclose()

Message ID 20240812083000.337744-2-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/{amdgpu,nouveau}: Remove old fbdev hooks | expand

Commit Message

Thomas Zimmermann Aug. 12, 2024, 8:28 a.m. UTC
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@suse.de>
---
 drivers/gpu/drm/drm_file.c       | 4 ++++
 drivers/gpu/vga/vga_switcheroo.c | 3 +--
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Daniel Vetter Aug. 12, 2024, 9:23 a.m. UTC | #1
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@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.

Reviewed-by: Daniel Vetter <daniel.vetter@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 Aug. 12, 2024, 10:18 a.m. UTC | #2
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@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.

- 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@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 Aug. 12, 2024, 10:41 a.m. UTC | #3
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@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@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
Daniel Vetter Aug. 12, 2024, 2:40 p.m. UTC | #4
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@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@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)
>
Alex Deucher Aug. 12, 2024, 6:47 p.m. UTC | #5
On Mon, Aug 12, 2024 at 10:40 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> 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@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 ...

The power management aspect would be better handled by runtime pm.
That just leaves the output muxes.  We could arguably limit
vgaswitcheroo to just systems with the old mux controls.  If and when
we come up with a proper solution for display muxes, we could migrate
to that, but I'm not sure how many systems out there with the old mux
controls are still in use.  I'm not even sure amdgpu supports any
chips which used the old mux controls.  I think those were all chips
supported by radeon.

Alex

> -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@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
Alex Deucher Aug. 12, 2024, 7:05 p.m. UTC | #6
On Mon, Aug 12, 2024 at 4:30 AM Thomas Zimmermann <tzimmermann@suse.de> 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@suse.de>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  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
>
Lyude Paul Aug. 20, 2024, 8:56 p.m. UTC | #7
I can't tell you which list it specifically is, since you might be signed up
on any of the email lists mentioned in the to/cc. But the relevant email
headers that you can use to figure this out are here (this is from a totally
unrelated email, and is just an example - you will have to look up the headers
for your own email):


List-Id: Direct Rendering Infrastructure - Development
<dri-devel.lists.freedesktop.org>
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/dri-devel>,
<mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/dri-devel>,
<mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>

Also, a full list of the email lists here:

amd-gfx@lists.freedesktop.org → https://lists.freedesktop.org/mailman/listinfo/amd-gfx
dri-devel@lists.freedesktop.org → https://lists.freedesktop.org/mailman/listinfo/dri-devel
nouveau@lists.freedesktop.org → https://lists.freedesktop.org/mailman/listinfo/nouveau

If you can't figure out how to view the email headers, it has to be at least
one of those lists 

On Mon, 2024-08-19 at 10:33 -0500, Blake McBride wrote:
> I do not know which list this is.  How can I get these emails to stop?
> 
> Thank you.
> 
> On Mon, Aug 12, 2024 at 3:40 AM Thomas Zimmermann <tzimmermann@suse.de>
> 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@suse.de>
> > ---
> >  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
diff mbox series

Patch

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