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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>