[Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Rodrigo Vivi]

First of all it was strange to see:
if (allowed) {
...
} else {
   D3COLD_ENABLE
}

But besides this misalignment, let's also use the pci
d3cold_allowed useful to us and know that we are not really
allowing d3cold.

Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 78df43c20cd2..0c4051f4f746 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -794,6 +794,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
 	pci_save_state(pdev);
 
 	if (xe->d3cold.allowed) {
+		d3cold_toggle(pdev, D3COLD_ENABLE);
 		pci_disable_device(pdev);
 		pci_ignore_hotplug(pdev);
 		pci_set_power_state(pdev, PCI_D3cold);
@@ -823,8 +824,6 @@ static int xe_pci_runtime_resume(struct device *dev)
 			return err;
 
 		pci_set_master(pdev);
-	} else {
-		d3cold_toggle(pdev, D3COLD_ENABLE);
 	}
 
 	return xe_pm_runtime_resume(xe);
-- 
2.41.0

[Intel-gfx] [PATCH 2/4] drm/xe: Move d3cold_allowed decision all together.

[Rodrigo Vivi]

And let's use the VRAM threshold to keep d3cold temporarily disabled.

With this we have the ability to run D3Cold experiments just by
touching the vram_d3cold_threshold sysfs entry.

Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_pci.c | 15 +--------------
 drivers/gpu/drm/xe/xe_pm.c  |  5 +++++
 drivers/gpu/drm/xe/xe_pm.h  |  7 ++++++-
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 0c4051f4f746..06759afb4224 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -834,20 +834,7 @@ static int xe_pci_runtime_idle(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct xe_device *xe = pdev_to_xe_device(pdev);
 
-	if (!xe->d3cold.capable) {
-		xe->d3cold.allowed = false;
-	} else {
-		xe_pm_d3cold_allowed_toggle(xe);
-
-		/*
-		 * TODO: d3cold should be allowed (true) if
-		 * (IS_DGFX(xe) && !xe_device_mem_access_ongoing(xe))
-		 * but maybe include some other conditions. So, before
-		 * we can re-enable the D3cold, we need to:
-		 * 1. rewrite the VRAM save / restore to avoid buffer object locks
-		 */
-		xe->d3cold.allowed = false;
-	}
+	xe_pm_d3cold_allowed_toggle(xe);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 17a69b7af155..a6459df2599e 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -326,6 +326,11 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
 	u64 vram_used;
 	int i;
 
+	if (!xe->d3cold.capable) {
+		xe->d3cold.allowed = false;
+		return;
+	}
+
 	for (i = XE_PL_VRAM0; i <= XE_PL_VRAM1; ++i) {
 		man = ttm_manager_type(&xe->ttm, i);
 		if (man) {
diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
index 08a633ce5145..6b9031f7af24 100644
--- a/drivers/gpu/drm/xe/xe_pm.h
+++ b/drivers/gpu/drm/xe/xe_pm.h
@@ -8,7 +8,12 @@
 
 #include <linux/pm_runtime.h>
 
-#define DEFAULT_VRAM_THRESHOLD 300 /* in MB */
+/*
+ * TODO: Threshold = 0 will block D3Cold.
+ *       Before we can move this to a higher value (like 300), we need to:
+ *           1. rewrite the VRAM save / restore to avoid buffer object locks
+ */
+#define DEFAULT_VRAM_THRESHOLD 0 /* in MB */
 
 struct xe_device;
 
-- 
2.41.0

[Intel-gfx] [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed decision.

[Rodrigo Vivi]

According to Documentation/power/runtime_pm.txt:

int pm_runtime_put(struct device *dev);
    - decrement the device's usage counter; if the result is 0 then run
      pm_request_idle(dev) and return its result

int pm_runtime_put_autosuspend(struct device *dev);
    - decrement the device's usage counter; if the result is 0 then run
      pm_request_autosuspend(dev) and return its result

We need to ensure that the idle function is called before suspending
so we take the right d3cold.allowed decision and respect the values
set on vram_d3cold_threshold sysfs. So we need pm_runtime_put()
instead of pm_runtime_put_autosuspend().

Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index a6459df2599e..73bcb76c2d42 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe)
 	pm_runtime_set_active(dev);
 	pm_runtime_allow(dev);
 	pm_runtime_mark_last_busy(dev);
-	pm_runtime_put_autosuspend(dev);
+	pm_runtime_put(dev);
 }
 
 void xe_pm_init(struct xe_device *xe)
@@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe)
 int xe_pm_runtime_put(struct xe_device *xe)
 {
 	pm_runtime_mark_last_busy(xe->drm.dev);
-	return pm_runtime_put_autosuspend(xe->drm.dev);
+	return pm_runtime_put(xe->drm.dev);
 }
 
 int xe_pm_runtime_get_if_active(struct xe_device *xe)
-- 
2.41.0

[Intel-gfx] [PATCH 4/4] drm/xe: Only init runtime PM after all d3cold config is in place.

[Rodrigo Vivi]

We cannot allow runtime pm suspend after we configured the
d3cold capable and threshold.

Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/xe_pm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 73bcb76c2d42..366ee446af38 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -152,10 +152,12 @@ void xe_pm_init(struct xe_device *xe)
 	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
 
 	drmm_mutex_init(&xe->drm, &xe->d3cold.lock);
-	xe_pm_runtime_init(xe);
+
 	xe->d3cold.capable = xe_pm_pci_d3cold_capable(pdev);
 	xe_device_sysfs_init(xe);
 	xe_pm_set_vram_threshold(xe, DEFAULT_VRAM_THRESHOLD);
+
+	xe_pm_runtime_init(xe);
 }
 
 void xe_pm_runtime_fini(struct xe_device *xe)
-- 
2.41.0

[Intel-gfx] ✗ Fi.CI.BUILD: failure for series starting with [1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.
URL   : https://patchwork.freedesktop.org/series/121071/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/121071/revisions/1/mbox/ not applied
Applying: drm/xe: Only set PCI d3cold_allowed when we are really allowing.
error: sha1 information is lacking or useless (drivers/gpu/drm/xe/xe_pci.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/xe: Only set PCI d3cold_allowed when we are really allowing.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

Re: [Intel-gfx] [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed decision.

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Friday, July 21, 2023 2:34 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed
> decision.
> 
> According to Documentation/power/runtime_pm.txt:
I tried to fix runtime idle https://patchwork.freedesktop.org/patch/543024/?series=119467&rev=1 
But forgot to CC to you.
Anyway some comment below,

> 
> int pm_runtime_put(struct device *dev);
>     - decrement the device's usage counter; if the result is 0 then run
>       pm_request_idle(dev) and return its result
> 
> int pm_runtime_put_autosuspend(struct device *dev);
>     - decrement the device's usage counter; if the result is 0 then run
>       pm_request_autosuspend(dev) and return its result
> 
> We need to ensure that the idle function is called before suspending so we
> take the right d3cold.allowed decision and respect the values set on
> vram_d3cold_threshold sysfs. So we need pm_runtime_put() instead of
> pm_runtime_put_autosuspend().
> 
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index a6459df2599e..73bcb76c2d42 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe)
>  	pm_runtime_set_active(dev);
>  	pm_runtime_allow(dev);
>  	pm_runtime_mark_last_busy(dev);
I have not thought of using last_busy() here in _put().
If we mark last_busy during _put then pm core auto-suspend timer will start ticking from _put().
Theoretically  that can lead to idle() and runtime_suspend() call to race with each other ? [1]
[1] Documentation/power/runtime_pm.txt
(1) The callbacks are mutually exclusive (e.g. it is forbidden to execute
    ->runtime_suspend() in parallel with ->runtime_resume() or with another
    instance of ->runtime_suspend() for the same device) with the exception that
    ->runtime_suspend() or ->runtime_resume() can be executed in parallel with
    ->runtime_idle() (although ->runtime_idle() will not be started while any
    of the other callbacks is being executed for the same device).
Thanks,
Anshuman Gupta.
> -	pm_runtime_put_autosuspend(dev);
> +	pm_runtime_put(dev);
We need to fix this in intel_runtime_pm_put()  compat-i915-headers as well.
>  }
> 
>  void xe_pm_init(struct xe_device *xe)
> @@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe)  int
> xe_pm_runtime_put(struct xe_device *xe)  {
>  	pm_runtime_mark_last_busy(xe->drm.dev);
> -	return pm_runtime_put_autosuspend(xe->drm.dev);
> +	return pm_runtime_put(xe->drm.dev);
>  }
> 
>  int xe_pm_runtime_get_if_active(struct xe_device *xe)
> --
> 2.41.0

Re: [Intel-gfx] [PATCH 4/4] drm/xe: Only init runtime PM after all d3cold config is in place.

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Friday, July 21, 2023 2:34 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: [PATCH 4/4] drm/xe: Only init runtime PM after all d3cold config is in
> place.
> 
> We cannot allow runtime pm suspend after we configured the d3cold
> capable and threshold.
> 
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 73bcb76c2d42..366ee446af38 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -152,10 +152,12 @@ void xe_pm_init(struct xe_device *xe)
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> 
>  	drmm_mutex_init(&xe->drm, &xe->d3cold.lock);
> -	xe_pm_runtime_init(xe);
> +
>  	xe->d3cold.capable = xe_pm_pci_d3cold_capable(pdev);
>  	xe_device_sysfs_init(xe);
>  	xe_pm_set_vram_threshold(xe, DEFAULT_VRAM_THRESHOLD);
> +
> +	xe_pm_runtime_init(xe);
LGTM.
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>  }
> 
>  void xe_pm_runtime_fini(struct xe_device *xe)
> --
> 2.41.0

Re: [Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Friday, July 21, 2023 2:34 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> really allowing.
> 
> First of all it was strange to see:
> if (allowed) {
> ...
> } else {
>    D3COLD_ENABLE
> }
> 
> But besides this misalignment, let's also use the pci d3cold_allowed useful to
> us and know that we are not really allowing d3cold.
> 
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 78df43c20cd2..0c4051f4f746 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -794,6 +794,7 @@ static int xe_pci_runtime_suspend(struct device
> *dev)
>  	pci_save_state(pdev);
> 
>  	if (xe->d3cold.allowed) {
> +		d3cold_toggle(pdev, D3COLD_ENABLE);
>  		pci_disable_device(pdev);
>  		pci_ignore_hotplug(pdev);
>  		pci_set_power_state(pdev, PCI_D3cold); @@ -823,8 +824,6
> @@ static int xe_pci_runtime_resume(struct device *dev)
>  			return err;
> 
>  		pci_set_master(pdev);
> -	} else {
> -		d3cold_toggle(pdev, D3COLD_ENABLE);
>  	}
During s2idle , d3cold may get disabled if won't restore it's state in runtime resume.
Thanks,
Anshuman Gupta.
> 
>  	return xe_pm_runtime_resume(xe);
> --
> 2.41.0

Re: [Intel-gfx] [PATCH 2/4] drm/xe: Move d3cold_allowed decision all together.

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Friday, July 21, 2023 2:34 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: [PATCH 2/4] drm/xe: Move d3cold_allowed decision all together.
> 
> And let's use the VRAM threshold to keep d3cold temporarily disabled.
> 
> With this we have the ability to run D3Cold experiments just by touching the
> vram_d3cold_threshold sysfs entry.
> 
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
LGTM,
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pci.c | 15 +--------------
> drivers/gpu/drm/xe/xe_pm.c  |  5 +++++  drivers/gpu/drm/xe/xe_pm.h  |  7
> ++++++-
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 0c4051f4f746..06759afb4224 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -834,20 +834,7 @@ static int xe_pci_runtime_idle(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct xe_device *xe = pdev_to_xe_device(pdev);
> 
> -	if (!xe->d3cold.capable) {
> -		xe->d3cold.allowed = false;
> -	} else {
> -		xe_pm_d3cold_allowed_toggle(xe);
> -
> -		/*
> -		 * TODO: d3cold should be allowed (true) if
> -		 * (IS_DGFX(xe) && !xe_device_mem_access_ongoing(xe))
> -		 * but maybe include some other conditions. So, before
> -		 * we can re-enable the D3cold, we need to:
> -		 * 1. rewrite the VRAM save / restore to avoid buffer object
> locks
> -		 */
> -		xe->d3cold.allowed = false;
> -	}
> +	xe_pm_d3cold_allowed_toggle(xe);
> 
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 17a69b7af155..a6459df2599e 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -326,6 +326,11 @@ void xe_pm_d3cold_allowed_toggle(struct
> xe_device *xe)
>  	u64 vram_used;
>  	int i;
> 
> +	if (!xe->d3cold.capable) {
> +		xe->d3cold.allowed = false;
> +		return;
> +	}
> +
>  	for (i = XE_PL_VRAM0; i <= XE_PL_VRAM1; ++i) {
>  		man = ttm_manager_type(&xe->ttm, i);
>  		if (man) {
> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> index 08a633ce5145..6b9031f7af24 100644
> --- a/drivers/gpu/drm/xe/xe_pm.h
> +++ b/drivers/gpu/drm/xe/xe_pm.h
> @@ -8,7 +8,12 @@
> 
>  #include <linux/pm_runtime.h>
> 
> -#define DEFAULT_VRAM_THRESHOLD 300 /* in MB */
> +/*
> + * TODO: Threshold = 0 will block D3Cold.
> + *       Before we can move this to a higher value (like 300), we need to:
> + *           1. rewrite the VRAM save / restore to avoid buffer object locks
> + */
> +#define DEFAULT_VRAM_THRESHOLD 0 /* in MB */
> 
>  struct xe_device;
> 
> --
> 2.41.0

Re: [Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Rodrigo Vivi]

On Fri, Jul 21, 2023 at 03:39:35AM -0400, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Friday, July 21, 2023 2:34 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>
> > Subject: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> > really allowing.
> > 
> > First of all it was strange to see:
> > if (allowed) {
> > ...
> > } else {
> >    D3COLD_ENABLE
> > }
> > 
> > But besides this misalignment, let's also use the pci d3cold_allowed useful to
> > us and know that we are not really allowing d3cold.
> > 
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pci.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index 78df43c20cd2..0c4051f4f746 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -794,6 +794,7 @@ static int xe_pci_runtime_suspend(struct device
> > *dev)
> >  	pci_save_state(pdev);
> > 
> >  	if (xe->d3cold.allowed) {
> > +		d3cold_toggle(pdev, D3COLD_ENABLE);
> >  		pci_disable_device(pdev);
> >  		pci_ignore_hotplug(pdev);
> >  		pci_set_power_state(pdev, PCI_D3cold); @@ -823,8 +824,6
> > @@ static int xe_pci_runtime_resume(struct device *dev)
> >  			return err;
> > 
> >  		pci_set_master(pdev);
> > -	} else {
> > -		d3cold_toggle(pdev, D3COLD_ENABLE);
> >  	}
> During s2idle , d3cold may get disabled if won't restore it's state in runtime resume.

I always forget about that case... please disregard this patch.

> Thanks,
> Anshuman Gupta.
> > 
> >  	return xe_pm_runtime_resume(xe);
> > --
> > 2.41.0
> 

Re: [Intel-gfx] [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed decision.

[Rodrigo Vivi]

On Fri, Jul 21, 2023 at 02:00:52AM -0400, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Friday, July 21, 2023 2:34 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>
> > Subject: [PATCH 3/4] drm/xe: Fix the runtime_idle call and d3cold.allowed
> > decision.
> > 
> > According to Documentation/power/runtime_pm.txt:
> I tried to fix runtime idle https://patchwork.freedesktop.org/patch/543024/?series=119467&rev=1 
> But forgot to CC to you.

I'm really sorry for having missed that.

2 comments on your version:

1. it forgets to remove the autosuspend from the init,
so on the very first entry at driver load it may miss the idle call.

2. I don't like the way other drivers are doing with idle. The rpm infra
expects 0 return to then call the suspend. I really don't understand
because I few drivers decided to workaround that and return 1 and call
the autosuspend themselves from within the idle.

> Anyway some comment below,
> 
> > 
> > int pm_runtime_put(struct device *dev);
> >     - decrement the device's usage counter; if the result is 0 then run
> >       pm_request_idle(dev) and return its result
> > 
> > int pm_runtime_put_autosuspend(struct device *dev);
> >     - decrement the device's usage counter; if the result is 0 then run
> >       pm_request_autosuspend(dev) and return its result
> > 
> > We need to ensure that the idle function is called before suspending so we
> > take the right d3cold.allowed decision and respect the values set on
> > vram_d3cold_threshold sysfs. So we need pm_runtime_put() instead of
> > pm_runtime_put_autosuspend().
> > 
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_pm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index a6459df2599e..73bcb76c2d42 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -144,7 +144,7 @@ static void xe_pm_runtime_init(struct xe_device *xe)
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_allow(dev);
> >  	pm_runtime_mark_last_busy(dev);
> I have not thought of using last_busy() here in _put().
> If we mark last_busy during _put then pm core auto-suspend timer will start ticking from _put().
> Theoretically  that can lead to idle() and runtime_suspend() call to race with each other ? [1]

that shouldn't happen if you are using the rpm as designed and returning 0
from idle instead of 1 and autosuspend.

> [1] Documentation/power/runtime_pm.txt
> (1) The callbacks are mutually exclusive (e.g. it is forbidden to execute
>     ->runtime_suspend() in parallel with ->runtime_resume() or with another
>     instance of ->runtime_suspend() for the same device) with the exception that
>     ->runtime_suspend() or ->runtime_resume() can be executed in parallel with
>     ->runtime_idle() (although ->runtime_idle() will not be started while any
>     of the other callbacks is being executed for the same device).
> Thanks,
> Anshuman Gupta.
> > -	pm_runtime_put_autosuspend(dev);
> > +	pm_runtime_put(dev);
> We need to fix this in intel_runtime_pm_put()  compat-i915-headers as well.

I can't see that... I see the compat headers calling the xe_ variants
so we should be covered from here.

what exactly am I missing?

> >  }
> > 
> >  void xe_pm_init(struct xe_device *xe)
> > @@ -273,7 +273,7 @@ int xe_pm_runtime_get(struct xe_device *xe)  int
> > xe_pm_runtime_put(struct xe_device *xe)  {
> >  	pm_runtime_mark_last_busy(xe->drm.dev);
> > -	return pm_runtime_put_autosuspend(xe->drm.dev);
> > +	return pm_runtime_put(xe->drm.dev);
> >  }
> > 
> >  int xe_pm_runtime_get_if_active(struct xe_device *xe)
> > --
> > 2.41.0
> 

Re: [Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Saturday, July 22, 2023 12:30 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> really allowing.
> 
> On Fri, Jul 21, 2023 at 03:39:35AM -0400, Gupta, Anshuman wrote:
> >
> >
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Sent: Friday, July 21, 2023 2:34 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> > > <anshuman.gupta@intel.com>
> > > Subject: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> > > really allowing.
> > >
> > > First of all it was strange to see:
> > > if (allowed) {
> > > ...
> > > } else {
> > >    D3COLD_ENABLE
> > > }
> > >
> > > But besides this misalignment, let's also use the pci d3cold_allowed
> > > useful to us and know that we are not really allowing d3cold.
> > >
> > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pci.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_pci.c
> > > b/drivers/gpu/drm/xe/xe_pci.c index 78df43c20cd2..0c4051f4f746
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > @@ -794,6 +794,7 @@ static int xe_pci_runtime_suspend(struct device
> > > *dev)
> > >  	pci_save_state(pdev);
> > >
> > >  	if (xe->d3cold.allowed) {
> > > +		d3cold_toggle(pdev, D3COLD_ENABLE);
> > >  		pci_disable_device(pdev);
> > >  		pci_ignore_hotplug(pdev);
> > >  		pci_set_power_state(pdev, PCI_D3cold); @@ -823,8 +824,6
> @@ static
> > > int xe_pci_runtime_resume(struct device *dev)
> > >  			return err;
> > >
> > >  		pci_set_master(pdev);
> > > -	} else {
> > > -		d3cold_toggle(pdev, D3COLD_ENABLE);
> > >  	}
> > During s2idle , d3cold may get disabled if won't restore it's state in runtime
> resume.
> 
> I always forget about that case... please disregard this patch.
I think , we can have this patch.
I realized that system wide suspend/resume d3cold need some brainstorming.
If device is already in runtime suspend state during system wide suspend, PM core may smartly skip device suspend/resume callback on Xe driver.
This wasn't the case on I915 driver as it explicitly disables those smart optimization by 
dev_pm_set_driver_flags(kdev, DPM_FLAG_NO_DIRECT_COMPLETE). 
Thanks,
Anshuman.
Thanks,
Anshuman Gupta.
> 
> > Thanks,
> > Anshuman Gupta.
> > >
> > >  	return xe_pm_runtime_resume(xe);
> > > --
> > > 2.41.0
> >

Re: [Intel-gfx] [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are really allowing.

[Rodrigo Vivi]

On Tue, Jul 25, 2023 at 01:08:11AM -0400, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Saturday, July 22, 2023 12:30 AM
> > To: Gupta, Anshuman <anshuman.gupta@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> > really allowing.
> > 
> > On Fri, Jul 21, 2023 at 03:39:35AM -0400, Gupta, Anshuman wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Sent: Friday, July 21, 2023 2:34 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta, Anshuman
> > > > <anshuman.gupta@intel.com>
> > > > Subject: [PATCH 1/4] drm/xe: Only set PCI d3cold_allowed when we are
> > > > really allowing.
> > > >
> > > > First of all it was strange to see:
> > > > if (allowed) {
> > > > ...
> > > > } else {
> > > >    D3COLD_ENABLE
> > > > }
> > > >
> > > > But besides this misalignment, let's also use the pci d3cold_allowed
> > > > useful to us and know that we are not really allowing d3cold.
> > > >
> > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_pci.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c
> > > > b/drivers/gpu/drm/xe/xe_pci.c index 78df43c20cd2..0c4051f4f746
> > > > 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > @@ -794,6 +794,7 @@ static int xe_pci_runtime_suspend(struct device
> > > > *dev)
> > > >  	pci_save_state(pdev);
> > > >
> > > >  	if (xe->d3cold.allowed) {
> > > > +		d3cold_toggle(pdev, D3COLD_ENABLE);
> > > >  		pci_disable_device(pdev);
> > > >  		pci_ignore_hotplug(pdev);
> > > >  		pci_set_power_state(pdev, PCI_D3cold); @@ -823,8 +824,6
> > @@ static
> > > > int xe_pci_runtime_resume(struct device *dev)
> > > >  			return err;
> > > >
> > > >  		pci_set_master(pdev);
> > > > -	} else {
> > > > -		d3cold_toggle(pdev, D3COLD_ENABLE);
> > > >  	}
> > > During s2idle , d3cold may get disabled if won't restore it's state in runtime
> > resume.
> > 
> > I always forget about that case... please disregard this patch.
> I think , we can have this patch.
> I realized that system wide suspend/resume d3cold need some brainstorming.
> If device is already in runtime suspend state during system wide suspend, PM core may smartly skip device suspend/resume callback on Xe driver.
> This wasn't the case on I915 driver as it explicitly disables those smart optimization by 
> dev_pm_set_driver_flags(kdev, DPM_FLAG_NO_DIRECT_COMPLETE). 

so, it looks that we do need this as well anyway.

1. because we might not be in the runtime-d3cold, but on the system suspend
   we will lose power, hence the eviction/restore needs to happen.
2. because of the hda audio as documented in i915: "becaue the HDA driver
   may require us to enable the audio power domain during system suspend."

then, on device suspend we enable the d3cold and disable again on
device resume.

> Thanks,
> Anshuman.
> Thanks,
> Anshuman Gupta.
> > 
> > > Thanks,
> > > Anshuman Gupta.
> > > >
> > > >  	return xe_pm_runtime_resume(xe);
> > > > --
> > > > 2.41.0
> > >