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

[Daniel Vetter <daniel.vetter@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch