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

2023-01-30 Thread Lyude Paul 
Acked-by: Lyude Paul 

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 
> Signed-off-by: Thomas Zimmermann 
> Reviewed-by: Daniel Vetter 
> Reviewed-by: Alex Deucher 
> Link: 
> https://lore.kernel.org/amd-gfx/20221020143603.563929-1-alexander.deuc...@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 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -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 
>  #include 
>  #include 
>  #include 
> @@ -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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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 
>  #include 
> +#include  /* for FBINFO_STATE_ */
>  #include 
>  #include 
>  #include 
> @@ -37,7 +38,6 @@
>  #include 
>  #include 
>  #include

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

2023-01-17 Thread Thomas Zimmermann 

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 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -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


OpenPGP_signature
Description: OpenPGP digital signature

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

2023-01-17 Thread 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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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

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

2023-01-12 Thread 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 
Signed-off-by: Thomas Zimmermann 
Reviewed-by: Daniel Vetter 
Reviewed-by: Alex Deucher 
Link: 
https://lore.kernel.org/amd-gfx/20221020143603.563929-1-alexander.deuc...@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 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -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 
 #include 
 #include 
 #include 
@@ -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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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 
 #include 
+#include  /* for FBINFO_STATE_ */
 #include 
 #include 
 #include 
@@ -37,7 +38,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include 
@@ -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
- *