Re: [PATCH] drm/amdgpu: don't call drm_fb_helper_lastclose in lastclose()

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 24.10.22 um 18:48 schrieb Alex Deucher:
> On Mon, Oct 24, 2022 at 3:33 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> > Hi
> >
> > Am 24.10.22 um 08:20 schrieb Quan, Evan:
> > > [AMD Official Use Only - General]
> > >
> > > Reviewed-by: Evan Quan <evan.quan@xxxxxxx>
> > >
> > > > -----Original Message-----
> > > > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Alex
> > > > Deucher
> > > > Sent: Thursday, October 20, 2022 10:36 PM
> > > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Thomas
> > > > Zimmermann <tzimmermann@xxxxxxx>
> > > > Subject: [PATCH] drm/amdgpu: don't call drm_fb_helper_lastclose in
> > > > lastclose()
> > > >
> > > > It's used to restore the fbdev console, but as amdgpu uses
> > > > generic fbdev emulation, the console is being restored by the
> > > > DRM client helpers already. See the call to drm_client_dev_restore()
> > > > in drm_lastclose().
> > > >
> > > > Fixes: 087451f372bf76 ("drm/amdgpu: use generic fb helpers instead of
> > > > setting up AMD own's.")
> > > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 1 -
> > > >    1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > > index fe23e09eec98..474b9f40f792 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > > @@ -1106,7 +1106,6 @@ int amdgpu_info_ioctl(struct drm_device *dev,
> > > > void *data, struct drm_file *filp)
> > > >     */
> > > >    void amdgpu_driver_lastclose_kms(struct drm_device *dev)
> > > >    {
> > > > -    drm_fb_helper_lastclose(dev);
> > > >       vga_switcheroo_process_delayed_switch();
> > > >    }
> >
> > Without the call to drm_fb_helper_lastclose(), the console emulation
> > will be restored by drm_client_dev_restore() from drm_lastclose(). [1]
> > It means that it's now changing order with the call to
> > vga_switcheroo_process_delay_switch(). Can this become a problem?
> >
> > I looked at the other callers of that function. Most restore the console
> > before doing the switcheroo. Nouveau doesn't seem to care about the
> > console at all.
>
> I don't know off hand.  I suppose if the switch powered down the GPU
> and then we tried to restore it's console state that would be a
> problem, but it looks like vga_switchto_stage2() just powers down the
> GPU without going through suspend so I'm not sure if this actually
> worked correctly?  What are the potential problems with calling
> drm_fb_helper_lastclose twice?

It should do fbdev console modesetting twice; so no problem.

AFAIU calling vga switcheroo does nothing for devices without support. 
So let's call vga_switcheroo_process_delayed_switch() at the very end of 
drm_lastclose() and remove the amdgpu callback entirely. [1]  This 
should not be a problem and if, we can refactor the whole function.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_file.c#L468

> Alex
>
> > Best regards
> > Thomas
> >
> > [1]
> > https://elixir.bootlin.com/linux/v6.0.3/source/drivers/gpu/drm/drm_file.c#L467
> >
> > > > --
> > > > 2.37.3
> >
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > (HRB 36809, AG Nürnberg)
> > Geschäftsführer: Ivo Totev

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature