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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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