[PATCH v2 0/8] Fix PSR-vblank-DMC interaction

[PATCH v2 0/8] Fix PSR-vblank-DMC interaction

[Dhinakaran Pandiyan]

The first 3 patches are minor PSR improvements. The last 5 introduce a new
power domain to disable DC states when vblanks are required. The tricky
part is to enable and disable the DC_OFF power well in atomic context. I
have addressed the locking issues that CI caught in the last revision.

Dhinakaran Pandiyan (8):
  drm/i915/psr: Kill psr.source_ok flag.
  drm/i915/psr: CAN_PSR() macro to check for PSR source and sink
    support.
  drm/i915/psr: Avoid initializing PSR if there is no sink support.
  drm/vblank: Do not update vblank counts if vblanks are already
    disabled.
  drm/vblank: Restoring vblank counts after device runtime PM events.
  drm/i915: Use an atomic_t array to track power domain use count.
  drm/i915: Introduce a non-blocking power domain for vblank interrupts
  drm/i915: Use the vblank power domain disallow or disable DC states.

 drivers/gpu/drm/drm_vblank.c            |  56 ++++++---
 drivers/gpu/drm/i915/i915_debugfs.c     |  10 +-
 drivers/gpu/drm/i915/i915_drv.h         |  21 +++-
 drivers/gpu/drm/i915/i915_irq.c         |   6 +
 drivers/gpu/drm/i915/intel_drv.h        |   4 +
 drivers/gpu/drm/i915/intel_psr.c        |  30 +++--
 drivers/gpu/drm/i915/intel_runtime_pm.c | 195 ++++++++++++++++++++++++++++----
 include/drm/drm_vblank.h                |   1 +
 8 files changed, 268 insertions(+), 55 deletions(-)

-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/8] drm/i915/psr: Kill psr.source_ok flag.

[Dhinakaran Pandiyan]

This flag has become redundant since
commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc state")
It is set at the same place as psr.enabled, which is also exposed via
debugfs.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 1 -
 drivers/gpu/drm/i915/i915_drv.h     | 1 -
 drivers/gpu/drm/i915/intel_psr.c    | 2 --
 3 files changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0ddce72552bf..64e5a263458c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2540,7 +2540,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 	mutex_lock(&dev_priv->psr.lock);
 	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
-	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
 	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1aba5657f5f0..1e4e613e7b41 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1199,7 +1199,6 @@ struct i915_drrs {
 struct i915_psr {
 	struct mutex lock;
 	bool sink_support;
-	bool source_ok;
 	struct intel_dp *enabled;
 	bool active;
 	struct delayed_work work;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index a1ad85fa5c1a..c4d75e82a1df 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -522,8 +522,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	}
 
 	dev_priv->psr.psr2_support = crtc_state->has_psr2;
-	dev_priv->psr.source_ok = true;
-
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/8] drm/i915/psr: CAN_PSR() macro to check for PSR source and sink support.

[Dhinakaran Pandiyan]

The global variable dev_priv->psr.sink_support is set if an eDP sink
supports PSR. Use this instead of redoing the check with is_edp_psr().
Combine source and sink support checks into a macro that can be used to
return early from psr_{invalidate, single_frame_update, flush}.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 19 ++++---------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f89d64..48676e99316e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1760,6 +1760,7 @@ static inline void intel_backlight_device_unregister(struct intel_connector *con
 
 
 /* intel_psr.c */
+#define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
 void intel_psr_enable(struct intel_dp *intel_dp,
 		      const struct intel_crtc_state *crtc_state);
 void intel_psr_disable(struct intel_dp *intel_dp,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c4d75e82a1df..76339cf387cb 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -56,14 +56,6 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static bool is_edp_psr(struct intel_dp *intel_dp)
-{
-	if (!intel_dp_is_edp(intel_dp))
-		return false;
-
-	return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
-}
-
 static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -358,10 +350,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 		&crtc_state->base.adjusted_mode;
 	int psr_setup_time;
 
-	if (!HAS_PSR(dev_priv))
-		return;
-
-	if (!is_edp_psr(intel_dp))
+	if (!CAN_PSR(dev_priv))
 		return;
 
 	if (!i915_modparams.enable_psr) {
@@ -794,7 +783,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
 	enum pipe pipe;
 	u32 val;
 
-	if (!HAS_PSR(dev_priv))
+	if (!CAN_PSR(dev_priv))
 		return;
 
 	/*
@@ -843,7 +832,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!HAS_PSR(dev_priv))
+	if (!CAN_PSR(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->psr.lock);
@@ -883,7 +872,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!HAS_PSR(dev_priv))
+	if (!CAN_PSR(dev_priv))
 		return;
 
 	mutex_lock(&dev_priv->psr.lock);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/8] drm/i915/psr: Avoid initializing PSR if there is no sink support.

[Dhinakaran Pandiyan]

DPCD read for the eDP is complete by the time intel_psr_init() is
called, which means we can avoid initializing PSR structures and state
if there is no sink support.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++-
 drivers/gpu/drm/i915/intel_psr.c    | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 64e5a263458c..1a7b28f62570 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	u32 stat[3];
 	enum pipe pipe;
 	bool enabled = false;
+	bool sink_support;
 
 	if (!HAS_PSR(dev_priv))
 		return -ENODEV;
 
+	sink_support = dev_priv->psr.sink_support;
+	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
+	if (!sink_support)
+		return 0;
+
 	intel_runtime_pm_get(dev_priv);
 
 	mutex_lock(&dev_priv->psr.lock);
-	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
 	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 76339cf387cb..095e0a5a8574 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	if (!crtc_state->has_psr)
 		return;
 
+	if (WARN_ON(!CAN_PSR(dev_priv)))
+		return;
+
 	WARN_ON(dev_priv->drrs.dp);
 	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
@@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	if (!old_crtc_state->has_psr)
 		return;
 
+	if (WARN_ON(!CAN_PSR(dev_priv)))
+		return;
+
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
 		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
 
+	if (!dev_priv->psr.sink_support)
+		return;
+
 	/* Per platform default: all disabled. */
 	if (i915_modparams.enable_psr == -1)
 		i915_modparams.enable_psr = 0;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/8] drm/vblank: Do not update vblank counts if vblanks are already disabled.

[Dhinakaran Pandiyan]

Updating the vblank counts requires register reads and these reads may not
return meaningful values after the vblank interrupts are disabled as the
device may go to low power state. An additional change would be to allow
the driver to save the vblank counts before entering a low power state, but
that's for the future.

Also, disable vblanks after reading the HW counter in the case where
_crtc_vblank_off() is disabling vblanks.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..7eee82c06ed8 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -347,23 +347,14 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
 
 	/*
-	 * Only disable vblank interrupts if they're enabled. This avoids
-	 * calling the ->disable_vblank() operation in atomic context with the
-	 * hardware potentially runtime suspended.
-	 */
-	if (vblank->enabled) {
-		__disable_vblank(dev, pipe);
-		vblank->enabled = false;
-	}
-
-	/*
-	 * Always update the count and timestamp to maintain the
+	 * Update the count and timestamp to maintain the
 	 * appearance that the counter has been ticking all along until
 	 * this time. This makes the count account for the entire time
 	 * between drm_crtc_vblank_on() and drm_crtc_vblank_off().
 	 */
 	drm_update_vblank_count(dev, pipe, false);
-
+	__disable_vblank(dev, pipe);
+	vblank->enabled = false;
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 }
 
@@ -1122,8 +1113,12 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
 		      pipe, vblank->enabled, vblank->inmodeset);
 
 	/* Avoid redundant vblank disables without previous
-	 * drm_crtc_vblank_on(). */
-	if (drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset)
+	 * drm_crtc_vblank_on() and only disable them if they're enabled. This
+	 * avoids calling the ->disable_vblank() operation in atomic context
+	 * with the hardware potentially runtime suspended.
+	 */
+	if ((drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset) &&
+	    vblank->enabled)
 		drm_vblank_disable_and_save(dev, pipe);
 
 	wake_up(&vblank->queue);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/8] drm/vblank: Restoring vblank counts after device runtime PM events.

[Dhinakaran Pandiyan]

The HW frame counter can get reset when devices enters low power
states and this messes up any following vblank count updates. So, compute
the missed vblank interrupts for that low power state duration using time
stamps. This is similar to _crtc_vblank_on() except that it doesn't enable
vblank interrupts because this function is expected to be called from
the driver _enable_vblank() vfunc.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_vblank.h     |  1 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 7eee82c06ed8..494e2cff6e55 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1230,6 +1230,39 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_vblank_on);
 
+void drm_crtc_vblank_restore(struct drm_device *dev, unsigned int pipe)
+{
+	ktime_t t_vblank;
+	struct drm_vblank_crtc *vblank;
+	int framedur_ns;
+	u64 diff_ns;
+	u32 cur_vblank, diff = 1;
+	int count = DRM_TIMESTAMP_MAXRETRIES;
+
+	if (WARN_ON(pipe >= dev->num_crtcs))
+		return;
+
+	vblank = &dev->vblank[pipe];
+	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
+		  "Cannot compute missed vblanks without frame duration\n");
+	framedur_ns = vblank->framedur_ns;
+
+	do {
+		cur_vblank = __get_vblank_counter(dev, pipe);
+		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
+	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+
+	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
+	if (framedur_ns)
+		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
+
+
+	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
+		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
+	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
+}
+EXPORT_SYMBOL(drm_crtc_vblank_restore);
+
 static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
 					  unsigned int pipe)
 {
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 848b463a0af5..aafcbef91bd7 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -180,6 +180,7 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
 void drm_crtc_vblank_reset(struct drm_crtc *crtc);
 void drm_crtc_vblank_on(struct drm_crtc *crtc);
 u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
+void drm_crtc_vblank_restore(struct drm_device *dev, unsigned int pipe);
 
 bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe, int *max_error,
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/8] drm/i915: Use an atomic_t array to track power domain use count.

[Dhinakaran Pandiyan]

Convert the power_domains->domain_use_count array that tracks per-domain
use count to atomic_t type. This is needed to be able to read/write the use
counts outside of the power domain mutex.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++------
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1a7b28f62570..1f1d9162f2c2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2764,7 +2764,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
 		for_each_power_domain(power_domain, power_well->domains)
 			seq_printf(m, "  %-23s %d\n",
 				 intel_display_power_domain_str(power_domain),
-				 power_domains->domain_use_count[power_domain]);
+				 atomic_read(&power_domains->domain_use_count[power_domain]));
 	}
 
 	mutex_unlock(&power_domains->lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e4e613e7b41..ddadeb9eaf49 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1489,7 +1489,7 @@ struct i915_power_domains {
 	int power_well_count;
 
 	struct mutex lock;
-	int domain_use_count[POWER_DOMAIN_NUM];
+	atomic_t domain_use_count[POWER_DOMAIN_NUM];
 	struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 96ab74f3d101..992caec1fbc4 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1453,7 +1453,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
 		intel_power_well_get(dev_priv, power_well);
 
-	power_domains->domain_use_count[domain]++;
+	atomic_inc(&power_domains->domain_use_count[domain]);
 }
 
 /**
@@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&power_domains->lock);
 
-	WARN(!power_domains->domain_use_count[domain],
-	     "Use count on domain %s is already zero\n",
+	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
+	     "Use count on domain %s was already zero\n",
 	     intel_display_power_domain_str(domain));
-	power_domains->domain_use_count[domain]--;
 
 	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
 		intel_power_well_put(dev_priv, power_well);
@@ -3049,7 +3048,7 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
 		for_each_power_domain(domain, power_well->domains)
 			DRM_DEBUG_DRIVER("  %-23s %d\n",
 					 intel_display_power_domain_str(domain),
-					 power_domains->domain_use_count[domain]);
+					 atomic_read(&power_domains->domain_use_count[domain]));
 	}
 }
 
@@ -3092,7 +3091,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 
 		domains_count = 0;
 		for_each_power_domain(domain, power_well->domains)
-			domains_count += power_domains->domain_use_count[domain];
+			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
 
 		if (power_well->count != domains_count) {
 			DRM_ERROR("power well %s refcount/domain refcount mismatch "
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 7/8] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Dhinakaran Pandiyan]

When DC states are enabled and PSR is active, the hardware enters DC5/DC6
states resulting in frame counter resets. The frame counter resets mess
up the vblank counting logic. In order to disable DC states when
vblank interrupts are required and to disallow DC states when vblanks
interrupts are already enabled, introduce a new VBLANK power domain.
Since this power domain reference needs to be acquired and released in
atomic context, _vblank_get() and _vblank_put() methods skip the
power_domain mutex.

v2: Fix deadlock by switching irqsave spinlock.
    Implement atomic version of get_if_enabled.
    Modify power_domain_verify_state to check power well use count and
enabled status atomically.
    Rewrite of intel_power_well_{get,put}

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  18 ++++
 drivers/gpu/drm/i915/intel_drv.h        |   3 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 184 +++++++++++++++++++++++++++++---
 3 files changed, 192 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ddadeb9eaf49..db597c5ebaed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -397,6 +397,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_AUX_C,
 	POWER_DOMAIN_AUX_D,
 	POWER_DOMAIN_GMBUS,
+	POWER_DOMAIN_VBLANK,
 	POWER_DOMAIN_MODESET,
 	POWER_DOMAIN_GT_IRQ,
 	POWER_DOMAIN_INIT,
@@ -1476,6 +1477,23 @@ struct i915_power_well {
 			bool has_fuses:1;
 		} hsw;
 	};
+
+	/* Lock to serialize access to count, hw_enabled and ops, used for
+	 * power wells that have supports_atomix_ctx set to True.
+	 */
+	spinlock_t lock;
+
+	/* Indicates that the get/put methods for this power well can be called
+	 * in atomic contexts, requires .ops to not sleep. This is valid
+	 * only for the DC_OFF power well currently.
+	 */
+	bool supports_atomic_ctx;
+
+	/* DC_OFF power well was disabled since the last time vblanks were
+	 * disabled.
+	 */
+	bool dc_off_disabled;
+
 	const struct i915_power_well_ops *ops;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 48676e99316e..6822118f3c4d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1798,6 +1798,9 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 					enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
+				    bool *needs_restore);
+void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
 
 static inline void
 assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 992caec1fbc4..fc6812ed6137 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -56,6 +56,19 @@ static struct i915_power_well *
 lookup_power_well(struct drm_i915_private *dev_priv,
 		  enum i915_power_well_id power_well_id);
 
+/* Optimize for the case when this is called from atomic contexts,
+ * although the case is unlikely.
+ */
+#define power_well_lock(power_well, flags) do {			\
+	if (likely(power_well->supports_atomic_ctx))		\
+		spin_lock_irqsave(&power_well->lock, flags);	\
+	} while (0)
+
+#define power_well_unlock(power_well, flags) do {			\
+	if (likely(power_well->supports_atomic_ctx))			\
+		spin_unlock_irqrestore(&power_well->lock, flags);	\
+	} while (0)
+
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain)
 {
@@ -126,6 +139,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 		return "AUX_D";
 	case POWER_DOMAIN_GMBUS:
 		return "GMBUS";
+	case POWER_DOMAIN_VBLANK:
+		return "VBLANK";
 	case POWER_DOMAIN_INIT:
 		return "INIT";
 	case POWER_DOMAIN_MODESET:
@@ -141,6 +156,9 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
 static void intel_power_well_enable(struct drm_i915_private *dev_priv,
 				    struct i915_power_well *power_well)
 {
+	if (power_well->supports_atomic_ctx)
+		assert_spin_locked(&power_well->lock);
+
 	DRM_DEBUG_KMS("enabling %s\n", power_well->name);
 	power_well->ops->enable(dev_priv, power_well);
 	power_well->hw_enabled = true;
@@ -149,26 +167,43 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
 static void intel_power_well_disable(struct drm_i915_private *dev_priv,
 				     struct i915_power_well *power_well)
 {
+	if (power_well->supports_atomic_ctx)
+		assert_spin_locked(&power_well->lock);
+
 	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
 	power_well->hw_enabled = false;
 	power_well->ops->disable(dev_priv, power_well);
 }
 
-static void intel_power_well_get(struct drm_i915_private *dev_priv,
+static void __intel_power_well_get(struct drm_i915_private *dev_priv,
 				 struct i915_power_well *power_well)
 {
 	if (!power_well->count++)
 		intel_power_well_enable(dev_priv, power_well);
 }
 
+static void intel_power_well_get(struct drm_i915_private *dev_priv,
+				 struct i915_power_well *power_well)
+{
+	unsigned long flags = 0;
+
+	power_well_lock(power_well, flags);
+	__intel_power_well_get(dev_priv, power_well);
+	power_well_unlock(power_well, flags);
+}
+
 static void intel_power_well_put(struct drm_i915_private *dev_priv,
 				 struct i915_power_well *power_well)
 {
+	unsigned long flags = 0;
+
+	power_well_lock(power_well, flags);
 	WARN(!power_well->count, "Use count on power well %s is already zero",
 	     power_well->name);
 
 	if (!--power_well->count)
 		intel_power_well_disable(dev_priv, power_well);
+	power_well_unlock(power_well, flags);
 }
 
 /**
@@ -726,6 +761,7 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
 		skl_enable_dc6(dev_priv);
 	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
 		gen9_enable_dc5(dev_priv);
+	power_well->dc_off_disabled = true;
 }
 
 static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
@@ -1443,6 +1479,58 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
+void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
+				    bool *needs_restore)
+{
+	struct i915_power_domains *power_domains  = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
+	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
+
+	*needs_restore = false;
+
+	if (!HAS_CSR(dev_priv))
+		return;
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	intel_runtime_pm_get_noresume(dev_priv);
+
+	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) {
+		unsigned long flags = 0;
+
+		power_well_lock(power_well, flags);
+		__intel_power_well_get(dev_priv, power_well);
+		*needs_restore = power_well->dc_off_disabled;
+		power_well->dc_off_disabled = false;
+		power_well_unlock(power_well, flags);
+	}
+
+	atomic_inc(&power_domains->domain_use_count[domain]);
+}
+
+void intel_display_power_vblank_put(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
+	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
+
+	if (!HAS_CSR(dev_priv))
+		return;
+
+	if (!CAN_PSR(dev_priv))
+		return;
+
+	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
+	     "Use count on domain %s was already zero\n",
+	     intel_display_power_domain_str(domain));
+
+	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
+		intel_power_well_put(dev_priv, power_well);
+
+	intel_runtime_pm_put(dev_priv);
+}
+
 static void
 __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 				 enum intel_display_power_domain domain)
@@ -1482,6 +1570,38 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 	mutex_unlock(&power_domains->lock);
 }
 
+static bool dc_off_get_if_enabled(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well;
+	bool is_enabled;
+	unsigned long flags = 0;
+
+	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
+	if (!power_well || !(power_well->domains & domain))
+		return true;
+
+	power_well_lock(power_well, flags);
+	is_enabled = power_well->hw_enabled;
+	if (is_enabled)
+		__intel_power_well_get(dev_priv, power_well);
+	power_well_unlock(power_well, flags);
+
+	return is_enabled;
+}
+
+static void dc_off_put(struct drm_i915_private *dev_priv,
+		       enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well;
+
+	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
+	if (!power_well || !(power_well->domains & domain))
+		return;
+
+	intel_power_well_put(dev_priv, power_well);
+}
+
 /**
  * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
  * @dev_priv: i915 device instance
@@ -1498,20 +1618,24 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
 					enum intel_display_power_domain domain)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	bool is_enabled;
+	bool is_enabled = false;
 
 	if (!intel_runtime_pm_get_if_in_use(dev_priv))
 		return false;
 
 	mutex_lock(&power_domains->lock);
 
+	if (!dc_off_get_if_enabled(dev_priv, domain))
+		goto out;
+
 	if (__intel_display_power_is_enabled(dev_priv, domain)) {
 		__intel_display_power_get_domain(dev_priv, domain);
 		is_enabled = true;
-	} else {
-		is_enabled = false;
 	}
 
+	dc_off_put(dev_priv, domain);
+
+out:
 	mutex_unlock(&power_domains->lock);
 
 	if (!is_enabled)
@@ -1709,6 +1833,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
 	BIT_ULL(POWER_DOMAIN_MODESET) |			\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))
 
 #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
@@ -1732,6 +1857,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
 	BIT_ULL(POWER_DOMAIN_MODESET) |			\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))
 #define BXT_DPIO_CMN_A_POWER_DOMAINS (			\
 	BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
@@ -1791,6 +1917,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
 	BIT_ULL(POWER_DOMAIN_MODESET) |			\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))
 
 #define CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
@@ -1838,6 +1965,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
 	BIT_ULL(POWER_DOMAIN_MODESET) |			\
 	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
+	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
 	BIT_ULL(POWER_DOMAIN_INIT))
 
 static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
@@ -2071,9 +2199,12 @@ bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
 {
 	struct i915_power_well *power_well;
 	bool ret;
+	unsigned long flags = 0;
 
 	power_well = lookup_power_well(dev_priv, power_well_id);
+	power_well_lock(power_well, flags);
 	ret = power_well->ops->is_enabled(dev_priv, power_well);
+	power_well_unlock(power_well, flags);
 
 	return ret;
 }
@@ -2108,6 +2239,8 @@ static struct i915_power_well skl_power_wells[] = {
 		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
 		.id = SKL_DISP_PW_DC_OFF,
+		.supports_atomic_ctx = true,
+		.dc_off_disabled = false,
 	},
 	{
 		.name = "power well 2",
@@ -2168,6 +2301,8 @@ static struct i915_power_well bxt_power_wells[] = {
 		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
 		.id = SKL_DISP_PW_DC_OFF,
+		.supports_atomic_ctx = true,
+		.dc_off_disabled = false,
 	},
 	{
 		.name = "power well 2",
@@ -2223,6 +2358,8 @@ static struct i915_power_well glk_power_wells[] = {
 		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
 		.id = SKL_DISP_PW_DC_OFF,
+		.supports_atomic_ctx = true,
+		.dc_off_disabled = false,
 	},
 	{
 		.name = "power well 2",
@@ -2347,6 +2484,8 @@ static struct i915_power_well cnl_power_wells[] = {
 		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
 		.id = SKL_DISP_PW_DC_OFF,
+		.supports_atomic_ctx = true,
+		.dc_off_disabled = false,
 	},
 	{
 		.name = "power well 2",
@@ -2475,6 +2614,7 @@ static void assert_power_well_ids_unique(struct drm_i915_private *dev_priv)
 int intel_power_domains_init(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
 	i915_modparams.disable_power_well =
 		sanitize_disable_power_well_option(dev_priv,
@@ -2512,6 +2652,10 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 		set_power_wells(power_domains, i9xx_always_on_power_well);
 	}
 
+	for_each_power_well(dev_priv, power_well)
+		if (power_well->supports_atomic_ctx)
+			spin_lock_init(&power_well->lock);
+
 	assert_power_well_ids_unique(dev_priv);
 
 	return 0;
@@ -2559,9 +2703,13 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&power_domains->lock);
 	for_each_power_well(dev_priv, power_well) {
+		unsigned long flags = 0;
+
+		power_well_lock(power_well, flags);
 		power_well->ops->sync_hw(dev_priv, power_well);
 		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
 								     power_well);
+		power_well_unlock(power_well, flags);
 	}
 	mutex_unlock(&power_domains->lock);
 }
@@ -3034,16 +3182,18 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
 		bxt_display_core_uninit(dev_priv);
 }
 
-static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
+static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv,
+					  const int *power_well_use)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
+	int i = 0;
 
 	for_each_power_well(dev_priv, power_well) {
 		enum intel_display_power_domain domain;
 
 		DRM_DEBUG_DRIVER("%-25s %d\n",
-				 power_well->name, power_well->count);
+				 power_well->name, power_well_use[i++]);
 
 		for_each_power_domain(domain, power_well->domains)
 			DRM_DEBUG_DRIVER("  %-23s %d\n",
@@ -3067,6 +3217,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
 	bool dump_domain_info;
+	int power_well_use[dev_priv->power_domains.power_well_count];
 
 	mutex_lock(&power_domains->lock);
 
@@ -3075,6 +3226,14 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 		enum intel_display_power_domain domain;
 		int domains_count;
 		bool enabled;
+		int well_count, i = 0;
+		unsigned long flags = 0;
+
+		power_well_lock(power_well, flags);
+		well_count = power_well->count;
+		enabled = power_well->ops->is_enabled(dev_priv, power_well);
+		power_well_unlock(power_well, flags);
+		power_well_use[i++] = well_count;
 
 		/*
 		 * Power wells not belonging to any domain (like the MISC_IO
@@ -3084,20 +3243,19 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 		if (!power_well->domains)
 			continue;
 
-		enabled = power_well->ops->is_enabled(dev_priv, power_well);
-		if ((power_well->count || power_well->always_on) != enabled)
+
+		if ((well_count || power_well->always_on) != enabled)
 			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
-				  power_well->name, power_well->count, enabled);
+				  power_well->name, well_count, enabled);
 
 		domains_count = 0;
 		for_each_power_domain(domain, power_well->domains)
 			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
 
-		if (power_well->count != domains_count) {
+		if (well_count != domains_count) {
 			DRM_ERROR("power well %s refcount/domain refcount mismatch "
 				  "(refcount %d/domains refcount %d)\n",
-				  power_well->name, power_well->count,
-				  domains_count);
+				  power_well->name, well_count, domains_count);
 			dump_domain_info = true;
 		}
 	}
@@ -3106,7 +3264,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 		static bool dumped;
 
 		if (!dumped) {
-			intel_power_domains_dump_info(dev_priv);
+			intel_power_domains_dump_info(dev_priv, power_well_use);
 			dumped = true;
 		}
 	}
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 8/8] drm/i915: Use the vblank power domain disallow or disable DC states.

[Dhinakaran Pandiyan]

Disable DC states before enabling vblank interrupts and conversely
enable DC states after disabling. Since the frame counter may have got
reset between disabling and enabling, use drm_crtc_vblank_restore() to
compute the missed vblanks.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3517c6548e2c..88b4ceac55d0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2963,6 +2963,11 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
+	bool needs_restore = false;
+
+	intel_display_power_vblank_get(dev_priv, &needs_restore);
+	if (needs_restore)
+		drm_crtc_vblank_restore(dev, pipe);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
@@ -3015,6 +3020,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	intel_display_power_vblank_put(dev_priv);
 }
 
 static void ibx_irq_reset(struct drm_i915_private *dev_priv)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/8] drm/i915/psr: Kill psr.source_ok flag.

[Rodrigo Vivi]

On Tue, Dec 19, 2017 at 05:26:52AM +0000, Dhinakaran Pandiyan wrote:
> This flag has become redundant since
> commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc state")
> It is set at the same place as psr.enabled, which is also exposed via
> debugfs.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

I just hope this doesn't break any igt debugfs parse, but the patch here seems
right way to go, so:

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

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 1 -
>  drivers/gpu/drm/i915/i915_drv.h     | 1 -
>  drivers/gpu/drm/i915/intel_psr.c    | 2 --
>  3 files changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0ddce72552bf..64e5a263458c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2540,7 +2540,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
> -	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
>  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1aba5657f5f0..1e4e613e7b41 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1199,7 +1199,6 @@ struct i915_drrs {
>  struct i915_psr {
>  	struct mutex lock;
>  	bool sink_support;
> -	bool source_ok;
>  	struct intel_dp *enabled;
>  	bool active;
>  	struct delayed_work work;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index a1ad85fa5c1a..c4d75e82a1df 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -522,8 +522,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	}
>  
>  	dev_priv->psr.psr2_support = crtc_state->has_psr2;
> -	dev_priv->psr.source_ok = true;
> -
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  
>  	dev_priv->psr.setup_vsc(intel_dp, crtc_state);
> -- 
> 2.11.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/8] drm/i915/psr: CAN_PSR() macro to check for PSR source and sink support.

[Rodrigo Vivi]

On Tue, Dec 19, 2017 at 05:26:53AM +0000, Dhinakaran Pandiyan wrote:
> The global variable dev_priv->psr.sink_support is set if an eDP sink
> supports PSR. Use this instead of redoing the check with is_edp_psr().
> Combine source and sink support checks into a macro that can be used to
> return early from psr_{invalidate, single_frame_update, flush}.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 19 ++++---------------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f89d64..48676e99316e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1760,6 +1760,7 @@ static inline void intel_backlight_device_unregister(struct intel_connector *con
>  
>  
>  /* intel_psr.c */
> +#define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)

not sure about the "CAN"... but I don't have better suggestion and I believe
it is somehow clear enough...

>  void intel_psr_enable(struct intel_dp *intel_dp,
>  		      const struct intel_crtc_state *crtc_state);
>  void intel_psr_disable(struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index c4d75e82a1df..76339cf387cb 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,14 +56,6 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> -static bool is_edp_psr(struct intel_dp *intel_dp)
> -{
> -	if (!intel_dp_is_edp(intel_dp))
> -		return false;
> -
> -	return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
> -}

I wonder why we haven't done this before in favor of sink_support///
my bad... 

> -
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -358,10 +350,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		&crtc_state->base.adjusted_mode;
>  	int psr_setup_time;
>  
> -	if (!HAS_PSR(dev_priv))
> -		return;
> -
> -	if (!is_edp_psr(intel_dp))
> +	if (!CAN_PSR(dev_priv))
>  		return;
>  
>  	if (!i915_modparams.enable_psr) {
> @@ -794,7 +783,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
>  	enum pipe pipe;
>  	u32 val;
>  
> -	if (!HAS_PSR(dev_priv))
> +	if (!CAN_PSR(dev_priv))
>  		return;
>  
>  	/*
> @@ -843,7 +832,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
>  
> -	if (!HAS_PSR(dev_priv))
> +	if (!CAN_PSR(dev_priv))
>  		return;
>  
>  	mutex_lock(&dev_priv->psr.lock);
> @@ -883,7 +872,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
>  
> -	if (!HAS_PSR(dev_priv))
> +	if (!CAN_PSR(dev_priv))
>  		return;

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

>  
>  	mutex_lock(&dev_priv->psr.lock);
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915/psr: Avoid initializing PSR if there is no sink support.

[Rodrigo Vivi]

On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote:
> DPCD read for the eDP is complete by the time intel_psr_init() is
> called, which means we can avoid initializing PSR structures and state
> if there is no sink support.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c    | 9 +++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 64e5a263458c..1a7b28f62570 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	u32 stat[3];
>  	enum pipe pipe;
>  	bool enabled = false;
> +	bool sink_support;
>  
>  	if (!HAS_PSR(dev_priv))
>  		return -ENODEV;
>  
> +	sink_support = dev_priv->psr.sink_support;
> +	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> +	if (!sink_support)
> +		return 0;
> +
>  	intel_runtime_pm_get(dev_priv);
>  
>  	mutex_lock(&dev_priv->psr.lock);
> -	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
>  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 76339cf387cb..095e0a5a8574 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	if (!crtc_state->has_psr)
>  		return;
>  
> +	if (WARN_ON(!CAN_PSR(dev_priv)))
> +		return;

hmm... I believe we will see this warning sooner than later...

has_psr is not the same as CAN_PSR.

also, btw I didn't like all this crtc_state has_psr x has_psr2. :/

probably this series could also unify that and clean it up.
to many has_psr like cases.

> +
>  	WARN_ON(dev_priv->drrs.dp);
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (dev_priv->psr.enabled) {
> @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  	if (!old_crtc_state->has_psr)
>  		return;
>  
> +	if (WARN_ON(!CAN_PSR(dev_priv)))
> +		return;
> +
>  	mutex_lock(&dev_priv->psr.lock);
>  	if (!dev_priv->psr.enabled) {
>  		mutex_unlock(&dev_priv->psr.lock);
> @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>  
> +	if (!dev_priv->psr.sink_support)
> +		return;
> +

Why not use CAN_PSR here?

>  	/* Per platform default: all disabled. */
>  	if (i915_modparams.enable_psr == -1)
>  		i915_modparams.enable_psr = 0;
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915/psr: Avoid initializing PSR if there is no sink support.

[Pandiyan, Dhinakaran]

On Tue, 2017-12-19 at 13:29 -0800, Rodrigo Vivi wrote:
> On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote:
> > DPCD read for the eDP is complete by the time intel_psr_init() is
> > called, which means we can avoid initializing PSR structures and state
> > if there is no sink support.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++-
> >  drivers/gpu/drm/i915/intel_psr.c    | 9 +++++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 64e5a263458c..1a7b28f62570 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  	u32 stat[3];
> >  	enum pipe pipe;
> >  	bool enabled = false;
> > +	bool sink_support;
> >  
> >  	if (!HAS_PSR(dev_priv))
> >  		return -ENODEV;
> >  
> > +	sink_support = dev_priv->psr.sink_support;
> > +	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> > +	if (!sink_support)
> > +		return 0;
> > +
> >  	intel_runtime_pm_get(dev_priv);
> >  
> >  	mutex_lock(&dev_priv->psr.lock);
> > -	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
> >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> >  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
> >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 76339cf387cb..095e0a5a8574 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  	if (!crtc_state->has_psr)
> >  		return;
> >  
> > +	if (WARN_ON(!CAN_PSR(dev_priv)))
> > +		return;
> 
> hmm... I believe we will see this warning sooner than later...
> 
> has_psr is not the same as CAN_PSR.

has_psr should not be set in psr_compute_config() unless both source and
sink have PSR. So, the warn_on is if we mess up the state preparation.
> 
> also, btw I didn't like all this crtc_state has_psr x has_psr2. :/
> 
> probably this series could also unify that and clean it up.
> to many has_psr like cases.
> 
> > +
> >  	WARN_ON(dev_priv->drrs.dp);
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	if (dev_priv->psr.enabled) {
> > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >  	if (!old_crtc_state->has_psr)
> >  		return;
> >  
> > +	if (WARN_ON(!CAN_PSR(dev_priv)))
> > +		return;
> > +
> >  	mutex_lock(&dev_priv->psr.lock);
> >  	if (!dev_priv->psr.enabled) {
> >  		mutex_unlock(&dev_priv->psr.lock);
> > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> >  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> >  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> >  
> > +	if (!dev_priv->psr.sink_support)
> > +		return;
> > +
> 
> Why not use CAN_PSR here?


So that we have the right MMIO's in case we want to probe the HW with no
sink support. I wasn't what would happen if we reg_read() on PSR
registers without the correct MMIO base.

> 
> >  	/* Per platform default: all disabled. */
> >  	if (i915_modparams.enable_psr == -1)
> >  		i915_modparams.enable_psr = 0;
> > -- 
> > 2.11.0
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/8] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Rodrigo Vivi]

On Tue, Dec 19, 2017 at 05:26:58AM +0000, Dhinakaran Pandiyan wrote:
> When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> states resulting in frame counter resets. The frame counter resets mess
> up the vblank counting logic. In order to disable DC states when
> vblank interrupts are required and to disallow DC states when vblanks
> interrupts are already enabled, introduce a new VBLANK power domain.
> Since this power domain reference needs to be acquired and released in
> atomic context, _vblank_get() and _vblank_put() methods skip the
> power_domain mutex.
> 
> v2: Fix deadlock by switching irqsave spinlock.
>     Implement atomic version of get_if_enabled.
>     Modify power_domain_verify_state to check power well use count and
> enabled status atomically.
>     Rewrite of intel_power_well_{get,put}
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

+ Cc: Imre Deak <imre.deak@intel.com>

> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  18 ++++
>  drivers/gpu/drm/i915/intel_drv.h        |   3 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 184 +++++++++++++++++++++++++++++---
>  3 files changed, 192 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ddadeb9eaf49..db597c5ebaed 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -397,6 +397,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_AUX_C,
>  	POWER_DOMAIN_AUX_D,
>  	POWER_DOMAIN_GMBUS,
> +	POWER_DOMAIN_VBLANK,
>  	POWER_DOMAIN_MODESET,
>  	POWER_DOMAIN_GT_IRQ,
>  	POWER_DOMAIN_INIT,
> @@ -1476,6 +1477,23 @@ struct i915_power_well {
>  			bool has_fuses:1;
>  		} hsw;
>  	};
> +
> +	/* Lock to serialize access to count, hw_enabled and ops, used for
> +	 * power wells that have supports_atomix_ctx set to True.
> +	 */
> +	spinlock_t lock;
> +
> +	/* Indicates that the get/put methods for this power well can be called
> +	 * in atomic contexts, requires .ops to not sleep. This is valid
> +	 * only for the DC_OFF power well currently.
> +	 */
> +	bool supports_atomic_ctx;
> +
> +	/* DC_OFF power well was disabled since the last time vblanks were
> +	 * disabled.
> +	 */
> +	bool dc_off_disabled;
> +
>  	const struct i915_power_well_ops *ops;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 48676e99316e..6822118f3c4d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1798,6 +1798,9 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>  					enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
> +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
> +				    bool *needs_restore);
> +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
>  
>  static inline void
>  assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 992caec1fbc4..fc6812ed6137 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -56,6 +56,19 @@ static struct i915_power_well *
>  lookup_power_well(struct drm_i915_private *dev_priv,
>  		  enum i915_power_well_id power_well_id);
>  
> +/* Optimize for the case when this is called from atomic contexts,
> + * although the case is unlikely.
> + */
> +#define power_well_lock(power_well, flags) do {			\
> +	if (likely(power_well->supports_atomic_ctx))		\

you mention unlikely on commend and call likely here, why?

> +		spin_lock_irqsave(&power_well->lock, flags);	\
> +	} while (0)
> +
> +#define power_well_unlock(power_well, flags) do {			\
> +	if (likely(power_well->supports_atomic_ctx))			\
> +		spin_unlock_irqrestore(&power_well->lock, flags);	\
> +	} while (0)
> +
>  const char *
>  intel_display_power_domain_str(enum intel_display_power_domain domain)
>  {
> @@ -126,6 +139,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "AUX_D";
>  	case POWER_DOMAIN_GMBUS:
>  		return "GMBUS";
> +	case POWER_DOMAIN_VBLANK:
> +		return "VBLANK";
>  	case POWER_DOMAIN_INIT:
>  		return "INIT";
>  	case POWER_DOMAIN_MODESET:
> @@ -141,6 +156,9 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>  				    struct i915_power_well *power_well)
>  {
> +	if (power_well->supports_atomic_ctx)

why not (un)likely check here as well?

> +		assert_spin_locked(&power_well->lock);
> +
>  	DRM_DEBUG_KMS("enabling %s\n", power_well->name);
>  	power_well->ops->enable(dev_priv, power_well);
>  	power_well->hw_enabled = true;
> @@ -149,26 +167,43 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>  static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>  				     struct i915_power_well *power_well)
>  {
> +	if (power_well->supports_atomic_ctx)
> +		assert_spin_locked(&power_well->lock);
> +
>  	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>  	power_well->hw_enabled = false;
>  	power_well->ops->disable(dev_priv, power_well);
>  }
>  
> -static void intel_power_well_get(struct drm_i915_private *dev_priv,
> +static void __intel_power_well_get(struct drm_i915_private *dev_priv,
>  				 struct i915_power_well *power_well)
>  {
>  	if (!power_well->count++)
>  		intel_power_well_enable(dev_priv, power_well);
>  }
>  
> +static void intel_power_well_get(struct drm_i915_private *dev_priv,
> +				 struct i915_power_well *power_well)
> +{
> +	unsigned long flags = 0;
> +
> +	power_well_lock(power_well, flags);
> +	__intel_power_well_get(dev_priv, power_well);
> +	power_well_unlock(power_well, flags);
> +}
> +
>  static void intel_power_well_put(struct drm_i915_private *dev_priv,
>  				 struct i915_power_well *power_well)
>  {
> +	unsigned long flags = 0;
> +
> +	power_well_lock(power_well, flags);
>  	WARN(!power_well->count, "Use count on power well %s is already zero",
>  	     power_well->name);
>  
>  	if (!--power_well->count)
>  		intel_power_well_disable(dev_priv, power_well);
> +	power_well_unlock(power_well, flags);
>  }
>  
>  /**
> @@ -726,6 +761,7 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>  		skl_enable_dc6(dev_priv);
>  	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
>  		gen9_enable_dc5(dev_priv);
> +	power_well->dc_off_disabled = true;

why do we need to track this like this?

Seems a big deviation of the hole generic power well state...

>  }
>  
>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> @@ -1443,6 +1479,58 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
>  	chv_set_pipe_power_well(dev_priv, power_well, false);
>  }
>  
> +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
> +				    bool *needs_restore)
> +{
> +	struct i915_power_domains *power_domains  = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
> +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
> +
> +	*needs_restore = false;
> +
> +	if (!HAS_CSR(dev_priv))
> +		return;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	intel_runtime_pm_get_noresume(dev_priv);
> +
> +	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) {
> +		unsigned long flags = 0;
> +
> +		power_well_lock(power_well, flags);
> +		__intel_power_well_get(dev_priv, power_well);
> +		*needs_restore = power_well->dc_off_disabled;
> +		power_well->dc_off_disabled = false;
> +		power_well_unlock(power_well, flags);
> +	}
> +
> +	atomic_inc(&power_domains->domain_use_count[domain]);
> +}
> +
> +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
> +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
> +
> +	if (!HAS_CSR(dev_priv))
> +		return;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return;
> +
> +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
> +	     "Use count on domain %s was already zero\n",
> +	     intel_display_power_domain_str(domain));
> +
> +	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
> +		intel_power_well_put(dev_priv, power_well);
> +
> +	intel_runtime_pm_put(dev_priv);
> +}
> +
>  static void
>  __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  				 enum intel_display_power_domain domain)
> @@ -1482,6 +1570,38 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>  	mutex_unlock(&power_domains->lock);
>  }
>  
> +static bool dc_off_get_if_enabled(struct drm_i915_private *dev_priv,
> +				  enum intel_display_power_domain domain)
> +{
> +	struct i915_power_well *power_well;
> +	bool is_enabled;
> +	unsigned long flags = 0;
> +
> +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
> +	if (!power_well || !(power_well->domains & domain))
> +		return true;
> +
> +	power_well_lock(power_well, flags);
> +	is_enabled = power_well->hw_enabled;
> +	if (is_enabled)
> +		__intel_power_well_get(dev_priv, power_well);
> +	power_well_unlock(power_well, flags);
> +
> +	return is_enabled;
> +}
> +
> +static void dc_off_put(struct drm_i915_private *dev_priv,
> +		       enum intel_display_power_domain domain)
> +{
> +	struct i915_power_well *power_well;
> +
> +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
> +	if (!power_well || !(power_well->domains & domain))
> +		return;
> +
> +	intel_power_well_put(dev_priv, power_well);
> +}
> +
>  /**
>   * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
>   * @dev_priv: i915 device instance
> @@ -1498,20 +1618,24 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>  					enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> -	bool is_enabled;
> +	bool is_enabled = false;
>  
>  	if (!intel_runtime_pm_get_if_in_use(dev_priv))
>  		return false;
>  
>  	mutex_lock(&power_domains->lock);
>  
> +	if (!dc_off_get_if_enabled(dev_priv, domain))
> +		goto out;
> +
>  	if (__intel_display_power_is_enabled(dev_priv, domain)) {
>  		__intel_display_power_get_domain(dev_priv, domain);
>  		is_enabled = true;
> -	} else {
> -		is_enabled = false;
>  	}
>  
> +	dc_off_put(dev_priv, domain);
> +
> +out:
>  	mutex_unlock(&power_domains->lock);
>  
>  	if (!is_enabled)
> @@ -1709,6 +1833,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
> @@ -1732,6 +1857,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  #define BXT_DPIO_CMN_A_POWER_DOMAINS (			\
>  	BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
> @@ -1791,6 +1917,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  #define CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
> @@ -1838,6 +1965,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
> @@ -2071,9 +2199,12 @@ bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>  {
>  	struct i915_power_well *power_well;
>  	bool ret;
> +	unsigned long flags = 0;
>  
>  	power_well = lookup_power_well(dev_priv, power_well_id);
> +	power_well_lock(power_well, flags);
>  	ret = power_well->ops->is_enabled(dev_priv, power_well);
> +	power_well_unlock(power_well, flags);
>  
>  	return ret;
>  }
> @@ -2108,6 +2239,8 @@ static struct i915_power_well skl_power_wells[] = {
>  		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
>  		.id = SKL_DISP_PW_DC_OFF,
> +		.supports_atomic_ctx = true,
> +		.dc_off_disabled = false,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -2168,6 +2301,8 @@ static struct i915_power_well bxt_power_wells[] = {
>  		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
>  		.id = SKL_DISP_PW_DC_OFF,
> +		.supports_atomic_ctx = true,
> +		.dc_off_disabled = false,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -2223,6 +2358,8 @@ static struct i915_power_well glk_power_wells[] = {
>  		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
>  		.id = SKL_DISP_PW_DC_OFF,
> +		.supports_atomic_ctx = true,
> +		.dc_off_disabled = false,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -2347,6 +2484,8 @@ static struct i915_power_well cnl_power_wells[] = {
>  		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
>  		.id = SKL_DISP_PW_DC_OFF,
> +		.supports_atomic_ctx = true,
> +		.dc_off_disabled = false,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -2475,6 +2614,7 @@ static void assert_power_well_ids_unique(struct drm_i915_private *dev_priv)
>  int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
>  
>  	i915_modparams.disable_power_well =
>  		sanitize_disable_power_well_option(dev_priv,
> @@ -2512,6 +2652,10 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  		set_power_wells(power_domains, i9xx_always_on_power_well);
>  	}
>  
> +	for_each_power_well(dev_priv, power_well)
> +		if (power_well->supports_atomic_ctx)
> +			spin_lock_init(&power_well->lock);
> +
>  	assert_power_well_ids_unique(dev_priv);
>  
>  	return 0;
> @@ -2559,9 +2703,13 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
>  
>  	mutex_lock(&power_domains->lock);
>  	for_each_power_well(dev_priv, power_well) {
> +		unsigned long flags = 0;
> +
> +		power_well_lock(power_well, flags);
>  		power_well->ops->sync_hw(dev_priv, power_well);
>  		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
>  								     power_well);
> +		power_well_unlock(power_well, flags);
>  	}
>  	mutex_unlock(&power_domains->lock);
>  }
> @@ -3034,16 +3182,18 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
>  		bxt_display_core_uninit(dev_priv);
>  }
>  
> -static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
> +static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv,
> +					  const int *power_well_use)
>  {
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  	struct i915_power_well *power_well;
> +	int i = 0;
>  
>  	for_each_power_well(dev_priv, power_well) {
>  		enum intel_display_power_domain domain;
>  
>  		DRM_DEBUG_DRIVER("%-25s %d\n",
> -				 power_well->name, power_well->count);
> +				 power_well->name, power_well_use[i++]);
>  
>  		for_each_power_domain(domain, power_well->domains)
>  			DRM_DEBUG_DRIVER("  %-23s %d\n",
> @@ -3067,6 +3217,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  	struct i915_power_well *power_well;
>  	bool dump_domain_info;
> +	int power_well_use[dev_priv->power_domains.power_well_count];
>  
>  	mutex_lock(&power_domains->lock);
>  
> @@ -3075,6 +3226,14 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  		enum intel_display_power_domain domain;
>  		int domains_count;
>  		bool enabled;
> +		int well_count, i = 0;
> +		unsigned long flags = 0;
> +
> +		power_well_lock(power_well, flags);
> +		well_count = power_well->count;
> +		enabled = power_well->ops->is_enabled(dev_priv, power_well);
> +		power_well_unlock(power_well, flags);
> +		power_well_use[i++] = well_count;
>  
>  		/*
>  		 * Power wells not belonging to any domain (like the MISC_IO
> @@ -3084,20 +3243,19 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  		if (!power_well->domains)
>  			continue;
>  
> -		enabled = power_well->ops->is_enabled(dev_priv, power_well);
> -		if ((power_well->count || power_well->always_on) != enabled)
> +
> +		if ((well_count || power_well->always_on) != enabled)
>  			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
> -				  power_well->name, power_well->count, enabled);
> +				  power_well->name, well_count, enabled);
>  
>  		domains_count = 0;
>  		for_each_power_domain(domain, power_well->domains)
>  			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
>  
> -		if (power_well->count != domains_count) {
> +		if (well_count != domains_count) {
>  			DRM_ERROR("power well %s refcount/domain refcount mismatch "
>  				  "(refcount %d/domains refcount %d)\n",
> -				  power_well->name, power_well->count,
> -				  domains_count);
> +				  power_well->name, well_count, domains_count);
>  			dump_domain_info = true;
>  		}
>  	}
> @@ -3106,7 +3264,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  		static bool dumped;
>  
>  		if (!dumped) {
> -			intel_power_domains_dump_info(dev_priv);
> +			intel_power_domains_dump_info(dev_priv, power_well_use);
>  			dumped = true;
>  		}
>  	}
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 7/8] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Pandiyan, Dhinakaran]

On Tue, 2017-12-19 at 13:41 -0800, Rodrigo Vivi wrote:
> On Tue, Dec 19, 2017 at 05:26:58AM +0000, Dhinakaran Pandiyan wrote:
> > When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> > states resulting in frame counter resets. The frame counter resets mess
> > up the vblank counting logic. In order to disable DC states when
> > vblank interrupts are required and to disallow DC states when vblanks
> > interrupts are already enabled, introduce a new VBLANK power domain.
> > Since this power domain reference needs to be acquired and released in
> > atomic context, _vblank_get() and _vblank_put() methods skip the
> > power_domain mutex.
> > 
> > v2: Fix deadlock by switching irqsave spinlock.
> >     Implement atomic version of get_if_enabled.
> >     Modify power_domain_verify_state to check power well use count and
> > enabled status atomically.
> >     Rewrite of intel_power_well_{get,put}
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> + Cc: Imre Deak <imre.deak@intel.com>
> 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  18 ++++
> >  drivers/gpu/drm/i915/intel_drv.h        |   3 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 184 +++++++++++++++++++++++++++++---
> >  3 files changed, 192 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ddadeb9eaf49..db597c5ebaed 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -397,6 +397,7 @@ enum intel_display_power_domain {
> >  	POWER_DOMAIN_AUX_C,
> >  	POWER_DOMAIN_AUX_D,
> >  	POWER_DOMAIN_GMBUS,
> > +	POWER_DOMAIN_VBLANK,
> >  	POWER_DOMAIN_MODESET,
> >  	POWER_DOMAIN_GT_IRQ,
> >  	POWER_DOMAIN_INIT,
> > @@ -1476,6 +1477,23 @@ struct i915_power_well {
> >  			bool has_fuses:1;
> >  		} hsw;
> >  	};
> > +
> > +	/* Lock to serialize access to count, hw_enabled and ops, used for
> > +	 * power wells that have supports_atomix_ctx set to True.
> > +	 */
> > +	spinlock_t lock;
> > +
> > +	/* Indicates that the get/put methods for this power well can be called
> > +	 * in atomic contexts, requires .ops to not sleep. This is valid
> > +	 * only for the DC_OFF power well currently.
> > +	 */
> > +	bool supports_atomic_ctx;
> > +
> > +	/* DC_OFF power well was disabled since the last time vblanks were
> > +	 * disabled.
> > +	 */
> > +	bool dc_off_disabled;
> > +
> >  	const struct i915_power_well_ops *ops;
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 48676e99316e..6822118f3c4d 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1798,6 +1798,9 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> >  					enum intel_display_power_domain domain);
> >  void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  			     enum intel_display_power_domain domain);
> > +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
> > +				    bool *needs_restore);
> > +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
> >  
> >  static inline void
> >  assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 992caec1fbc4..fc6812ed6137 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -56,6 +56,19 @@ static struct i915_power_well *
> >  lookup_power_well(struct drm_i915_private *dev_priv,
> >  		  enum i915_power_well_id power_well_id);
> >  
> > +/* Optimize for the case when this is called from atomic contexts,
> > + * although the case is unlikely.
> > + */
> > +#define power_well_lock(power_well, flags) do {			\
> > +	if (likely(power_well->supports_atomic_ctx))		\
> 
> you mention unlikely on commend and call likely here, why?

It is unlikely that the power well is going to be DC_OFF, but we need to
optimize for that case as it can be called from atomic contexts.

> 
> > +		spin_lock_irqsave(&power_well->lock, flags);	\
> > +	} while (0)
> > +
> > +#define power_well_unlock(power_well, flags) do {			\
> > +	if (likely(power_well->supports_atomic_ctx))			\
> > +		spin_unlock_irqrestore(&power_well->lock, flags);	\
> > +	} while (0)
> > +
> >  const char *
> >  intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  {
> > @@ -126,6 +139,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  		return "AUX_D";
> >  	case POWER_DOMAIN_GMBUS:
> >  		return "GMBUS";
> > +	case POWER_DOMAIN_VBLANK:
> > +		return "VBLANK";
> >  	case POWER_DOMAIN_INIT:
> >  		return "INIT";
> >  	case POWER_DOMAIN_MODESET:
> > @@ -141,6 +156,9 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
> >  static void intel_power_well_enable(struct drm_i915_private *dev_priv,
> >  				    struct i915_power_well *power_well)
> >  {
> > +	if (power_well->supports_atomic_ctx)
> 
> why not (un)likely check here as well?
> 
> > +		assert_spin_locked(&power_well->lock);
> > +
> >  	DRM_DEBUG_KMS("enabling %s\n", power_well->name);
> >  	power_well->ops->enable(dev_priv, power_well);
> >  	power_well->hw_enabled = true;
> > @@ -149,26 +167,43 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
> >  static void intel_power_well_disable(struct drm_i915_private *dev_priv,
> >  				     struct i915_power_well *power_well)
> >  {
> > +	if (power_well->supports_atomic_ctx)
> > +		assert_spin_locked(&power_well->lock);
> > +
> >  	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> >  	power_well->hw_enabled = false;
> >  	power_well->ops->disable(dev_priv, power_well);
> >  }
> >  
> > -static void intel_power_well_get(struct drm_i915_private *dev_priv,
> > +static void __intel_power_well_get(struct drm_i915_private *dev_priv,
> >  				 struct i915_power_well *power_well)
> >  {
> >  	if (!power_well->count++)
> >  		intel_power_well_enable(dev_priv, power_well);
> >  }
> >  
> > +static void intel_power_well_get(struct drm_i915_private *dev_priv,
> > +				 struct i915_power_well *power_well)
> > +{
> > +	unsigned long flags = 0;
> > +
> > +	power_well_lock(power_well, flags);
> > +	__intel_power_well_get(dev_priv, power_well);
> > +	power_well_unlock(power_well, flags);
> > +}
> > +
> >  static void intel_power_well_put(struct drm_i915_private *dev_priv,
> >  				 struct i915_power_well *power_well)
> >  {
> > +	unsigned long flags = 0;
> > +
> > +	power_well_lock(power_well, flags);
> >  	WARN(!power_well->count, "Use count on power well %s is already zero",
> >  	     power_well->name);
> >  
> >  	if (!--power_well->count)
> >  		intel_power_well_disable(dev_priv, power_well);
> > +	power_well_unlock(power_well, flags);
> >  }
> >  
> >  /**
> > @@ -726,6 +761,7 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> >  		skl_enable_dc6(dev_priv);
> >  	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> >  		gen9_enable_dc5(dev_priv);
> > +	power_well->dc_off_disabled = true;
> 
> why do we need to track this like this?
> 
> Seems a big deviation of the hole generic power well state...


If the power well was turned off much later than disable_vblank, the
following enable_vblank call has no idea whether it has to compute the
missed vblanks. 


1) put vblank ref -> put AUX A ref -> get vblank ref
dc_off is turned off after we put AUX_A ref, needs computing missed
vblanks.


2) get AUX A ref -> put vblank ref -> get_vblank ref
dc_off is never turned off, no need to compute missed vblanks.



> 
> >  }
> >  
> >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > @@ -1443,6 +1479,58 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> >  	chv_set_pipe_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
> > +				    bool *needs_restore)
> > +{
> > +	struct i915_power_domains *power_domains  = &dev_priv->power_domains;
> > +	struct i915_power_well *power_well;
> > +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
> > +
> > +	*needs_restore = false;
> > +
> > +	if (!HAS_CSR(dev_priv))
> > +		return;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	intel_runtime_pm_get_noresume(dev_priv);
> > +
> > +	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) {
> > +		unsigned long flags = 0;
> > +
> > +		power_well_lock(power_well, flags);
> > +		__intel_power_well_get(dev_priv, power_well);
> > +		*needs_restore = power_well->dc_off_disabled;
> > +		power_well->dc_off_disabled = false;
> > +		power_well_unlock(power_well, flags);
> > +	}
> > +
> > +	atomic_inc(&power_domains->domain_use_count[domain]);
> > +}
> > +
> > +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv)
> > +{
> > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +	struct i915_power_well *power_well;
> > +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
> > +
> > +	if (!HAS_CSR(dev_priv))
> > +		return;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
> > +	     "Use count on domain %s was already zero\n",
> > +	     intel_display_power_domain_str(domain));
> > +
> > +	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
> > +		intel_power_well_put(dev_priv, power_well);
> > +
> > +	intel_runtime_pm_put(dev_priv);
> > +}
> > +
> >  static void
> >  __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> >  				 enum intel_display_power_domain domain)
> > @@ -1482,6 +1570,38 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
> >  	mutex_unlock(&power_domains->lock);
> >  }
> >  
> > +static bool dc_off_get_if_enabled(struct drm_i915_private *dev_priv,
> > +				  enum intel_display_power_domain domain)
> > +{
> > +	struct i915_power_well *power_well;
> > +	bool is_enabled;
> > +	unsigned long flags = 0;
> > +
> > +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
> > +	if (!power_well || !(power_well->domains & domain))
> > +		return true;
> > +
> > +	power_well_lock(power_well, flags);
> > +	is_enabled = power_well->hw_enabled;
> > +	if (is_enabled)
> > +		__intel_power_well_get(dev_priv, power_well);
> > +	power_well_unlock(power_well, flags);
> > +
> > +	return is_enabled;
> > +}
> > +
> > +static void dc_off_put(struct drm_i915_private *dev_priv,
> > +		       enum intel_display_power_domain domain)
> > +{
> > +	struct i915_power_well *power_well;
> > +
> > +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
> > +	if (!power_well || !(power_well->domains & domain))
> > +		return;
> > +
> > +	intel_power_well_put(dev_priv, power_well);
> > +}
> > +
> >  /**
> >   * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
> >   * @dev_priv: i915 device instance
> > @@ -1498,20 +1618,24 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> >  					enum intel_display_power_domain domain)
> >  {
> >  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > -	bool is_enabled;
> > +	bool is_enabled = false;
> >  
> >  	if (!intel_runtime_pm_get_if_in_use(dev_priv))
> >  		return false;
> >  
> >  	mutex_lock(&power_domains->lock);
> >  
> > +	if (!dc_off_get_if_enabled(dev_priv, domain))
> > +		goto out;
> > +
> >  	if (__intel_display_power_is_enabled(dev_priv, domain)) {
> >  		__intel_display_power_get_domain(dev_priv, domain);
> >  		is_enabled = true;
> > -	} else {
> > -		is_enabled = false;
> >  	}
> >  
> > +	dc_off_put(dev_priv, domain);
> > +
> > +out:
> >  	mutex_unlock(&power_domains->lock);
> >  
> >  	if (!is_enabled)
> > @@ -1709,6 +1833,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  
> >  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
> > @@ -1732,6 +1857,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  #define BXT_DPIO_CMN_A_POWER_DOMAINS (			\
> >  	BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
> > @@ -1791,6 +1917,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  
> >  #define CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
> > @@ -1838,6 +1965,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> >  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> >  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> > +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
> >  	BIT_ULL(POWER_DOMAIN_INIT))
> >  
> >  static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
> > @@ -2071,9 +2199,12 @@ bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> >  {
> >  	struct i915_power_well *power_well;
> >  	bool ret;
> > +	unsigned long flags = 0;
> >  
> >  	power_well = lookup_power_well(dev_priv, power_well_id);
> > +	power_well_lock(power_well, flags);
> >  	ret = power_well->ops->is_enabled(dev_priv, power_well);
> > +	power_well_unlock(power_well, flags);
> >  
> >  	return ret;
> >  }
> > @@ -2108,6 +2239,8 @@ static struct i915_power_well skl_power_wells[] = {
> >  		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> >  		.id = SKL_DISP_PW_DC_OFF,
> > +		.supports_atomic_ctx = true,
> > +		.dc_off_disabled = false,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -2168,6 +2301,8 @@ static struct i915_power_well bxt_power_wells[] = {
> >  		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> >  		.id = SKL_DISP_PW_DC_OFF,
> > +		.supports_atomic_ctx = true,
> > +		.dc_off_disabled = false,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -2223,6 +2358,8 @@ static struct i915_power_well glk_power_wells[] = {
> >  		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> >  		.id = SKL_DISP_PW_DC_OFF,
> > +		.supports_atomic_ctx = true,
> > +		.dc_off_disabled = false,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -2347,6 +2484,8 @@ static struct i915_power_well cnl_power_wells[] = {
> >  		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> >  		.id = SKL_DISP_PW_DC_OFF,
> > +		.supports_atomic_ctx = true,
> > +		.dc_off_disabled = false,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -2475,6 +2614,7 @@ static void assert_power_well_ids_unique(struct drm_i915_private *dev_priv)
> >  int intel_power_domains_init(struct drm_i915_private *dev_priv)
> >  {
> >  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +	struct i915_power_well *power_well;
> >  
> >  	i915_modparams.disable_power_well =
> >  		sanitize_disable_power_well_option(dev_priv,
> > @@ -2512,6 +2652,10 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> >  		set_power_wells(power_domains, i9xx_always_on_power_well);
> >  	}
> >  
> > +	for_each_power_well(dev_priv, power_well)
> > +		if (power_well->supports_atomic_ctx)
> > +			spin_lock_init(&power_well->lock);
> > +
> >  	assert_power_well_ids_unique(dev_priv);
> >  
> >  	return 0;
> > @@ -2559,9 +2703,13 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
> >  
> >  	mutex_lock(&power_domains->lock);
> >  	for_each_power_well(dev_priv, power_well) {
> > +		unsigned long flags = 0;
> > +
> > +		power_well_lock(power_well, flags);
> >  		power_well->ops->sync_hw(dev_priv, power_well);
> >  		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
> >  								     power_well);
> > +		power_well_unlock(power_well, flags);
> >  	}
> >  	mutex_unlock(&power_domains->lock);
> >  }
> > @@ -3034,16 +3182,18 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
> >  		bxt_display_core_uninit(dev_priv);
> >  }
> >  
> > -static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
> > +static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv,
> > +					  const int *power_well_use)
> >  {
> >  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >  	struct i915_power_well *power_well;
> > +	int i = 0;
> >  
> >  	for_each_power_well(dev_priv, power_well) {
> >  		enum intel_display_power_domain domain;
> >  
> >  		DRM_DEBUG_DRIVER("%-25s %d\n",
> > -				 power_well->name, power_well->count);
> > +				 power_well->name, power_well_use[i++]);
> >  
> >  		for_each_power_domain(domain, power_well->domains)
> >  			DRM_DEBUG_DRIVER("  %-23s %d\n",
> > @@ -3067,6 +3217,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >  	struct i915_power_well *power_well;
> >  	bool dump_domain_info;
> > +	int power_well_use[dev_priv->power_domains.power_well_count];
> >  
> >  	mutex_lock(&power_domains->lock);
> >  
> > @@ -3075,6 +3226,14 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  		enum intel_display_power_domain domain;
> >  		int domains_count;
> >  		bool enabled;
> > +		int well_count, i = 0;
> > +		unsigned long flags = 0;
> > +
> > +		power_well_lock(power_well, flags);
> > +		well_count = power_well->count;
> > +		enabled = power_well->ops->is_enabled(dev_priv, power_well);
> > +		power_well_unlock(power_well, flags);
> > +		power_well_use[i++] = well_count;
> >  
> >  		/*
> >  		 * Power wells not belonging to any domain (like the MISC_IO
> > @@ -3084,20 +3243,19 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  		if (!power_well->domains)
> >  			continue;
> >  
> > -		enabled = power_well->ops->is_enabled(dev_priv, power_well);
> > -		if ((power_well->count || power_well->always_on) != enabled)
> > +
> > +		if ((well_count || power_well->always_on) != enabled)
> >  			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
> > -				  power_well->name, power_well->count, enabled);
> > +				  power_well->name, well_count, enabled);
> >  
> >  		domains_count = 0;
> >  		for_each_power_domain(domain, power_well->domains)
> >  			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
> >  
> > -		if (power_well->count != domains_count) {
> > +		if (well_count != domains_count) {
> >  			DRM_ERROR("power well %s refcount/domain refcount mismatch "
> >  				  "(refcount %d/domains refcount %d)\n",
> > -				  power_well->name, power_well->count,
> > -				  domains_count);
> > +				  power_well->name, well_count, domains_count);
> >  			dump_domain_info = true;
> >  		}
> >  	}
> > @@ -3106,7 +3264,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  		static bool dumped;
> >  
> >  		if (!dumped) {
> > -			intel_power_domains_dump_info(dev_priv);
> > +			intel_power_domains_dump_info(dev_priv, power_well_use);
> >  			dumped = true;
> >  		}
> >  	}
> > -- 
> > 2.11.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/8] drm/i915: Use an atomic_t array to track power domain use count.

[Maarten Lankhorst]

Hey,

Op 19-12-17 om 06:26 schreef Dhinakaran Pandiyan:
> Convert the power_domains->domain_use_count array that tracks per-domain
> use count to atomic_t type. This is needed to be able to read/write the use
> counts outside of the power domain mutex.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++------
>  3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1a7b28f62570..1f1d9162f2c2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2764,7 +2764,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
>  		for_each_power_domain(power_domain, power_well->domains)
>  			seq_printf(m, "  %-23s %d\n",
>  				 intel_display_power_domain_str(power_domain),
> -				 power_domains->domain_use_count[power_domain]);
> +				 atomic_read(&power_domains->domain_use_count[power_domain]));
>  	}
>  
>  	mutex_unlock(&power_domains->lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e4e613e7b41..ddadeb9eaf49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1489,7 +1489,7 @@ struct i915_power_domains {
>  	int power_well_count;
>  
>  	struct mutex lock;
> -	int domain_use_count[POWER_DOMAIN_NUM];
> +	atomic_t domain_use_count[POWER_DOMAIN_NUM];
>  	struct i915_power_well *power_wells;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 96ab74f3d101..992caec1fbc4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1453,7 +1453,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
>  		intel_power_well_get(dev_priv, power_well);
>  
> -	power_domains->domain_use_count[domain]++;
> +	atomic_inc(&power_domains->domain_use_count[domain]);
>  }
>  
>  /**
> @@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -	WARN(!power_domains->domain_use_count[domain],
> -	     "Use count on domain %s is already zero\n",
> +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
> +	     "Use count on domain %s was already zero\n",
>  	     intel_display_power_domain_str(domain));
> -	power_domains->domain_use_count[domain]--;
>  
>  	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
>  		intel_power_well_put(dev_priv, power_well);
> @@ -3049,7 +3048,7 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
>  		for_each_power_domain(domain, power_well->domains)
>  			DRM_DEBUG_DRIVER("  %-23s %d\n",
>  					 intel_display_power_domain_str(domain),
> -					 power_domains->domain_use_count[domain]);
> +					 atomic_read(&power_domains->domain_use_count[domain]));
>  	}
>  }
>  
> @@ -3092,7 +3091,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>  
>  		domains_count = 0;
>  		for_each_power_domain(domain, power_well->domains)
> -			domains_count += power_domains->domain_use_count[domain];
> +			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
>  
>  		if (power_well->count != domains_count) {
>  			DRM_ERROR("power well %s refcount/domain refcount mismatch "

I can imagine this will start failing really badly. The previous code assumed that
everything is protected by power_domains->lock, and now this changes makes it no
longer the case..

I see the rest of the code changes things even more, but it would be better if the
locking rework was done in a single patch, and not bolted on..

And instead of using atomic_t, there is a refcount implementation in refcount.h,
it could be used here for locking power wells only if it would drop to zero..

Cheers,
Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/8] drm/i915: Use an atomic_t array to track power domain use count.

[Pandiyan, Dhinakaran]

On Thu, 2017-12-21 at 13:37 +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 19-12-17 om 06:26 schreef Dhinakaran Pandiyan:
> > Convert the power_domains->domain_use_count array that tracks per-domain
> > use count to atomic_t type. This is needed to be able to read/write the use
> > counts outside of the power domain mutex.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c     |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++------
> >  3 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 1a7b28f62570..1f1d9162f2c2 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2764,7 +2764,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
> >  		for_each_power_domain(power_domain, power_well->domains)
> >  			seq_printf(m, "  %-23s %d\n",
> >  				 intel_display_power_domain_str(power_domain),
> > -				 power_domains->domain_use_count[power_domain]);
> > +				 atomic_read(&power_domains->domain_use_count[power_domain]));
> >  	}
> >  
> >  	mutex_unlock(&power_domains->lock);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1e4e613e7b41..ddadeb9eaf49 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1489,7 +1489,7 @@ struct i915_power_domains {
> >  	int power_well_count;
> >  
> >  	struct mutex lock;
> > -	int domain_use_count[POWER_DOMAIN_NUM];
> > +	atomic_t domain_use_count[POWER_DOMAIN_NUM];
> >  	struct i915_power_well *power_wells;
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 96ab74f3d101..992caec1fbc4 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1453,7 +1453,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> >  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
> >  		intel_power_well_get(dev_priv, power_well);
> >  
> > -	power_domains->domain_use_count[domain]++;
> > +	atomic_inc(&power_domains->domain_use_count[domain]);
> >  }
> >  
> >  /**
> > @@ -1539,10 +1539,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  
> >  	mutex_lock(&power_domains->lock);
> >  
> > -	WARN(!power_domains->domain_use_count[domain],
> > -	     "Use count on domain %s is already zero\n",
> > +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
> > +	     "Use count on domain %s was already zero\n",
> >  	     intel_display_power_domain_str(domain));
> > -	power_domains->domain_use_count[domain]--;
> >  
> >  	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
> >  		intel_power_well_put(dev_priv, power_well);
> > @@ -3049,7 +3048,7 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
> >  		for_each_power_domain(domain, power_well->domains)
> >  			DRM_DEBUG_DRIVER("  %-23s %d\n",
> >  					 intel_display_power_domain_str(domain),
> > -					 power_domains->domain_use_count[domain]);
> > +					 atomic_read(&power_domains->domain_use_count[domain]));
> >  	}
> >  }
> >  
> > @@ -3092,7 +3091,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
> >  
> >  		domains_count = 0;
> >  		for_each_power_domain(domain, power_well->domains)
> > -			domains_count += power_domains->domain_use_count[domain];
> > +			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
> >  
> >  		if (power_well->count != domains_count) {
> >  			DRM_ERROR("power well %s refcount/domain refcount mismatch "
> 
> I can imagine this will start failing really badly. The previous code assumed that
> everything is protected by power_domains->lock, and now this changes makes it no
> longer the case..
> 

This won't fail until the next patch where it is read outside of the
mutex. And that patch reads these values within the new spin_lock. I was
trying to split the changes so that the next patch does not become too
heavy.

> I see the rest of the code changes things even more, but it would be better if the
> locking rework was done in a single patch, and not bolted on..
> 
I see your point, I can squash them together.

> And instead of using atomic_t, there is a refcount implementation in refcount.h,
> it could be used here for locking power wells only if it would drop to zero..
> 

So, the power_wells have another refcount (controls the power well
enable and disable), which needs the lock. Not very clear why we need to
lock the power wells if the domain_use_count goes to zero. The
domain_use_count array that I am converting over to atomic_t is used for
debug and verifying that the power well users are accounted for. It does
not control any hardware state. And the reason I am converting it to
atomic_t is to update it outside the spin locks. Let me know if my
understand is wrong.



> Cheers,
> Maarten
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915/psr: Avoid initializing PSR if there is no sink support.

[Rodrigo Vivi]

first of all sorry for not getting back sooner on this...

On Tue, Dec 19, 2017 at 09:40:01PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Tue, 2017-12-19 at 13:29 -0800, Rodrigo Vivi wrote:
> > On Tue, Dec 19, 2017 at 05:26:54AM +0000, Dhinakaran Pandiyan wrote:
> > > DPCD read for the eDP is complete by the time intel_psr_init() is
> > > called, which means we can avoid initializing PSR structures and state
> > > if there is no sink support.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 7 ++++++-
> > >  drivers/gpu/drm/i915/intel_psr.c    | 9 +++++++++
> > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 64e5a263458c..1a7b28f62570 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2532,14 +2532,19 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  	u32 stat[3];
> > >  	enum pipe pipe;
> > >  	bool enabled = false;
> > > +	bool sink_support;
> > >  
> > >  	if (!HAS_PSR(dev_priv))
> > >  		return -ENODEV;
> > >  
> > > +	sink_support = dev_priv->psr.sink_support;
> > > +	seq_printf(m, "Sink_Support: %s\n", yesno(sink_support));
> > > +	if (!sink_support)
> > > +		return 0;
> > > +
> > >  	intel_runtime_pm_get(dev_priv);
> > >  
> > >  	mutex_lock(&dev_priv->psr.lock);
> > > -	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
> > >  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled));
> > >  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
> > >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 76339cf387cb..095e0a5a8574 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -503,6 +503,9 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > >  	if (!crtc_state->has_psr)
> > >  		return;
> > >  
> > > +	if (WARN_ON(!CAN_PSR(dev_priv)))
> > > +		return;
> > 
> > hmm... I believe we will see this warning sooner than later...
> > 
> > has_psr is not the same as CAN_PSR.
> 
> has_psr should not be set in psr_compute_config() unless both source and
> sink have PSR. So, the warn_on is if we mess up the state preparation.
> > 
> > also, btw I didn't like all this crtc_state has_psr x has_psr2. :/
> > 
> > probably this series could also unify that and clean it up.
> > to many has_psr like cases.
> > 
> > > +
> > >  	WARN_ON(dev_priv->drrs.dp);
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  	if (dev_priv->psr.enabled) {
> > > @@ -633,6 +636,9 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> > >  	if (!old_crtc_state->has_psr)
> > >  		return;
> > >  
> > > +	if (WARN_ON(!CAN_PSR(dev_priv)))
> > > +		return;
> > > +
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  	if (!dev_priv->psr.enabled) {
> > >  		mutex_unlock(&dev_priv->psr.lock);
> > > @@ -913,6 +919,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > >  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > >  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > >  
> > > +	if (!dev_priv->psr.sink_support)
> > > +		return;
> > > +
> > 
> > Why not use CAN_PSR here?
> 
> 
> So that we have the right MMIO's in case we want to probe the HW with no
> sink support. I wasn't what would happen if we reg_read() on PSR
> registers without the correct MMIO base.

But isn't the goal of CAN_PSR to avoid any extra calls on platforms
that either doesn't have support nor the panel? In this case we should
never being touching any register.

> 
> > 
> > >  	/* Per platform default: all disabled. */
> > >  	if (i915_modparams.enable_psr == -1)
> > >  		i915_modparams.enable_psr = 0;
> > > -- 
> > > 2.11.0
> > > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel