[Intel-gfx] [PATCH 0/8] drm/i915: Add D3Cold-Off support for runtime-pm

[Intel-gfx] [PATCH 0/8] drm/i915: Add D3Cold-Off support for runtime-pm

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

1. Added D3Cold-Off support for runtime pm for discrete gpu.
2. LMEM is switched off and gpu is in off state in D3Cold-Off
   so, lmem & GT deep suspend/resume is added.
3. Re-used i915_gem_backup_suspend, i915_gem_suspend_late
   and i915_gem_resume to handle above 2.
4. These functions use runtime helpers, which in-turn call
   runtime suspend/resume callbacks and leads to deadlock.
   So, these helpers need to be avoided.
5. Added is_intel_rpm_allowed and disallowed rpm callbacks
   during suspending and resuming.
6. Integrated D3Cold policy patch, but is a FIXME, as LMEM
   usage is not queried, lmem->avail stopped tracking lmem 
   usage after ttm port.
7. Added/used i915_save/load_pci_state helpers
8. In intel_guc_global_policies_update, guarded intel_guc_is_ready
   with rpm helpers as it needs guc interaction.
9. Fixed error *ERROR DC state mismatch (0x8 -> 0x0)".
10. Guarded rc6 rpm helpers with is_intel_rpm_allowed as these
    are called in suspend/resume cllbacks. 

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>

Anshuman Gupta (1):
  Drm/i915/rpm: Add intel_runtime_idle

Aravind Iddamsetty (1):
  drm/i915: Add i915_save/load_pci_state helpers

Tilak Tangudu (6):
  drm/i915: Added is_intel_rpm_allowed helper
  drm/i915: Guard rc6 helpers with is_intel_rpm_allowed
  drm/i915: Extend rpm in intel_guc_global_policies_update
  drm/i915: sanitize dc state in rpm resume
  drm/i915/rpm: d3cold Policy
  drm/i915 : Add D3COLD OFF support

 .../drm/i915/display/intel_display_power.c    |   1 +
 drivers/gpu/drm/i915/gt/intel_rc6.c           |  13 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    |   9 +-
 drivers/gpu/drm/i915/i915_driver.c            | 126 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_driver.h            |   2 +
 drivers/gpu/drm/i915/i915_drv.h               |   1 +
 drivers/gpu/drm/i915/i915_params.c            |   5 +
 drivers/gpu/drm/i915/i915_params.h            |   1 +
 drivers/gpu/drm/i915/intel_pm.c               |  35 +++++
 drivers/gpu/drm/i915/intel_pm.h               |   1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c       |  26 +++-
 drivers/gpu/drm/i915/intel_runtime_pm.h       |   8 ++
 12 files changed, 206 insertions(+), 22 deletions(-)

-- 
2.25.1

[Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

Added is_intel_rpm_allowed function to query the runtime_pm
status and disllow during suspending and resuming.

v2: Return -2 if runtime pm is not allowed in runtime_pm_get
and skip wakeref release in runtime_pm_put if wakeref value
is -2. - Jani N
Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 23 ++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6ed5786bcd29..704beeeb560b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -113,7 +113,7 @@ static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
 	unsigned long flags, n;
 	bool found = false;
 
-	if (unlikely(stack == -1))
+	if (unlikely(stack == -1) || unlikely(stack == -2))
 		return;
 
 	spin_lock_irqsave(&rpm->debug.lock, flags);
@@ -320,6 +320,21 @@ untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)
 }
 
 #endif
+static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
+{
+	return rpm->kdev->power.runtime_status;
+}
+
+bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
+{
+	int rpm_status;
+
+	rpm_status = intel_runtime_pm_status(rpm);
+	if (rpm_status == RPM_RESUMING || rpm_status == RPM_SUSPENDING)
+		return false;
+	else
+		return true;
+}
 
 static void
 intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock)
@@ -354,6 +369,9 @@ static intel_wakeref_t __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
 						     runtime_pm);
 	int ret;
 
+	if (!is_intel_rpm_allowed(rpm))
+		return -2;
+
 	ret = pm_runtime_get_sync(rpm->kdev);
 	drm_WARN_ONCE(&i915->drm, ret < 0,
 		      "pm_runtime_get_sync() failed: %d\n", ret);
@@ -490,6 +508,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
 
 	untrack_intel_runtime_pm_wakeref(rpm, wref);
 
+	if (wref == -2)
+		return;
+
 	intel_runtime_pm_release(rpm, wakelock);
 
 	pm_runtime_mark_last_busy(kdev);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
index d9160e3ff4af..99418c3a934a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
+bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
 
 intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
 intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
-- 
2.25.1

[Intel-gfx] [PATCH 2/8] drm/i915: Guard rc6 helpers with is_intel_rpm_allowed

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

Guard intel_rc6_sanitize/intel_rc6_enable/intel_rc6_disable
rc6 helpers with is_intel_rpm_allowed as these
are called in intel_gt_resume/intel_gt_suspend_late.

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rc6.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
index f8d0523f4c18..73e2fb9420a2 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -507,9 +507,14 @@ static bool rc6_supported(struct intel_rc6 *rc6)
 
 static void rpm_get(struct intel_rc6 *rc6)
 {
+	struct drm_i915_private *i915 = rc6_to_i915(rc6);
+
 	GEM_BUG_ON(rc6->wakeref);
-	pm_runtime_get_sync(rc6_to_i915(rc6)->drm.dev);
-	rc6->wakeref = true;
+
+	if (is_intel_rpm_allowed(&i915->runtime_pm)) {
+		pm_runtime_get_sync(i915->drm.dev);
+		rc6->wakeref = true;
+	}
 }
 
 static void rpm_put(struct intel_rc6 *rc6)
@@ -623,7 +628,9 @@ void intel_rc6_enable(struct intel_rc6 *rc6)
 		return;
 
 	/* rc6 is ready, runtime-pm is go! */
-	rpm_put(rc6);
+	if (rc6->wakeref)
+		rpm_put(rc6);
+
 	rc6->enabled = true;
 }
 
-- 
2.25.1

[Intel-gfx] [PATCH 3/8] drm/i915: Extend rpm in intel_guc_global_policies_update

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

intel_guc_is_ready need to be guarded with rpm
helpers as it needs guc interaction.

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index ba7541f3ca61..1d3b3559420d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -195,11 +195,14 @@ int intel_guc_global_policies_update(struct intel_guc *guc)
 
 	guc_policies_init(guc);
 
-	if (!intel_guc_is_ready(guc))
+	wakeref = intel_runtime_pm_get(&gt->i915->runtime_pm);
+	if (!intel_guc_is_ready(guc)) {
+		intel_runtime_pm_put(&gt->i915->runtime_pm, wakeref);
 		return 0;
+	}
 
-	with_intel_runtime_pm(&gt->i915->runtime_pm, wakeref)
-		ret = guc_action_policies_update(guc, scheduler_policies);
+	ret = guc_action_policies_update(guc, scheduler_policies);
+	intel_runtime_pm_put(&gt->i915->runtime_pm, wakeref);
 
 	return ret;
 }
-- 
2.25.1

[Intel-gfx] [PATCH 4/8] drm/i915: sanitize dc state in rpm resume

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

During runtime resume the display init sequence is called via
intel_display_power_resume() -> icl_display_core_init()
which should restore the display HW state. For restoring the DC9 enabled
state in DC_STATE_EN, gen9_sanitize_dc_state() should be called on the
 runtime resume path too to avoid the

[  513.818190] i915 0000:03:00.0: [drm] *ERROR DC state mismatch (0x8 -> 0x0)*

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_power.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 589af257edeb..799f84d3eed6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -2229,6 +2229,7 @@ void intel_display_power_suspend(struct drm_i915_private *i915)
 void intel_display_power_resume(struct drm_i915_private *i915)
 {
 	if (DISPLAY_VER(i915) >= 11) {
+		gen9_sanitize_dc_state(i915);
 		bxt_disable_dc9(i915);
 		icl_display_core_init(i915, true);
 		if (intel_dmc_has_payload(i915)) {
-- 
2.25.1

[Intel-gfx] [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle

[tilak.tangudu]

From: Anshuman Gupta <anshuman.gupta@intel.com>

Adding intel_runtime_idle (runtime_idle callback) to prepare the
tageted D3 state.

Since we have introduced i915 runtime_idle callback.
It need to be warranted that Runtime PM Core invokes runtime_idle
callback when runtime usages count becomes zero. That requires
to use pm_runtime_put instead of pm_runtime_put_autosuspend.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris.p.wilson@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c      | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index deb8a8b76965..4c36554567fd 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1576,6 +1576,17 @@ static int i915_pm_restore(struct device *kdev)
 	return i915_pm_resume(kdev);
 }
 
+static int intel_runtime_idle(struct device *kdev)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(kdev);
+	int ret = 1;
+
+	pm_runtime_mark_last_busy(kdev);
+	pm_runtime_autosuspend(kdev);
+
+	return ret;
+}
+
 static int intel_runtime_suspend(struct device *kdev)
 {
 	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
@@ -1752,6 +1763,7 @@ const struct dev_pm_ops i915_pm_ops = {
 	.restore = i915_pm_restore,
 
 	/* S0ix (via runtime suspend) event handlers */
+	.runtime_idle = intel_runtime_idle,
 	.runtime_suspend = intel_runtime_suspend,
 	.runtime_resume = intel_runtime_resume,
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 704beeeb560b..1c3ed0c29330 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -513,8 +513,7 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
 
 	intel_runtime_pm_release(rpm, wakelock);
 
-	pm_runtime_mark_last_busy(kdev);
-	pm_runtime_put_autosuspend(kdev);
+	pm_runtime_put(kdev);
 }
 
 /**
-- 
2.25.1

[Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

Add d3cold_sr_lmem_threshold modparam to choose between
d3cold-off zero watt and  d3hot/d3cold-VRAM Self Refresh.
i915 requires to evict the lmem objects to smem in order to
support d3cold-Off. if platform does not supports vram_sr
feature then fall back to d3hot by disabling d3cold to
avoid the rpm suspend/resume latency.
Extend the d3cold_sr_lmem_threshold modparam to debugfs
i915_params so that, it can be used by igt test.

If gfx root port is not capable of sending PME from d3cold
or doesn't have _PR3 power resources then only d3hot state
can be supported.

Adding intel_pm_prepare_targeted_d3_state() to choose the
correct target d3 state and cache it to intel_runtime_pm
structure, it can be used in rpm suspend/resume callback
accordingly.

v2: lmem->avail stopped tracking lmem usage since ttm is
introduced, so removed lmem->avail usage in policy.
FIXME here, lmem usage is not added, need to be added
by using query functions.
FIXME, Forcing the policy to enter D3COLD_OFF for
validation purpose.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
 drivers/gpu/drm/i915/i915_params.c      |  5 ++++
 drivers/gpu/drm/i915/i915_params.h      |  1 +
 drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.h         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
 6 files changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 4c36554567fd..2b2e9563f149 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
 	struct drm_i915_private *i915 = kdev_to_i915(kdev);
 	int ret = 1;
 
+	disable_rpm_wakeref_asserts(&i915->runtime_pm);
+	ret = intel_pm_prepare_targeted_d3_state(i915);
+	if (!ret)
+		ret = 1;
+
+	enable_rpm_wakeref_asserts(&i915->runtime_pm);
 	pm_runtime_mark_last_busy(kdev);
 	pm_runtime_autosuspend(kdev);
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 6fc475a5db61..4603f5c2ed77 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
 	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
 #endif
 
+i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
+	"Enable VRAM Self refresh when size of lmem is greater to this threshold. "
+	"If VRAM Self Refresh is not available then fall back to d3cold. "
+	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
+
 #if CONFIG_DRM_I915_REQUEST_TIMEOUT
 i915_param_named_unsafe(request_timeout_ms, uint, 0600,
 			"Default request/fence/batch buffer expiration timeout.");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 2733cb6cfe09..1a86711038da 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -75,6 +75,7 @@ struct drm_printer;
 	param(unsigned int, request_timeout_ms, CONFIG_DRM_I915_REQUEST_TIMEOUT, CONFIG_DRM_I915_REQUEST_TIMEOUT ? 0600 : 0) \
 	param(unsigned int, lmem_size, 0, 0400) \
 	param(unsigned int, lmem_bar_size, 0, 0400) \
+	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
 	/* leave bools at the end to not create holes */ \
 	param(bool, enable_hangcheck, true, 0600) \
 	param(bool, load_detect_test, false, 0600) \
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f06babdb3a8c..20b0638ecd5c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private *dev_priv)
 	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);
 }
 
+int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915)
+{
+	struct intel_runtime_pm *rpm = &i915->runtime_pm;
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
+	u64 lmem_used = 0;
+	struct pci_dev *root_pdev;
+	int ret = 0;
+
+	/* igfx will return from here */
+	root_pdev = pcie_find_root_port(pdev);
+	if (!root_pdev)
+		return ret;
+
+	/* D3Cold requires PME capability and _PR3 power resource */
+	if (!pci_pme_capable(root_pdev, PCI_D3cold) || !pci_pr3_present(root_pdev))
+		return ret;
+
+	/* FXME query the LMEM usage and fill lmem_used */
+	/* Trigger D3COLD_OFF always to validate with all tests */
+	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 * 1024) {
+		rpm->d3_state = INTEL_D3COLD_OFF;
+		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
+	} else {
+		/* Disable D3Cold to reduce the eviction latency */
+		rpm->d3_state = INTEL_D3HOT;
+	}
+
+	if (rpm->d3_state == INTEL_D3HOT)
+		pci_d3cold_disable(root_pdev);
+	else
+		pci_d3cold_enable(root_pdev);
+
+	return ret;
+}
+
 static struct intel_global_state *intel_dbuf_duplicate_state(struct intel_global_obj *obj)
 {
 	struct intel_dbuf_state *dbuf_state;
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 945503ae493e..7827b0c1a2f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private *dev_priv);
 void intel_init_pm(struct drm_i915_private *dev_priv);
 void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
 void intel_pm_setup(struct drm_i915_private *dev_priv);
+int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915);
 void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);
 void vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);
 void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
index 99418c3a934a..568559b71b70 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
 	I915_DRM_SUSPEND_HIBERNATE,
 };
 
+enum intel_gfx_d3_state {
+	INTEL_D3HOT,
+	INTEL_D3COLD_OFF,
+	INTEL_D3COLD_VRAM_SR,
+};
+
 /*
  * This struct helps tracking the state needed for runtime PM, which puts the
  * device in PCI D3 state. Notice that when this happens, nothing on the
@@ -52,6 +58,7 @@ struct intel_runtime_pm {
 	bool suspended;
 	bool irqs_enabled;
 	bool no_wakeref_tracking;
+	enum intel_gfx_d3_state d3_state;
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
 	/*
-- 
2.25.1

[Intel-gfx] [PATCH 7/8] drm/i915: Add i915_save/load_pci_state helpers

[tilak.tangudu]

From: Aravind Iddamsetty <aravind.iddamsetty@intel.com>

Add i915_save/load_pci_state helpers which saves
pci config state and restores the saved state.

Signed-off-by: Aravind Iddamsetty  <aravind.iddamsetty@intel.com>
Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 34 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_driver.h |  2 ++
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 2b2e9563f149..3697ecb2c138 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -105,6 +105,40 @@ static const char irst_name[] = "INT3392";
 
 static const struct drm_driver i915_drm_driver;
 
+bool i915_save_pci_state(struct pci_dev *pdev)
+{
+	struct drm_i915_private *i915 = pci_get_drvdata(pdev);
+
+	if (pci_save_state(pdev))
+		return false;
+
+	kfree(i915->pci_state);
+
+	i915->pci_state = pci_store_saved_state(pdev);
+
+	if (!i915->pci_state) {
+		drm_err(&i915->drm, "Failed to store PCI saved state\n");
+		return false;
+	}
+
+	return true;
+}
+
+void i915_load_pci_state(struct pci_dev *pdev)
+{
+	struct drm_i915_private *i915 = pci_get_drvdata(pdev);
+	int ret;
+
+	if (!i915->pci_state)
+		return;
+
+	ret = pci_load_saved_state(pdev, i915->pci_state);
+	if (!ret) {
+		pci_restore_state(pdev);
+	} else {
+		drm_warn(&i915->drm, "Failed to load PCI state, err:%d\n", ret);
+	}
+}
 static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
 {
 	int domain = pci_domain_nr(to_pci_dev(dev_priv->drm.dev)->bus);
diff --git a/drivers/gpu/drm/i915/i915_driver.h b/drivers/gpu/drm/i915/i915_driver.h
index 44ec543d92cb..fb19db69bc3f 100644
--- a/drivers/gpu/drm/i915/i915_driver.h
+++ b/drivers/gpu/drm/i915/i915_driver.h
@@ -26,6 +26,8 @@ void i915_driver_shutdown(struct drm_i915_private *i915);
 
 int i915_driver_resume_switcheroo(struct drm_i915_private *i915);
 int i915_driver_suspend_switcheroo(struct drm_i915_private *i915, pm_message_t state);
+bool i915_save_pci_state(struct pci_dev *pdev);
+void i915_load_pci_state(struct pci_dev *pdev);
 
 void
 i915_print_iommu_status(struct drm_i915_private *i915, struct drm_printer *p);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d25647be25d1..c30ac9219b7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -777,6 +777,7 @@ struct drm_i915_private {
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
 	 */
+	struct pci_saved_state *pci_state;
 };
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
-- 
2.25.1

[Intel-gfx] [PATCH 8/8] drm/i915 : Add D3COLD OFF support

[tilak.tangudu]

From: Tilak Tangudu <tilak.tangudu@intel.com>

Added lmem deep suspend/resume, which covers lmem
eviction and added GT/GUC deep suspend/resume
using i915_gem_backup_suspend, i915_gem_suspend_late
and i915_gem_resume.

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 74 ++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 3697ecb2c138..608287bb27ea 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1630,6 +1630,7 @@ static int intel_runtime_idle(struct device *kdev)
 static int intel_runtime_suspend(struct device *kdev)
 {
 	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
 	int ret;
 
@@ -1644,9 +1645,14 @@ static int intel_runtime_suspend(struct device *kdev)
 	 * We are safe here against re-faults, since the fault handler takes
 	 * an RPM reference.
 	 */
-	i915_gem_runtime_suspend(dev_priv);
-
-	intel_gt_runtime_suspend(to_gt(dev_priv));
+	if (rpm->d3_state == INTEL_D3COLD_OFF) {
+		i915_gem_backup_suspend(dev_priv);
+		i915_ggtt_suspend(to_gt(dev_priv)->ggtt);
+		i915_gem_suspend_late(dev_priv);
+	} else {
+		i915_gem_runtime_suspend(dev_priv);
+		intel_gt_runtime_suspend(to_gt(dev_priv));
+	}
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
 
@@ -1691,14 +1697,18 @@ static int intel_runtime_suspend(struct device *kdev)
 		 */
 		intel_opregion_notify_adapter(dev_priv, PCI_D3hot);
 	} else {
-		/*
-		 * current versions of firmware which depend on this opregion
-		 * notification have repurposed the D1 definition to mean
-		 * "runtime suspended" vs. what you would normally expect (D3)
-		 * to distinguish it from notifications that might be sent via
-		 * the suspend path.
-		 */
-		intel_opregion_notify_adapter(dev_priv, PCI_D1);
+		if (rpm->d3_state == INTEL_D3COLD_OFF) {
+			intel_opregion_suspend(dev_priv, PCI_D3cold);
+		} else {
+			/*
+			 * current versions of firmware which depend on this opregion
+			 * notification have repurposed the D1 definition to mean
+			 * "runtime suspended" vs. what you would normally expect (D3)
+			 * to distinguish it from notifications that might be sent via
+			 * the suspend path.
+			 */
+			intel_opregion_notify_adapter(dev_priv, PCI_D1);
+		}
 	}
 
 	assert_forcewakes_inactive(&dev_priv->uncore);
@@ -1706,6 +1716,12 @@ static int intel_runtime_suspend(struct device *kdev)
 	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
 		intel_hpd_poll_enable(dev_priv);
 
+	if (rpm->d3_state == INTEL_D3COLD_OFF) {
+		i915_save_pci_state(pdev);
+		pci_disable_device(pdev);
+		pci_set_power_state(pdev, PCI_D3cold);
+	}
+
 	drm_dbg(&dev_priv->drm, "Device suspended\n");
 	return 0;
 }
@@ -1713,6 +1729,7 @@ static int intel_runtime_suspend(struct device *kdev)
 static int intel_runtime_resume(struct device *kdev)
 {
 	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
 	int ret;
 
@@ -1724,7 +1741,25 @@ static int intel_runtime_resume(struct device *kdev)
 	drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm->wakeref_count));
 	disable_rpm_wakeref_asserts(rpm);
 
-	intel_opregion_notify_adapter(dev_priv, PCI_D0);
+	if (rpm->d3_state == INTEL_D3COLD_OFF) {
+		ret = pci_set_power_state(pdev, PCI_D0);
+		if (ret) {
+			drm_err(&dev_priv->drm,
+				"failed to set PCI D0 power state (%d)\n", ret);
+			goto out;
+		}
+
+		i915_load_pci_state(pdev);
+
+		ret = pci_enable_device(pdev);
+		if (ret)
+			goto out;
+		pci_set_master(pdev);
+		intel_opregion_resume(dev_priv);
+	} else {
+		intel_opregion_notify_adapter(dev_priv, PCI_D0);
+	}
+
 	rpm->suspended = false;
 	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
 		drm_dbg(&dev_priv->drm,
@@ -1742,8 +1777,20 @@ static int intel_runtime_resume(struct device *kdev)
 	 * No point of rolling back things in case of an error, as the best
 	 * we can do is to hope that things will still work (and disable RPM).
 	 */
-	intel_gt_runtime_resume(to_gt(dev_priv));
+	if (rpm->d3_state == INTEL_D3COLD_OFF) {
+		ret = i915_pcode_init(dev_priv);
+		if (ret)
+			goto out;
 
+		sanitize_gpu(dev_priv);
+		ret = i915_ggtt_enable_hw(dev_priv);
+		if (ret)
+			drm_err(&dev_priv->drm, "failed to re-enable GGTT\n");
+		i915_ggtt_resume(to_gt(dev_priv)->ggtt);
+		i915_gem_resume(dev_priv);
+	} else {
+		intel_gt_runtime_resume(to_gt(dev_priv));
+	}
 	/*
 	 * On VLV/CHV display interrupts are part of the display
 	 * power well, so hpd is reinitialized from there. For
@@ -1756,6 +1803,7 @@ static int intel_runtime_resume(struct device *kdev)
 
 	intel_enable_ipc(dev_priv);
 
+out:
 	enable_rpm_wakeref_asserts(rpm);
 
 	if (ret)
-- 
2.25.1

Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[Gupta, Anshuman]

> -----Original Message-----
> From: Tangudu, Tilak <tilak.tangudu@intel.com>
> Sent: Thursday, July 21, 2022 3:30 PM
> To: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta,
> Anshuman <anshuman.gupta@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>; Deak,
> Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: [PATCH 6/8] drm/i915/rpm: d3cold Policy
> 
> From: Tilak Tangudu <tilak.tangudu@intel.com>
Please don't change the authorship of patch.
> 
> Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off zero
> watt and  d3hot/d3cold-VRAM Self Refresh.
> i915 requires to evict the lmem objects to smem in order to support d3cold-Off.
> if platform does not supports vram_sr feature then fall back to d3hot by
> disabling d3cold to avoid the rpm suspend/resume latency.
> Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params so
> that, it can be used by igt test.
> 
> If gfx root port is not capable of sending PME from d3cold or doesn't have _PR3
> power resources then only d3hot state can be supported.
> 
> Adding intel_pm_prepare_targeted_d3_state() to choose the correct target d3
> state and cache it to intel_runtime_pm structure, it can be used in rpm
> suspend/resume callback accordingly.
> 
> v2: lmem->avail stopped tracking lmem usage since ttm is introduced, so
> removed lmem->avail usage in policy.
> FIXME here, lmem usage is not added, need to be added by using query
> functions.
> FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
>  drivers/gpu/drm/i915/i915_params.c      |  5 ++++
>  drivers/gpu/drm/i915/i915_params.h      |  1 +
>  drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.h         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
>  6 files changed, 55 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 4c36554567fd..2b2e9563f149 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
>  	struct drm_i915_private *i915 = kdev_to_i915(kdev);
>  	int ret = 1;
> 
> +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> +	ret = intel_pm_prepare_targeted_d3_state(i915);
> +	if (!ret)
> +		ret = 1;
> +
> +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
>  	pm_runtime_mark_last_busy(kdev);
>  	pm_runtime_autosuspend(kdev);
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 6fc475a5db61..4603f5c2ed77 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
>  	"Enable support for Intel GVT-g graphics virtualization host
> support(default:false)");  #endif
> 
> +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
> +	"Enable VRAM Self refresh when size of lmem is greater to this
> threshold. "
> +	"If VRAM Self Refresh is not available then fall back to d3cold. "
> +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> +
>  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
>  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
>  			"Default request/fence/batch buffer expiration
> timeout."); diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 2733cb6cfe09..1a86711038da 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -75,6 +75,7 @@ struct drm_printer;
>  	param(unsigned int, request_timeout_ms,
> CONFIG_DRM_I915_REQUEST_TIMEOUT,
> CONFIG_DRM_I915_REQUEST_TIMEOUT ? 0600 : 0) \
>  	param(unsigned int, lmem_size, 0, 0400) \
>  	param(unsigned int, lmem_bar_size, 0, 0400) \
> +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
>  	/* leave bools at the end to not create holes */ \
>  	param(bool, enable_hangcheck, true, 0600) \
>  	param(bool, load_detect_test, false, 0600) \ diff --git
> a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index
> f06babdb3a8c..20b0638ecd5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private
> *dev_priv)
>  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);  }
> 
> +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915) {
> +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> +	u64 lmem_used = 0;
> +	struct pci_dev *root_pdev;
> +	int ret = 0;
> +
> +	/* igfx will return from here */
> +	root_pdev = pcie_find_root_port(pdev);
> +	if (!root_pdev)
> +		return ret;
> +
> +	/* D3Cold requires PME capability and _PR3 power resource */
> +	if (!pci_pme_capable(root_pdev, PCI_D3cold) ||
> !pci_pr3_present(root_pdev))
> +		return ret;
> +
> +	/* FXME query the LMEM usage and fill lmem_used */
> +	/* Trigger D3COLD_OFF always to validate with all tests */
> +	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 *
> 1024) {
> +		rpm->d3_state = INTEL_D3COLD_OFF;
> +		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
> +	} else {
> +		/* Disable D3Cold to reduce the eviction latency */
> +		rpm->d3_state = INTEL_D3HOT;
> +	}
> +
> +	if (rpm->d3_state == INTEL_D3HOT)
> +		pci_d3cold_disable(root_pdev);
> +	else
> +		pci_d3cold_enable(root_pdev);
> +
> +	return ret;
> +}
> +
>  static struct intel_global_state *intel_dbuf_duplicate_state(struct
> intel_global_obj *obj)  {
>  	struct intel_dbuf_state *dbuf_state;
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 945503ae493e..7827b0c1a2f3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private
> *dev_priv);  void intel_init_pm(struct drm_i915_private *dev_priv);  void
> intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);  void
> intel_pm_setup(struct drm_i915_private *dev_priv);
> +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915);
>  void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> ilk_wm_get_hw_state(struct drm_i915_private *dev_priv); diff --git
> a/drivers/gpu/drm/i915/intel_runtime_pm.h
> b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index 99418c3a934a..568559b71b70 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
>  	I915_DRM_SUSPEND_HIBERNATE,
>  };
> 
> +enum intel_gfx_d3_state {
> +	INTEL_D3HOT,
> +	INTEL_D3COLD_OFF,
> +	INTEL_D3COLD_VRAM_SR,
> +};
> +
>  /*
>   * This struct helps tracking the state needed for runtime PM, which puts the
>   * device in PCI D3 state. Notice that when this happens, nothing on the @@ -
> 52,6 +58,7 @@ struct intel_runtime_pm {
>  	bool suspended;
>  	bool irqs_enabled;
>  	bool no_wakeref_tracking;
> +	enum intel_gfx_d3_state d3_state;
> 
>  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>  	/*
> --
> 2.25.1

Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[Tangudu, Tilak]

> -----Original Message-----
> From: Gupta, Anshuman <anshuman.gupta@intel.com>
> Sent: Thursday, July 21, 2022 5:00 PM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>; Ewins, Jon
> <jon.ewins@intel.com>; Belgaumkar, Vinay <vinay.belgaumkar@intel.com>;
> Roper, Matthew D <matthew.d.roper@intel.com>; Wilson, Chris P
> <chris.p.wilson@intel.com>; Nikula, Jani <jani.nikula@intel.com>; Gupta,
> saurabhg <saurabhg.gupta@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>; Deak,
> Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 6/8] drm/i915/rpm: d3cold Policy
> 
> 
> 
> > -----Original Message-----
> > From: Tangudu, Tilak <tilak.tangudu@intel.com>
> > Sent: Thursday, July 21, 2022 3:30 PM
> > To: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > <matthew.d.roper@intel.com>; Wilson, Chris P
> > <chris.p.wilson@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> > Gupta, saurabhg <saurabhg.gupta@intel.com>; Vivi, Rodrigo
> > <rodrigo.vivi@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>;
> > Nilawar, Badal <badal.nilawar@intel.com>; Tangudu, Tilak
> > <tilak.tangudu@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > intel-gfx@lists.freedesktop.org
> > Subject: [PATCH 6/8] drm/i915/rpm: d3cold Policy
> >
> > From: Tilak Tangudu <tilak.tangudu@intel.com>
> Please don't change the authorship of patch.
I have not changed and at the same time I missed to add author explicitly 
I will make a note of it

> >
> > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off
> > zero watt and  d3hot/d3cold-VRAM Self Refresh.
> > i915 requires to evict the lmem objects to smem in order to support d3cold-
> Off.
> > if platform does not supports vram_sr feature then fall back to d3hot
> > by disabling d3cold to avoid the rpm suspend/resume latency.
> > Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params
> so
> > that, it can be used by igt test.
> >
> > If gfx root port is not capable of sending PME from d3cold or doesn't
> > have _PR3 power resources then only d3hot state can be supported.
> >
> > Adding intel_pm_prepare_targeted_d3_state() to choose the correct
> > target d3 state and cache it to intel_runtime_pm structure, it can be
> > used in rpm suspend/resume callback accordingly.
> >
> > v2: lmem->avail stopped tracking lmem usage since ttm is introduced,
> > so removed lmem->avail usage in policy.
> > FIXME here, lmem usage is not added, need to be added by using query
> > functions.
> > FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
> >  drivers/gpu/drm/i915/i915_params.c      |  5 ++++
> >  drivers/gpu/drm/i915/i915_params.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.h         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
> >  6 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index 4c36554567fd..2b2e9563f149 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
> >  	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> >  	int ret = 1;
> >
> > +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> > +	ret = intel_pm_prepare_targeted_d3_state(i915);
> > +	if (!ret)
> > +		ret = 1;
> > +
> > +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> >  	pm_runtime_mark_last_busy(kdev);
> >  	pm_runtime_autosuspend(kdev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 6fc475a5db61..4603f5c2ed77 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
> >  	"Enable support for Intel GVT-g graphics virtualization host
> > support(default:false)");  #endif
> >
> > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
> > +	"Enable VRAM Self refresh when size of lmem is greater to this
> > threshold. "
> > +	"If VRAM Self Refresh is not available then fall back to d3cold. "
> > +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> > +
> >  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
> >  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
> >  			"Default request/fence/batch buffer expiration
> timeout."); diff
> > --git a/drivers/gpu/drm/i915/i915_params.h
> > b/drivers/gpu/drm/i915/i915_params.h
> > index 2733cb6cfe09..1a86711038da 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -75,6 +75,7 @@ struct drm_printer;
> >  	param(unsigned int, request_timeout_ms,
> > CONFIG_DRM_I915_REQUEST_TIMEOUT,
> CONFIG_DRM_I915_REQUEST_TIMEOUT ?
> > 0600 : 0) \
> >  	param(unsigned int, lmem_size, 0, 0400) \
> >  	param(unsigned int, lmem_bar_size, 0, 0400) \
> > +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
> >  	/* leave bools at the end to not create holes */ \
> >  	param(bool, enable_hangcheck, true, 0600) \
> >  	param(bool, load_detect_test, false, 0600) \ diff --git
> > a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f06babdb3a8c..20b0638ecd5c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private
> > *dev_priv)
> >  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);  }
> >
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915) {
> > +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > +	u64 lmem_used = 0;
> > +	struct pci_dev *root_pdev;
> > +	int ret = 0;
> > +
> > +	/* igfx will return from here */
> > +	root_pdev = pcie_find_root_port(pdev);
> > +	if (!root_pdev)
> > +		return ret;
> > +
> > +	/* D3Cold requires PME capability and _PR3 power resource */
> > +	if (!pci_pme_capable(root_pdev, PCI_D3cold) ||
> > !pci_pr3_present(root_pdev))
> > +		return ret;
> > +
> > +	/* FXME query the LMEM usage and fill lmem_used */
> > +	/* Trigger D3COLD_OFF always to validate with all tests */
> > +	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 *
> > 1024) {
> > +		rpm->d3_state = INTEL_D3COLD_OFF;
> > +		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
> > +	} else {
> > +		/* Disable D3Cold to reduce the eviction latency */
> > +		rpm->d3_state = INTEL_D3HOT;
> > +	}
> > +
> > +	if (rpm->d3_state == INTEL_D3HOT)
> > +		pci_d3cold_disable(root_pdev);
> > +	else
> > +		pci_d3cold_enable(root_pdev);
> > +
> > +	return ret;
> > +}
> > +
> >  static struct intel_global_state *intel_dbuf_duplicate_state(struct
> > intel_global_obj *obj)  {
> >  	struct intel_dbuf_state *dbuf_state; diff --git
> > a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 945503ae493e..7827b0c1a2f3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private
> > *dev_priv);  void intel_init_pm(struct drm_i915_private *dev_priv);
> > void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
> > void intel_pm_setup(struct drm_i915_private *dev_priv);
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private
> > +*i915);
> >  void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > ilk_wm_get_hw_state(struct drm_i915_private *dev_priv); diff --git
> > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index 99418c3a934a..568559b71b70 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
> >  	I915_DRM_SUSPEND_HIBERNATE,
> >  };
> >
> > +enum intel_gfx_d3_state {
> > +	INTEL_D3HOT,
> > +	INTEL_D3COLD_OFF,
> > +	INTEL_D3COLD_VRAM_SR,
> > +};
> > +
> >  /*
> >   * This struct helps tracking the state needed for runtime PM, which puts
> the
> >   * device in PCI D3 state. Notice that when this happens, nothing on
> > the @@ -
> > 52,6 +58,7 @@ struct intel_runtime_pm {
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  	bool no_wakeref_tracking;
> > +	enum intel_gfx_d3_state d3_state;
> >
> >  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> >  	/*
> > --
> > 2.25.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add D3Cold-Off support for runtime-pm (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Add D3Cold-Off support for runtime-pm (rev3)
URL   : https://patchwork.freedesktop.org/series/105427/
State : warning

== Summary ==

Error: dim checkpatch failed
4f11e13b3cc3 drm/i915: Added is_intel_rpm_allowed helper
6171adbfb722 drm/i915: Guard rc6 helpers with is_intel_rpm_allowed
b7376087143b drm/i915: Extend rpm in intel_guc_global_policies_update
af8d888e4322 drm/i915: sanitize dc state in rpm resume
b8b9121feb43 Drm/i915/rpm: Add intel_runtime_idle
11e785317730 drm/i915/rpm: d3cold Policy
-:61: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#61: FILE: drivers/gpu/drm/i915/i915_params.c:201:
+i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
+	"Enable VRAM Self refresh when size of lmem is greater to this threshold. "

total: 0 errors, 0 warnings, 1 checks, 97 lines checked
f6bc98625c72 drm/i915: Add i915_save/load_pci_state helpers
038d9ea49a5c drm/i915 : Add D3COLD OFF support

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Add D3Cold-Off support for runtime-pm (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 18801 bytes --]

== Series Details ==

Series: drm/i915: Add D3Cold-Off support for runtime-pm (rev3)
URL   : https://patchwork.freedesktop.org/series/105427/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11933 -> Patchwork_105427v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_105427v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_105427v3, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/index.html

Participating hosts (47 -> 45)
------------------------------

  Additional (3): fi-kbl-soraka bat-adls-5 bat-jsl-3 
  Missing    (5): fi-hsw-4200u fi-icl-u2 fi-ctg-p8600 fi-hsw-4770 fi-bdw-samus 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_105427v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-adl-ddr5:        [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-adl-ddr5/igt@i915_pm_rpm@basic-pci-d3-state.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-adl-ddr5/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-cfl-guc:         [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-cfl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-cfl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-bdw-5557u:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-bdw-5557u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bdw-5557u/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-rkl-guc:         [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-rkl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-rkl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-cfl-8700k:       [PASS][9] -> [DMESG-WARN][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-cfl-8700k/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-cfl-8700k/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-bdw-gvtdvm:      [PASS][11] -> [DMESG-WARN][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-bdw-gvtdvm/igt@i915_pm_rpm@basic-pci-d3-state.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bdw-gvtdvm/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-skl-guc:         [PASS][13] -> [DMESG-WARN][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-skl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
    - bat-adlp-4:         [PASS][15] -> [DMESG-WARN][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-hsw-g3258:       [PASS][17] -> [DMESG-WARN][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-hsw-g3258/igt@i915_pm_rpm@basic-pci-d3-state.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-hsw-g3258/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-glk-dsi:         [PASS][19] -> [DMESG-WARN][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-glk-dsi/igt@i915_pm_rpm@basic-pci-d3-state.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-glk-dsi/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-glk-j4005:       [PASS][21] -> [DMESG-WARN][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-glk-j4005/igt@i915_pm_rpm@basic-pci-d3-state.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-glk-j4005/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-rkl-11600:       [PASS][23] -> [DMESG-WARN][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-rkl-11600/igt@i915_pm_rpm@basic-pci-d3-state.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-rkl-11600/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-skl-6700k2:      [PASS][25] -> [DMESG-WARN][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-skl-6700k2/igt@i915_pm_rpm@basic-pci-d3-state.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-6700k2/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-cfl-8109u:       [PASS][27] -> [INCOMPLETE][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-cfl-8109u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-cfl-8109u/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-kbl-7567u:       [PASS][29] -> [INCOMPLETE][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-kbl-7567u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-7567u/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-kbl-8809g:       [PASS][31] -> [DMESG-WARN][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-kbl-8809g/igt@i915_pm_rpm@basic-pci-d3-state.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-8809g/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@basic-rte:
    - fi-adl-ddr5:        [PASS][33] -> [DMESG-FAIL][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-adl-ddr5/igt@i915_pm_rpm@basic-rte.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-adl-ddr5/igt@i915_pm_rpm@basic-rte.html
    - fi-rkl-guc:         [PASS][35] -> [INCOMPLETE][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-rkl-guc/igt@i915_pm_rpm@basic-rte.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-rkl-guc/igt@i915_pm_rpm@basic-rte.html
    - fi-bsw-kefka:       [PASS][37] -> [DMESG-WARN][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-bsw-kefka/igt@i915_pm_rpm@basic-rte.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bsw-kefka/igt@i915_pm_rpm@basic-rte.html
    - bat-dg1-5:          [PASS][39] -> [DMESG-WARN][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-dg1-5/igt@i915_pm_rpm@basic-rte.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-dg1-5/igt@i915_pm_rpm@basic-rte.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - {bat-adlp-6}:       [PASS][41] -> [DMESG-WARN][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-adlp-6/igt@i915_pm_rpm@basic-pci-d3-state.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adlp-6/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-jsl-3}:        NOTRUN -> [INCOMPLETE][43]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-jsl-3/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-dg2-8}:        [PASS][44] -> [INCOMPLETE][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-dg2-8/igt@i915_pm_rpm@basic-pci-d3-state.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-dg2-8/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-adlm-1}:       [PASS][46] -> [DMESG-WARN][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-adlm-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adlm-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-jsl-1}:        [PASS][48] -> [INCOMPLETE][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {fi-jsl-1}:         [PASS][50] -> [INCOMPLETE][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-rpls-1}:       [PASS][52] -> [DMESG-WARN][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-rpls-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-rpls-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-rplp-1}:       [PASS][54] -> [DMESG-WARN][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-rplp-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-rplp-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-adls-5}:       NOTRUN -> [DMESG-WARN][56]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adls-5/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-dg2-9}:        [PASS][57] -> [DMESG-WARN][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-dg2-9/igt@i915_pm_rpm@basic-pci-d3-state.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-dg2-9/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-adln-1}:       [PASS][59] -> [INCOMPLETE][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/bat-adln-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adln-1/igt@i915_pm_rpm@basic-pci-d3-state.html

  
Known issues
------------

  Here are the changes found in Patchwork_105427v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][61] ([fdo#109271]) +7 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-soraka/igt@gem_exec_gttfill@basic.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][62] ([fdo#109271] / [i915#2190])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-6600u:       [PASS][63] -> [DMESG-WARN][64] ([i915#4391])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][65] ([i915#4391])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-soraka/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-bxt-dsi:         [PASS][66] -> [DMESG-WARN][67] ([i915#4391])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-bxt-dsi/igt@i915_pm_rpm@basic-pci-d3-state.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bxt-dsi/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-kbl-x1275:       [PASS][68] -> [DMESG-WARN][69] ([i915#4391])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-kbl-x1275/igt@i915_pm_rpm@basic-pci-d3-state.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-x1275/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@module-reload:
    - fi-bsw-n3050:       [PASS][70] -> [INCOMPLETE][71] ([i915#5847])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bsw-n3050/igt@i915_pm_rpm@module-reload.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-elk-e7500:       NOTRUN -> [SKIP][72] ([fdo#109271])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-elk-e7500/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][73] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-soraka/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@runner@aborted:
    - bat-adlp-4:         NOTRUN -> [FAIL][74] ([i915#4312])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/bat-adlp-4/igt@runner@aborted.html
    - fi-bxt-dsi:         NOTRUN -> [FAIL][75] ([i915#4312] / [i915#5257])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bxt-dsi/igt@runner@aborted.html
    - fi-bsw-n3050:       NOTRUN -> [FAIL][76] ([fdo#109271] / [i915#4312])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bsw-n3050/igt@runner@aborted.html
    - fi-glk-dsi:         NOTRUN -> [FAIL][77] ([i915#4312] / [i915#5257])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-glk-dsi/igt@runner@aborted.html
    - fi-glk-j4005:       NOTRUN -> [FAIL][78] ([i915#4312] / [i915#5257])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-glk-j4005/igt@runner@aborted.html
    - fi-rkl-11600:       NOTRUN -> [FAIL][79] ([i915#4312])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-rkl-11600/igt@runner@aborted.html
    - fi-adl-ddr5:        NOTRUN -> [FAIL][80] ([i915#4312])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-adl-ddr5/igt@runner@aborted.html
    - fi-cfl-guc:         NOTRUN -> [FAIL][81] ([fdo#109271] / [i915#4312] / [i915#5257])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-cfl-guc/igt@runner@aborted.html
    - fi-kbl-x1275:       NOTRUN -> [FAIL][82] ([fdo#109271] / [i915#4312] / [i915#5257])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-x1275/igt@runner@aborted.html
    - fi-skl-6700k2:      NOTRUN -> [FAIL][83] ([i915#4312] / [i915#5257])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-6700k2/igt@runner@aborted.html
    - fi-kbl-8809g:       NOTRUN -> [FAIL][84] ([fdo#109271] / [i915#4312] / [i915#5257])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-8809g/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][85] ([i915#4312] / [i915#5257])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-6600u/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][86] ([i915#4312])
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-rkl-guc/igt@runner@aborted.html
    - fi-skl-guc:         NOTRUN -> [FAIL][87] ([i915#4312] / [i915#5257])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-skl-guc/igt@runner@aborted.html
    - fi-kbl-soraka:      NOTRUN -> [FAIL][88] ([fdo#109271] / [i915#4312] / [i915#5257])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-kbl-soraka/igt@runner@aborted.html
    - fi-cfl-8700k:       NOTRUN -> [FAIL][89] ([fdo#109271] / [i915#4312] / [i915#5257])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-cfl-8700k/igt@runner@aborted.html
    - fi-hsw-g3258:       NOTRUN -> [FAIL][90] ([i915#4312] / [i915#6246])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-hsw-g3258/igt@runner@aborted.html
    - fi-bdw-gvtdvm:      NOTRUN -> [FAIL][91] ([i915#4312])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bdw-gvtdvm/igt@runner@aborted.html
    - fi-bsw-kefka:       NOTRUN -> [FAIL][92] ([i915#4312])
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-bsw-kefka/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@requests:
    - fi-elk-e7500:       [DMESG-FAIL][93] ([i915#4528]) -> [PASS][94]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11933/fi-elk-e7500/igt@i915_selftest@live@requests.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/fi-elk-e7500/igt@i915_selftest@live@requests.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#3003]: https://gitlab.freedesktop.org/drm/intel/issues/3003
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4423]: https://gitlab.freedesktop.org/drm/intel/issues/4423
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#5257]: https://gitlab.freedesktop.org/drm/intel/issues/5257
  [i915#5847]: https://gitlab.freedesktop.org/drm/intel/issues/5847
  [i915#6246]: https://gitlab.freedesktop.org/drm/intel/issues/6246


Build changes
-------------

  * Linux: CI_DRM_11933 -> Patchwork_105427v3

  CI-20190529: 20190529
  CI_DRM_11933: c631f4cc41b2981c245ef03c2baae78da61c7f70 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6594: 326629f105459f9bd201456a0454759628e6a43d @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_105427v3: c631f4cc41b2981c245ef03c2baae78da61c7f70 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

c4775d4fa2c2 drm/i915 : Add D3COLD OFF support
6b216c48d3c3 drm/i915: Add i915_save/load_pci_state helpers
619f74133deb drm/i915/rpm: d3cold Policy
4ea694d3cc2b Drm/i915/rpm: Add intel_runtime_idle
e02349946a9b drm/i915: sanitize dc state in rpm resume
42076cfee81a drm/i915: Extend rpm in intel_guc_global_policies_update
4c97e79e13cc drm/i915: Guard rc6 helpers with is_intel_rpm_allowed
94e0c5c03db4 drm/i915: Added is_intel_rpm_allowed helper

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105427v3/index.html

[-- Attachment #2: Type: text/html, Size: 22517 bytes --]

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/tilak-tangudu-intel-com/drm-i915-Add-D3Cold-Off-support-for-runtime-pm/20220721-174913
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-c002 (https://download.01.org/0day-ci/archive/20220725/202207250617.9oNZv0Uz-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/232683eebcfe8c5abd46d4caad082bea98f13687
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review tilak-tangudu-intel-com/drm-i915-Add-D3Cold-Off-support-for-runtime-pm/20220721-174913
        git checkout 232683eebcfe8c5abd46d4caad082bea98f13687
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_runtime_pm.c: In function 'intel_runtime_pm_status':
>> drivers/gpu/drm/i915/intel_runtime_pm.c:325:32: error: 'struct dev_pm_info' has no member named 'runtime_status'
     325 |         return rpm->kdev->power.runtime_status;
         |                                ^
   drivers/gpu/drm/i915/intel_runtime_pm.c:326:1: error: control reaches end of non-void function [-Werror=return-type]
     326 | }
         | ^
   cc1: all warnings being treated as errors


vim +325 drivers/gpu/drm/i915/intel_runtime_pm.c

   321	
   322	#endif
   323	static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
   324	{
 > 325		return rpm->kdev->power.runtime_status;
   326	}
   327	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Rodrigo Vivi]

On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu@intel.com wrote:
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> Added is_intel_rpm_allowed function to query the runtime_pm
> status and disllow during suspending and resuming.

> 
> v2: Return -2 if runtime pm is not allowed in runtime_pm_get
> and skip wakeref release in runtime_pm_put if wakeref value
> is -2. - Jani N

Should we have some defines instead of the -#s?

> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 ++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6ed5786bcd29..704beeeb560b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -113,7 +113,7 @@ static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
>  	unsigned long flags, n;
>  	bool found = false;
>  
> -	if (unlikely(stack == -1))
> +	if (unlikely(stack == -1) || unlikely(stack == -2))
>  		return;
>  
>  	spin_lock_irqsave(&rpm->debug.lock, flags);
> @@ -320,6 +320,21 @@ untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)
>  }
>  
>  #endif
> +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
> +{
> +	return rpm->kdev->power.runtime_status;
> +}
> +
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)

why not static?

> +{
> +	int rpm_status;
> +
> +	rpm_status = intel_runtime_pm_status(rpm);
> +	if (rpm_status == RPM_RESUMING

I don't have a good feeling about this. If we are resuming we shouldn't
grab extra references... This seems a workaround for the lock mess.

> || rpm_status == RPM_SUSPENDING)

and when we are suspending and we call this function is because we need
to wake up, no?!

> +		return false;
> +	else
> +		return true;
> +}
>  
>  static void
>  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock)
> @@ -354,6 +369,9 @@ static intel_wakeref_t __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
>  						     runtime_pm);
>  	int ret;
>  
> +	if (!is_intel_rpm_allowed(rpm))
> +		return -2;
> +
>  	ret = pm_runtime_get_sync(rpm->kdev);
>  	drm_WARN_ONCE(&i915->drm, ret < 0,
>  		      "pm_runtime_get_sync() failed: %d\n", ret);
> @@ -490,6 +508,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
>  
>  	untrack_intel_runtime_pm_wakeref(rpm, wref);
>  
> +	if (wref == -2)
> +		return;
> +
>  	intel_runtime_pm_release(rpm, wakelock);
>  
>  	pm_runtime_mark_last_busy(kdev);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index d9160e3ff4af..99418c3a934a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);

if really need to export please follow the naming convention.\

>  
>  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
>  intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 4/8] drm/i915: sanitize dc state in rpm resume

[Rodrigo Vivi]

On Thu, Jul 21, 2022 at 03:29:51PM +0530, tilak.tangudu@intel.com wrote:
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> During runtime resume the display init sequence is called via
> intel_display_power_resume() -> icl_display_core_init()
> which should restore the display HW state. For restoring the DC9 enabled
> state in DC_STATE_EN, gen9_sanitize_dc_state() should be called on the
>  runtime resume path too to avoid the
> 
> [  513.818190] i915 0000:03:00.0: [drm] *ERROR DC state mismatch (0x8 -> 0x0)*
> 
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
Cc: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display_power.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 589af257edeb..799f84d3eed6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -2229,6 +2229,7 @@ void intel_display_power_suspend(struct drm_i915_private *i915)
>  void intel_display_power_resume(struct drm_i915_private *i915)
>  {
>  	if (DISPLAY_VER(i915) >= 11) {
> +		gen9_sanitize_dc_state(i915);
>  		bxt_disable_dc9(i915);
>  		icl_display_core_init(i915, true);
>  		if (intel_dmc_has_payload(i915)) {
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[Rodrigo Vivi]

On Thu, Jul 21, 2022 at 03:29:53PM +0530, tilak.tangudu@intel.com wrote:
> From: Tilak Tangudu <tilak.tangudu@intel.com>

This needs to be sorted out... or the patch split or use
the Co-developed-by:...

> 
> Add d3cold_sr_lmem_threshold modparam to choose between
> d3cold-off zero watt and  d3hot/d3cold-VRAM Self Refresh.
> i915 requires to evict the lmem objects to smem in order to
> support d3cold-Off. if platform does not supports vram_sr
> feature then fall back to d3hot by disabling d3cold to
> avoid the rpm suspend/resume latency.
> Extend the d3cold_sr_lmem_threshold modparam to debugfs
> i915_params so that, it can be used by igt test.
> 
> If gfx root port is not capable of sending PME from d3cold
> or doesn't have _PR3 power resources then only d3hot state
> can be supported.
> 
> Adding intel_pm_prepare_targeted_d3_state() to choose the
> correct target d3 state and cache it to intel_runtime_pm
> structure, it can be used in rpm suspend/resume callback
> accordingly.
> 
> v2: lmem->avail stopped tracking lmem usage since ttm is
> introduced, so removed lmem->avail usage in policy.
> FIXME here, lmem usage is not added, need to be added
> by using query functions.
> FIXME, Forcing the policy to enter D3COLD_OFF for
> validation purpose.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
>  drivers/gpu/drm/i915/i915_params.c      |  5 ++++
>  drivers/gpu/drm/i915/i915_params.h      |  1 +
>  drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.h         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
>  6 files changed, 55 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 4c36554567fd..2b2e9563f149 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
>  	struct drm_i915_private *i915 = kdev_to_i915(kdev);
>  	int ret = 1;
>  
> +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> +	ret = intel_pm_prepare_targeted_d3_state(i915);
> +	if (!ret)
> +		ret = 1;

why?

> +
> +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
>  	pm_runtime_mark_last_busy(kdev);
>  	pm_runtime_autosuspend(kdev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 6fc475a5db61..4603f5c2ed77 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
>  	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
>  #endif
>  
> +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
> +	"Enable VRAM Self refresh when size of lmem is greater to this threshold. "
> +	"If VRAM Self Refresh is not available then fall back to d3cold. "
> +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> +
>  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
>  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
>  			"Default request/fence/batch buffer expiration timeout.");
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 2733cb6cfe09..1a86711038da 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -75,6 +75,7 @@ struct drm_printer;
>  	param(unsigned int, request_timeout_ms, CONFIG_DRM_I915_REQUEST_TIMEOUT, CONFIG_DRM_I915_REQUEST_TIMEOUT ? 0600 : 0) \
>  	param(unsigned int, lmem_size, 0, 0400) \
>  	param(unsigned int, lmem_bar_size, 0, 0400) \
> +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
>  	/* leave bools at the end to not create holes */ \
>  	param(bool, enable_hangcheck, true, 0600) \
>  	param(bool, load_detect_test, false, 0600) \
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f06babdb3a8c..20b0638ecd5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private *dev_priv)
>  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);
>  }
>  
> +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915)
> +{
> +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> +	u64 lmem_used = 0;
> +	struct pci_dev *root_pdev;
> +	int ret = 0;
> +
> +	/* igfx will return from here */

then this patch is preventing runtime_pm on igfx no?!
or we need to return 0...

> +	root_pdev = pcie_find_root_port(pdev);
> +	if (!root_pdev)
> +		return ret;
> +
> +	/* D3Cold requires PME capability and _PR3 power resource */
> +	if (!pci_pme_capable(root_pdev, PCI_D3cold) || !pci_pr3_present(root_pdev))
> +		return ret;

If this is the case we probably need to block D3cold, but not D3hot, right?

> +
> +	/* FXME query the LMEM usage and fill lmem_used */
> +	/* Trigger D3COLD_OFF always to validate with all tests */
> +	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 * 1024) {
> +		rpm->d3_state = INTEL_D3COLD_OFF;
> +		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
> +	} else {
> +		/* Disable D3Cold to reduce the eviction latency */
> +		rpm->d3_state = INTEL_D3HOT;
> +	}
> +
> +	if (rpm->d3_state == INTEL_D3HOT)
> +		pci_d3cold_disable(root_pdev);
> +	else
> +		pci_d3cold_enable(root_pdev);

why not merge these both if states?

> +
> +	return ret;
> +}
> +
>  static struct intel_global_state *intel_dbuf_duplicate_state(struct intel_global_obj *obj)
>  {
>  	struct intel_dbuf_state *dbuf_state;
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 945503ae493e..7827b0c1a2f3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private *dev_priv);
>  void intel_init_pm(struct drm_i915_private *dev_priv);
>  void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
>  void intel_pm_setup(struct drm_i915_private *dev_priv);
> +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915);
>  void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);
>  void vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index 99418c3a934a..568559b71b70 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
>  	I915_DRM_SUSPEND_HIBERNATE,
>  };
>  
> +enum intel_gfx_d3_state {
> +	INTEL_D3HOT,
> +	INTEL_D3COLD_OFF,
> +	INTEL_D3COLD_VRAM_SR,
> +};
> +
>  /*
>   * This struct helps tracking the state needed for runtime PM, which puts the
>   * device in PCI D3 state. Notice that when this happens, nothing on the
> @@ -52,6 +58,7 @@ struct intel_runtime_pm {
>  	bool suspended;
>  	bool irqs_enabled;
>  	bool no_wakeref_tracking;
> +	enum intel_gfx_d3_state d3_state;
>  
>  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>  	/*
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle

[Rodrigo Vivi]

On Thu, Jul 21, 2022 at 03:29:52PM +0530, tilak.tangudu@intel.com wrote:
> From: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Adding intel_runtime_idle (runtime_idle callback) to prepare the
> tageted D3 state.
> 
> Since we have introduced i915 runtime_idle callback.
> It need to be warranted that Runtime PM Core invokes runtime_idle
> callback when runtime usages count becomes zero. That requires
> to use pm_runtime_put instead of pm_runtime_put_autosuspend.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c      | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index deb8a8b76965..4c36554567fd 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1576,6 +1576,17 @@ static int i915_pm_restore(struct device *kdev)
>  	return i915_pm_resume(kdev);
>  }
>  
> +static int intel_runtime_idle(struct device *kdev)
> +{
> +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> +	int ret = 1;
> +
> +	pm_runtime_mark_last_busy(kdev);
> +	pm_runtime_autosuspend(kdev);

oh, I see the ret = 1 like the other drm drivers..
do we really know why this flow and not use the runtime_idle
like the rest of the kernel?

I believe we could use more the runtime_idle to block the
runtime_pm, but following the original documented design of it.

> +
> +	return ret;
> +}
> +
>  static int intel_runtime_suspend(struct device *kdev)
>  {
>  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> @@ -1752,6 +1763,7 @@ const struct dev_pm_ops i915_pm_ops = {
>  	.restore = i915_pm_restore,
>  
>  	/* S0ix (via runtime suspend) event handlers */
> +	.runtime_idle = intel_runtime_idle,
>  	.runtime_suspend = intel_runtime_suspend,
>  	.runtime_resume = intel_runtime_resume,
>  };
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 704beeeb560b..1c3ed0c29330 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -513,8 +513,7 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
>  
>  	intel_runtime_pm_release(rpm, wakelock);
>  
> -	pm_runtime_mark_last_busy(kdev);
> -	pm_runtime_put_autosuspend(kdev);
> +	pm_runtime_put(kdev);
>  }
>  
>  /**
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[Tangudu, Tilak]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Thursday, August 4, 2022 2:16 AM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>
> Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 6/8] drm/i915/rpm: d3cold Policy
> 
> On Thu, Jul 21, 2022 at 03:29:53PM +0530, tilak.tangudu@intel.com wrote:
> > From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> This needs to be sorted out... or the patch split or use the Co-developed-
> by:...

Anshuman is sole author of the patch, I have modified a little in rev2  with a FIXME 
to fix the broken patch and to unblock my series and I missed to explicitly specify 
Anushman as author, Anshuman has a fix in rev3. I will specify him as author expliclty
 for next patches.  No conflicts here. 😊

> 
> >
> > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off
> > zero watt and  d3hot/d3cold-VRAM Self Refresh.
> > i915 requires to evict the lmem objects to smem in order to support
> > d3cold-Off. if platform does not supports vram_sr feature then fall
> > back to d3hot by disabling d3cold to avoid the rpm suspend/resume
> > latency.
> > Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params
> so
> > that, it can be used by igt test.
> >
> > If gfx root port is not capable of sending PME from d3cold or doesn't
> > have _PR3 power resources then only d3hot state can be supported.
> >
> > Adding intel_pm_prepare_targeted_d3_state() to choose the correct
> > target d3 state and cache it to intel_runtime_pm structure, it can be
> > used in rpm suspend/resume callback accordingly.
> >
> > v2: lmem->avail stopped tracking lmem usage since ttm is introduced,
> > so removed lmem->avail usage in policy.
> > FIXME here, lmem usage is not added, need to be added by using query
> > functions.
> > FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
> >  drivers/gpu/drm/i915/i915_params.c      |  5 ++++
> >  drivers/gpu/drm/i915/i915_params.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.h         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
> >  6 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index 4c36554567fd..2b2e9563f149 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
> >  	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> >  	int ret = 1;
> >
> > +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> > +	ret = intel_pm_prepare_targeted_d3_state(i915);
> > +	if (!ret)
> > +		ret = 1;
> 
> why?
> 
> > +
> > +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> >  	pm_runtime_mark_last_busy(kdev);
> >  	pm_runtime_autosuspend(kdev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 6fc475a5db61..4603f5c2ed77 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
> >  	"Enable support for Intel GVT-g graphics virtualization host
> > support(default:false)");  #endif
> >
> > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
> > +	"Enable VRAM Self refresh when size of lmem is greater to this
> threshold. "
> > +	"If VRAM Self Refresh is not available then fall back to d3cold. "
> > +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> > +
> >  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
> >  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
> >  			"Default request/fence/batch buffer expiration
> timeout."); diff
> > --git a/drivers/gpu/drm/i915/i915_params.h
> > b/drivers/gpu/drm/i915/i915_params.h
> > index 2733cb6cfe09..1a86711038da 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -75,6 +75,7 @@ struct drm_printer;
> >  	param(unsigned int, request_timeout_ms,
> CONFIG_DRM_I915_REQUEST_TIMEOUT,
> CONFIG_DRM_I915_REQUEST_TIMEOUT ? 0600 : 0) \
> >  	param(unsigned int, lmem_size, 0, 0400) \
> >  	param(unsigned int, lmem_bar_size, 0, 0400) \
> > +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
> >  	/* leave bools at the end to not create holes */ \
> >  	param(bool, enable_hangcheck, true, 0600) \
> >  	param(bool, load_detect_test, false, 0600) \ diff --git
> > a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f06babdb3a8c..20b0638ecd5c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private
> *dev_priv)
> >  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);  }
> >
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915)
> > +{
> > +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > +	u64 lmem_used = 0;
> > +	struct pci_dev *root_pdev;
> > +	int ret = 0;
> > +
> > +	/* igfx will return from here */
> 
> then this patch is preventing runtime_pm on igfx no?!
> or we need to return 0...
> 
> > +	root_pdev = pcie_find_root_port(pdev);
> > +	if (!root_pdev)
> > +		return ret;
> > +
> > +	/* D3Cold requires PME capability and _PR3 power resource */
> > +	if (!pci_pme_capable(root_pdev, PCI_D3cold) ||
> !pci_pr3_present(root_pdev))
> > +		return ret;
> 
> If this is the case we probably need to block D3cold, but not D3hot, right?
> 
> > +
> > +	/* FXME query the LMEM usage and fill lmem_used */
> > +	/* Trigger D3COLD_OFF always to validate with all tests */
> > +	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 *
> 1024) {
> > +		rpm->d3_state = INTEL_D3COLD_OFF;
> > +		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
> > +	} else {
> > +		/* Disable D3Cold to reduce the eviction latency */
> > +		rpm->d3_state = INTEL_D3HOT;
> > +	}
> > +
> > +	if (rpm->d3_state == INTEL_D3HOT)
> > +		pci_d3cold_disable(root_pdev);
> > +	else
> > +		pci_d3cold_enable(root_pdev);
> 
> why not merge these both if states?
> 
> > +
> > +	return ret;
> > +}
> > +
> >  static struct intel_global_state *intel_dbuf_duplicate_state(struct
> > intel_global_obj *obj)  {
> >  	struct intel_dbuf_state *dbuf_state; diff --git
> > a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 945503ae493e..7827b0c1a2f3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private
> > *dev_priv);  void intel_init_pm(struct drm_i915_private *dev_priv);
> > void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
> > void intel_pm_setup(struct drm_i915_private *dev_priv);
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private
> > +*i915);
> >  void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > ilk_wm_get_hw_state(struct drm_i915_private *dev_priv); diff --git
> > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index 99418c3a934a..568559b71b70 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
> >  	I915_DRM_SUSPEND_HIBERNATE,
> >  };
> >
> > +enum intel_gfx_d3_state {
> > +	INTEL_D3HOT,
> > +	INTEL_D3COLD_OFF,
> > +	INTEL_D3COLD_VRAM_SR,
> > +};
> > +
> >  /*
> >   * This struct helps tracking the state needed for runtime PM, which puts
> the
> >   * device in PCI D3 state. Notice that when this happens, nothing on
> > the @@ -52,6 +58,7 @@ struct intel_runtime_pm {
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  	bool no_wakeref_tracking;
> > +	enum intel_gfx_d3_state d3_state;
> >
> >  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> >  	/*
> > --
> > 2.25.1
> >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Tangudu, Tilak]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Thursday, August 4, 2022 2:01 AM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>
> Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> 
> On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu@intel.com wrote:
> > From: Tilak Tangudu <tilak.tangudu@intel.com>
> >
> > Added is_intel_rpm_allowed function to query the runtime_pm status and
> > disllow during suspending and resuming.
> 
> >
> > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and skip
> > wakeref release in runtime_pm_put if wakeref value is -2. - Jani N
> 
> Should we have some defines instead of the -#s?
Ok will address this.
> 
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> ++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 6ed5786bcd29..704beeeb560b 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -113,7 +113,7 @@ static void
> untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> >  	unsigned long flags, n;
> >  	bool found = false;
> >
> > -	if (unlikely(stack == -1))
> > +	if (unlikely(stack == -1) || unlikely(stack == -2))
> >  		return;
> >
> >  	spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6 +320,21
> @@
> > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)  }
> >
> >  #endif
> > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm) {
> > +	return rpm->kdev->power.runtime_status; }
> > +
> > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> 
> why not static?
 We need is_intel_rpm_allowed for rc6 helpers , the helpers use pm_runtime_get_sync,
To avoid lock issue we need to use it here too.

See this patch " drm/i915: Guard rc6 helpers with is_intel_rpm_allowed" 

> 
> > +{
> > +	int rpm_status;
> > +
> > +	rpm_status = intel_runtime_pm_status(rpm);
> > +	if (rpm_status == RPM_RESUMING
> 
> I don't have a good feeling about this. If we are resuming we shouldn't grab
> extra references... This seems a workaround for the lock mess.
> 
> > || rpm_status == RPM_SUSPENDING)
> 
> and when we are suspending and we call this function is because we need to
> wake up, no?!

As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
 and i915_gem_resume , These functions use runtime helpers, which in-turn call
 runtime suspend/resume callbacks and leads to deadlock.
 So, these helpers need to be avoided.  we have added is_intel_rpm_allowed
 and disallowed rpm callbacks with above condition during suspending and resuming
 to avoid deadlock.
> 
> > +		return false;
> > +	else
> > +		return true;
> > +}
> >
> >  static void
> >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock)
> > @@ -354,6 +369,9 @@ static intel_wakeref_t
> __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> >  						     runtime_pm);
> >  	int ret;
> >
> > +	if (!is_intel_rpm_allowed(rpm))
> > +		return -2;
> > +
> >  	ret = pm_runtime_get_sync(rpm->kdev);
> >  	drm_WARN_ONCE(&i915->drm, ret < 0,
> >  		      "pm_runtime_get_sync() failed: %d\n", ret); @@ -490,6
> +508,9
> > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
> >
> >  	untrack_intel_runtime_pm_wakeref(rpm, wref);
> >
> > +	if (wref == -2)
> > +		return;
> > +
> >  	intel_runtime_pm_release(rpm, wakelock);
> >
> >  	pm_runtime_mark_last_busy(kdev);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index d9160e3ff4af..99418c3a934a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> > intel_runtime_pm *rpm);
> > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> 
> if really need to export please follow the naming convention.\

Ok will address this.

-Tilak
> 
> >
> >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm
> > *rpm);
> > --
> > 2.25.1
> >

Re: [Intel-gfx] [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Thursday, August 4, 2022 2:22 AM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>
> Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 5/8] Drm/i915/rpm: Add intel_runtime_idle
> 
> On Thu, Jul 21, 2022 at 03:29:52PM +0530, tilak.tangudu@intel.com wrote:
> > From: Anshuman Gupta <anshuman.gupta@intel.com>
> >
> > Adding intel_runtime_idle (runtime_idle callback) to prepare the
> > tageted D3 state.
> >
> > Since we have introduced i915 runtime_idle callback.
> > It need to be warranted that Runtime PM Core invokes runtime_idle
> > callback when runtime usages count becomes zero. That requires to use
> > pm_runtime_put instead of pm_runtime_put_autosuspend.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Chris Wilson <chris.p.wilson@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c      | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +--
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index deb8a8b76965..4c36554567fd 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1576,6 +1576,17 @@ static int i915_pm_restore(struct device *kdev)
> >  	return i915_pm_resume(kdev);
> >  }
> >
> > +static int intel_runtime_idle(struct device *kdev) {
> > +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > +	int ret = 1;
> > +
> > +	pm_runtime_mark_last_busy(kdev);
> > +	pm_runtime_autosuspend(kdev);
> 
> oh, I see the ret = 1 like the other drm drivers..
> do we really know why this flow and not use the runtime_idle like the rest of the
> kernel?
AFAIU it is the mixed type of usages (some drivers like USB use -EBUSY), 
the crux of it to use the pm_runtime_mark_last_busy() and pm_runtime_autosuspend correctly ,
PM Core autosuspend timer start ticking since it was last busy. 

int usb_runtime_idle(struct device *dev)
{
        struct usb_device       *udev = to_usb_device(dev);

        /* An idle USB device can be suspended if it passes the various
         * autosuspend checks.
         */
        if (autosuspend_check(udev) == 0)
                pm_runtime_autosuspend(dev);
        /* Tell the core not to suspend it, though. */
        return -EBUSY;
}

Intel snd driver uses -EBUSY and 0 as a return value.
static int azx_runtime_idle(struct device *dev)
{
        struct snd_card *card = dev_get_drvdata(dev);
        struct azx *chip;
        struct hda_intel *hda;

        if (!card)
                return 0;

        chip = card->private_data;
        hda = container_of(chip, struct hda_intel, chip);
        if (chip->disabled || hda->init_failed)
                return 0;

        if (!power_save_controller || !azx_has_pm_runtime(chip) ||
            azx_bus(chip)->codec_powered || !chip->running)
                return -EBUSY;

        /* ELD notification gets broken when HD-audio bus is off */
        if (needs_eld_notify_link(chip))
                return -EBUSY;

        return 0;
}

When rutime_idle callback returns 0, then only PM core will call rpm_suspend with RPM_AUTO flag.
return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);

> 
> I believe we could use more the runtime_idle to block the runtime_pm, but
> following the original documented design of it.
Do you mean it is not correct to use runtime_idle callback for d3cold policy ?
Yes it is being used to block the runtime pm, if device is busy for other kernel drivers.

Thanks,
Anshuman Gupta.
> 
> > +
> > +	return ret;
> > +}
> > +
> >  static int intel_runtime_suspend(struct device *kdev)  {
> >  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev); @@ -1752,6
> > +1763,7 @@ const struct dev_pm_ops i915_pm_ops = {
> >  	.restore = i915_pm_restore,
> >
> >  	/* S0ix (via runtime suspend) event handlers */
> > +	.runtime_idle = intel_runtime_idle,
> >  	.runtime_suspend = intel_runtime_suspend,
> >  	.runtime_resume = intel_runtime_resume,  }; diff --git
> > a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 704beeeb560b..1c3ed0c29330 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -513,8 +513,7 @@ static void __intel_runtime_pm_put(struct
> > intel_runtime_pm *rpm,
> >
> >  	intel_runtime_pm_release(rpm, wakelock);
> >
> > -	pm_runtime_mark_last_busy(kdev);
> > -	pm_runtime_put_autosuspend(kdev);
> > +	pm_runtime_put(kdev);
> >  }
> >
> >  /**
> > --
> > 2.25.1
> >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Gupta, Anshuman]

> -----Original Message-----
> From: Tangudu, Tilak <tilak.tangudu@intel.com>
> Sent: Thursday, July 21, 2022 3:30 PM
> To: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta,
> Anshuman <anshuman.gupta@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>; Deak,
> Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> 
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> Added is_intel_rpm_allowed function to query the runtime_pm status and
> disllow during suspending and resuming.
> 
> v2: Return -2 if runtime pm is not allowed in runtime_pm_get and skip wakeref
> release in runtime_pm_put if wakeref value is -2. - Jani N
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 ++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6ed5786bcd29..704beeeb560b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -113,7 +113,7 @@ static void untrack_intel_runtime_pm_wakeref(struct
> intel_runtime_pm *rpm,
>  	unsigned long flags, n;
>  	bool found = false;
> 
> -	if (unlikely(stack == -1))
> +	if (unlikely(stack == -1) || unlikely(stack == -2))
>  		return;
> 
>  	spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6 +320,21 @@
> untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)  }
> 
>  #endif
> +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm) {
> +	return rpm->kdev->power.runtime_status; }
> +
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm) {
> +	int rpm_status;
> +
> +	rpm_status = intel_runtime_pm_status(rpm);
> +	if (rpm_status == RPM_RESUMING || rpm_status ==
> RPM_SUSPENDING)
AFAIR I have commented on first patch, we may need dev->power.lock here.
It is racy to test  kdev->power.runtime_status with PM core can change it any time. 
Please check the pm_runtime_status_suspended() kernel doc in include/linux/pm_runtime.h
Thanks,
Anshuman Gupta.
> +		return false;
> +	else
> +		return true;
> +}
> 
>  static void
>  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock) @@ -
> 354,6 +369,9 @@ static intel_wakeref_t __intel_runtime_pm_get(struct
> intel_runtime_pm *rpm,
>  						     runtime_pm);
>  	int ret;
> 
> +	if (!is_intel_rpm_allowed(rpm))
> +		return -2;
> +
>  	ret = pm_runtime_get_sync(rpm->kdev);
>  	drm_WARN_ONCE(&i915->drm, ret < 0,
>  		      "pm_runtime_get_sync() failed: %d\n", ret); @@ -490,6
> +508,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
> 
>  	untrack_intel_runtime_pm_wakeref(rpm, wref);
> 
> +	if (wref == -2)
> +		return;
> +
>  	intel_runtime_pm_release(rpm, wakelock);
> 
>  	pm_runtime_mark_last_busy(kdev);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index d9160e3ff4af..99418c3a934a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> intel_runtime_pm *rpm);
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> 
>  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm
> *rpm);
> --
> 2.25.1

Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Thursday, August 4, 2022 2:16 AM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>
> Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 6/8] drm/i915/rpm: d3cold Policy
> 
> On Thu, Jul 21, 2022 at 03:29:53PM +0530, tilak.tangudu@intel.com wrote:
> > From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> This needs to be sorted out... or the patch split or use the Co-developed-by:...
> 
> >
> > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off
> > zero watt and  d3hot/d3cold-VRAM Self Refresh.
> > i915 requires to evict the lmem objects to smem in order to support
> > d3cold-Off. if platform does not supports vram_sr feature then fall
> > back to d3hot by disabling d3cold to avoid the rpm suspend/resume
> > latency.
> > Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params so
> > that, it can be used by igt test.
> >
> > If gfx root port is not capable of sending PME from d3cold or doesn't
> > have _PR3 power resources then only d3hot state can be supported.
> >
> > Adding intel_pm_prepare_targeted_d3_state() to choose the correct
> > target d3 state and cache it to intel_runtime_pm structure, it can be
> > used in rpm suspend/resume callback accordingly.
> >
> > v2: lmem->avail stopped tracking lmem usage since ttm is introduced,
> > so removed lmem->avail usage in policy.
> > FIXME here, lmem usage is not added, need to be added by using query
> > functions.
> > FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c      |  6 +++++
> >  drivers/gpu/drm/i915/i915_params.c      |  5 ++++
> >  drivers/gpu/drm/i915/i915_params.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c         | 35 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.h         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.h |  7 +++++
> >  6 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index 4c36554567fd..2b2e9563f149 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev)
> >  	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> >  	int ret = 1;
> >
> > +	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> > +	ret = intel_pm_prepare_targeted_d3_state(i915);
> > +	if (!ret)
> > +		ret = 1;
> 
> why?
> 
> > +
> > +	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> >  	pm_runtime_mark_last_busy(kdev);
> >  	pm_runtime_autosuspend(kdev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 6fc475a5db61..4603f5c2ed77 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400,
> >  	"Enable support for Intel GVT-g graphics virtualization host
> > support(default:false)");  #endif
> >
> > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600,
> > +	"Enable VRAM Self refresh when size of lmem is greater to this
> threshold. "
> > +	"If VRAM Self Refresh is not available then fall back to d3cold. "
> > +	"It helps to optimize the suspend/resume latecy. (default: 300mb)");
> > +
> >  #if CONFIG_DRM_I915_REQUEST_TIMEOUT
> >  i915_param_named_unsafe(request_timeout_ms, uint, 0600,
> >  			"Default request/fence/batch buffer expiration
> timeout."); diff
> > --git a/drivers/gpu/drm/i915/i915_params.h
> > b/drivers/gpu/drm/i915/i915_params.h
> > index 2733cb6cfe09..1a86711038da 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -75,6 +75,7 @@ struct drm_printer;
> >  	param(unsigned int, request_timeout_ms,
> CONFIG_DRM_I915_REQUEST_TIMEOUT,
> CONFIG_DRM_I915_REQUEST_TIMEOUT ? 0600 : 0) \
> >  	param(unsigned int, lmem_size, 0, 0400) \
> >  	param(unsigned int, lmem_bar_size, 0, 0400) \
> > +	param(int, d3cold_sr_lmem_threshold, 300, 0600) \
> >  	/* leave bools at the end to not create holes */ \
> >  	param(bool, enable_hangcheck, true, 0600) \
> >  	param(bool, load_detect_test, false, 0600) \ diff --git
> > a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f06babdb3a8c..20b0638ecd5c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8287,6 +8287,41 @@ void intel_pm_setup(struct drm_i915_private
> *dev_priv)
> >  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);  }
> >
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private *i915)
> > +{
> > +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > +	u64 lmem_used = 0;
> > +	struct pci_dev *root_pdev;
> > +	int ret = 0;
> > +
> > +	/* igfx will return from here */
> 
> then this patch is preventing runtime_pm on igfx no?!
> or we need to return 0...
ret is initialized with 0. I will change it to return with explicit 'return 0'.
This won't block runtime PM for igfx as caller will request the autosuspend.
rpm->d3_state will be by default initialized with INTEL_D3HOT i.e. 0.
Please correct me in case I am missing something.   
> 
> > +	root_pdev = pcie_find_root_port(pdev);
> > +	if (!root_pdev)
> > +		return ret;
> > +
> > +	/* D3Cold requires PME capability and _PR3 power resource */
> > +	if (!pci_pme_capable(root_pdev, PCI_D3cold) ||
> !pci_pr3_present(root_pdev))
> > +		return ret;
> 
> If this is the case we probably need to block D3cold, but not D3hot, right?
Similar to above rpm->d3_state will be by default initialized with INTEL_D3HOT i.e. 0.
And we do not need to block the d3cold here as device is already incapable of it.
Server platforms may hit this code path.  
> 
> > +
> > +	/* FXME query the LMEM usage and fill lmem_used */
> > +	/* Trigger D3COLD_OFF always to validate with all tests */
> > +	if (lmem_used < i915->params.d3cold_sr_lmem_threshold  * 1024 *
> 1024) {
> > +		rpm->d3_state = INTEL_D3COLD_OFF;
> > +		drm_dbg(&i915->drm, "Prepared for D3Cold off\n");
> > +	} else {
> > +		/* Disable D3Cold to reduce the eviction latency */
> > +		rpm->d3_state = INTEL_D3HOT;
> > +	}
> > +
> > +	if (rpm->d3_state == INTEL_D3HOT)
> > +		pci_d3cold_disable(root_pdev);
> > +	else
> > +		pci_d3cold_enable(root_pdev);
> 
> why not merge these both if states?
For future D3COLD_VRAM_SR support scalability , target d3 state assignment and d3cold disabling kept separate.
Let me know it is must to merge both if /else block.
Thanks,
Anshuman Gupta.
> 
> > +
> > +	return ret;
> > +}
> > +
> >  static struct intel_global_state *intel_dbuf_duplicate_state(struct
> > intel_global_obj *obj)  {
> >  	struct intel_dbuf_state *dbuf_state; diff --git
> > a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 945503ae493e..7827b0c1a2f3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -31,6 +31,7 @@ int ilk_wm_max_level(const struct drm_i915_private
> > *dev_priv);  void intel_init_pm(struct drm_i915_private *dev_priv);
> > void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv);
> > void intel_pm_setup(struct drm_i915_private *dev_priv);
> > +int intel_pm_prepare_targeted_d3_state(struct drm_i915_private
> > +*i915);
> >  void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);  void
> > ilk_wm_get_hw_state(struct drm_i915_private *dev_priv); diff --git
> > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index 99418c3a934a..568559b71b70 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -22,6 +22,12 @@ enum i915_drm_suspend_mode {
> >  	I915_DRM_SUSPEND_HIBERNATE,
> >  };
> >
> > +enum intel_gfx_d3_state {
> > +	INTEL_D3HOT,
> > +	INTEL_D3COLD_OFF,
> > +	INTEL_D3COLD_VRAM_SR,
> > +};
> > +
> >  /*
> >   * This struct helps tracking the state needed for runtime PM, which puts the
> >   * device in PCI D3 state. Notice that when this happens, nothing on
> > the @@ -52,6 +58,7 @@ struct intel_runtime_pm {
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  	bool no_wakeref_tracking;
> > +	enum intel_gfx_d3_state d3_state;
> >
> >  #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
> >  	/*
> > --
> > 2.25.1
> >

Re: [Intel-gfx] [PATCH 4/8] drm/i915: sanitize dc state in rpm resume

[Tangudu, Tilak]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Thursday, August 4, 2022 2:03 AM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>; Srivatsa, Anusha
> <anusha.srivatsa@intel.com>; Deak, Imre <imre.deak@intel.com>
> Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 4/8] drm/i915: sanitize dc state in rpm resume
> 
> On Thu, Jul 21, 2022 at 03:29:51PM +0530, tilak.tangudu@intel.com wrote:
> > From: Tilak Tangudu <tilak.tangudu@intel.com>
> >
> > During runtime resume the display init sequence is called via
> > intel_display_power_resume() -> icl_display_core_init() which should
> > restore the display HW state. For restoring the DC9 enabled state in
> > DC_STATE_EN, gen9_sanitize_dc_state() should be called on the  runtime
> > resume path too to avoid the
> >
> > [  513.818190] i915 0000:03:00.0: [drm] *ERROR DC state mismatch (0x8
> > -> 0x0)*
> >
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>

This is suggested by Imre in the JIRA,
In next patches I will add the below 
Suggested-by: Imre Deak <imre.deak@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display_power.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 589af257edeb..799f84d3eed6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -2229,6 +2229,7 @@ void intel_display_power_suspend(struct
> > drm_i915_private *i915)  void intel_display_power_resume(struct
> > drm_i915_private *i915)  {
> >  	if (DISPLAY_VER(i915) >= 11) {
> > +		gen9_sanitize_dc_state(i915);
> >  		bxt_disable_dc9(i915);
> >  		icl_display_core_init(i915, true);
> >  		if (intel_dmc_has_payload(i915)) {
> > --
> > 2.25.1
> >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Vivi, Rodrigo]

On Thu, 2022-08-04 at 05:32 +0000, Tangudu, Tilak wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Thursday, August 4, 2022 2:01 AM
> > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > <matthew.d.roper@intel.com>; Wilson, Chris P
> > <chris.p.wilson@intel.com>;
> > Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> > <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Nilawar, Badal
> > <badal.nilawar@intel.com>;
> > Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> > <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> > helper
> > 
> > On Thu, Jul 21, 2022 at 03:29:48PM +0530,
> > tilak.tangudu@intel.com wrote:
> > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > > 
> > > Added is_intel_rpm_allowed function to query the runtime_pm
> > > status and
> > > disllow during suspending and resuming.
> > 
> > > 
> > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > skip
> > > wakeref release in runtime_pm_put if wakeref value is -2. - Jani
> > > N
> > 
> > Should we have some defines instead of the -#s?
> Ok will address this.
> > 
> > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > ++++++++++++++++++++++-
> > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 6ed5786bcd29..704beeeb560b 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -113,7 +113,7 @@ static void
> > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> > >         unsigned long flags, n;
> > >         bool found = false;
> > > 
> > > -       if (unlikely(stack == -1))
> > > +       if (unlikely(stack == -1) || unlikely(stack == -2))
> > >                 return;
> > > 
> > >         spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6
> > > +320,21
> > @@
> > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm
> > > *rpm)  }
> > > 
> > >  #endif
> > > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
> > > {
> > > +       return rpm->kdev->power.runtime_status; }
> > > +
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> > 
> > why not static?
>  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> pm_runtime_get_sync,
> To avoid lock issue we need to use it here too.
> 
> See this patch " drm/i915: Guard rc6 helpers with
> is_intel_rpm_allowed" 
> 
> > 
> > > +{
> > > +       int rpm_status;
> > > +
> > > +       rpm_status = intel_runtime_pm_status(rpm);
> > > +       if (rpm_status == RPM_RESUMING
> > 
> > I don't have a good feeling about this. If we are resuming we
> > shouldn't grab
> > extra references... This seems a workaround for the lock mess.
> > 
> > > > > rpm_status == RPM_SUSPENDING)
> > 
> > and when we are suspending and we call this function is because we
> > need to
> > wake up, no?!
> 
> As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
>  and i915_gem_resume , These functions use runtime helpers, which in-
> turn call
>  runtime suspend/resume callbacks and leads to deadlock.
>  So, these helpers need to be avoided.  we have added
> is_intel_rpm_allowed
>  and disallowed rpm callbacks with above condition during suspending
> and resuming
>  to avoid deadlock.

Why does these functions in suspend resume path are using the runtime
callbacks?
Can't we have a refactor on that area that allows the paths that we
need in a place where we are sure that device is not going to d3?
then we can have a clear infra?

> > 
> > > +               return false;
> > > +       else
> > > +               return true;
> > > +}
> > > 
> > >  static void
> > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > wakelock)
> > > @@ -354,6 +369,9 @@ static intel_wakeref_t
> > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > >                                                      runtime_pm);
> > >         int ret;
> > > 
> > > +       if (!is_intel_rpm_allowed(rpm))
> > > +               return -2;
> > > +
> > >         ret = pm_runtime_get_sync(rpm->kdev);
> > >         drm_WARN_ONCE(&i915->drm, ret < 0,
> > >                       "pm_runtime_get_sync() failed: %d\n", ret);
> > > @@ -490,6
> > +508,9
> > > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm
> > > *rpm,
> > > 
> > >         untrack_intel_runtime_pm_wakeref(rpm, wref);
> > > 
> > > +       if (wref == -2)
> > > +               return;
> > > +
> > >         intel_runtime_pm_release(rpm, wakelock);
> > > 
> > >         pm_runtime_mark_last_busy(kdev);
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > index d9160e3ff4af..99418c3a934a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > intel_runtime_pm *rpm);  void
> > > intel_runtime_pm_driver_release(struct
> > > intel_runtime_pm *rpm);
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > 
> > if really need to export please follow the naming convention.\
> 
> Ok will address this.
> 
> -Tilak
> > 
> > > 
> > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > > *rpm);
> > > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > intel_runtime_pm
> > > *rpm);
> > > --
> > > 2.25.1
> > > 

Re: [Intel-gfx] [PATCH 8/8] drm/i915 : Add D3COLD OFF support

[Imre Deak]

On Thu, Jul 21, 2022 at 03:29:55PM +0530, tilak.tangudu@intel.com wrote:
> [...]
> @@ -1706,6 +1716,12 @@ static int intel_runtime_suspend(struct device *kdev)
>  	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>  		intel_hpd_poll_enable(dev_priv);
>  
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		i915_save_pci_state(pdev);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, PCI_D3cold);
> +	}

Could you add a code comment describing why the above is required? For
standard PCI devices pci_pm_runtime_suspend() should do this except for
calling pci_disable_device().

> +
>  	drm_dbg(&dev_priv->drm, "Device suspended\n");
>  	return 0;
>  }
> @@ -1713,6 +1729,7 @@ static int intel_runtime_suspend(struct device *kdev)
>  static int intel_runtime_resume(struct device *kdev)
>  {
>  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
>  	int ret;
>  
> @@ -1724,7 +1741,25 @@ static int intel_runtime_resume(struct device *kdev)
>  	drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm->wakeref_count));
>  	disable_rpm_wakeref_asserts(rpm);
>  
> -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		ret = pci_set_power_state(pdev, PCI_D0);
> +		if (ret) {
> +			drm_err(&dev_priv->drm,
> +				"failed to set PCI D0 power state (%d)\n", ret);
> +			goto out;
> +		}
> +
> +		i915_load_pci_state(pdev);
> +
> +		ret = pci_enable_device(pdev);
> +		if (ret)
> +			goto out;
> +		pci_set_master(pdev);
> +		intel_opregion_resume(dev_priv);
> +	} else {
> +		intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	}
> +
>  	rpm->suspended = false;
>  	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
>  		drm_dbg(&dev_priv->drm,
> @@ -1742,8 +1777,20 @@ static int intel_runtime_resume(struct device *kdev)
>  	 * No point of rolling back things in case of an error, as the best
>  	 * we can do is to hope that things will still work (and disable RPM).
>  	 */
> -	intel_gt_runtime_resume(to_gt(dev_priv));
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		ret = i915_pcode_init(dev_priv);
> +		if (ret)
> +			goto out;
>  
> +		sanitize_gpu(dev_priv);
> +		ret = i915_ggtt_enable_hw(dev_priv);
> +		if (ret)
> +			drm_err(&dev_priv->drm, "failed to re-enable GGTT\n");
> +		i915_ggtt_resume(to_gt(dev_priv)->ggtt);
> +		i915_gem_resume(dev_priv);
> +	} else {
> +		intel_gt_runtime_resume(to_gt(dev_priv));
> +	}
>  	/*
>  	 * On VLV/CHV display interrupts are part of the display
>  	 * power well, so hpd is reinitialized from there. For
> @@ -1756,6 +1803,7 @@ static int intel_runtime_resume(struct device *kdev)
>  
>  	intel_enable_ipc(dev_priv);
>  
> +out:
>  	enable_rpm_wakeref_asserts(rpm);
>  
>  	if (ret)
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 8/8] drm/i915 : Add D3COLD OFF support

[Gupta, Anshuman]

> -----Original Message-----
> From: Tangudu, Tilak <tilak.tangudu@intel.com>
> Sent: Thursday, July 21, 2022 3:30 PM
> To: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> <vinay.belgaumkar@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nikula, Jani <jani.nikula@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Gupta,
> Anshuman <anshuman.gupta@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>; Deak,
> Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: [PATCH 8/8] drm/i915 : Add D3COLD OFF support
> 
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> Added lmem deep suspend/resume, which covers lmem eviction and added
> GT/GUC deep suspend/resume using i915_gem_backup_suspend,
> i915_gem_suspend_late and i915_gem_resume.
> 
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 74 ++++++++++++++++++++++++------
>  1 file changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 3697ecb2c138..608287bb27ea 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1630,6 +1630,7 @@ static int intel_runtime_idle(struct device *kdev)
> static int intel_runtime_suspend(struct device *kdev)  {
>  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
>  	int ret;
> 
> @@ -1644,9 +1645,14 @@ static int intel_runtime_suspend(struct device *kdev)
>  	 * We are safe here against re-faults, since the fault handler takes
>  	 * an RPM reference.
>  	 */
> -	i915_gem_runtime_suspend(dev_priv);
> -
> -	intel_gt_runtime_suspend(to_gt(dev_priv));
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		i915_gem_backup_suspend(dev_priv);
> +		i915_ggtt_suspend(to_gt(dev_priv)->ggtt);
> +		i915_gem_suspend_late(dev_priv);
		We should use the *runtime* naming convention rather then system wide suspend naming convention here. 
> +	} else {
> +		i915_gem_runtime_suspend(dev_priv);
> +		intel_gt_runtime_suspend(to_gt(dev_priv));
> +	}
> 
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> 
> @@ -1691,14 +1697,18 @@ static int intel_runtime_suspend(struct device
> *kdev)
>  		 */
>  		intel_opregion_notify_adapter(dev_priv, PCI_D3hot);
>  	} else {
> -		/*
> -		 * current versions of firmware which depend on this opregion
> -		 * notification have repurposed the D1 definition to mean
> -		 * "runtime suspended" vs. what you would normally expect
> (D3)
> -		 * to distinguish it from notifications that might be sent via
> -		 * the suspend path.
> -		 */
> -		intel_opregion_notify_adapter(dev_priv, PCI_D1);
> +		if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +			intel_opregion_suspend(dev_priv, PCI_D3cold);
			Not needed.
> +		} else {
> +			/*
> +			 * current versions of firmware which depend on this
> opregion
> +			 * notification have repurposed the D1 definition to
> mean
> +			 * "runtime suspended" vs. what you would normally
> expect (D3)
> +			 * to distinguish it from notifications that might be sent
> via
> +			 * the suspend path.
> +			 */
> +			intel_opregion_notify_adapter(dev_priv, PCI_D1);
			I have also provided the comment on rev1, this is not needed anymore.
                                           This particular opregion is deprecated.
> +		}
>  	}
> 
>  	assert_forcewakes_inactive(&dev_priv->uncore);
> @@ -1706,6 +1716,12 @@ static int intel_runtime_suspend(struct device *kdev)
>  	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>  		intel_hpd_poll_enable(dev_priv);
> 
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		i915_save_pci_state(pdev);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, PCI_D3cold);
		This is not needed, it can set max up to d3hot by config space write.
		PCI core set the device sate or , it is not required by driver.
                             I had provided the same comment on Rev1, please try to address all the comment before floating
                             next revision.

		Thanks,
		Anshuman Gupta.
					
> +	}
> +
>  	drm_dbg(&dev_priv->drm, "Device suspended\n");
>  	return 0;
>  }
> @@ -1713,6 +1729,7 @@ static int intel_runtime_suspend(struct device *kdev)
> static int intel_runtime_resume(struct device *kdev)  {
>  	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
>  	int ret;
> 
> @@ -1724,7 +1741,25 @@ static int intel_runtime_resume(struct device *kdev)
>  	drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm-
> >wakeref_count));
>  	disable_rpm_wakeref_asserts(rpm);
> 
> -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		ret = pci_set_power_state(pdev, PCI_D0);
> +		if (ret) {
> +			drm_err(&dev_priv->drm,
> +				"failed to set PCI D0 power state (%d)\n", ret);
> +			goto out;
> +		}
> +
> +		i915_load_pci_state(pdev);
> +
> +		ret = pci_enable_device(pdev);
> +		if (ret)
> +			goto out;
> +		pci_set_master(pdev);
> +		intel_opregion_resume(dev_priv);
> +	} else {
> +		intel_opregion_notify_adapter(dev_priv, PCI_D0);
> +	}
> +
>  	rpm->suspended = false;
>  	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
>  		drm_dbg(&dev_priv->drm,
> @@ -1742,8 +1777,20 @@ static int intel_runtime_resume(struct device *kdev)
>  	 * No point of rolling back things in case of an error, as the best
>  	 * we can do is to hope that things will still work (and disable RPM).
>  	 */
> -	intel_gt_runtime_resume(to_gt(dev_priv));
> +	if (rpm->d3_state == INTEL_D3COLD_OFF) {
> +		ret = i915_pcode_init(dev_priv);
> +		if (ret)
> +			goto out;
> 
> +		sanitize_gpu(dev_priv);
> +		ret = i915_ggtt_enable_hw(dev_priv);
> +		if (ret)
> +			drm_err(&dev_priv->drm, "failed to re-enable
> GGTT\n");
> +		i915_ggtt_resume(to_gt(dev_priv)->ggtt);
> +		i915_gem_resume(dev_priv);
> +	} else {
> +		intel_gt_runtime_resume(to_gt(dev_priv));
> +	}
>  	/*
>  	 * On VLV/CHV display interrupts are part of the display
>  	 * power well, so hpd is reinitialized from there. For @@ -1756,6
> +1803,7 @@ static int intel_runtime_resume(struct device *kdev)
> 
>  	intel_enable_ipc(dev_priv);
> 
> +out:
>  	enable_rpm_wakeref_asserts(rpm);
> 
>  	if (ret)
> --
> 2.25.1

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Tangudu, Tilak]

 @Nikula, Jani,

As you know we have reused i915_gem_backup_suspend, i915_gem_suspend_late and 
i915_gem_resume in runtime_pm_suspend/resume callbacks ,they use runtime pm 
helpers (intel_runtime_pm_get/put).
These need to be avoided in callbacks as they lead to deadlock.

This can be done in two ways 
1) push runtime pm helpers usage at higher level functions,
Which requires code refactoring (https://patchwork.freedesktop.org/series/105427/#rev2    is partially implemented following this)
2) Add is_intel_rpm_allowed helper and avoid the runtime helpers (https://patchwork.freedesktop.org/series/105427/#rev3 is completely implemented following this)

Hope I gave you the context,

As per your review comment in rev2,  the below is implemented in rev3

""""""""""""""""""""""""
v2: Return -2 if runtime pm is not allowed in runtime_pm_get and skip
wakeref release in runtime_pm_put if wakeref value is -2. - Jani N
Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
"""""""""""""""""""""""""

Rodrigo and myself want to trigger a discussion, if 2) is a proper approach or go with 1) which requires lot of code refactoring.
Or Is there any way we follow 1) with less code refactoring.?
Or Do you think there is any other proper approach ?

(Please note rev3 is not clean, d3cold off support need to be restricted to Headless clients for now , we see some Display related warnings in headed case ).

Thanks
Tilak


> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Tangudu, Tilak
> Sent: Thursday, August 4, 2022 11:03 AM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> <chris.p.wilson@intel.com>; Gupta, saurabhg <saurabhg.gupta@intel.com>;
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> helper
> 
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Thursday, August 4, 2022 2:01 AM
> > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > <matthew.d.roper@intel.com>; Wilson, Chris P
> > <chris.p.wilson@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> > Gupta, saurabhg <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> > Deak, Imre <imre.deak@intel.com>; Iddamsetty, Aravind
> > <aravind.iddamsetty@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> >
> > On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu@intel.com wrote:
> > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > >
> > > Added is_intel_rpm_allowed function to query the runtime_pm status
> > > and disllow during suspending and resuming.
> >
> > >
> > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > skip wakeref release in runtime_pm_put if wakeref value is -2. -
> > > Jani N
> >
> > Should we have some defines instead of the -#s?
> Ok will address this.
> >
> > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > ++++++++++++++++++++++-
> > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 6ed5786bcd29..704beeeb560b 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -113,7 +113,7 @@ static void
> > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> > >  	unsigned long flags, n;
> > >  	bool found = false;
> > >
> > > -	if (unlikely(stack == -1))
> > > +	if (unlikely(stack == -1) || unlikely(stack == -2))
> > >  		return;
> > >
> > >  	spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6 +320,21
> > @@
> > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)
> > > }
> > >
> > >  #endif
> > > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm) {
> > > +	return rpm->kdev->power.runtime_status; }
> > > +
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> >
> > why not static?
>  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> pm_runtime_get_sync, To avoid lock issue we need to use it here too.
> 
> See this patch " drm/i915: Guard rc6 helpers with is_intel_rpm_allowed"
> 
> >
> > > +{
> > > +	int rpm_status;
> > > +
> > > +	rpm_status = intel_runtime_pm_status(rpm);
> > > +	if (rpm_status == RPM_RESUMING
> >
> > I don't have a good feeling about this. If we are resuming we
> > shouldn't grab extra references... This seems a workaround for the lock
> mess.
> >
> > > || rpm_status == RPM_SUSPENDING)
> >
> > and when we are suspending and we call this function is because we
> > need to wake up, no?!
> 
> As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
> and i915_gem_resume , These functions use runtime helpers, which in-turn
> call  runtime suspend/resume callbacks and leads to deadlock.
>  So, these helpers need to be avoided.  we have added is_intel_rpm_allowed
> and disallowed rpm callbacks with above condition during suspending and
> resuming  to avoid deadlock.
> >
> > > +		return false;
> > > +	else
> > > +		return true;
> > > +}
> > >
> > >  static void
> > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > >  						     runtime_pm);
> > >  	int ret;
> > >
> > > +	if (!is_intel_rpm_allowed(rpm))
> > > +		return -2;
> > > +
> > >  	ret = pm_runtime_get_sync(rpm->kdev);
> > >  	drm_WARN_ONCE(&i915->drm, ret < 0,
> > >  		      "pm_runtime_get_sync() failed: %d\n", ret); @@ -490,6
> > +508,9
> > > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
> > >
> > >  	untrack_intel_runtime_pm_wakeref(rpm, wref);
> > >
> > > +	if (wref == -2)
> > > +		return;
> > > +
> > >  	intel_runtime_pm_release(rpm, wakelock);
> > >
> > >  	pm_runtime_mark_last_busy(kdev);
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > index d9160e3ff4af..99418c3a934a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> > > intel_runtime_pm *rpm);
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> >
> > if really need to export please follow the naming convention.\
> 
> Ok will address this.
> 
> -Tilak
> >
> > >
> > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> > > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > intel_runtime_pm *rpm);
> > > --
> > > 2.25.1
> > >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Tangudu, Tilak]

+

> -----Original Message-----
> From: Tangudu, Tilak
> Sent: Wednesday, September 28, 2022 5:46 PM
> To: Tangudu, Tilak <tilak.tangudu@intel.com>; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> Cc: Wilson, Chris P <Chris.P.Wilson@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> 
>  @Nikula, Jani,
> 
> As you know we have reused i915_gem_backup_suspend,
> i915_gem_suspend_late and i915_gem_resume in
> runtime_pm_suspend/resume callbacks ,they use runtime pm helpers
> (intel_runtime_pm_get/put).
> These need to be avoided in callbacks as they lead to deadlock.
> 
> This can be done in two ways
> 1) push runtime pm helpers usage at higher level functions,
> Which requires code refactoring
> (https://patchwork.freedesktop.org/series/105427/#rev2    is partially
> implemented following this)
> 2) Add is_intel_rpm_allowed helper and avoid the runtime helpers
> (https://patchwork.freedesktop.org/series/105427/#rev3 is completely
> implemented following this)
> 
> Hope I gave you the context,
> 
> As per your review comment in rev2,  the below is implemented in rev3
> 
> """"""""""""""""""""""""
> v2: Return -2 if runtime pm is not allowed in runtime_pm_get and skip
> wakeref release in runtime_pm_put if wakeref value is -2. - Jani N
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> """""""""""""""""""""""""
> 
> Rodrigo and myself want to trigger a discussion, if 2) is a proper approach or
> go with 1) which requires lot of code refactoring.
> Or Is there any way we follow 1) with less code refactoring.?
> Or Do you think there is any other proper approach ?
> 
> (Please note rev3 is not clean, d3cold off support need to be restricted to
> Headless clients for now , we see some Display related warnings in headed
> case ).
> 
> Thanks
> Tilak
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Tangudu, Tilak
> > Sent: Thursday, August 4, 2022 11:03 AM
> > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> > <chris.p.wilson@intel.com>; Gupta, saurabhg
> > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added
> > is_intel_rpm_allowed helper
> >
> >
> >
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Sent: Thursday, August 4, 2022 2:01 AM
> > > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > > <matthew.d.roper@intel.com>; Wilson, Chris P
> > > <chris.p.wilson@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> > > Gupta, saurabhg <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > > <anshuman.gupta@intel.com>; Nilawar, Badal
> > > <badal.nilawar@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > > intel-gfx@lists.freedesktop.org
> > > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> > >
> > > On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu@intel.com
> wrote:
> > > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > > >
> > > > Added is_intel_rpm_allowed function to query the runtime_pm status
> > > > and disllow during suspending and resuming.
> > >
> > > >
> > > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > > skip wakeref release in runtime_pm_put if wakeref value is -2. -
> > > > Jani N
> > >
> > > Should we have some defines instead of the -#s?
> > Ok will address this.
> > >
> > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > > ++++++++++++++++++++++-
> > > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index 6ed5786bcd29..704beeeb560b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -113,7 +113,7 @@ static void
> > > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> > > >  	unsigned long flags, n;
> > > >  	bool found = false;
> > > >
> > > > -	if (unlikely(stack == -1))
> > > > +	if (unlikely(stack == -1) || unlikely(stack == -2))
> > > >  		return;
> > > >
> > > >  	spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6 +320,21
> > > @@
> > > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm
> > > > *rpm) }
> > > >
> > > >  #endif
> > > > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm) {
> > > > +	return rpm->kdev->power.runtime_status; }
> > > > +
> > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> > >
> > > why not static?
> >  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> > pm_runtime_get_sync, To avoid lock issue we need to use it here too.
> >
> > See this patch " drm/i915: Guard rc6 helpers with is_intel_rpm_allowed"
> >
> > >
> > > > +{
> > > > +	int rpm_status;
> > > > +
> > > > +	rpm_status = intel_runtime_pm_status(rpm);
> > > > +	if (rpm_status == RPM_RESUMING
> > >
> > > I don't have a good feeling about this. If we are resuming we
> > > shouldn't grab extra references... This seems a workaround for the
> > > lock
> > mess.
> > >
> > > > || rpm_status == RPM_SUSPENDING)
> > >
> > > and when we are suspending and we call this function is because we
> > > need to wake up, no?!
> >
> > As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
> and
> > i915_gem_resume , These functions use runtime helpers, which in-turn
> > call  runtime suspend/resume callbacks and leads to deadlock.
> >  So, these helpers need to be avoided.  we have added
> > is_intel_rpm_allowed and disallowed rpm callbacks with above condition
> > during suspending and resuming  to avoid deadlock.
> > >
> > > > +		return false;
> > > > +	else
> > > > +		return true;
> > > > +}
> > > >
> > > >  static void
> > > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > > >  						     runtime_pm);
> > > >  	int ret;
> > > >
> > > > +	if (!is_intel_rpm_allowed(rpm))
> > > > +		return -2;
> > > > +
> > > >  	ret = pm_runtime_get_sync(rpm->kdev);
> > > >  	drm_WARN_ONCE(&i915->drm, ret < 0,
> > > >  		      "pm_runtime_get_sync() failed: %d\n", ret); @@ -490,6
> > > +508,9
> > > > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm
> > > > *rpm,
> > > >
> > > >  	untrack_intel_runtime_pm_wakeref(rpm, wref);
> > > >
> > > > +	if (wref == -2)
> > > > +		return;
> > > > +
> > > >  	intel_runtime_pm_release(rpm, wakelock);
> > > >
> > > >  	pm_runtime_mark_last_busy(kdev); diff --git
> > > > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > index d9160e3ff4af..99418c3a934a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > > intel_runtime_pm *rpm);  void
> > > > intel_runtime_pm_driver_release(struct
> > > > intel_runtime_pm *rpm);
> > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > >
> > > if really need to export please follow the naming convention.\
> >
> > Ok will address this.
> >
> > -Tilak
> > >
> > > >
> > > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > > > *rpm); intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > > intel_runtime_pm *rpm);
> > > > --
> > > > 2.25.1
> > > >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Vivi, Rodrigo]

On Wed, 2022-09-28 at 12:31 +0000, Tangudu, Tilak wrote:
> +
> 
> > -----Original Message-----
> > From: Tangudu, Tilak
> > Sent: Wednesday, September 28, 2022 5:46 PM
> > To: Tangudu, Tilak <tilak.tangudu@intel.com>; Vivi, Rodrigo
> > <rodrigo.vivi@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> > Cc: Wilson, Chris P <Chris.P.Wilson@intel.com>; Gupta, saurabhg
> > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: RE: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> > helper
> > 
> >  @Nikula, Jani,
> > 
> > As you know we have reused i915_gem_backup_suspend,
> > i915_gem_suspend_late and i915_gem_resume in
> > runtime_pm_suspend/resume callbacks ,they use runtime pm helpers
> > (intel_runtime_pm_get/put).
> > These need to be avoided in callbacks as they lead to deadlock.
> > 
> > This can be done in two ways
> > 1) push runtime pm helpers usage at higher level functions,
> > Which requires code refactoring
> > (https://patchwork.freedesktop.org/series/105427/#rev2    is
> > partially
> > implemented following this)
> > 2) Add is_intel_rpm_allowed helper and avoid the runtime helpers
> > (https://patchwork.freedesktop.org/series/105427/#rev3 is
> > completely
> > implemented following this)
> > 
> > Hope I gave you the context,
> > 
> > As per your review comment in rev2,  the below is implemented in
> > rev3
> > 
> > """"""""""""""""""""""""
> > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > skip
> > wakeref release in runtime_pm_put if wakeref value is -2. - Jani N
> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > """""""""""""""""""""""""
> > 
> > Rodrigo and myself want to trigger a discussion, if 2) is a proper
> > approach or
> > go with 1) which requires lot of code refactoring.
> > Or Is there any way we follow 1) with less code refactoring.?
> > Or Do you think there is any other proper approach ?
> > 
> > (Please note rev3 is not clean, d3cold off support need to be
> > restricted to
> > Headless clients for now , we see some Display related warnings in
> > headed
> > case ).

I believe this warnings shows that the solution 2 has some flaws or
corner cases that we don't fully understand.

I honestly believe we need to go with option 1, moving the runtime_pm_
{get,put} to higher levels.

One way or another, we should not go partial here, but with full
implementation so we can see if we are really covered.

Jani, thoughts?

> > 
> > Thanks
> > Tilak
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > Behalf Of
> > > Tangudu, Tilak
> > > Sent: Thursday, August 4, 2022 11:03 AM
> > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> > > <chris.p.wilson@intel.com>; Gupta, saurabhg
> > > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added
> > > is_intel_rpm_allowed helper
> > > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Sent: Thursday, August 4, 2022 2:01 AM
> > > > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > > > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > > > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > > > <matthew.d.roper@intel.com>; Wilson, Chris P
> > > > <chris.p.wilson@intel.com>; Nikula, Jani
> > > > <jani.nikula@intel.com>;
> > > > Gupta, saurabhg <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > > > <anshuman.gupta@intel.com>; Nilawar, Badal
> > > > <badal.nilawar@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > > > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > > > intel-gfx@lists.freedesktop.org
> > > > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> > > > helper
> > > > 
> > > > On Thu, Jul 21, 2022 at 03:29:48PM +0530,
> > > > tilak.tangudu@intel.com
> > wrote:
> > > > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > 
> > > > > Added is_intel_rpm_allowed function to query the runtime_pm
> > > > > status
> > > > > and disllow during suspending and resuming.
> > > > 
> > > > > 
> > > > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get
> > > > > and
> > > > > skip wakeref release in runtime_pm_put if wakeref value is -
> > > > > 2. -
> > > > > Jani N
> > > > 
> > > > Should we have some defines instead of the -#s?
> > > Ok will address this.
> > > > 
> > > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > > > ++++++++++++++++++++++-
> > > > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > > > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index 6ed5786bcd29..704beeeb560b 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -113,7 +113,7 @@ static void
> > > > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> > > > >         unsigned long flags, n;
> > > > >         bool found = false;
> > > > > 
> > > > > -       if (unlikely(stack == -1))
> > > > > +       if (unlikely(stack == -1) || unlikely(stack == -2))
> > > > >                 return;
> > > > > 
> > > > >         spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6
> > > > > +320,21
> > > > @@
> > > > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm
> > > > > *rpm) }
> > > > > 
> > > > >  #endif
> > > > > +static int intel_runtime_pm_status(struct intel_runtime_pm
> > > > > *rpm) {
> > > > > +       return rpm->kdev->power.runtime_status; }
> > > > > +
> > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> > > > 
> > > > why not static?
> > >  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> > > pm_runtime_get_sync, To avoid lock issue we need to use it here
> > > too.
> > > 
> > > See this patch " drm/i915: Guard rc6 helpers with
> > > is_intel_rpm_allowed"
> > > 
> > > > 
> > > > > +{
> > > > > +       int rpm_status;
> > > > > +
> > > > > +       rpm_status = intel_runtime_pm_status(rpm);
> > > > > +       if (rpm_status == RPM_RESUMING
> > > > 
> > > > I don't have a good feeling about this. If we are resuming we
> > > > shouldn't grab extra references... This seems a workaround for
> > > > the
> > > > lock
> > > mess.
> > > > 
> > > > > > > rpm_status == RPM_SUSPENDING)
> > > > 
> > > > and when we are suspending and we call this function is because
> > > > we
> > > > need to wake up, no?!
> > > 
> > > As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
> > and
> > > i915_gem_resume , These functions use runtime helpers, which in-
> > > turn
> > > call  runtime suspend/resume callbacks and leads to deadlock.
> > >  So, these helpers need to be avoided.  we have added
> > > is_intel_rpm_allowed and disallowed rpm callbacks with above
> > > condition
> > > during suspending and resuming  to avoid deadlock.
> > > > 
> > > > > +               return false;
> > > > > +       else
> > > > > +               return true;
> > > > > +}
> > > > > 
> > > > >  static void
> > > > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > > > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > > > >                                                     
> > > > > runtime_pm);
> > > > >         int ret;
> > > > > 
> > > > > +       if (!is_intel_rpm_allowed(rpm))
> > > > > +               return -2;
> > > > > +
> > > > >         ret = pm_runtime_get_sync(rpm->kdev);
> > > > >         drm_WARN_ONCE(&i915->drm, ret < 0,
> > > > >                       "pm_runtime_get_sync() failed: %d\n",
> > > > > ret); @@ -490,6
> > > > +508,9
> > > > > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm
> > > > > *rpm,
> > > > > 
> > > > >         untrack_intel_runtime_pm_wakeref(rpm, wref);
> > > > > 
> > > > > +       if (wref == -2)
> > > > > +               return;
> > > > > +
> > > > >         intel_runtime_pm_release(rpm, wakelock);
> > > > > 
> > > > >         pm_runtime_mark_last_busy(kdev); diff --git
> > > > > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > index d9160e3ff4af..99418c3a934a 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > > > intel_runtime_pm *rpm);  void
> > > > > intel_runtime_pm_driver_release(struct
> > > > > intel_runtime_pm *rpm);
> > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > > > 
> > > > if really need to export please follow the naming convention.\
> > > 
> > > Ok will address this.
> > > 
> > > -Tilak
> > > > 
> > > > > 
> > > > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > > > > *rpm); intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > > > intel_runtime_pm *rpm);
> > > > > --
> > > > > 2.25.1
> > > > > 

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Gupta, Anshuman]

Quoting Tilak.
> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Wednesday, September 28, 2022 8:00 PM
> To: Nikula, Jani <jani.nikula@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>
> Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Gupta, saurabhg
> <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> 
> On Wed, 2022-09-28 at 12:31 +0000, Tangudu, Tilak wrote:
> > +
> >
> > > -----Original Message-----
> > > From: Tangudu, Tilak
> > > Sent: Wednesday, September 28, 2022 5:46 PM
> > > To: Tangudu, Tilak <tilak.tangudu@intel.com>; Vivi, Rodrigo
> > > <rodrigo.vivi@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> > > Cc: Wilson, Chris P <Chris.P.Wilson@intel.com>; Gupta, saurabhg
> > > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > > Subject: RE: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> > >
> > >  @Nikula, Jani,
> > >
> > > As you know we have reused i915_gem_backup_suspend,
> > > i915_gem_suspend_late and i915_gem_resume in
> > > runtime_pm_suspend/resume callbacks ,they use runtime pm helpers
> > > (intel_runtime_pm_get/put).
> > > These need to be avoided in callbacks as they lead to deadlock.
> > >
> > > This can be done in two ways
> > > 1) push runtime pm helpers usage at higher level functions, Which
> > > requires code refactoring
AFAIK pushing runtime PM to higher level need to asses case by case,
Moving runtime PM wakeref to higher level will also burn more power as
Wakeref will be active for longer period.
This has to be resolve case by case, as a simple rule of thumb we don't need any wakeref in suspend path.
So refactoring based upon suspend specific function and general use function would be the correct approach.
Rest Jani and Rodrigo can provide their input here.

Thanks,
Anshuman Gupta.
 
> > > (https://patchwork.freedesktop.org/series/105427/#rev2    is
> > > partially implemented following this)
> > > 2) Add is_intel_rpm_allowed helper and avoid the runtime helpers
> > > (https://patchwork.freedesktop.org/series/105427/#rev3 is completely
> > > implemented following this)
> > >
> > > Hope I gave you the context,
> > >
> > > As per your review comment in rev2,  the below is implemented in
> > > rev3
> > >
> > > """"""""""""""""""""""""
> > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > skip wakeref release in runtime_pm_put if wakeref value is -2. -
> > > Jani N
> > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > """""""""""""""""""""""""
> > >
> > > Rodrigo and myself want to trigger a discussion, if 2) is a proper
> > > approach or go with 1) which requires lot of code refactoring.
> > > Or Is there any way we follow 1) with less code refactoring.?
> > > Or Do you think there is any other proper approach ?
> > >
> > > (Please note rev3 is not clean, d3cold off support need to be
> > > restricted to Headless clients for now , we see some Display related
> > > warnings in headed case ).
> 
> I believe this warnings shows that the solution 2 has some flaws or corner
> cases that we don't fully understand.
> 
> I honestly believe we need to go with option 1, moving the runtime_pm_
> {get,put} to higher levels.
> 
> One way or another, we should not go partial here, but with full
> implementation so we can see if we are really covered.
> 
> Jani, thoughts?
> 
> > >
> > > Thanks
> > > Tilak
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > Behalf Of Tangudu, Tilak
> > > > Sent: Thursday, August 4, 2022 11:03 AM
> > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> > > > <chris.p.wilson@intel.com>; Gupta, saurabhg
> > > > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > > > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added
> > > > is_intel_rpm_allowed helper
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > Sent: Thursday, August 4, 2022 2:01 AM
> > > > > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > > > > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > > > > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > > > > <matthew.d.roper@intel.com>; Wilson, Chris P
> > > > > <chris.p.wilson@intel.com>; Nikula, Jani
> > > > > <jani.nikula@intel.com>; Gupta, saurabhg
> > > > > <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > > > > <anshuman.gupta@intel.com>; Nilawar, Badal
> > > > > <badal.nilawar@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > > > > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > > > > intel-gfx@lists.freedesktop.org
> > > > > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> > > > > helper
> > > > >
> > > > > On Thu, Jul 21, 2022 at 03:29:48PM +0530,
> > > > > tilak.tangudu@intel.com
> > > wrote:
> > > > > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > >
> > > > > > Added is_intel_rpm_allowed function to query the runtime_pm
> > > > > > status and disllow during suspending and resuming.
> > > > >
> > > > > >
> > > > > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get
> > > > > > and skip wakeref release in runtime_pm_put if wakeref value is
> > > > > > - 2. - Jani N
> > > > >
> > > > > Should we have some defines instead of the -#s?
> > > > Ok will address this.
> > > > >
> > > > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > > > > ++++++++++++++++++++++-
> > > > > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > > > > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > index 6ed5786bcd29..704beeeb560b 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > @@ -113,7 +113,7 @@ static void
> > > > > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm
> *rpm,
> > > > > >         unsigned long flags, n;
> > > > > >         bool found = false;
> > > > > >
> > > > > > -       if (unlikely(stack == -1))
> > > > > > +       if (unlikely(stack == -1) || unlikely(stack == -2))
> > > > > >                 return;
> > > > > >
> > > > > >         spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6
> > > > > > +320,21
> > > > > @@
> > > > > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm
> > > > > > *rpm) }
> > > > > >
> > > > > >  #endif
> > > > > > +static int intel_runtime_pm_status(struct intel_runtime_pm
> > > > > > *rpm) {
> > > > > > +       return rpm->kdev->power.runtime_status; }
> > > > > > +
> > > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> > > > >
> > > > > why not static?
> > > >  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> > > > pm_runtime_get_sync, To avoid lock issue we need to use it here
> > > > too.
> > > >
> > > > See this patch " drm/i915: Guard rc6 helpers with
> > > > is_intel_rpm_allowed"
> > > >
> > > > >
> > > > > > +{
> > > > > > +       int rpm_status;
> > > > > > +
> > > > > > +       rpm_status = intel_runtime_pm_status(rpm);
> > > > > > +       if (rpm_status == RPM_RESUMING
> > > > >
> > > > > I don't have a good feeling about this. If we are resuming we
> > > > > shouldn't grab extra references... This seems a workaround for
> > > > > the lock
> > > > mess.
> > > > >
> > > > > > > > rpm_status == RPM_SUSPENDING)
> > > > >
> > > > > and when we are suspending and we call this function is because
> > > > > we need to wake up, no?!
> > > >
> > > > As we have re-used i915_gem_backup_suspend,
> i915_gem_suspend_late
> > > and
> > > > i915_gem_resume , These functions use runtime helpers, which in-
> > > > turn call  runtime suspend/resume callbacks and leads to deadlock.
> > > >  So, these helpers need to be avoided.  we have added
> > > > is_intel_rpm_allowed and disallowed rpm callbacks with above
> > > > condition during suspending and resuming  to avoid deadlock.
> > > > >
> > > > > > +               return false;
> > > > > > +       else
> > > > > > +               return true;
> > > > > > +}
> > > > > >
> > > > > >  static void
> > > > > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > > > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > > > > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > > > > >
> > > > > > runtime_pm);
> > > > > >         int ret;
> > > > > >
> > > > > > +       if (!is_intel_rpm_allowed(rpm))
> > > > > > +               return -2;
> > > > > > +
> > > > > >         ret = pm_runtime_get_sync(rpm->kdev);
> > > > > >         drm_WARN_ONCE(&i915->drm, ret < 0,
> > > > > >                       "pm_runtime_get_sync() failed: %d\n",
> > > > > > ret); @@ -490,6
> > > > > +508,9
> > > > > > @@ static void __intel_runtime_pm_put(struct
> intel_runtime_pm
> > > > > > *rpm,
> > > > > >
> > > > > >         untrack_intel_runtime_pm_wakeref(rpm, wref);
> > > > > >
> > > > > > +       if (wref == -2)
> > > > > > +               return;
> > > > > > +
> > > > > >         intel_runtime_pm_release(rpm, wakelock);
> > > > > >
> > > > > >         pm_runtime_mark_last_busy(kdev); diff --git
> > > > > > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > index d9160e3ff4af..99418c3a934a 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > > > > intel_runtime_pm *rpm);  void
> > > > > > intel_runtime_pm_driver_release(struct
> > > > > > intel_runtime_pm *rpm);
> > > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > > > >
> > > > > if really need to export please follow the naming convention.\
> > > >
> > > > Ok will address this.
> > > >
> > > > -Tilak
> > > > >
> > > > > >
> > > > > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > > > > > *rpm); intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > > > > intel_runtime_pm *rpm);
> > > > > > --
> > > > > > 2.25.1
> > > > > >

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Rodrigo Vivi]

+Rafael

On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu@intel.com wrote:
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> Added is_intel_rpm_allowed function to query the runtime_pm
> status and disllow during suspending and resuming.
> 
> v2: Return -2 if runtime pm is not allowed in runtime_pm_get
> and skip wakeref release in runtime_pm_put if wakeref value
> is -2. - Jani N
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 ++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6ed5786bcd29..704beeeb560b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -113,7 +113,7 @@ static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
>  	unsigned long flags, n;
>  	bool found = false;
>  
> -	if (unlikely(stack == -1))
> +	if (unlikely(stack == -1) || unlikely(stack == -2))
>  		return;
>  
>  	spin_lock_irqsave(&rpm->debug.lock, flags);
> @@ -320,6 +320,21 @@ untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)
>  }
>  
>  #endif
> +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm)
> +{
> +	return rpm->kdev->power.runtime_status;
> +}
> +
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> +{
> +	int rpm_status;
> +
> +	rpm_status = intel_runtime_pm_status(rpm);
> +	if (rpm_status == RPM_RESUMING || rpm_status == RPM_SUSPENDING)

Rafael, do you believe we could add something like this to linux/pm_runtime.h :

static inline bool pm_runtime_is_transitioning(struct device *dev)
{
	return dev->power.runtime_status == RPM_RESUMING ||
		dev->power.runtime_status == RPM_SUSPENDING;
}

> +		return false;
> +	else
> +		return true;
> +}
>  
>  static void
>  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock)
> @@ -354,6 +369,9 @@ static intel_wakeref_t __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
>  						     runtime_pm);
>  	int ret;
>  
> +	if (!is_intel_rpm_allowed(rpm))
> +		return -2;
> +
>  	ret = pm_runtime_get_sync(rpm->kdev);
>  	drm_WARN_ONCE(&i915->drm, ret < 0,
>  		      "pm_runtime_get_sync() failed: %d\n", ret);
> @@ -490,6 +508,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
>  
>  	untrack_intel_runtime_pm_wakeref(rpm, wref);
>  
> +	if (wref == -2)
> +		return;
> +
>  	intel_runtime_pm_release(rpm, wakelock);
>  
>  	pm_runtime_mark_last_busy(kdev);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index d9160e3ff4af..99418c3a934a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
> +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
>  
>  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
>  intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

[Rodrigo Vivi]

On Thu, Sep 29, 2022 at 05:56:36AM +0000, Gupta, Anshuman wrote:
> 
> Quoting Tilak.
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Wednesday, September 28, 2022 8:00 PM
> > To: Nikula, Jani <jani.nikula@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>
> > Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Gupta, saurabhg
> > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> > 
> > On Wed, 2022-09-28 at 12:31 +0000, Tangudu, Tilak wrote:
> > > +
> > >
> > > > -----Original Message-----
> > > > From: Tangudu, Tilak
> > > > Sent: Wednesday, September 28, 2022 5:46 PM
> > > > To: Tangudu, Tilak <tilak.tangudu@intel.com>; Vivi, Rodrigo
> > > > <rodrigo.vivi@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> > > > Cc: Wilson, Chris P <Chris.P.Wilson@intel.com>; Gupta, saurabhg
> > > > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > > > Subject: RE: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> > > >
> > > >  @Nikula, Jani,
> > > >
> > > > As you know we have reused i915_gem_backup_suspend,
> > > > i915_gem_suspend_late and i915_gem_resume in
> > > > runtime_pm_suspend/resume callbacks ,they use runtime pm helpers
> > > > (intel_runtime_pm_get/put).
> > > > These need to be avoided in callbacks as they lead to deadlock.
> > > >
> > > > This can be done in two ways
> > > > 1) push runtime pm helpers usage at higher level functions, Which
> > > > requires code refactoring
> AFAIK pushing runtime PM to higher level need to asses case by case,
> Moving runtime PM wakeref to higher level will also burn more power as
> Wakeref will be active for longer period.
> This has to be resolve case by case, as a simple rule of thumb we don't need any wakeref in suspend path.
> So refactoring based upon suspend specific function and general use function would be the correct approach.
> Rest Jani and Rodrigo can provide their input here.

Okay, I'm convinced now that the better path is to use the status.
But this patch needs some clean-up and split.

Hopefully we can get the runtime_is_transitioninig function in the
linux/pm_runtime.h directly later, but one way or another, this
part of the patch needs to be separated with the '-2' return.

And that one with a different explanation and probably some enums?!

> 
> Thanks,
> Anshuman Gupta.
>  
> > > > (https://patchwork.freedesktop.org/series/105427/#rev2    is
> > > > partially implemented following this)
> > > > 2) Add is_intel_rpm_allowed helper and avoid the runtime helpers
> > > > (https://patchwork.freedesktop.org/series/105427/#rev3 is completely
> > > > implemented following this)
> > > >
> > > > Hope I gave you the context,
> > > >
> > > > As per your review comment in rev2,  the below is implemented in
> > > > rev3
> > > >
> > > > """"""""""""""""""""""""
> > > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > > skip wakeref release in runtime_pm_put if wakeref value is -2. -
> > > > Jani N
> > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > """""""""""""""""""""""""
> > > >
> > > > Rodrigo and myself want to trigger a discussion, if 2) is a proper
> > > > approach or go with 1) which requires lot of code refactoring.
> > > > Or Is there any way we follow 1) with less code refactoring.?
> > > > Or Do you think there is any other proper approach ?
> > > >
> > > > (Please note rev3 is not clean, d3cold off support need to be
> > > > restricted to Headless clients for now , we see some Display related
> > > > warnings in headed case ).
> > 
> > I believe this warnings shows that the solution 2 has some flaws or corner
> > cases that we don't fully understand.
> > 
> > I honestly believe we need to go with option 1, moving the runtime_pm_
> > {get,put} to higher levels.
> > 
> > One way or another, we should not go partial here, but with full
> > implementation so we can see if we are really covered.
> > 
> > Jani, thoughts?
> > 
> > > >
> > > > Thanks
> > > > Tilak
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > > Behalf Of Tangudu, Tilak
> > > > > Sent: Thursday, August 4, 2022 11:03 AM
> > > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> > > > > <chris.p.wilson@intel.com>; Gupta, saurabhg
> > > > > <saurabhg.gupta@intel.com>; intel-gfx@lists.freedesktop.org
> > > > > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added
> > > > > is_intel_rpm_allowed helper
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > > Sent: Thursday, August 4, 2022 2:01 AM
> > > > > > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > > > > > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > > > > > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > > > > > <matthew.d.roper@intel.com>; Wilson, Chris P
> > > > > > <chris.p.wilson@intel.com>; Nikula, Jani
> > > > > > <jani.nikula@intel.com>; Gupta, saurabhg
> > > > > > <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > > > > > <anshuman.gupta@intel.com>; Nilawar, Badal
> > > > > > <badal.nilawar@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > > > > > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > > > > > intel-gfx@lists.freedesktop.org
> > > > > > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> > > > > > helper
> > > > > >
> > > > > > On Thu, Jul 21, 2022 at 03:29:48PM +0530,
> > > > > > tilak.tangudu@intel.com
> > > > wrote:
> > > > > > > From: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > > >
> > > > > > > Added is_intel_rpm_allowed function to query the runtime_pm
> > > > > > > status and disllow during suspending and resuming.
> > > > > >
> > > > > > >
> > > > > > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get
> > > > > > > and skip wakeref release in runtime_pm_put if wakeref value is
> > > > > > > - 2. - Jani N
> > > > > >
> > > > > > Should we have some defines instead of the -#s?
> > > > > Ok will address this.
> > > > > >
> > > > > > > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > > > > > ++++++++++++++++++++++-
> > > > > > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > > > > > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > index 6ed5786bcd29..704beeeb560b 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > > > @@ -113,7 +113,7 @@ static void
> > > > > > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm
> > *rpm,
> > > > > > >         unsigned long flags, n;
> > > > > > >         bool found = false;
> > > > > > >
> > > > > > > -       if (unlikely(stack == -1))
> > > > > > > +       if (unlikely(stack == -1) || unlikely(stack == -2))
> > > > > > >                 return;
> > > > > > >
> > > > > > >         spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6
> > > > > > > +320,21
> > > > > > @@
> > > > > > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm
> > > > > > > *rpm) }
> > > > > > >
> > > > > > >  #endif
> > > > > > > +static int intel_runtime_pm_status(struct intel_runtime_pm
> > > > > > > *rpm) {
> > > > > > > +       return rpm->kdev->power.runtime_status; }
> > > > > > > +
> > > > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> > > > > >
> > > > > > why not static?
> > > > >  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> > > > > pm_runtime_get_sync, To avoid lock issue we need to use it here
> > > > > too.
> > > > >
> > > > > See this patch " drm/i915: Guard rc6 helpers with
> > > > > is_intel_rpm_allowed"
> > > > >
> > > > > >
> > > > > > > +{
> > > > > > > +       int rpm_status;
> > > > > > > +
> > > > > > > +       rpm_status = intel_runtime_pm_status(rpm);
> > > > > > > +       if (rpm_status == RPM_RESUMING
> > > > > >
> > > > > > I don't have a good feeling about this. If we are resuming we
> > > > > > shouldn't grab extra references... This seems a workaround for
> > > > > > the lock
> > > > > mess.
> > > > > >
> > > > > > > > > rpm_status == RPM_SUSPENDING)
> > > > > >
> > > > > > and when we are suspending and we call this function is because
> > > > > > we need to wake up, no?!
> > > > >
> > > > > As we have re-used i915_gem_backup_suspend,
> > i915_gem_suspend_late
> > > > and
> > > > > i915_gem_resume , These functions use runtime helpers, which in-
> > > > > turn call  runtime suspend/resume callbacks and leads to deadlock.
> > > > >  So, these helpers need to be avoided.  we have added
> > > > > is_intel_rpm_allowed and disallowed rpm callbacks with above
> > > > > condition during suspending and resuming  to avoid deadlock.
> > > > > >
> > > > > > > +               return false;
> > > > > > > +       else
> > > > > > > +               return true;
> > > > > > > +}
> > > > > > >
> > > > > > >  static void
> > > > > > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > > > > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > > > > > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > > > > > >
> > > > > > > runtime_pm);
> > > > > > >         int ret;
> > > > > > >
> > > > > > > +       if (!is_intel_rpm_allowed(rpm))
> > > > > > > +               return -2;
> > > > > > > +
> > > > > > >         ret = pm_runtime_get_sync(rpm->kdev);
> > > > > > >         drm_WARN_ONCE(&i915->drm, ret < 0,
> > > > > > >                       "pm_runtime_get_sync() failed: %d\n",
> > > > > > > ret); @@ -490,6
> > > > > > +508,9
> > > > > > > @@ static void __intel_runtime_pm_put(struct
> > intel_runtime_pm
> > > > > > > *rpm,
> > > > > > >
> > > > > > >         untrack_intel_runtime_pm_wakeref(rpm, wref);
> > > > > > >
> > > > > > > +       if (wref == -2)
> > > > > > > +               return;
> > > > > > > +
> > > > > > >         intel_runtime_pm_release(rpm, wakelock);
> > > > > > >
> > > > > > >         pm_runtime_mark_last_busy(kdev); diff --git
> > > > > > > a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > > index d9160e3ff4af..99418c3a934a 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > > > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > > > > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > > > > > intel_runtime_pm *rpm);  void
> > > > > > > intel_runtime_pm_driver_release(struct
> > > > > > > intel_runtime_pm *rpm);
> > > > > > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> > > > > >
> > > > > > if really need to export please follow the naming convention.\
> > > > >
> > > > > Ok will address this.
> > > > >
> > > > > -Tilak
> > > > > >
> > > > > > >
> > > > > > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm
> > > > > > > *rpm); intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > > > > > intel_runtime_pm *rpm);
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
>