[Nouveau] [PATCH v2 0/3] drm: Generic fbdev and vga-switcheroo

[Nouveau] [PATCH v2 0/3] drm: Generic fbdev and vga-switcheroo

[Thomas Zimmermann]

(was: drm/fb-helper: Set framebuffer for vga-switcheroo clients)

This patch has now turned into a little series. The first two patches
are bug fixes for the existing code. The third patch cleans up the
drivers.

Patch 1 fixes i915 to do the correct thing if the device has not been
initialized yet. Switching to the device is only possible after the
initialization, but switching away is always possible.

Patch 2 is the original patch without the amdgpu changes. Installs
the fbdev framebuffer in vga-switcheroo for the PCI device. Does
nothing for drivers without vga-switcheroo.

Patch 3 cleans up vga_switcheroo_process_delayed_switch() in amdgpu
and the other related drivers (i.e., i915, nouveau and radeon). The
call is now located at the end of drm_lastclose() and drivers do not
need their own lastclose helpers any longer.

I kept the r-bs from v1, but patch 1 is entirely new and patch 3 has
significantly grown in size.

Thomas Zimmermann (3):
  drm/i915: Allow switching away via vga-switcheroo if uninitialized
  drm/fb-helper: Set framebuffer for vga-switcheroo clients
  drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 13 -------------
 drivers/gpu/drm/drm_fb_helper.c         |  8 ++++++++
 drivers/gpu/drm/drm_file.c              |  3 +++
 drivers/gpu/drm/i915/i915_driver.c      | 26 ++-----------------------
 drivers/gpu/drm/i915/i915_switcheroo.c  |  6 +++++-
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
 drivers/gpu/drm/nouveau/nouveau_vga.c   |  7 -------
 drivers/gpu/drm/nouveau/nouveau_vga.h   |  1 -
 drivers/gpu/drm/radeon/radeon_drv.c     |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.h     |  1 -
 drivers/gpu/drm/radeon/radeon_kms.c     | 18 -----------------
 drivers/gpu/vga/vga_switcheroo.c        |  4 ++--
 14 files changed, 21 insertions(+), 72 deletions(-)


base-commit: ac04152253dccfb02dcedfa0c57443122cf79314
-- 
2.39.0

[Nouveau] [PATCH v2 1/3] drm/i915: Allow switching away via vga-switcheroo if uninitialized

[Thomas Zimmermann]

Always allow switching away via vga-switcheroo if the display is
uninitalized. Instead prevent switching to i915 if the device has
not been initialized.

This issue was introduced by commit 5df7bd130818 ("drm/i915: skip
display initialization when there is no display") protected, which
protects code paths from being executed on uninitialized devices.
In the case of vga-switcheroo, we want to allow a switch away from
i915's device. So run vga_switcheroo_process_delayed_switch() and
test in the switcheroo callbacks if the i915 device is available.

Fixes: 5df7bd130818 ("drm/i915: skip display initialization when there is no display")
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: "Jouni Högander" <jouni.hogander@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
Cc: Julia Lawall <Julia.Lawall@inria.fr>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.14+
---
 drivers/gpu/drm/i915/i915_driver.c     | 3 +--
 drivers/gpu/drm/i915/i915_switcheroo.c | 6 +++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c1e427ba57ae..33e231b120c1 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1075,8 +1075,7 @@ static void i915_driver_lastclose(struct drm_device *dev)
 
 	intel_fbdev_restore_mode(dev);
 
-	if (HAS_DISPLAY(i915))
-		vga_switcheroo_process_delayed_switch();
+	vga_switcheroo_process_delayed_switch();
 }
 
 static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/i915_switcheroo.c b/drivers/gpu/drm/i915/i915_switcheroo.c
index 23777d500cdf..f45bd6b6cede 100644
--- a/drivers/gpu/drm/i915/i915_switcheroo.c
+++ b/drivers/gpu/drm/i915/i915_switcheroo.c
@@ -19,6 +19,10 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev,
 		dev_err(&pdev->dev, "DRM not initialized, aborting switch.\n");
 		return;
 	}
+	if (!HAS_DISPLAY(i915)) {
+		dev_err(&pdev->dev, "Device state not initialized, aborting switch.\n");
+		return;
+	}
 
 	if (state == VGA_SWITCHEROO_ON) {
 		drm_info(&i915->drm, "switched on\n");
@@ -44,7 +48,7 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 	 * locking inversion with the driver load path. And the access here is
 	 * completely racy anyway. So don't bother with locking for now.
 	 */
-	return i915 && atomic_read(&i915->drm.open_count) == 0;
+	return i915 && HAS_DISPLAY(i915) && atomic_read(&i915->drm.open_count) == 0;
 }
 
 static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
-- 
2.39.0

[Nouveau] [PATCH v2 2/3] drm/fb-helper: Set framebuffer for vga-switcheroo clients

[Thomas Zimmermann]

Set the framebuffer info for drivers that support VGA switcheroo. Only
affects the amdgpu and nouveau drivers, which use VGA switcheroo and
generic fbdev emulation. For other drivers, this does nothing.

This fixes a potential regression in the console code. Both, amdgpu and
nouveau, invoked vga_switcheroo_client_fb_set() from their internal fbdev
code. But the call got lost when the drivers switched to the generic
emulation.

Fixes: 087451f372bf ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Karol Herbst <kherbst@redhat.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Evan Quan <evan.quan@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Cc: Likun Gao <Likun.Gao@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Stanley Yang <Stanley.Yang@amd.com>
Cc: "Tianci.Yin" <tianci.yin@amd.com>
Cc: Xiaojian Du <Xiaojian.Du@amd.com>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Cc: YiPeng Chai <YiPeng.Chai@amd.com>
Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
Cc: Bokun Zhang <Bokun.Zhang@amd.com>
Cc: Guchun Chen <guchun.chen@amd.com>
Cc: Hamza Mahfooz <hamza.mahfooz@amd.com>
Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Solomon Chiu <solomon.chiu@amd.com>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Marek Olšák" <marek.olsak@amd.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.17+
---
 drivers/gpu/drm/drm_fb_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 427631706128..5e445c61252d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -30,7 +30,9 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/console.h>
+#include <linux/pci.h>
 #include <linux/sysrq.h>
+#include <linux/vga_switcheroo.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_drv.h>
@@ -1940,6 +1942,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
 {
 	struct drm_client_dev *client = &fb_helper->client;
+	struct drm_device *dev = fb_helper->dev;
 	struct drm_fb_helper_surface_size sizes;
 	int ret;
 
@@ -1961,6 +1964,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		return ret;
 
 	strcpy(fb_helper->fb->comm, "[fbcon]");
+
+	/* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
+	if (dev_is_pci(dev->dev))
+		vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), fb_helper->info);
+
 	return 0;
 }
 
-- 
2.39.0

Re: [Nouveau] [Intel-gfx] [PATCH v2 2/3] drm/fb-helper: Set framebuffer for vga-switcheroo clients

[Rodrigo Vivi]

On Thu, Jan 12, 2023 at 09:11:55PM +0100, Thomas Zimmermann wrote:
> Set the framebuffer info for drivers that support VGA switcheroo. Only
> affects the amdgpu and nouveau drivers, which use VGA switcheroo and
> generic fbdev emulation. For other drivers, this does nothing.
> 
> This fixes a potential regression in the console code. Both, amdgpu and
> nouveau, invoked vga_switcheroo_client_fb_set() from their internal fbdev
> code. But the call got lost when the drivers switched to the generic
> emulation.
> 
> Fixes: 087451f372bf ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Karol Herbst <kherbst@redhat.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Evan Quan <evan.quan@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Likun Gao <Likun.Gao@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Stanley Yang <Stanley.Yang@amd.com>
> Cc: "Tianci.Yin" <tianci.yin@amd.com>
> Cc: Xiaojian Du <Xiaojian.Du@amd.com>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: YiPeng Chai <YiPeng.Chai@amd.com>
> Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
> Cc: Bokun Zhang <Bokun.Zhang@amd.com>
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Hamza Mahfooz <hamza.mahfooz@amd.com>
> Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Solomon Chiu <solomon.chiu@amd.com>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Marek Olšák" <marek.olsak@amd.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: nouveau@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.17+
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 427631706128..5e445c61252d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -30,7 +30,9 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/console.h>
> +#include <linux/pci.h>
>  #include <linux/sysrq.h>
> +#include <linux/vga_switcheroo.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_drv.h>
> @@ -1940,6 +1942,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  					 int preferred_bpp)
>  {
>  	struct drm_client_dev *client = &fb_helper->client;
> +	struct drm_device *dev = fb_helper->dev;

On drm-tip, this commit has a silent conflict with
cff84bac9922 ("drm/fh-helper: Split fbdev single-probe helper")
that's already in drm-next.

I had created a fix-up patch in drm-tip re-introducing this line.

We probably need a backmerge from drm-next into drm-misc-fixes with
the resolution applied there. And probably propagated that resolution
later...

>  	struct drm_fb_helper_surface_size sizes;
>  	int ret;
>  
> @@ -1961,6 +1964,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  		return ret;
>  
>  	strcpy(fb_helper->fb->comm, "[fbcon]");
> +
> +	/* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
> +	if (dev_is_pci(dev->dev))
> +		vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), fb_helper->info);
> +
>  	return 0;
>  }
>  
> -- 
> 2.39.0
> 

Re: [Nouveau] [Intel-gfx] [PATCH v2 2/3] drm/fb-helper: Set framebuffer for vga-switcheroo clients

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4790 bytes --]

Hi

Am 18.01.23 um 20:21 schrieb Rodrigo Vivi:
> On Thu, Jan 12, 2023 at 09:11:55PM +0100, Thomas Zimmermann wrote:
>> Set the framebuffer info for drivers that support VGA switcheroo. Only
>> affects the amdgpu and nouveau drivers, which use VGA switcheroo and
>> generic fbdev emulation. For other drivers, this does nothing.
>>
>> This fixes a potential regression in the console code. Both, amdgpu and
>> nouveau, invoked vga_switcheroo_client_fb_set() from their internal fbdev
>> code. But the call got lost when the drivers switched to the generic
>> emulation.
>>
>> Fixes: 087451f372bf ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
>> Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Karol Herbst <kherbst@redhat.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Evan Quan <evan.quan@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
>> Cc: Likun Gao <Likun.Gao@amd.com>
>> Cc: "Christian König" <christian.koenig@amd.com>
>> Cc: Stanley Yang <Stanley.Yang@amd.com>
>> Cc: "Tianci.Yin" <tianci.yin@amd.com>
>> Cc: Xiaojian Du <Xiaojian.Du@amd.com>
>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> Cc: YiPeng Chai <YiPeng.Chai@amd.com>
>> Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
>> Cc: Bokun Zhang <Bokun.Zhang@amd.com>
>> Cc: Guchun Chen <guchun.chen@amd.com>
>> Cc: Hamza Mahfooz <hamza.mahfooz@amd.com>
>> Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>> Cc: Solomon Chiu <solomon.chiu@amd.com>
>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: "Marek Olšák" <marek.olsak@amd.com>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: nouveau@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.17+
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 427631706128..5e445c61252d 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -30,7 +30,9 @@
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>   
>>   #include <linux/console.h>
>> +#include <linux/pci.h>
>>   #include <linux/sysrq.h>
>> +#include <linux/vga_switcheroo.h>
>>   
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_drv.h>
>> @@ -1940,6 +1942,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>   					 int preferred_bpp)
>>   {
>>   	struct drm_client_dev *client = &fb_helper->client;
>> +	struct drm_device *dev = fb_helper->dev;
> 
> On drm-tip, this commit has a silent conflict with
> cff84bac9922 ("drm/fh-helper: Split fbdev single-probe helper")
> that's already in drm-next.
> 
> I had created a fix-up patch in drm-tip re-introducing this line.

Thank you. Is it fixed for now?

> 
> We probably need a backmerge from drm-next into drm-misc-fixes with
> the resolution applied there. And probably propagated that resolution
> later...

Backmerging from -next into -fixes branches is a problem, as -fixes 
should be close to the latest release.

Can we solve this by merging -fixes into upstream and backmerging this 
into our -next branches?

Best regards
Thomas

> 
>>   	struct drm_fb_helper_surface_size sizes;
>>   	int ret;
>>   
>> @@ -1961,6 +1964,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>   		return ret;
>>   
>>   	strcpy(fb_helper->fb->comm, "[fbcon]");
>> +
>> +	/* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
>> +	if (dev_is_pci(dev->dev))
>> +		vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), fb_helper->info);
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.39.0
>>

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [Intel-gfx] [PATCH v2 2/3] drm/fb-helper: Set framebuffer for vga-switcheroo clients

[Rodrigo Vivi]

On Thu, Jan 19, 2023 at 09:06:54AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 18.01.23 um 20:21 schrieb Rodrigo Vivi:
> > On Thu, Jan 12, 2023 at 09:11:55PM +0100, Thomas Zimmermann wrote:
> > > Set the framebuffer info for drivers that support VGA switcheroo. Only
> > > affects the amdgpu and nouveau drivers, which use VGA switcheroo and
> > > generic fbdev emulation. For other drivers, this does nothing.
> > > 
> > > This fixes a potential regression in the console code. Both, amdgpu and
> > > nouveau, invoked vga_switcheroo_client_fb_set() from their internal fbdev
> > > code. But the call got lost when the drivers switched to the generic
> > > emulation.
> > > 
> > > Fixes: 087451f372bf ("drm/amdgpu: use generic fb helpers instead of setting up AMD own's.")
> > > Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > Cc: Karol Herbst <kherbst@redhat.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Evan Quan <evan.quan@amd.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> > > Cc: Likun Gao <Likun.Gao@amd.com>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > Cc: Stanley Yang <Stanley.Yang@amd.com>
> > > Cc: "Tianci.Yin" <tianci.yin@amd.com>
> > > Cc: Xiaojian Du <Xiaojian.Du@amd.com>
> > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > Cc: YiPeng Chai <YiPeng.Chai@amd.com>
> > > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
> > > Cc: Bokun Zhang <Bokun.Zhang@amd.com>
> > > Cc: Guchun Chen <guchun.chen@amd.com>
> > > Cc: Hamza Mahfooz <hamza.mahfooz@amd.com>
> > > Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > > Cc: Mario Limonciello <mario.limonciello@amd.com>
> > > Cc: Solomon Chiu <solomon.chiu@amd.com>
> > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: "Marek Olšák" <marek.olsak@amd.com>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: nouveau@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v5.17+
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 427631706128..5e445c61252d 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -30,7 +30,9 @@
> > >   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >   #include <linux/console.h>
> > > +#include <linux/pci.h>
> > >   #include <linux/sysrq.h>
> > > +#include <linux/vga_switcheroo.h>
> > >   #include <drm/drm_atomic.h>
> > >   #include <drm/drm_drv.h>
> > > @@ -1940,6 +1942,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> > >   					 int preferred_bpp)
> > >   {
> > >   	struct drm_client_dev *client = &fb_helper->client;
> > > +	struct drm_device *dev = fb_helper->dev;
> > 
> > On drm-tip, this commit has a silent conflict with
> > cff84bac9922 ("drm/fh-helper: Split fbdev single-probe helper")
> > that's already in drm-next.
> > 
> > I had created a fix-up patch in drm-tip re-introducing this line.
> 
> Thank you. Is it fixed for now?

Fixed in drm-tip, yes. But broke linux-next as well. This
resolution will need to go up with any merge...

> 
> > 
> > We probably need a backmerge from drm-next into drm-misc-fixes with
> > the resolution applied there. And probably propagated that resolution
> > later...
> 
> Backmerging from -next into -fixes branches is a problem, as -fixes should
> be close to the latest release.

doh! Indeed I should've known this by heart... -ENOTEHOUGHCOFFEE

> 
> Can we solve this by merging -fixes into upstream and backmerging this into
> our -next branches?

hmmm... probably better just wait and let folks know the resolution
when the patches are moving up...

> 
> Best regards
> Thomas
> 
> > 
> > >   	struct drm_fb_helper_surface_size sizes;
> > >   	int ret;
> > > @@ -1961,6 +1964,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> > >   		return ret;
> > >   	strcpy(fb_helper->fb->comm, "[fbcon]");
> > > +
> > > +	/* Set the fb info for vgaswitcheroo clients. Does nothing otherwise. */
> > > +	if (dev_is_pci(dev->dev))
> > > +		vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), fb_helper->info);
> > > +
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.39.0
> > > 
> 
> -- 
> 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

[Nouveau] [PATCH v2 3/3] drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

[Thomas Zimmermann]

Several lastclose helpers call vga_switcheroo_process_delayed_switch().
It's better to call the helper from drm_lastclose() after the kernel
client's screen has been restored. This way, all drivers can benefit
without having to implement their own lastclose helper. For drivers
without vga-switcheroo, vga_switcheroo_process_delayed_switch() does
nothing.

There was an earlier patchset to do something similar. [1]

v2:
	* handle vga_switcheroo_client_fb_set() in a separate patch
	* also update i915, nouveau and radeon
	* remove unnecessary include statements
	* update vga-switcheroo docs

Suggested-by: Alexander Deucher <Alexander.Deucher@amd.com>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Link: https://lore.kernel.org/amd-gfx/20221020143603.563929-1-alexander.deucher@amd.com/ # 1
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 13 -------------
 drivers/gpu/drm/drm_file.c              |  3 +++
 drivers/gpu/drm/i915/i915_driver.c      | 25 ++-----------------------
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
 drivers/gpu/drm/nouveau/nouveau_vga.c   |  7 -------
 drivers/gpu/drm/nouveau/nouveau_vga.h   |  1 -
 drivers/gpu/drm/radeon/radeon_drv.c     |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.h     |  1 -
 drivers/gpu/drm/radeon/radeon_kms.c     | 18 ------------------
 drivers/gpu/vga/vga_switcheroo.c        |  4 ++--
 12 files changed, 8 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 63c921c55fb9..7120b9b6e580 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1330,7 +1330,6 @@ extern const int amdgpu_max_kms_ioctl;
 
 int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags);
 void amdgpu_driver_unload_kms(struct drm_device *dev);
-void amdgpu_driver_lastclose_kms(struct drm_device *dev);
 int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv);
 void amdgpu_driver_postclose_kms(struct drm_device *dev,
 				 struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 1353ffd08988..783c1e284a22 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -34,7 +34,6 @@
 #include <drm/drm_pciids.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vga_switcheroo.h>
 #include <drm/drm_probe_helper.h>
 #include <linux/mmu_notifier.h>
 #include <linux/suspend.h>
@@ -2785,7 +2784,6 @@ static const struct drm_driver amdgpu_kms_driver = {
 	    DRIVER_SYNCOBJ_TIMELINE,
 	.open = amdgpu_driver_open_kms,
 	.postclose = amdgpu_driver_postclose_kms,
-	.lastclose = amdgpu_driver_lastclose_kms,
 	.ioctls = amdgpu_ioctls_kms,
 	.num_ioctls = ARRAY_SIZE(amdgpu_ioctls_kms),
 	.dumb_create = amdgpu_mode_dumb_create,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 7aa7e52ca784..a37be02fb2fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -34,7 +34,6 @@
 #include "amdgpu_vce.h"
 #include "atom.h"
 
-#include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/pci.h>
@@ -1104,18 +1103,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 /*
  * Outdated mess for old drm with Xorg being in charge (void function now).
  */
-/**
- * amdgpu_driver_lastclose_kms - drm callback for last close
- *
- * @dev: drm dev pointer
- *
- * Switch vga_switcheroo state after last close (all asics).
- */
-void amdgpu_driver_lastclose_kms(struct drm_device *dev)
-{
-	drm_fb_helper_lastclose(dev);
-	vga_switcheroo_process_delayed_switch();
-}
 
 /**
  * amdgpu_driver_open_kms - drm callback for open
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index a51ff8cee049..314c309db9a3 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>
@@ -460,6 +461,8 @@ void drm_lastclose(struct drm_device * dev)
 		drm_legacy_dev_reinit(dev);
 
 	drm_client_dev_restore(dev);
+
+	vga_switcheroo_process_delayed_switch();
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 33e231b120c1..bf6ad8620970 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -29,6 +29,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/fb.h> /* for FBINFO_STATE_ */
 #include <linux/module.h>
 #include <linux/oom.h>
 #include <linux/pci.h>
@@ -37,7 +38,6 @@
 #include <linux/pnp.h>
 #include <linux/slab.h>
 #include <linux/string_helpers.h>
-#include <linux/vga_switcheroo.h>
 #include <linux/vt.h>
 
 #include <drm/drm_aperture.h>
@@ -1057,27 +1057,6 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file)
 	return 0;
 }
 
-/**
- * i915_driver_lastclose - clean up after all DRM clients have exited
- * @dev: DRM device
- *
- * Take care of cleaning up after all DRM clients have exited.  In the
- * mode setting case, we want to restore the kernel's initial mode (just
- * in case the last client left us in a bad state).
- *
- * Additionally, in the non-mode setting case, we'll tear down the GTT
- * and DMA structures, since the kernel won't be using them, and clea
- * up any GEM state.
- */
-static void i915_driver_lastclose(struct drm_device *dev)
-{
-	struct drm_i915_private *i915 = to_i915(dev);
-
-	intel_fbdev_restore_mode(dev);
-
-	vga_switcheroo_process_delayed_switch();
-}
-
 static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
 {
 	struct drm_i915_file_private *file_priv = file->driver_priv;
@@ -1921,7 +1900,7 @@ static const struct drm_driver i915_drm_driver = {
 	    DRIVER_SYNCOBJ_TIMELINE,
 	.release = i915_driver_release,
 	.open = i915_driver_open,
-	.lastclose = i915_driver_lastclose,
+	.lastclose = intel_fbdev_restore_mode,
 	.postclose = i915_driver_postclose,
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 80f154b6adab..c3bd2a1d3d2e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1230,7 +1230,6 @@ driver_stub = {
 
 	.open = nouveau_drm_open,
 	.postclose = nouveau_drm_postclose,
-	.lastclose = nouveau_vga_lastclose,
 
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = nouveau_drm_debugfs_init,
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 789393b94291..fc125e2247f7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -126,10 +126,3 @@ nouveau_vga_fini(struct nouveau_drm *drm)
 	if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
 		vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
 }
-
-
-void
-nouveau_vga_lastclose(struct drm_device *dev)
-{
-	vga_switcheroo_process_delayed_switch();
-}
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.h b/drivers/gpu/drm/nouveau/nouveau_vga.h
index 951a83f984dd..63be415d2a44 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.h
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.h
@@ -4,6 +4,5 @@
 
 void nouveau_vga_init(struct nouveau_drm *);
 void nouveau_vga_fini(struct nouveau_drm *);
-void nouveau_vga_lastclose(struct drm_device *dev);
 
 #endif
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 6cbe1ab81aba..dbf85e1f0061 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -597,7 +597,7 @@ static const struct drm_driver kms_driver = {
 	.load = radeon_driver_load_kms,
 	.open = radeon_driver_open_kms,
 	.postclose = radeon_driver_postclose_kms,
-	.lastclose = radeon_driver_lastclose_kms,
+	.lastclose = drm_fb_helper_lastclose,
 	.unload = radeon_driver_unload_kms,
 	.ioctls = radeon_ioctls_kms,
 	.num_ioctls = ARRAY_SIZE(radeon_ioctls_kms),
diff --git a/drivers/gpu/drm/radeon/radeon_drv.h b/drivers/gpu/drm/radeon/radeon_drv.h
index ac7970919c4d..2ffe0975ee54 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.h
+++ b/drivers/gpu/drm/radeon/radeon_drv.h
@@ -120,7 +120,6 @@ long radeon_drm_ioctl(struct file *filp,
 
 int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags);
 void radeon_driver_unload_kms(struct drm_device *dev);
-void radeon_driver_lastclose_kms(struct drm_device *dev);
 int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv);
 void radeon_driver_postclose_kms(struct drm_device *dev,
 				 struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 965161b8565b..e0214cf1b43b 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -32,7 +32,6 @@
 #include <linux/uaccess.h>
 #include <linux/vga_switcheroo.h>
 
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
 #include <drm/radeon_drm.h>
@@ -622,23 +621,6 @@ int radeon_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	return 0;
 }
 
-
-/*
- * Outdated mess for old drm with Xorg being in charge (void function now).
- */
-/**
- * radeon_driver_lastclose_kms - drm callback for last close
- *
- * @dev: drm dev pointer
- *
- * Switch vga_switcheroo state after last close (all asics).
- */
-void radeon_driver_lastclose_kms(struct drm_device *dev)
-{
-	drm_fb_helper_lastclose(dev);
-	vga_switcheroo_process_delayed_switch();
-}
-
 /**
  * radeon_driver_open_kms - drm callback for open
  *
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 365e6ddbe90f..6a198cb80c12 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -926,8 +926,8 @@ 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. DRM automatically calls this
+ * at the end of its lastclose function.
  *
  * 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.39.0

Re: [Nouveau] [PATCH v2 3/3] drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

[Lukas Wunner]

On Thu, Jan 12, 2023 at 09:11:56PM +0100, Thomas Zimmermann wrote:
> Several lastclose helpers call vga_switcheroo_process_delayed_switch().
> It's better to call the helper from drm_lastclose() after the kernel
> client's screen has been restored. This way, all drivers can benefit
> without having to implement their own lastclose helper. For drivers
> without vga-switcheroo, vga_switcheroo_process_delayed_switch() does
> nothing.
[...]
> --- 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>
> @@ -460,6 +461,8 @@ void drm_lastclose(struct drm_device * dev)
>  		drm_legacy_dev_reinit(dev);
>  
>  	drm_client_dev_restore(dev);
> +
> +	vga_switcheroo_process_delayed_switch();
>  }

Hm, this looks like a case of midlayer fallacy:

https://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html

It is a departure from the opt-in library approach we've had so far.

For switcheroo-aware EDID retrieval, there's a drm_get_edid_switcheroo()
helper.  How about introducing a switcheroo-aware lastclose helper which
drivers can reference?

Thanks,

Lukas

Re: [Nouveau] [PATCH v2 3/3] drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2594 bytes --]

Hi

Am 17.01.23 um 10:44 schrieb Lukas Wunner:
> On Thu, Jan 12, 2023 at 09:11:56PM +0100, Thomas Zimmermann wrote:
>> Several lastclose helpers call vga_switcheroo_process_delayed_switch().
>> It's better to call the helper from drm_lastclose() after the kernel
>> client's screen has been restored. This way, all drivers can benefit
>> without having to implement their own lastclose helper. For drivers
>> without vga-switcheroo, vga_switcheroo_process_delayed_switch() does
>> nothing.
> [...]
>> --- 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>
>> @@ -460,6 +461,8 @@ void drm_lastclose(struct drm_device * dev)
>>   		drm_legacy_dev_reinit(dev);
>>   
>>   	drm_client_dev_restore(dev);
>> +
>> +	vga_switcheroo_process_delayed_switch();
>>   }
> 
> Hm, this looks like a case of midlayer fallacy:
> 
> https://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html
> 
> It is a departure from the opt-in library approach we've had so far.
> 
> For switcheroo-aware EDID retrieval, there's a drm_get_edid_switcheroo()
> helper.  How about introducing a switcheroo-aware lastclose helper which
> drivers can reference?

Console emulation is a mess. I'm working on it, but it takes time. 
Therefore a nice solution is probably not possible ATM.

We could have something like drm_fb_helper_lastclose_switcheroo(), which 
does drm_fb_helper_lastclose() + 
vga_switcheroo_process_delayed_switch(). i915 and radeon could use that 
as-is. amdgpu and nouveau have already switched to generic fbdev 
emulation. We could use drm_fb_helper_lastclose_switcheroo() from the 
generic code as well, but it will still be somewhat midlayerish.

But with all that, it's probably better to not land patch 3 at all. The 
hook drm_driver.lastclose is just for the old fbdev emulation. For the 
new fbdev emulation, we have better data structures (see drm_client_dev).

The correct design would be to switch amdgpu and nouveau back to their 
own fbdev emulation and handle switcheroo there. But their old fbdev 
code needed an overhaul, so just reverting isn't an option.

Best regards
Thomas

> 
> Thanks,
> 
> Lukas

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [PATCH v2 3/3] drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

[Lyude Paul]

Acked-by: Lyude Paul <lyude@redhat.com>

On Thu, 2023-01-12 at 21:11 +0100, Thomas Zimmermann wrote:
> Several lastclose helpers call vga_switcheroo_process_delayed_switch().
> It's better to call the helper from drm_lastclose() after the kernel
> client's screen has been restored. This way, all drivers can benefit
> without having to implement their own lastclose helper. For drivers
> without vga-switcheroo, vga_switcheroo_process_delayed_switch() does
> nothing.
> 
> There was an earlier patchset to do something similar. [1]
> 
> v2:
> 	* handle vga_switcheroo_client_fb_set() in a separate patch
> 	* also update i915, nouveau and radeon
> 	* remove unnecessary include statements
> 	* update vga-switcheroo docs
> 
> Suggested-by: Alexander Deucher <Alexander.Deucher@amd.com>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> Link: https://lore.kernel.org/amd-gfx/20221020143603.563929-1-alexander.deucher@amd.com/ # 1
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 --
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 13 -------------
>  drivers/gpu/drm/drm_file.c              |  3 +++
>  drivers/gpu/drm/i915/i915_driver.c      | 25 ++-----------------------
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
>  drivers/gpu/drm/nouveau/nouveau_vga.c   |  7 -------
>  drivers/gpu/drm/nouveau/nouveau_vga.h   |  1 -
>  drivers/gpu/drm/radeon/radeon_drv.c     |  2 +-
>  drivers/gpu/drm/radeon/radeon_drv.h     |  1 -
>  drivers/gpu/drm/radeon/radeon_kms.c     | 18 ------------------
>  drivers/gpu/vga/vga_switcheroo.c        |  4 ++--
>  12 files changed, 8 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 63c921c55fb9..7120b9b6e580 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1330,7 +1330,6 @@ extern const int amdgpu_max_kms_ioctl;
>  
>  int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags);
>  void amdgpu_driver_unload_kms(struct drm_device *dev);
> -void amdgpu_driver_lastclose_kms(struct drm_device *dev);
>  int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv);
>  void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  				 struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 1353ffd08988..783c1e284a22 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -34,7 +34,6 @@
>  #include <drm/drm_pciids.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/vga_switcheroo.h>
>  #include <drm/drm_probe_helper.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/suspend.h>
> @@ -2785,7 +2784,6 @@ static const struct drm_driver amdgpu_kms_driver = {
>  	    DRIVER_SYNCOBJ_TIMELINE,
>  	.open = amdgpu_driver_open_kms,
>  	.postclose = amdgpu_driver_postclose_kms,
> -	.lastclose = amdgpu_driver_lastclose_kms,
>  	.ioctls = amdgpu_ioctls_kms,
>  	.num_ioctls = ARRAY_SIZE(amdgpu_ioctls_kms),
>  	.dumb_create = amdgpu_mode_dumb_create,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 7aa7e52ca784..a37be02fb2fc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -34,7 +34,6 @@
>  #include "amdgpu_vce.h"
>  #include "atom.h"
>  
> -#include <linux/vga_switcheroo.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/pci.h>
> @@ -1104,18 +1103,6 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  /*
>   * Outdated mess for old drm with Xorg being in charge (void function now).
>   */
> -/**
> - * amdgpu_driver_lastclose_kms - drm callback for last close
> - *
> - * @dev: drm dev pointer
> - *
> - * Switch vga_switcheroo state after last close (all asics).
> - */
> -void amdgpu_driver_lastclose_kms(struct drm_device *dev)
> -{
> -	drm_fb_helper_lastclose(dev);
> -	vga_switcheroo_process_delayed_switch();
> -}
>  
>  /**
>   * amdgpu_driver_open_kms - drm callback for open
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index a51ff8cee049..314c309db9a3 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>
> @@ -460,6 +461,8 @@ void drm_lastclose(struct drm_device * dev)
>  		drm_legacy_dev_reinit(dev);
>  
>  	drm_client_dev_restore(dev);
> +
> +	vga_switcheroo_process_delayed_switch();
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 33e231b120c1..bf6ad8620970 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -29,6 +29,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/device.h>
> +#include <linux/fb.h> /* for FBINFO_STATE_ */
>  #include <linux/module.h>
>  #include <linux/oom.h>
>  #include <linux/pci.h>
> @@ -37,7 +38,6 @@
>  #include <linux/pnp.h>
>  #include <linux/slab.h>
>  #include <linux/string_helpers.h>
> -#include <linux/vga_switcheroo.h>
>  #include <linux/vt.h>
>  
>  #include <drm/drm_aperture.h>
> @@ -1057,27 +1057,6 @@ static int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>  	return 0;
>  }
>  
> -/**
> - * i915_driver_lastclose - clean up after all DRM clients have exited
> - * @dev: DRM device
> - *
> - * Take care of cleaning up after all DRM clients have exited.  In the
> - * mode setting case, we want to restore the kernel's initial mode (just
> - * in case the last client left us in a bad state).
> - *
> - * Additionally, in the non-mode setting case, we'll tear down the GTT
> - * and DMA structures, since the kernel won't be using them, and clea
> - * up any GEM state.
> - */
> -static void i915_driver_lastclose(struct drm_device *dev)
> -{
> -	struct drm_i915_private *i915 = to_i915(dev);
> -
> -	intel_fbdev_restore_mode(dev);
> -
> -	vga_switcheroo_process_delayed_switch();
> -}
> -
>  static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_i915_file_private *file_priv = file->driver_priv;
> @@ -1921,7 +1900,7 @@ static const struct drm_driver i915_drm_driver = {
>  	    DRIVER_SYNCOBJ_TIMELINE,
>  	.release = i915_driver_release,
>  	.open = i915_driver_open,
> -	.lastclose = i915_driver_lastclose,
> +	.lastclose = intel_fbdev_restore_mode,
>  	.postclose = i915_driver_postclose,
>  
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 80f154b6adab..c3bd2a1d3d2e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1230,7 +1230,6 @@ driver_stub = {
>  
>  	.open = nouveau_drm_open,
>  	.postclose = nouveau_drm_postclose,
> -	.lastclose = nouveau_vga_lastclose,
>  
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = nouveau_drm_debugfs_init,
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index 789393b94291..fc125e2247f7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -126,10 +126,3 @@ nouveau_vga_fini(struct nouveau_drm *drm)
>  	if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
>  		vga_switcheroo_fini_domain_pm_ops(drm->dev->dev);
>  }
> -
> -
> -void
> -nouveau_vga_lastclose(struct drm_device *dev)
> -{
> -	vga_switcheroo_process_delayed_switch();
> -}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.h b/drivers/gpu/drm/nouveau/nouveau_vga.h
> index 951a83f984dd..63be415d2a44 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.h
> @@ -4,6 +4,5 @@
>  
>  void nouveau_vga_init(struct nouveau_drm *);
>  void nouveau_vga_fini(struct nouveau_drm *);
> -void nouveau_vga_lastclose(struct drm_device *dev);
>  
>  #endif
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 6cbe1ab81aba..dbf85e1f0061 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -597,7 +597,7 @@ static const struct drm_driver kms_driver = {
>  	.load = radeon_driver_load_kms,
>  	.open = radeon_driver_open_kms,
>  	.postclose = radeon_driver_postclose_kms,
> -	.lastclose = radeon_driver_lastclose_kms,
> +	.lastclose = drm_fb_helper_lastclose,
>  	.unload = radeon_driver_unload_kms,
>  	.ioctls = radeon_ioctls_kms,
>  	.num_ioctls = ARRAY_SIZE(radeon_ioctls_kms),
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.h b/drivers/gpu/drm/radeon/radeon_drv.h
> index ac7970919c4d..2ffe0975ee54 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.h
> +++ b/drivers/gpu/drm/radeon/radeon_drv.h
> @@ -120,7 +120,6 @@ long radeon_drm_ioctl(struct file *filp,
>  
>  int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags);
>  void radeon_driver_unload_kms(struct drm_device *dev);
> -void radeon_driver_lastclose_kms(struct drm_device *dev);
>  int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv);
>  void radeon_driver_postclose_kms(struct drm_device *dev,
>  				 struct drm_file *file_priv);
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 965161b8565b..e0214cf1b43b 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -32,7 +32,6 @@
>  #include <linux/uaccess.h>
>  #include <linux/vga_switcheroo.h>
>  
> -#include <drm/drm_fb_helper.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_ioctl.h>
>  #include <drm/radeon_drm.h>
> @@ -622,23 +621,6 @@ int radeon_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  	return 0;
>  }
>  
> -
> -/*
> - * Outdated mess for old drm with Xorg being in charge (void function now).
> - */
> -/**
> - * radeon_driver_lastclose_kms - drm callback for last close
> - *
> - * @dev: drm dev pointer
> - *
> - * Switch vga_switcheroo state after last close (all asics).
> - */
> -void radeon_driver_lastclose_kms(struct drm_device *dev)
> -{
> -	drm_fb_helper_lastclose(dev);
> -	vga_switcheroo_process_delayed_switch();
> -}
> -
>  /**
>   * radeon_driver_open_kms - drm callback for open
>   *
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 365e6ddbe90f..6a198cb80c12 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -926,8 +926,8 @@ 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. DRM automatically calls this
> + * at the end of its lastclose function.
>   *
>   * 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

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat