[Intel-xe] [PATCH] drm/xe: Improve s2idle handling.

[Intel-xe] [PATCH] drm/xe: Improve s2idle handling.

[Maarten Lankhorst]

We accidentally always pass true as s2idle argument, instead of
calculating it in the same way as i915.

Suspend modes were removed to achieve compatibility with i915, but
accidentally left in the source code.

While at it, fix all other cases too, s2idle will go into a D1 state and
setting a lower power state should be handled by PCI core.

Maybe my laptop stops draining so much power during suspend now? I can
only hope..

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h | 6 ------
 drivers/gpu/drm/xe/xe_display.c                           | 6 ++++--
 drivers/gpu/drm/xe/xe_pci.c                               | 6 ------
 3 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
index 9251c05dfd11..ce2b9bf3918d 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
@@ -5,12 +5,6 @@
 
 #include "intel_wakeref.h"
 
-enum i915_drm_suspend_mode {
-	I915_DRM_SUSPEND_IDLE,
-	I915_DRM_SUSPEND_MEM,
-	I915_DRM_SUSPEND_HIBERNATE,
-};
-
 static inline void disable_rpm_wakeref_asserts(void *rpm)
 {
 }
diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
index 8e39fb7f1fc8..ec7a654bf587 100644
--- a/drivers/gpu/drm/xe/xe_display.c
+++ b/drivers/gpu/drm/xe/xe_display.c
@@ -333,6 +333,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
 
 void xe_display_pm_suspend(struct xe_device *xe)
 {
+	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
 	if (!xe->info.enable_display)
 		return;
 
@@ -352,7 +353,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
 
 	intel_suspend_encoders(xe);
 
-	intel_opregion_suspend(xe, PCI_D3cold);
+	intel_opregion_suspend(xe, s2idle ? PCI_D1 : PCI_D3cold);
 
 	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
 
@@ -361,10 +362,11 @@ void xe_display_pm_suspend(struct xe_device *xe)
 
 void xe_display_pm_suspend_late(struct xe_device *xe)
 {
+	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
 	if (!xe->info.enable_display)
 		return;
 
-	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
+	intel_power_domains_suspend(xe, s2idle);
 
 	intel_display_power_suspend_late(xe);
 }
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 78df43c20cd2..63387d99b3ed 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -728,10 +728,6 @@ static int xe_pci_suspend(struct device *dev)
 	pci_save_state(pdev);
 	pci_disable_device(pdev);
 
-	err = pci_set_power_state(pdev, PCI_D3hot);
-	if (err)
-		return err;
-
 	return 0;
 }
 
@@ -744,8 +740,6 @@ static int xe_pci_resume(struct device *dev)
 	if (err)
 		return err;
 
-	pci_restore_state(pdev);
-
 	err = pci_enable_device(pdev);
 	if (err)
 		return err;
-- 
2.39.2

Re: [Intel-xe] [PATCH] drm/xe: Improve s2idle handling.

[Rodrigo Vivi]

On Fri, Jul 28, 2023 at 04:13:22PM +0200, Maarten Lankhorst wrote:
> We accidentally always pass true as s2idle argument, instead of
> calculating it in the same way as i915.
> 
> Suspend modes were removed to achieve compatibility with i915, but
> accidentally left in the source code.
> 
> While at it, fix all other cases too, s2idle will go into a D1 state and
> setting a lower power state should be handled by PCI core.
> 
> Maybe my laptop stops draining so much power during suspend now? I can
> only hope..
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h | 6 ------
>  drivers/gpu/drm/xe/xe_display.c                           | 6 ++++--
>  drivers/gpu/drm/xe/xe_pci.c                               | 6 ------
>  3 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> index 9251c05dfd11..ce2b9bf3918d 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> @@ -5,12 +5,6 @@
>  
>  #include "intel_wakeref.h"
>  
> -enum i915_drm_suspend_mode {
> -	I915_DRM_SUSPEND_IDLE,
> -	I915_DRM_SUSPEND_MEM,
> -	I915_DRM_SUSPEND_HIBERNATE,
> -};

good catch on this one. let's kill it.

> -
>  static inline void disable_rpm_wakeref_asserts(void *rpm)
>  {
>  }
> diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
> index 8e39fb7f1fc8..ec7a654bf587 100644
> --- a/drivers/gpu/drm/xe/xe_display.c
> +++ b/drivers/gpu/drm/xe/xe_display.c
> @@ -333,6 +333,7 @@ static void intel_suspend_encoders(struct xe_device *xe)
>  
>  void xe_display_pm_suspend(struct xe_device *xe)
>  {
> +	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>  	if (!xe->info.enable_display)
>  		return;
>  
> @@ -352,7 +353,7 @@ void xe_display_pm_suspend(struct xe_device *xe)
>  
>  	intel_suspend_encoders(xe);
>  
> -	intel_opregion_suspend(xe, PCI_D3cold);
> +	intel_opregion_suspend(xe, s2idle ? PCI_D1 : PCI_D3cold);

I don't believe we need this. From what I could dig from i915 history
this was needed in some old bios. But setting the lowest d3cold
should be okay for any platform supported in xe.

>  
>  	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
>  
> @@ -361,10 +362,11 @@ void xe_display_pm_suspend(struct xe_device *xe)
>  
>  void xe_display_pm_suspend_late(struct xe_device *xe)
>  {
> +	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>  	if (!xe->info.enable_display)
>  		return;
>  
> -	intel_power_domains_suspend(xe, I915_DRM_SUSPEND_MEM);
> +	intel_power_domains_suspend(xe, s2idle);

hmm good catch here as well. I believe it should deserve a separated patch.

But anyway, the patch itself in general is already a good step forward,
specially if this is fixing your battery drainage. So, let's move ahead
with it and leave any bikesheding for later ;)

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Btw, please check https://patchwork.freedesktop.org/series/121336/ that
maybe affects you as well.

>
>  	intel_display_power_suspend_late(xe);
>  }
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 78df43c20cd2..63387d99b3ed 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -728,10 +728,6 @@ static int xe_pci_suspend(struct device *dev)
>  	pci_save_state(pdev);
>  	pci_disable_device(pdev);
>  
> -	err = pci_set_power_state(pdev, PCI_D3hot);
> -	if (err)
> -		return err;

yeap, we likely don't need to toggle ourselves the pci state on the
regular suspend/resume path.

> -
>  	return 0;
>  }
>  
> @@ -744,8 +740,6 @@ static int xe_pci_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> -	pci_restore_state(pdev);
> -
>  	err = pci_enable_device(pdev);
>  	if (err)
>  		return err;
> -- 
> 2.39.2
>