| Message ID | 20221020143603.563929-1-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: don't call drm_fb_helper_lastclose in lastclose() | expand |
[AMD Official Use Only - General] Reviewed-by: Evan Quan <evan.quan@amd.com> > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex > Deucher > Sent: Thursday, October 20, 2022 10:36 PM > To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Thomas > Zimmermann <tzimmermann@suse.de> > 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@suse.de> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > --- > 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(); > } > > -- > 2.37.3
Hi Am 24.10.22 um 08:20 schrieb Quan, Evan: > [AMD Official Use Only - General] > > Reviewed-by: Evan Quan <evan.quan@amd.com> > >> -----Original Message----- >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex >> Deucher >> Sent: Thursday, October 20, 2022 10:36 PM >> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Thomas >> Zimmermann <tzimmermann@suse.de> >> 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@suse.de> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >> --- >> 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. Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.0.3/source/drivers/gpu/drm/drm_file.c#L467 >> >> -- >> 2.37.3
On Mon, Oct 24, 2022 at 3:33 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 24.10.22 um 08:20 schrieb Quan, Evan: > > [AMD Official Use Only - General] > > > > Reviewed-by: Evan Quan <evan.quan@amd.com> > > > >> -----Original Message----- > >> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex > >> Deucher > >> Sent: Thursday, October 20, 2022 10:36 PM > >> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Thomas > >> Zimmermann <tzimmermann@suse.de> > >> 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@suse.de> > >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > >> --- > >> 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? 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
Hi Am 24.10.22 um 18:48 schrieb Alex Deucher: > On Mon, Oct 24, 2022 at 3:33 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 24.10.22 um 08:20 schrieb Quan, Evan: >>> [AMD Official Use Only - General] >>> >>> Reviewed-by: Evan Quan <evan.quan@amd.com> >>> >>>> -----Original Message----- >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex >>>> Deucher >>>> Sent: Thursday, October 20, 2022 10:36 PM >>>> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Thomas >>>> Zimmermann <tzimmermann@suse.de> >>>> 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@suse.de> >>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >>>> --- >>>> 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
On Tue, Oct 25, 2022 at 3:25 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 24.10.22 um 18:48 schrieb Alex Deucher: > > On Mon, Oct 24, 2022 at 3:33 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi > >> > >> Am 24.10.22 um 08:20 schrieb Quan, Evan: > >>> [AMD Official Use Only - General] > >>> > >>> Reviewed-by: Evan Quan <evan.quan@amd.com> > >>> > >>>> -----Original Message----- > >>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex > >>>> Deucher > >>>> Sent: Thursday, October 20, 2022 10:36 PM > >>>> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > >>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Thomas > >>>> Zimmermann <tzimmermann@suse.de> > >>>> 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@suse.de> > >>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > >>>> --- > >>>> 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. Sounds like a plan. Thanks! Alex > > 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
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(); }
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@suse.de> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 1 - 1 file changed, 1 deletion(-)