[PATCH 0/6] preparation for multiple power-wells

[PATCH 0/6] preparation for multiple power-wells

[Imre Deak]

This is a prepration for adding support for multiple power-wells needed
by future HW platforms. I pushed the rest of the enabling patches to [1].
I'd like to post the generic parts of those once we agreed how to do the
power-well abstraction.

Except a spinlock->mutex change these patches shouldn't cause any
functional change. I tested it on VLV/HSW VGA by doing a manual DPMS
on/off and checking that the power-well indeed toggles. Also on HSW I
checked that the audio power well release/request still works by doing a
rmmod/insmod snd_hda_intel.

[1] https://github.com/ideak/linux/commits/powerwells

Imre Deak (6):
  drm/i915: make the intel_display_power_domain enum compact
  drm/i915: factor out is_always_on_domain
  drm/i915: change power_well->lock to be mutex
  drm/i915: factor out modeset_update_power_wells
  drm/i915: enable only the needed power domains during modeset
  drm/i915: use power get/put instead of set for power on after init

 drivers/gpu/drm/i915/i915_dma.c      |   2 +-
 drivers/gpu/drm/i915/i915_drv.c      |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  23 ++++--
 drivers/gpu/drm/i915/intel_display.c |  69 ++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 133 ++++++++++-------------------------
 6 files changed, 124 insertions(+), 107 deletions(-)

-- 
1.8.4

[PATCH 1/6] drm/i915: make the intel_display_power_domain enum compact

[Imre Deak]

Upcoming patches will add tracking for a set of power domains via a
bitmask; to make things simple there remove the current gap in the
enum values.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2ea66f2..9b04d05 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -98,14 +98,16 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_A,
 	POWER_DOMAIN_TRANSCODER_B,
 	POWER_DOMAIN_TRANSCODER_C,
-	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
 #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
 		((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER)
-#define POWER_DOMAIN_TRANSCODER(tran) ((tran) + POWER_DOMAIN_TRANSCODER_A)
+#define POWER_DOMAIN_TRANSCODER(tran) \
+	((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
+	 (tran) + POWER_DOMAIN_TRANSCODER_A)
 
 enum hpd_pin {
 	HPD_NONE = 0,
-- 
1.8.4

[PATCH 2/6] drm/i915: factor out is_always_on_domain

[Imre Deak]

It is just cleaner this way and makes it easier to add support for
other HW generations with always-on power wells powering a different
set of domains.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  8 ++++
 drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++--------------------------
 2 files changed, 38 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b04d05..ca05f3a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -100,8 +100,12 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+
+	POWER_DOMAIN_NUM,
 };
 
+#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
+
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
 #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
 		((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER)
@@ -109,6 +113,10 @@ enum intel_display_power_domain {
 	((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
 	 (tran) + POWER_DOMAIN_TRANSCODER_A)
 
+#define HSW_ALWAYS_ON_POWER_DOMAINS (		\
+	BIT(POWER_DOMAIN_PIPE_A) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_EDP))
+
 enum hpd_pin {
 	HPD_NONE = 0,
 	HPD_PORT_A = HPD_NONE, /* PORT_A is internal */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3d3658c..57d08a2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5367,6 +5367,23 @@ void intel_suspend_hw(struct drm_device *dev)
 		lpt_suspend_hw(dev);
 }
 
+static bool is_always_on_power_domain(struct drm_device *dev,
+				      enum intel_display_power_domain domain)
+{
+	unsigned long always_on_domains;
+
+	BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
+
+	if (IS_HASWELL(dev)) {
+		always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
+	} else {
+		WARN_ON(1);
+		return true;
+	}
+
+	return BIT(domain) & always_on_domains;
+}
+
 /**
  * We should only use the power well if we explicitly asked the hardware to
  * enable it, so check if it's enabled and also check if we've requested it to
@@ -5380,24 +5397,11 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return true;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
+	if (is_always_on_power_domain(dev, domain))
 		return true;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		return I915_READ(HSW_PWR_WELL_DRIVER) ==
+
+	return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
-	default:
-		BUG();
-	}
 }
 
 static void __intel_set_power_well(struct drm_device *dev, bool enable)
@@ -5469,26 +5473,12 @@ void intel_display_power_get(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
+	if (is_always_on_power_domain(dev, domain))
 		return;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		spin_lock_irq(&power_well->lock);
-		__intel_power_well_get(power_well);
-		spin_unlock_irq(&power_well->lock);
-		return;
-	default:
-		BUG();
-	}
+
+	spin_lock_irq(&power_well->lock);
+	__intel_power_well_get(power_well);
+	spin_unlock_irq(&power_well->lock);
 }
 
 void intel_display_power_put(struct drm_device *dev,
@@ -5500,26 +5490,12 @@ void intel_display_power_put(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
+	if (is_always_on_power_domain(dev, domain))
 		return;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		spin_lock_irq(&power_well->lock);
-		__intel_power_well_put(power_well);
-		spin_unlock_irq(&power_well->lock);
-		return;
-	default:
-		BUG();
-	}
+
+	spin_lock_irq(&power_well->lock);
+	__intel_power_well_put(power_well);
+	spin_unlock_irq(&power_well->lock);
 }
 
 static struct i915_power_well *hsw_pwr;
-- 
1.8.4

[PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Imre Deak]

There is no hard need for this to be a spin lock, as we don't take these
locks in irq context from anywhere. An upcoming patch will add calls to
punit read/write functions from within regions protected by this lock
and those functions need a mutex in turn. As a solution for that convert
the spin lock to be a mutex.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca05f3a..e4354dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	struct drm_device *device;
-	spinlock_t lock;
+	struct mutex lock;
 	/* power well enable/disable usage count */
 	int count;
 	int i915_request;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 57d08a2..f7363a8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	spin_lock_irq(&power_well->lock);
+	mutex_lock(&power_well->lock);
 	__intel_power_well_get(power_well);
-	spin_unlock_irq(&power_well->lock);
+	mutex_unlock(&power_well->lock);
 }
 
 void intel_display_power_put(struct drm_device *dev,
@@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	spin_lock_irq(&power_well->lock);
+	mutex_lock(&power_well->lock);
 	__intel_power_well_put(power_well);
-	spin_unlock_irq(&power_well->lock);
+	mutex_unlock(&power_well->lock);
 }
 
 static struct i915_power_well *hsw_pwr;
@@ -5506,9 +5506,9 @@ void i915_request_power_well(void)
 	if (WARN_ON(!hsw_pwr))
 		return;
 
-	spin_lock_irq(&hsw_pwr->lock);
+	mutex_lock(&hsw_pwr->lock);
 	__intel_power_well_get(hsw_pwr);
-	spin_unlock_irq(&hsw_pwr->lock);
+	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
 
@@ -5518,9 +5518,9 @@ void i915_release_power_well(void)
 	if (WARN_ON(!hsw_pwr))
 		return;
 
-	spin_lock_irq(&hsw_pwr->lock);
+	mutex_lock(&hsw_pwr->lock);
 	__intel_power_well_put(hsw_pwr);
-	spin_unlock_irq(&hsw_pwr->lock);
+	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
@@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev)
 	hsw_pwr = &dev_priv->power_well;
 
 	hsw_pwr->device = dev;
-	spin_lock_init(&hsw_pwr->lock);
+	mutex_init(&hsw_pwr->lock);
 	hsw_pwr->count = 0;
 
 	return 0;
@@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	if (!i915_disable_power_well && !enable)
 		return;
 
-	spin_lock_irq(&power_well->lock);
+	mutex_lock(&power_well->lock);
 
 	/*
 	 * This function will only ever contribute one
@@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 		__intel_power_well_put(power_well);
 
  out:
-	spin_unlock_irq(&power_well->lock);
+	mutex_unlock(&power_well->lock);
 }
 
 static void intel_resume_power_well(struct drm_device *dev)
@@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev)
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	spin_lock_irq(&power_well->lock);
+	mutex_lock(&power_well->lock);
 	__intel_set_power_well(dev, power_well->count > 0);
-	spin_unlock_irq(&power_well->lock);
+	mutex_unlock(&power_well->lock);
 }
 
 /*
-- 
1.8.4

[PATCH 4/6] drm/i915: factor out modeset_update_power_wells

[Imre Deak]

We'll need the same functionality for other HW generations. The support
for these will be added by upcoming patches.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b334c50..8e734f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6561,7 +6561,7 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
 	}
 }
 
-static void haswell_modeset_global_resources(struct drm_device *dev)
+static void modeset_update_power_wells(struct drm_device *dev)
 {
 	bool enable = false;
 	struct intel_crtc *crtc;
@@ -6576,7 +6576,11 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	}
 
 	intel_set_power_well(dev, enable);
+}
 
+static void haswell_modeset_global_resources(struct drm_device *dev)
+{
+	modeset_update_power_wells(dev);
 	hsw_update_package_c8(dev);
 }
 
-- 
1.8.4

[PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Imre Deak]

So far the modeset code enabled all power domains if it needed any. It
wasn't a problem since HW generations so far only had one always-on
power well and one dynamic power well that can be enabled/disabled. For
domains powered by always-on power wells (panel fitter on pipe A and the
eDP transcoder) we didn't do anything, for all other domains we just
enabled the single dynamic power well.

Future HW generations will change this, as they add multiple dynamic
power wells. Support for these will be added later, this patch prepares
for those by making sure we only enable the required domains.

Note that after this change on HSW we'll enable all power domains even
if it was the domain for the panel fitter on pipe A or the eDP
transcoder. This isn't a problem since the power domain framework
already checks if the domain is on an always-on power well and doesn't
do anything in this case.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8e734f2..e2a4f3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
 	}
 }
 
+#define for_each_power_domain(domain, mask)				\
+	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
+		if ((1 << (domain)) & (mask))
+
+static unsigned long get_pipe_power_domains(struct drm_device *dev,
+					    enum pipe pipe, bool pfit_enabled)
+{
+	unsigned long mask;
+	enum transcoder transcoder;
+
+	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
+
+	mask = BIT(POWER_DOMAIN_PIPE(pipe));
+	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
+	if (pfit_enabled)
+		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
+
+	return mask;
+}
+
 static void modeset_update_power_wells(struct drm_device *dev)
 {
-	bool enable = false;
+	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
 	struct intel_crtc *crtc;
 
+	/*
+	 * First get all needed power domains, then put all unneeded, to avoid
+	 * any unnecessary toggling of the power wells.
+	 */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
+		enum intel_display_power_domain domain;
+
 		if (!crtc->base.enabled)
 			continue;
 
-		if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
-		    crtc->config.cpu_transcoder != TRANSCODER_EDP)
-			enable = true;
+		pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
+						crtc->pipe,
+						crtc->config.pch_pfit.enabled);
+
+		for_each_power_domain(domain, pipe_domains[crtc->pipe])
+			intel_display_power_get(dev, domain);
 	}
 
-	intel_set_power_well(dev, enable);
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
+		enum intel_display_power_domain domain;
+
+		for_each_power_domain(domain, crtc->enabled_power_domains)
+			intel_display_power_put(dev, domain);
+
+		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
+	}
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 189257d..63a5bfd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -320,6 +320,7 @@ struct intel_crtc {
 	 * some outputs connected to this crtc.
 	 */
 	bool active;
+	unsigned long enabled_power_domains;
 	bool eld_vld;
 	bool primary_enabled; /* is the primary plane (partially) visible? */
 	bool lowfreq_avail;
-- 
1.8.4

[PATCH 6/6] drm/i915: use power get/put instead of set for power on after init

[Imre Deak]

Currently we make sure that all power domains are enabled during driver
init and turn off unneded ones only after the first modeset. Similarly
during suspend we enable all power domains, which will remain on through
the following resume until the first modeset.

This logic is supported by intel_set_power_well() in the power domain
framework. It would be nice to simplify the API, so that we only have
get/put functions and make it more explicit on the higher level how this
"power well on during init" logic works. This will make it also easier
if in the future we want to shorten the time the power wells are on.

For this add a new device private flag tracking whether we have the
power wells on because of init/suspend and use only
intel_display_power_get()/put(). As nothing else uses
intel_set_power_well() we can remove it.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  7 ++++++-
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 35 +----------------------------------
 6 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b8bc887..dd7f1f6 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1708,7 +1708,7 @@ int i915_driver_unload(struct drm_device *dev)
 		/* The i915.ko module is still not prepared to be loaded when
 		 * the power well is not enabled, so just enable it in case
 		 * we're going to unload/reload. */
-		intel_set_power_well(dev, true);
+		intel_display_set_init_power(dev, true);
 		i915_remove_power_well(dev);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 59649c0..7299a96 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -469,7 +469,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	hsw_disable_package_c8(dev_priv);
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e4354dd..0557c6b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -100,6 +100,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
 };
@@ -913,7 +914,6 @@ struct i915_power_well {
 	struct mutex lock;
 	/* power well enable/disable usage count */
 	int count;
-	int i915_request;
 };
 
 struct i915_dri1_state {
@@ -1369,6 +1369,11 @@ typedef struct drm_i915_private {
 	 * mchdev_lock in intel_pm.c */
 	struct intel_ilk_power_mgmt ips;
 
+	/*
+	 * Power wells needed for initialization at driver init and suspend
+	 * time are on. They are kept on until after the first modeset.
+	 */
+	bool init_power_on;
 	/* Haswell power well */
 	struct i915_power_well power_well;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2a4f3b..e30db91 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6581,6 +6581,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
 	return mask;
 }
 
+void intel_display_set_init_power(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->init_power_on == enable)
+		return;
+
+	if (enable)
+		intel_display_power_get(dev, POWER_DOMAIN_INIT);
+	else
+		intel_display_power_put(dev, POWER_DOMAIN_INIT);
+
+	dev_priv->init_power_on = enable;
+}
+
 static void modeset_update_power_wells(struct drm_device *dev)
 {
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
@@ -6612,6 +6627,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
 
 		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
 	}
+
+	intel_display_set_init_power(dev, false);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 63a5bfd..e6306f5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -680,6 +680,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
 void i915_disable_vga_mem(struct drm_device *dev);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
+void intel_display_set_init_power(struct drm_device *dev, bool enable);
 
 
 /* intel_dp.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f7363a8..9291a2e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5542,39 +5542,6 @@ void i915_remove_power_well(struct drm_device *dev)
 	hsw_pwr = NULL;
 }
 
-void intel_set_power_well(struct drm_device *dev, bool enable)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
-
-	if (!HAS_POWER_WELL(dev))
-		return;
-
-	if (!i915_disable_power_well && !enable)
-		return;
-
-	mutex_lock(&power_well->lock);
-
-	/*
-	 * This function will only ever contribute one
-	 * to the power well reference count. i915_request
-	 * is what tracks whether we have or have not
-	 * added the one to the reference count.
-	 */
-	if (power_well->i915_request == enable)
-		goto out;
-
-	power_well->i915_request = enable;
-
-	if (enable)
-		__intel_power_well_get(power_well);
-	else
-		__intel_power_well_put(power_well);
-
- out:
-	mutex_unlock(&power_well->lock);
-}
-
 static void intel_resume_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5602,7 +5569,7 @@ void intel_init_power_well(struct drm_device *dev)
 		return;
 
 	/* For now, we need the power well to be always enabled. */
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 	intel_resume_power_well(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
-- 
1.8.4

Re: [PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Paulo Zanoni]

2013/10/16 Imre Deak <imre.deak@intel.com>:
> There is no hard need for this to be a spin lock, as we don't take these
> locks in irq context from anywhere. An upcoming patch will add calls to
> punit read/write functions from within regions protected by this lock
> and those functions need a mutex in turn. As a solution for that convert
> the spin lock to be a mutex.

Not even from snd_hda_intel? Did we check this?

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca05f3a..e4354dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>         struct drm_device *device;
> -       spinlock_t lock;
> +       struct mutex lock;
>         /* power well enable/disable usage count */
>         int count;
>         int i915_request;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 57d08a2..f7363a8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       spin_lock_irq(&power_well->lock);
> +       mutex_lock(&power_well->lock);
>         __intel_power_well_get(power_well);
> -       spin_unlock_irq(&power_well->lock);
> +       mutex_unlock(&power_well->lock);
>  }
>
>  void intel_display_power_put(struct drm_device *dev,
> @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       spin_lock_irq(&power_well->lock);
> +       mutex_lock(&power_well->lock);
>         __intel_power_well_put(power_well);
> -       spin_unlock_irq(&power_well->lock);
> +       mutex_unlock(&power_well->lock);
>  }
>
>  static struct i915_power_well *hsw_pwr;
> @@ -5506,9 +5506,9 @@ void i915_request_power_well(void)
>         if (WARN_ON(!hsw_pwr))
>                 return;
>
> -       spin_lock_irq(&hsw_pwr->lock);
> +       mutex_lock(&hsw_pwr->lock);
>         __intel_power_well_get(hsw_pwr);
> -       spin_unlock_irq(&hsw_pwr->lock);
> +       mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
>
> @@ -5518,9 +5518,9 @@ void i915_release_power_well(void)
>         if (WARN_ON(!hsw_pwr))
>                 return;
>
> -       spin_lock_irq(&hsw_pwr->lock);
> +       mutex_lock(&hsw_pwr->lock);
>         __intel_power_well_put(hsw_pwr);
> -       spin_unlock_irq(&hsw_pwr->lock);
> +       mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev)
>         hsw_pwr = &dev_priv->power_well;
>
>         hsw_pwr->device = dev;
> -       spin_lock_init(&hsw_pwr->lock);
> +       mutex_init(&hsw_pwr->lock);
>         hsw_pwr->count = 0;
>
>         return 0;
> @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>         if (!i915_disable_power_well && !enable)
>                 return;
>
> -       spin_lock_irq(&power_well->lock);
> +       mutex_lock(&power_well->lock);
>
>         /*
>          * This function will only ever contribute one
> @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 __intel_power_well_put(power_well);
>
>   out:
> -       spin_unlock_irq(&power_well->lock);
> +       mutex_unlock(&power_well->lock);
>  }
>
>  static void intel_resume_power_well(struct drm_device *dev)
> @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev)
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
> -       spin_lock_irq(&power_well->lock);
> +       mutex_lock(&power_well->lock);
>         __intel_set_power_well(dev, power_well->count > 0);
> -       spin_unlock_irq(&power_well->lock);
> +       mutex_unlock(&power_well->lock);
>  }
>
>  /*
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 5132 bytes --]

On Wed, 2013-10-16 at 13:19 -0300, Paulo Zanoni wrote:
> 2013/10/16 Imre Deak <imre.deak@intel.com>:
> > There is no hard need for this to be a spin lock, as we don't take these
> > locks in irq context from anywhere. An upcoming patch will add calls to
> > punit read/write functions from within regions protected by this lock
> > and those functions need a mutex in turn. As a solution for that convert
> > the spin lock to be a mutex.
> 
> Not even from snd_hda_intel? Did we check this?

Yea, at least I tried to check this at all call sites and haven't found
any place where they request the power well in atomic context. In any
case I think it's a design problem if they do ..

--Imre

> 
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
> >  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++-------------
> >  2 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ca05f3a..e4354dd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt {
> >  /* Power well structure for haswell */
> >  struct i915_power_well {
> >         struct drm_device *device;
> > -       spinlock_t lock;
> > +       struct mutex lock;
> >         /* power well enable/disable usage count */
> >         int count;
> >         int i915_request;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 57d08a2..f7363a8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev,
> >         if (is_always_on_power_domain(dev, domain))
> >                 return;
> >
> > -       spin_lock_irq(&power_well->lock);
> > +       mutex_lock(&power_well->lock);
> >         __intel_power_well_get(power_well);
> > -       spin_unlock_irq(&power_well->lock);
> > +       mutex_unlock(&power_well->lock);
> >  }
> >
> >  void intel_display_power_put(struct drm_device *dev,
> > @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev,
> >         if (is_always_on_power_domain(dev, domain))
> >                 return;
> >
> > -       spin_lock_irq(&power_well->lock);
> > +       mutex_lock(&power_well->lock);
> >         __intel_power_well_put(power_well);
> > -       spin_unlock_irq(&power_well->lock);
> > +       mutex_unlock(&power_well->lock);
> >  }
> >
> >  static struct i915_power_well *hsw_pwr;
> > @@ -5506,9 +5506,9 @@ void i915_request_power_well(void)
> >         if (WARN_ON(!hsw_pwr))
> >                 return;
> >
> > -       spin_lock_irq(&hsw_pwr->lock);
> > +       mutex_lock(&hsw_pwr->lock);
> >         __intel_power_well_get(hsw_pwr);
> > -       spin_unlock_irq(&hsw_pwr->lock);
> > +       mutex_unlock(&hsw_pwr->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(i915_request_power_well);
> >
> > @@ -5518,9 +5518,9 @@ void i915_release_power_well(void)
> >         if (WARN_ON(!hsw_pwr))
> >                 return;
> >
> > -       spin_lock_irq(&hsw_pwr->lock);
> > +       mutex_lock(&hsw_pwr->lock);
> >         __intel_power_well_put(hsw_pwr);
> > -       spin_unlock_irq(&hsw_pwr->lock);
> > +       mutex_unlock(&hsw_pwr->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(i915_release_power_well);
> >
> > @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev)
> >         hsw_pwr = &dev_priv->power_well;
> >
> >         hsw_pwr->device = dev;
> > -       spin_lock_init(&hsw_pwr->lock);
> > +       mutex_init(&hsw_pwr->lock);
> >         hsw_pwr->count = 0;
> >
> >         return 0;
> > @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> >         if (!i915_disable_power_well && !enable)
> >                 return;
> >
> > -       spin_lock_irq(&power_well->lock);
> > +       mutex_lock(&power_well->lock);
> >
> >         /*
> >          * This function will only ever contribute one
> > @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> >                 __intel_power_well_put(power_well);
> >
> >   out:
> > -       spin_unlock_irq(&power_well->lock);
> > +       mutex_unlock(&power_well->lock);
> >  }
> >
> >  static void intel_resume_power_well(struct drm_device *dev)
> > @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev)
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> >
> > -       spin_lock_irq(&power_well->lock);
> > +       mutex_lock(&power_well->lock);
> >         __intel_set_power_well(dev, power_well->count > 0);
> > -       spin_unlock_irq(&power_well->lock);
> > +       mutex_unlock(&power_well->lock);
> >  }
> >
> >  /*
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/6] drm/i915: make the intel_display_power_domain enum compact

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:48 +0300
Imre Deak <imre.deak@intel.com> wrote:

> Upcoming patches will add tracking for a set of power domains via a
> bitmask; to make things simple there remove the current gap in the
> enum values.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2ea66f2..9b04d05 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -98,14 +98,16 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_A,
>  	POWER_DOMAIN_TRANSCODER_B,
>  	POWER_DOMAIN_TRANSCODER_C,
> -	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
> +	POWER_DOMAIN_TRANSCODER_EDP,
>  	POWER_DOMAIN_VGA,
>  };
>  
>  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
>  #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
>  		((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER)
> -#define POWER_DOMAIN_TRANSCODER(tran) ((tran) + POWER_DOMAIN_TRANSCODER_A)
> +#define POWER_DOMAIN_TRANSCODER(tran) \
> +	((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
> +	 (tran) + POWER_DOMAIN_TRANSCODER_A)
>  
>  enum hpd_pin {
>  	HPD_NONE = 0,

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/6] drm/i915: factor out is_always_on_domain

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:49 +0300
Imre Deak <imre.deak@intel.com> wrote:

> It is just cleaner this way and makes it easier to add support for
> other HW generations with always-on power wells powering a different
> set of domains.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  8 ++++
>  drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++--------------------------
>  2 files changed, 38 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9b04d05..ca05f3a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,8 +100,12 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_C,
>  	POWER_DOMAIN_TRANSCODER_EDP,
>  	POWER_DOMAIN_VGA,
> +
> +	POWER_DOMAIN_NUM,
>  };
>  
> +#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
> +
>  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
>  #define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
>  		((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER)
> @@ -109,6 +113,10 @@ enum intel_display_power_domain {
>  	((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \
>  	 (tran) + POWER_DOMAIN_TRANSCODER_A)
>  
> +#define HSW_ALWAYS_ON_POWER_DOMAINS (		\
> +	BIT(POWER_DOMAIN_PIPE_A) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_EDP))
> +
>  enum hpd_pin {
>  	HPD_NONE = 0,
>  	HPD_PORT_A = HPD_NONE, /* PORT_A is internal */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3d3658c..57d08a2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5367,6 +5367,23 @@ void intel_suspend_hw(struct drm_device *dev)
>  		lpt_suspend_hw(dev);
>  }
>  
> +static bool is_always_on_power_domain(struct drm_device *dev,
> +				      enum intel_display_power_domain domain)
> +{
> +	unsigned long always_on_domains;
> +
> +	BUG_ON(BIT(domain) & ~POWER_DOMAIN_MASK);
> +
> +	if (IS_HASWELL(dev)) {
> +		always_on_domains = HSW_ALWAYS_ON_POWER_DOMAINS;
> +	} else {
> +		WARN_ON(1);
> +		return true;
> +	}
> +
> +	return BIT(domain) & always_on_domains;
> +}
> +
>  /**
>   * We should only use the power well if we explicitly asked the hardware to
>   * enable it, so check if it's enabled and also check if we've requested it to
> @@ -5380,24 +5397,11 @@ bool intel_display_power_enabled(struct drm_device *dev,
>  	if (!HAS_POWER_WELL(dev))
>  		return true;
>  
> -	switch (domain) {
> -	case POWER_DOMAIN_PIPE_A:
> -	case POWER_DOMAIN_TRANSCODER_EDP:
> +	if (is_always_on_power_domain(dev, domain))
>  		return true;
> -	case POWER_DOMAIN_VGA:
> -	case POWER_DOMAIN_PIPE_B:
> -	case POWER_DOMAIN_PIPE_C:
> -	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> -	case POWER_DOMAIN_TRANSCODER_A:
> -	case POWER_DOMAIN_TRANSCODER_B:
> -	case POWER_DOMAIN_TRANSCODER_C:
> -		return I915_READ(HSW_PWR_WELL_DRIVER) ==
> +
> +	return I915_READ(HSW_PWR_WELL_DRIVER) ==
>  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
> -	default:
> -		BUG();
> -	}
>  }
>  
>  static void __intel_set_power_well(struct drm_device *dev, bool enable)
> @@ -5469,26 +5473,12 @@ void intel_display_power_get(struct drm_device *dev,
>  	if (!HAS_POWER_WELL(dev))
>  		return;
>  
> -	switch (domain) {
> -	case POWER_DOMAIN_PIPE_A:
> -	case POWER_DOMAIN_TRANSCODER_EDP:
> +	if (is_always_on_power_domain(dev, domain))
>  		return;
> -	case POWER_DOMAIN_VGA:
> -	case POWER_DOMAIN_PIPE_B:
> -	case POWER_DOMAIN_PIPE_C:
> -	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> -	case POWER_DOMAIN_TRANSCODER_A:
> -	case POWER_DOMAIN_TRANSCODER_B:
> -	case POWER_DOMAIN_TRANSCODER_C:
> -		spin_lock_irq(&power_well->lock);
> -		__intel_power_well_get(power_well);
> -		spin_unlock_irq(&power_well->lock);
> -		return;
> -	default:
> -		BUG();
> -	}
> +
> +	spin_lock_irq(&power_well->lock);
> +	__intel_power_well_get(power_well);
> +	spin_unlock_irq(&power_well->lock);
>  }
>  
>  void intel_display_power_put(struct drm_device *dev,
> @@ -5500,26 +5490,12 @@ void intel_display_power_put(struct drm_device *dev,
>  	if (!HAS_POWER_WELL(dev))
>  		return;
>  
> -	switch (domain) {
> -	case POWER_DOMAIN_PIPE_A:
> -	case POWER_DOMAIN_TRANSCODER_EDP:
> +	if (is_always_on_power_domain(dev, domain))
>  		return;
> -	case POWER_DOMAIN_VGA:
> -	case POWER_DOMAIN_PIPE_B:
> -	case POWER_DOMAIN_PIPE_C:
> -	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> -	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> -	case POWER_DOMAIN_TRANSCODER_A:
> -	case POWER_DOMAIN_TRANSCODER_B:
> -	case POWER_DOMAIN_TRANSCODER_C:
> -		spin_lock_irq(&power_well->lock);
> -		__intel_power_well_put(power_well);
> -		spin_unlock_irq(&power_well->lock);
> -		return;
> -	default:
> -		BUG();
> -	}
> +
> +	spin_lock_irq(&power_well->lock);
> +	__intel_power_well_put(power_well);
> +	spin_unlock_irq(&power_well->lock);
>  }
>  
>  static struct i915_power_well *hsw_pwr;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:50 +0300
Imre Deak <imre.deak@intel.com> wrote:

> There is no hard need for this to be a spin lock, as we don't take these
> locks in irq context from anywhere. An upcoming patch will add calls to
> punit read/write functions from within regions protected by this lock
> and those functions need a mutex in turn. As a solution for that convert
> the spin lock to be a mutex.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca05f3a..e4354dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -910,7 +910,7 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>  	struct drm_device *device;
> -	spinlock_t lock;
> +	struct mutex lock;
>  	/* power well enable/disable usage count */
>  	int count;
>  	int i915_request;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 57d08a2..f7363a8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5476,9 +5476,9 @@ void intel_display_power_get(struct drm_device *dev,
>  	if (is_always_on_power_domain(dev, domain))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_power_well_get(power_well);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  void intel_display_power_put(struct drm_device *dev,
> @@ -5493,9 +5493,9 @@ void intel_display_power_put(struct drm_device *dev,
>  	if (is_always_on_power_domain(dev, domain))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_power_well_put(power_well);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  static struct i915_power_well *hsw_pwr;
> @@ -5506,9 +5506,9 @@ void i915_request_power_well(void)
>  	if (WARN_ON(!hsw_pwr))
>  		return;
>  
> -	spin_lock_irq(&hsw_pwr->lock);
> +	mutex_lock(&hsw_pwr->lock);
>  	__intel_power_well_get(hsw_pwr);
> -	spin_unlock_irq(&hsw_pwr->lock);
> +	mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
>  
> @@ -5518,9 +5518,9 @@ void i915_release_power_well(void)
>  	if (WARN_ON(!hsw_pwr))
>  		return;
>  
> -	spin_lock_irq(&hsw_pwr->lock);
> +	mutex_lock(&hsw_pwr->lock);
>  	__intel_power_well_put(hsw_pwr);
> -	spin_unlock_irq(&hsw_pwr->lock);
> +	mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>  
> @@ -5531,7 +5531,7 @@ int i915_init_power_well(struct drm_device *dev)
>  	hsw_pwr = &dev_priv->power_well;
>  
>  	hsw_pwr->device = dev;
> -	spin_lock_init(&hsw_pwr->lock);
> +	mutex_init(&hsw_pwr->lock);
>  	hsw_pwr->count = 0;
>  
>  	return 0;
> @@ -5553,7 +5553,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>  	if (!i915_disable_power_well && !enable)
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  
>  	/*
>  	 * This function will only ever contribute one
> @@ -5572,7 +5572,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>  		__intel_power_well_put(power_well);
>  
>   out:
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  static void intel_resume_power_well(struct drm_device *dev)
> @@ -5583,9 +5583,9 @@ static void intel_resume_power_well(struct drm_device *dev)
>  	if (!HAS_POWER_WELL(dev))
>  		return;
>  
> -	spin_lock_irq(&power_well->lock);
> +	mutex_lock(&power_well->lock);
>  	__intel_set_power_well(dev, power_well->count > 0);
> -	spin_unlock_irq(&power_well->lock);
> +	mutex_unlock(&power_well->lock);
>  }
>  
>  /*

Are there ordering requirements we should document?  E.g. always take
this after the mode config lock or something?

Otherwise:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: factor out modeset_update_power_wells

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:51 +0300
Imre Deak <imre.deak@intel.com> wrote:

> We'll need the same functionality for other HW generations. The support
> for these will be added by upcoming patches.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b334c50..8e734f2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6561,7 +6561,7 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> -static void haswell_modeset_global_resources(struct drm_device *dev)
> +static void modeset_update_power_wells(struct drm_device *dev)
>  {
>  	bool enable = false;
>  	struct intel_crtc *crtc;
> @@ -6576,7 +6576,11 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
>  	}
>  
>  	intel_set_power_well(dev, enable);
> +}
>  
> +static void haswell_modeset_global_resources(struct drm_device *dev)
> +{
> +	modeset_update_power_wells(dev);
>  	hsw_update_package_c8(dev);
>  }
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:52 +0300
Imre Deak <imre.deak@intel.com> wrote:

> So far the modeset code enabled all power domains if it needed any. It
> wasn't a problem since HW generations so far only had one always-on
> power well and one dynamic power well that can be enabled/disabled. For
> domains powered by always-on power wells (panel fitter on pipe A and the
> eDP transcoder) we didn't do anything, for all other domains we just
> enabled the single dynamic power well.
> 
> Future HW generations will change this, as they add multiple dynamic
> power wells. Support for these will be added later, this patch prepares
> for those by making sure we only enable the required domains.
> 
> Note that after this change on HSW we'll enable all power domains even
> if it was the domain for the panel fitter on pipe A or the eDP
> transcoder. This isn't a problem since the power domain framework
> already checks if the domain is on an always-on power well and doesn't
> do anything in this case.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e734f2..e2a4f3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> +#define for_each_power_domain(domain, mask)				\
> +	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> +		if ((1 << (domain)) & (mask))
> +
> +static unsigned long get_pipe_power_domains(struct drm_device *dev,
> +					    enum pipe pipe, bool pfit_enabled)
> +{
> +	unsigned long mask;
> +	enum transcoder transcoder;
> +
> +	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
> +
> +	mask = BIT(POWER_DOMAIN_PIPE(pipe));
> +	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> +	if (pfit_enabled)
> +		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
> +
> +	return mask;
> +}
> +
>  static void modeset_update_power_wells(struct drm_device *dev)
>  {
> -	bool enable = false;
> +	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
>  	struct intel_crtc *crtc;
>  
> +	/*
> +	 * First get all needed power domains, then put all unneeded, to avoid
> +	 * any unnecessary toggling of the power wells.
> +	 */
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +		enum intel_display_power_domain domain;
> +
>  		if (!crtc->base.enabled)
>  			continue;
>  
> -		if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
> -		    crtc->config.cpu_transcoder != TRANSCODER_EDP)
> -			enable = true;
> +		pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
> +						crtc->pipe,
> +						crtc->config.pch_pfit.enabled);
> +
> +		for_each_power_domain(domain, pipe_domains[crtc->pipe])
> +			intel_display_power_get(dev, domain);
>  	}
>  
> -	intel_set_power_well(dev, enable);
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +		enum intel_display_power_domain domain;
> +
> +		for_each_power_domain(domain, crtc->enabled_power_domains)
> +			intel_display_power_put(dev, domain);
> +
> +		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> +	}
>  }
>  
>  static void haswell_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 189257d..63a5bfd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -320,6 +320,7 @@ struct intel_crtc {
>  	 * some outputs connected to this crtc.
>  	 */
>  	bool active;
> +	unsigned long enabled_power_domains;
>  	bool eld_vld;
>  	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Hope we can get rid of the set_power_well() function soon...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/6] drm/i915: use power get/put instead of set for power on after init

[Jesse Barnes]

On Wed, 16 Oct 2013 17:25:53 +0300
Imre Deak <imre.deak@intel.com> wrote:

> Currently we make sure that all power domains are enabled during driver
> init and turn off unneded ones only after the first modeset. Similarly
> during suspend we enable all power domains, which will remain on through
> the following resume until the first modeset.
> 
> This logic is supported by intel_set_power_well() in the power domain
> framework. It would be nice to simplify the API, so that we only have
> get/put functions and make it more explicit on the higher level how this
> "power well on during init" logic works. This will make it also easier
> if in the future we want to shorten the time the power wells are on.
> 
> For this add a new device private flag tracking whether we have the
> power wells on because of init/suspend and use only
> intel_display_power_get()/put(). As nothing else uses
> intel_set_power_well() we can remove it.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 35 +----------------------------------
>  6 files changed, 27 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b8bc887..dd7f1f6 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1708,7 +1708,7 @@ int i915_driver_unload(struct drm_device *dev)
>  		/* The i915.ko module is still not prepared to be loaded when
>  		 * the power well is not enabled, so just enable it in case
>  		 * we're going to unload/reload. */
> -		intel_set_power_well(dev, true);
> +		intel_display_set_init_power(dev, true);
>  		i915_remove_power_well(dev);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 59649c0..7299a96 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -469,7 +469,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	/* We do a lot of poking in a lot of registers, make sure they work
>  	 * properly. */
>  	hsw_disable_package_c8(dev_priv);
> -	intel_set_power_well(dev, true);
> +	intel_display_set_init_power(dev, true);
>  
>  	drm_kms_helper_poll_disable(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4354dd..0557c6b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,6 +100,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_C,
>  	POWER_DOMAIN_TRANSCODER_EDP,
>  	POWER_DOMAIN_VGA,
> +	POWER_DOMAIN_INIT,
>  
>  	POWER_DOMAIN_NUM,
>  };
> @@ -913,7 +914,6 @@ struct i915_power_well {
>  	struct mutex lock;
>  	/* power well enable/disable usage count */
>  	int count;
> -	int i915_request;
>  };
>  
>  struct i915_dri1_state {
> @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private {
>  	 * mchdev_lock in intel_pm.c */
>  	struct intel_ilk_power_mgmt ips;
>  
> +	/*
> +	 * Power wells needed for initialization at driver init and suspend
> +	 * time are on. They are kept on until after the first modeset.
> +	 */
> +	bool init_power_on;
>  	/* Haswell power well */
>  	struct i915_power_well power_well;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e2a4f3b..e30db91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6581,6 +6581,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
>  	return mask;
>  }
>  
> +void intel_display_set_init_power(struct drm_device *dev, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->init_power_on == enable)
> +		return;
> +
> +	if (enable)
> +		intel_display_power_get(dev, POWER_DOMAIN_INIT);
> +	else
> +		intel_display_power_put(dev, POWER_DOMAIN_INIT);
> +
> +	dev_priv->init_power_on = enable;
> +}
> +
>  static void modeset_update_power_wells(struct drm_device *dev)
>  {
>  	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> @@ -6612,6 +6627,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
>  
>  		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
>  	}
> +
> +	intel_display_set_init_power(dev, false);
>  }
>  
>  static void haswell_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 63a5bfd..e6306f5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -680,6 +680,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
>  void i915_disable_vga_mem(struct drm_device *dev);
>  void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
> +void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  
>  
>  /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f7363a8..9291a2e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5542,39 +5542,6 @@ void i915_remove_power_well(struct drm_device *dev)
>  	hsw_pwr = NULL;
>  }
>  
> -void intel_set_power_well(struct drm_device *dev, bool enable)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct i915_power_well *power_well = &dev_priv->power_well;
> -
> -	if (!HAS_POWER_WELL(dev))
> -		return;
> -
> -	if (!i915_disable_power_well && !enable)
> -		return;
> -
> -	mutex_lock(&power_well->lock);
> -
> -	/*
> -	 * This function will only ever contribute one
> -	 * to the power well reference count. i915_request
> -	 * is what tracks whether we have or have not
> -	 * added the one to the reference count.
> -	 */
> -	if (power_well->i915_request == enable)
> -		goto out;
> -
> -	power_well->i915_request = enable;
> -
> -	if (enable)
> -		__intel_power_well_get(power_well);
> -	else
> -		__intel_power_well_put(power_well);
> -
> - out:
> -	mutex_unlock(&power_well->lock);
> -}
> -
>  static void intel_resume_power_well(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5602,7 +5569,7 @@ void intel_init_power_well(struct drm_device *dev)
>  		return;
>  
>  	/* For now, we need the power well to be always enabled. */
> -	intel_set_power_well(dev, true);
> +	intel_display_set_init_power(dev, true);
>  	intel_resume_power_well(dev);
>  
>  	/* We're taking over the BIOS, so clear any requests made by it since

And there it goes, yay! :)  Now we just have to get rid of the _INIT
domain and actually shut things down before the first mode set if
nothing is in use (maybe with a delayed shutdown to avoid toggling too
fast).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/6] drm/i915: change power_well->lock to be mutex

[Daniel Vetter]

On Fri, Oct 18, 2013 at 11:50:47AM -0700, Jesse Barnes wrote:
> Are there ordering requirements we should document?  E.g. always take
> this after the mode config lock or something?

The mode_config lock is pretty much the outermost thing, and for getting
it right we have lockdpe. As long as we ensure that we have full coverage
with our tests and as long as QA doesn't fumble running the debug kernel
builds we should be fine. So imo no need to document the locking.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: use power get/put instead of set for power on after init

[Daniel Vetter]

On Wed, Oct 16, 2013 at 05:25:53PM +0300, Imre Deak wrote:
> Currently we make sure that all power domains are enabled during driver
> init and turn off unneded ones only after the first modeset. Similarly
> during suspend we enable all power domains, which will remain on through
> the following resume until the first modeset.
> 
> This logic is supported by intel_set_power_well() in the power domain
> framework. It would be nice to simplify the API, so that we only have
> get/put functions and make it more explicit on the higher level how this
> "power well on during init" logic works. This will make it also easier
> if in the future we want to shorten the time the power wells are on.
> 
> For this add a new device private flag tracking whether we have the
> power wells on because of init/suspend and use only
> intel_display_power_get()/put(). As nothing else uses
> intel_set_power_well() we can remove it.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---

[snip]

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e4354dd..0557c6b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,6 +100,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_C,
>  	POWER_DOMAIN_TRANSCODER_EDP,
>  	POWER_DOMAIN_VGA,
> +	POWER_DOMAIN_INIT,
>  
>  	POWER_DOMAIN_NUM,
>  };
> @@ -913,7 +914,6 @@ struct i915_power_well {
>  	struct mutex lock;
>  	/* power well enable/disable usage count */
>  	int count;
> -	int i915_request;
>  };
>  
>  struct i915_dri1_state {
> @@ -1369,6 +1369,11 @@ typedef struct drm_i915_private {
>  	 * mchdev_lock in intel_pm.c */
>  	struct intel_ilk_power_mgmt ips;
>  
> +	/*
> +	 * Power wells needed for initialization at driver init and suspend
> +	 * time are on. They are kept on until after the first modeset.
> +	 */
> +	bool init_power_on;

Please move this into the nice power_well structure we have to avoid
overtly polluting our i915_driver_private thing ... All patches up to this
one merged, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/2] drm/i915: prepare for multiple power wells

[Imre Deak]

In the future we'll need to support multiple power wells, so prepare for
that here. Create a new power domains struct which contains all
power domain/well specific fields. Since we'll have one lock protecting
all power wells, move power_well->lock to the new struct too.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
 drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 80957ca..7315095 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	struct drm_device *device;
-	struct mutex lock;
 	/* power well enable/disable usage count */
 	int count;
 	int i915_request;
 };
 
+#define I915_MAX_POWER_WELLS 1
+
+struct i915_power_domains {
+	struct mutex lock;
+	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
+};
+
 struct i915_dri1_state {
 	unsigned allow_batchbuffer : 1;
 	u32 __iomem *gfx_hws_cpu_addr;
@@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
 	 * mchdev_lock in intel_pm.c */
 	struct intel_ilk_power_mgmt ips;
 
-	/* Haswell power well */
-	struct i915_power_well power_well;
+	struct i915_power_domains power_domains;
 
 	struct i915_psr psr;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3e140ab..09ca6f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	mutex_lock(&power_well->lock);
-	__intel_power_well_get(power_well);
-	mutex_unlock(&power_well->lock);
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	__intel_power_well_get(&power_domains->power_wells[0]);
+	mutex_unlock(&power_domains->lock);
 }
 
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	mutex_lock(&power_well->lock);
-	__intel_power_well_put(power_well);
-	mutex_unlock(&power_well->lock);
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	__intel_power_well_put(&power_domains->power_wells[0]);
+	mutex_unlock(&power_domains->lock);
 }
 
-static struct i915_power_well *hsw_pwr;
+static struct i915_power_domains *hsw_pwr;
 
 /* Display audio driver power well request */
 void i915_request_power_well(void)
@@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
 		return;
 
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(hsw_pwr);
+	__intel_power_well_get(&hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
 		return;
 
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(hsw_pwr);
+	__intel_power_well_put(&hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 int i915_init_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
-	hsw_pwr = &dev_priv->power_well;
+	mutex_init(&power_domains->lock);
+	hsw_pwr = power_domains;
 
-	hsw_pwr->device = dev;
-	mutex_init(&hsw_pwr->lock);
-	hsw_pwr->count = 0;
+	power_well = &power_domains->power_wells[0];
+	power_well->device = dev;
+	power_well->count = 0;
 
 	return 0;
 }
@@ -5702,7 +5709,8 @@ void i915_remove_power_well(struct drm_device *dev)
 void intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5710,8 +5718,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	if (!i915_disable_power_well && !enable)
 		return;
 
-	mutex_lock(&power_well->lock);
+	mutex_lock(&power_domains->lock);
 
+	power_well = &power_domains->power_wells[0];
 	/*
 	 * This function will only ever contribute one
 	 * to the power well reference count. i915_request
@@ -5729,20 +5738,24 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 		__intel_power_well_put(power_well);
 
  out:
-	mutex_unlock(&power_well->lock);
+	mutex_unlock(&power_domains->lock);
 }
 
 static void intel_resume_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	mutex_lock(&power_well->lock);
+	mutex_lock(&power_domains->lock);
+
+	power_well = &power_domains->power_wells[0];
 	__intel_set_power_well(dev, power_well->count > 0);
-	mutex_unlock(&power_well->lock);
+
+	mutex_unlock(&power_domains->lock);
 }
 
 /*
-- 
1.8.4

[PATCH v2 2/2] drm/i915: use power get/put instead of set for power on after init

[Imre Deak]

Currently we make sure that all power domains are enabled during driver
init and turn off unneded ones only after the first modeset. Similarly
during suspend we enable all power domains, which will remain on through
the following resume until the first modeset.

This logic is supported by intel_set_power_well() in the power domain
framework. It would be nice to simplify the API, so that we only have
get/put functions and make it more explicit on the higher level how this
"power well on during init" logic works. This will make it also easier
if in the future we want to shorten the time the power wells are on.

For this add a new device private flag tracking whether we have the
power wells on because of init/suspend and use only
intel_display_power_get()/put(). As nothing else uses
intel_set_power_well() we can remove it.

v2:
- move the init_power_on flag to the power_domains struct (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  8 +++++++-
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 37 +-----------------------------------
 6 files changed, 28 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fd848ef..b722b35 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1710,7 +1710,7 @@ int i915_driver_unload(struct drm_device *dev)
 		/* The i915.ko module is still not prepared to be loaded when
 		 * the power well is not enabled, so just enable it in case
 		 * we're going to unload/reload. */
-		intel_set_power_well(dev, true);
+		intel_display_set_init_power(dev, true);
 		i915_remove_power_well(dev);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 85ae0dc..770c9f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -477,7 +477,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	hsw_disable_package_c8(dev_priv);
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7315095..3565db2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -100,6 +100,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
 };
@@ -913,12 +914,17 @@ struct i915_power_well {
 	struct drm_device *device;
 	/* power well enable/disable usage count */
 	int count;
-	int i915_request;
 };
 
 #define I915_MAX_POWER_WELLS 1
 
 struct i915_power_domains {
+	/*
+	 * Power wells needed for initialization at driver init and suspend
+	 * time are on. They are kept on until after the first modeset.
+	 */
+	bool init_power_on;
+
 	struct mutex lock;
 	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e79a2a..0c2e83c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6573,6 +6573,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
 	return mask;
 }
 
+void intel_display_set_init_power(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->power_domains.init_power_on == enable)
+		return;
+
+	if (enable)
+		intel_display_power_get(dev, POWER_DOMAIN_INIT);
+	else
+		intel_display_power_put(dev, POWER_DOMAIN_INIT);
+
+	dev_priv->power_domains.init_power_on = enable;
+}
+
 static void modeset_update_power_wells(struct drm_device *dev)
 {
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
@@ -6604,6 +6619,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
 
 		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
 	}
+
+	intel_display_set_init_power(dev, false);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index af1553c..bf4394a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -692,6 +692,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
 void i915_disable_vga_mem(struct drm_device *dev);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
+void intel_display_set_init_power(struct drm_device *dev, bool enable);
 
 
 /* intel_dp.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 09ca6f9..4c38e28 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5706,41 +5706,6 @@ void i915_remove_power_well(struct drm_device *dev)
 	hsw_pwr = NULL;
 }
 
-void intel_set_power_well(struct drm_device *dev, bool enable)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	struct i915_power_well *power_well;
-
-	if (!HAS_POWER_WELL(dev))
-		return;
-
-	if (!i915_disable_power_well && !enable)
-		return;
-
-	mutex_lock(&power_domains->lock);
-
-	power_well = &power_domains->power_wells[0];
-	/*
-	 * This function will only ever contribute one
-	 * to the power well reference count. i915_request
-	 * is what tracks whether we have or have not
-	 * added the one to the reference count.
-	 */
-	if (power_well->i915_request == enable)
-		goto out;
-
-	power_well->i915_request = enable;
-
-	if (enable)
-		__intel_power_well_get(power_well);
-	else
-		__intel_power_well_put(power_well);
-
- out:
-	mutex_unlock(&power_domains->lock);
-}
-
 static void intel_resume_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5772,7 +5737,7 @@ void intel_init_power_well(struct drm_device *dev)
 		return;
 
 	/* For now, we need the power well to be always enabled. */
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 	intel_resume_power_well(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
-- 
1.8.4

Re: [PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Paulo Zanoni]

2013/10/18 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Wed, 16 Oct 2013 17:25:52 +0300
> Imre Deak <imre.deak@intel.com> wrote:
>
>> So far the modeset code enabled all power domains if it needed any. It
>> wasn't a problem since HW generations so far only had one always-on
>> power well and one dynamic power well that can be enabled/disabled. For
>> domains powered by always-on power wells (panel fitter on pipe A and the
>> eDP transcoder) we didn't do anything, for all other domains we just
>> enabled the single dynamic power well.
>>
>> Future HW generations will change this, as they add multiple dynamic
>> power wells. Support for these will be added later, this patch prepares
>> for those by making sure we only enable the required domains.
>>
>> Note that after this change on HSW we'll enable all power domains even
>> if it was the domain for the panel fitter on pipe A or the eDP
>> transcoder. This isn't a problem since the power domain framework
>> already checks if the domain is on an always-on power well and doesn't
>> do anything in this case.
>>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>

I updated drm-intel-nightly and now when I boot Haswell with eDP-only,
the power well is enabled, where it should be disabled. Reverting this
patch fixes the problem for me.


>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 42 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8e734f2..e2a4f3b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>>       }
>>  }
>>
>> +#define for_each_power_domain(domain, mask)                          \
>> +     for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)     \
>> +             if ((1 << (domain)) & (mask))
>> +
>> +static unsigned long get_pipe_power_domains(struct drm_device *dev,
>> +                                         enum pipe pipe, bool pfit_enabled)
>> +{
>> +     unsigned long mask;
>> +     enum transcoder transcoder;
>> +
>> +     transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
>> +
>> +     mask = BIT(POWER_DOMAIN_PIPE(pipe));
>> +     mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
>> +     if (pfit_enabled)
>> +             mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
>> +
>> +     return mask;
>> +}
>> +
>>  static void modeset_update_power_wells(struct drm_device *dev)
>>  {
>> -     bool enable = false;
>> +     unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
>>       struct intel_crtc *crtc;
>>
>> +     /*
>> +      * First get all needed power domains, then put all unneeded, to avoid
>> +      * any unnecessary toggling of the power wells.
>> +      */
>>       list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
>> +             enum intel_display_power_domain domain;
>> +
>>               if (!crtc->base.enabled)
>>                       continue;
>>
>> -             if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
>> -                 crtc->config.cpu_transcoder != TRANSCODER_EDP)
>> -                     enable = true;
>> +             pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
>> +                                             crtc->pipe,
>> +                                             crtc->config.pch_pfit.enabled);
>> +
>> +             for_each_power_domain(domain, pipe_domains[crtc->pipe])
>> +                     intel_display_power_get(dev, domain);
>>       }
>>
>> -     intel_set_power_well(dev, enable);
>> +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
>> +             enum intel_display_power_domain domain;
>> +
>> +             for_each_power_domain(domain, crtc->enabled_power_domains)
>> +                     intel_display_power_put(dev, domain);
>> +
>> +             crtc->enabled_power_domains = pipe_domains[crtc->pipe];
>> +     }
>>  }
>>
>>  static void haswell_modeset_global_resources(struct drm_device *dev)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 189257d..63a5bfd 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -320,6 +320,7 @@ struct intel_crtc {
>>        * some outputs connected to this crtc.
>>        */
>>       bool active;
>> +     unsigned long enabled_power_domains;
>>       bool eld_vld;
>>       bool primary_enabled; /* is the primary plane (partially) visible? */
>>       bool lowfreq_avail;
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> Hope we can get rid of the set_power_well() function soon...
>
> --
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Imre Deak]

On Tue, 2013-10-22 at 18:07 -0200, Paulo Zanoni wrote:
> 2013/10/18 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Wed, 16 Oct 2013 17:25:52 +0300
> > Imre Deak <imre.deak@intel.com> wrote:
> >
> >> So far the modeset code enabled all power domains if it needed any. It
> >> wasn't a problem since HW generations so far only had one always-on
> >> power well and one dynamic power well that can be enabled/disabled. For
> >> domains powered by always-on power wells (panel fitter on pipe A and the
> >> eDP transcoder) we didn't do anything, for all other domains we just
> >> enabled the single dynamic power well.
> >>
> >> Future HW generations will change this, as they add multiple dynamic
> >> power wells. Support for these will be added later, this patch prepares
> >> for those by making sure we only enable the required domains.
> >>
> >> Note that after this change on HSW we'll enable all power domains even
> >> if it was the domain for the panel fitter on pipe A or the eDP
> >> transcoder. This isn't a problem since the power domain framework
> >> already checks if the domain is on an always-on power well and doesn't
> >> do anything in this case.
> >>
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> I updated drm-intel-nightly and now when I boot Haswell with eDP-only,
> the power well is enabled, where it should be disabled. Reverting this
> patch fixes the problem for me.

As discussed on IRC, this broke b/c of a missing call to
intel_display_set_init_power() in modeset_update_power_wells(). I added
it only in patch 6/6, but that isn't applied yet since I had to rework
it. I've posted a v2 for this patch already, it should fix the issue.

Thanks for catching this.

--Imre

> 
> 
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
> >>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>  2 files changed, 42 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8e734f2..e2a4f3b 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
> >>       }
> >>  }
> >>
> >> +#define for_each_power_domain(domain, mask)                          \
> >> +     for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)     \
> >> +             if ((1 << (domain)) & (mask))
> >> +
> >> +static unsigned long get_pipe_power_domains(struct drm_device *dev,
> >> +                                         enum pipe pipe, bool pfit_enabled)
> >> +{
> >> +     unsigned long mask;
> >> +     enum transcoder transcoder;
> >> +
> >> +     transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
> >> +
> >> +     mask = BIT(POWER_DOMAIN_PIPE(pipe));
> >> +     mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> >> +     if (pfit_enabled)
> >> +             mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
> >> +
> >> +     return mask;
> >> +}
> >> +
> >>  static void modeset_update_power_wells(struct drm_device *dev)
> >>  {
> >> -     bool enable = false;
> >> +     unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> >>       struct intel_crtc *crtc;
> >>
> >> +     /*
> >> +      * First get all needed power domains, then put all unneeded, to avoid
> >> +      * any unnecessary toggling of the power wells.
> >> +      */
> >>       list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> >> +             enum intel_display_power_domain domain;
> >> +
> >>               if (!crtc->base.enabled)
> >>                       continue;
> >>
> >> -             if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
> >> -                 crtc->config.cpu_transcoder != TRANSCODER_EDP)
> >> -                     enable = true;
> >> +             pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
> >> +                                             crtc->pipe,
> >> +                                             crtc->config.pch_pfit.enabled);
> >> +
> >> +             for_each_power_domain(domain, pipe_domains[crtc->pipe])
> >> +                     intel_display_power_get(dev, domain);
> >>       }
> >>
> >> -     intel_set_power_well(dev, enable);
> >> +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> >> +             enum intel_display_power_domain domain;
> >> +
> >> +             for_each_power_domain(domain, crtc->enabled_power_domains)
> >> +                     intel_display_power_put(dev, domain);
> >> +
> >> +             crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> >> +     }
> >>  }
> >>
> >>  static void haswell_modeset_global_resources(struct drm_device *dev)
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 189257d..63a5bfd 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -320,6 +320,7 @@ struct intel_crtc {
> >>        * some outputs connected to this crtc.
> >>        */
> >>       bool active;
> >> +     unsigned long enabled_power_domains;
> >>       bool eld_vld;
> >>       bool primary_enabled; /* is the primary plane (partially) visible? */
> >>       bool lowfreq_avail;
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > Hope we can get rid of the set_power_well() function soon...
> >
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

Re: [PATCH 1/2] drm/i915: prepare for multiple power wells

[Paulo Zanoni]

2013/10/22 Imre Deak <imre.deak@intel.com>:
> In the future we'll need to support multiple power wells, so prepare for
> that here. Create a new power domains struct which contains all
> power domain/well specific fields. Since we'll have one lock protecting
> all power wells, move power_well->lock to the new struct too.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
>  drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
>  2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 80957ca..7315095 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
>  /* Power well structure for haswell */
>  struct i915_power_well {
>         struct drm_device *device;

Can't we also move "device" to i915_power_domains?


> -       struct mutex lock;
>         /* power well enable/disable usage count */
>         int count;
>         int i915_request;
>  };
>
> +#define I915_MAX_POWER_WELLS 1

How about this?

enum intel_power_wells {
        HASWELL_POWER_WELL = 0, /* Or any other more creative name,
since I guess we'll have an equivalent power well on other gens */
        I915_MAX_POWER_WELLS,
};

And then you switch all the code below that uses index zero for the
actual enum name.

Maybe "NON_LPSP_POWER_WELL = 0" since the docs describe LPSP (Low
Power Single Pipe) as a feature that is enabled when the power well is
disabled. Feel free to invent better names :)


> +
> +struct i915_power_domains {
> +       struct mutex lock;
> +       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> +};
> +
>  struct i915_dri1_state {
>         unsigned allow_batchbuffer : 1;
>         u32 __iomem *gfx_hws_cpu_addr;
> @@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
>          * mchdev_lock in intel_pm.c */
>         struct intel_ilk_power_mgmt ips;
>
> -       /* Haswell power well */
> -       struct i915_power_well power_well;
> +       struct i915_power_domains power_domains;
>
>         struct i915_psr psr;
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3e140ab..09ca6f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> -       __intel_power_well_get(power_well);
> -       mutex_unlock(&power_well->lock);
> +       power_domains = &dev_priv->power_domains;
> +
> +       mutex_lock(&power_domains->lock);
> +       __intel_power_well_get(&power_domains->power_wells[0]);
> +       mutex_unlock(&power_domains->lock);
>  }
>
>  void intel_display_power_put(struct drm_device *dev,
>                              enum intel_display_power_domain domain)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
>         if (is_always_on_power_domain(dev, domain))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> -       __intel_power_well_put(power_well);
> -       mutex_unlock(&power_well->lock);
> +       power_domains = &dev_priv->power_domains;
> +
> +       mutex_lock(&power_domains->lock);
> +       __intel_power_well_put(&power_domains->power_wells[0]);
> +       mutex_unlock(&power_domains->lock);
>  }
>
> -static struct i915_power_well *hsw_pwr;
> +static struct i915_power_domains *hsw_pwr;
>
>  /* Display audio driver power well request */
>  void i915_request_power_well(void)
> @@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
>                 return;
>
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_get(hsw_pwr);
> +       __intel_power_well_get(&hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
> @@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
>                 return;
>
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_put(hsw_pwr);
> +       __intel_power_well_put(&hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
> @@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
>  int i915_init_power_well(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
> -       hsw_pwr = &dev_priv->power_well;
> +       mutex_init(&power_domains->lock);
> +       hsw_pwr = power_domains;
>
> -       hsw_pwr->device = dev;
> -       mutex_init(&hsw_pwr->lock);
> -       hsw_pwr->count = 0;
> +       power_well = &power_domains->power_wells[0];
> +       power_well->device = dev;
> +       power_well->count = 0;

How about this?

for (i = 0; i < I915_MAX_POWER_WELLS; i++) {
        power_well = &power_domains->power_wells[0];
        power_well->device = dev; /* If you don't move this to the
power_domains struct. */
        power_well->count = 0;
}

Then we'll always be safe :)


With or without my 3 bikesheds (although I would really like to see them):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

By the way, do you also plan to change some functions so they take
struct i915_power_well (or an index representing the power well
number) as a parameter? E.g., intel_set_power_well.


>
>         return 0;
>  }
> @@ -5702,7 +5709,8 @@ void i915_remove_power_well(struct drm_device *dev)
>  void intel_set_power_well(struct drm_device *dev, bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
> @@ -5710,8 +5718,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>         if (!i915_disable_power_well && !enable)
>                 return;
>
> -       mutex_lock(&power_well->lock);
> +       mutex_lock(&power_domains->lock);
>
> +       power_well = &power_domains->power_wells[0];
>         /*
>          * This function will only ever contribute one
>          * to the power well reference count. i915_request
> @@ -5729,20 +5738,24 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 __intel_power_well_put(power_well);
>
>   out:
> -       mutex_unlock(&power_well->lock);
> +       mutex_unlock(&power_domains->lock);
>  }
>
>  static void intel_resume_power_well(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_well *power_well = &dev_priv->power_well;
> +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +       struct i915_power_well *power_well;
>
>         if (!HAS_POWER_WELL(dev))
>                 return;
>
> -       mutex_lock(&power_well->lock);
> +       mutex_lock(&power_domains->lock);
> +
> +       power_well = &power_domains->power_wells[0];
>         __intel_set_power_well(dev, power_well->count > 0);
> -       mutex_unlock(&power_well->lock);
> +
> +       mutex_unlock(&power_domains->lock);
>  }
>
>  /*
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: prepare for multiple power wells

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 8602 bytes --]

On Wed, 2013-10-23 at 11:56 -0200, Paulo Zanoni wrote:
> 2013/10/22 Imre Deak <imre.deak@intel.com>:
> > In the future we'll need to support multiple power wells, so prepare for
> > that here. Create a new power domains struct which contains all
> > power domain/well specific fields. Since we'll have one lock protecting
> > all power wells, move power_well->lock to the new struct too.
> >
> > No functional change.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
> >  drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
> >  2 files changed, 42 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 80957ca..7315095 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
> >  /* Power well structure for haswell */
> >  struct i915_power_well {
> >         struct drm_device *device;
> 
> Can't we also move "device" to i915_power_domains?

Yes, that's a better place for it, especially since there is no point in
duplicating it in every instance of the power well struct. Afaics the
only real reason we need to store device is
i915_{request,release}_power_well, but those could take it from
power_domains and pass it to __intel_power_well_{get,put}. This is a
somewhat bigger/independent change, so I'd follow up with a separate
patch for it.

> > -       struct mutex lock;
> >         /* power well enable/disable usage count */
> >         int count;
> >         int i915_request;
> >  };
> >
> > +#define I915_MAX_POWER_WELLS 1
> 
> How about this?
> 
> enum intel_power_wells {
>         HASWELL_POWER_WELL = 0, /* Or any other more creative name,
> since I guess we'll have an equivalent power well on other gens */
>         I915_MAX_POWER_WELLS,
> };

I was also thinking about naming the power wells somehow. They are
different on different platforms, so the above enum would have the same
values with different names, stg like:

enum intel_power_wells {
	HASWELL_POWER_WELL = 0,

	VALLEYVIEW_POWER_WELL = 0,

	XX_POWER_WELL0 = 0,
	XX_POWER_WELL1 = 1,
	XX_POWER_WELL2 = 2,

	I915_MAX_POWER_WELLS,
};

Which would work, but is a bit messy imo. Atm I don't see the point of
having these names, since we will only reference them in terms of the
power domains they support.

> And then you switch all the code below that uses index zero for the
> actual enum name.

Yea, that's not so nice, but it would vanish as soon as we switch to
reference the power wells in the above way. I have a preliminary patch
for this see: https://github.com/ideak/linux/commit/5be8b8b4

> Maybe "NON_LPSP_POWER_WELL = 0" since the docs describe LPSP (Low
> Power Single Pipe) as a feature that is enabled when the power well is
> disabled. Feel free to invent better names :)

See above, but perhaps for debugging purposes we could add a char *name,
and make the state of power wells available through debugfs.

> > +
> > +struct i915_power_domains {
> > +       struct mutex lock;
> > +       struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
> > +};
> > +
> >  struct i915_dri1_state {
> >         unsigned allow_batchbuffer : 1;
> >         u32 __iomem *gfx_hws_cpu_addr;
> > @@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
> >          * mchdev_lock in intel_pm.c */
> >         struct intel_ilk_power_mgmt ips;
> >
> > -       /* Haswell power well */
> > -       struct i915_power_well power_well;
> > +       struct i915_power_domains power_domains;
> >
> >         struct i915_psr psr;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 3e140ab..09ca6f9 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
> >                              enum intel_display_power_domain domain)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct i915_power_well *power_well = &dev_priv->power_well;
> > +       struct i915_power_domains *power_domains;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
> >         if (is_always_on_power_domain(dev, domain))
> >                 return;
> >
> > -       mutex_lock(&power_well->lock);
> > -       __intel_power_well_get(power_well);
> > -       mutex_unlock(&power_well->lock);
> > +       power_domains = &dev_priv->power_domains;
> > +
> > +       mutex_lock(&power_domains->lock);
> > +       __intel_power_well_get(&power_domains->power_wells[0]);
> > +       mutex_unlock(&power_domains->lock);
> >  }
> >
> >  void intel_display_power_put(struct drm_device *dev,
> >                              enum intel_display_power_domain domain)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct i915_power_well *power_well = &dev_priv->power_well;
> > +       struct i915_power_domains *power_domains;
> >
> >         if (!HAS_POWER_WELL(dev))
> >                 return;
> > @@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
> >         if (is_always_on_power_domain(dev, domain))
> >                 return;
> >
> > -       mutex_lock(&power_well->lock);
> > -       __intel_power_well_put(power_well);
> > -       mutex_unlock(&power_well->lock);
> > +       power_domains = &dev_priv->power_domains;
> > +
> > +       mutex_lock(&power_domains->lock);
> > +       __intel_power_well_put(&power_domains->power_wells[0]);
> > +       mutex_unlock(&power_domains->lock);
> >  }
> >
> > -static struct i915_power_well *hsw_pwr;
> > +static struct i915_power_domains *hsw_pwr;
> >
> >  /* Display audio driver power well request */
> >  void i915_request_power_well(void)
> > @@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
> >                 return;
> >
> >         mutex_lock(&hsw_pwr->lock);
> > -       __intel_power_well_get(hsw_pwr);
> > +       __intel_power_well_get(&hsw_pwr->power_wells[0]);
> >         mutex_unlock(&hsw_pwr->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(i915_request_power_well);
> > @@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
> >                 return;
> >
> >         mutex_lock(&hsw_pwr->lock);
> > -       __intel_power_well_put(hsw_pwr);
> > +       __intel_power_well_put(&hsw_pwr->power_wells[0]);
> >         mutex_unlock(&hsw_pwr->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(i915_release_power_well);
> > @@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
> >  int i915_init_power_well(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +       struct i915_power_well *power_well;
> >
> > -       hsw_pwr = &dev_priv->power_well;
> > +       mutex_init(&power_domains->lock);
> > +       hsw_pwr = power_domains;
> >
> > -       hsw_pwr->device = dev;
> > -       mutex_init(&hsw_pwr->lock);
> > -       hsw_pwr->count = 0;
> > +       power_well = &power_domains->power_wells[0];
> > +       power_well->device = dev;
> > +       power_well->count = 0;
> 
> How about this?
> 
> for (i = 0; i < I915_MAX_POWER_WELLS; i++) {
>         power_well = &power_domains->power_wells[0];
>         power_well->device = dev; /* If you don't move this to the
> power_domains struct. */
>         power_well->count = 0;
> }
> 
> Then we'll always be safe :)

Imo, this is safe now, since we only have a single power well. But the
above preliminary patch has something similar to what you suggest, so if
that's ok I'd address it there.

> With or without my 3 bikesheds (although I would really like to see them):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> By the way, do you also plan to change some functions so they take
> struct i915_power_well (or an index representing the power well
> number) as a parameter? E.g., intel_set_power_well.

See the above patch. But basically yes, I would find the power well(s)
based on the requested power domain and call
__intel_power_well_{get,put} for each, and these would in turn call a
platform dependent .set hook if needed.

Thanks for the review,
Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH v3 0/4] prepare for multiple power wells

[Imre Deak]

This patchset replaces
"[PATCH 6/6] drm/i915: use power get/put instead of set for power on
after init" from my previous patchset. I addressed the feedback from
Daniel and Paulo (patch 1-3) and included a related cleanup (patch 4).

Imre Deak (4):
  drm/i915: prepare for multiple power wells
  drm/i915: use power get/put instead of set for power on after init
  drm/i915: remove device field from struct power_well
  drm/i915: rename i915_init_power_well to i915_init_power_domains

 drivers/gpu/drm/i915/i915_dma.c      |  10 ++--
 drivers/gpu/drm/i915/i915_drv.c      |   4 +-
 drivers/gpu/drm/i915/i915_drv.h      |  20 +++++--
 drivers/gpu/drm/i915/intel_display.c |  17 ++++++
 drivers/gpu/drm/i915/intel_drv.h     |   7 ++-
 drivers/gpu/drm/i915/intel_pm.c      | 109 ++++++++++++++++-------------------
 6 files changed, 92 insertions(+), 75 deletions(-)

-- 
1.8.4

[PATCH v3 1/4] drm/i915: prepare for multiple power wells

[Imre Deak]

In the future we'll need to support multiple power wells, so prepare for
that here. Create a new power domains struct which contains all
power domain/well specific fields. Since we'll have one lock protecting
all power wells, move power_well->lock to the new struct too.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Paulo Zanoni <paulo.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 11 ++++++---
 drivers/gpu/drm/i915/intel_pm.c | 55 +++++++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 80957ca..7315095 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -911,12 +911,18 @@ struct intel_ilk_power_mgmt {
 /* Power well structure for haswell */
 struct i915_power_well {
 	struct drm_device *device;
-	struct mutex lock;
 	/* power well enable/disable usage count */
 	int count;
 	int i915_request;
 };
 
+#define I915_MAX_POWER_WELLS 1
+
+struct i915_power_domains {
+	struct mutex lock;
+	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
+};
+
 struct i915_dri1_state {
 	unsigned allow_batchbuffer : 1;
 	u32 __iomem *gfx_hws_cpu_addr;
@@ -1412,8 +1418,7 @@ typedef struct drm_i915_private {
 	 * mchdev_lock in intel_pm.c */
 	struct intel_ilk_power_mgmt ips;
 
-	/* Haswell power well */
-	struct i915_power_well power_well;
+	struct i915_power_domains power_domains;
 
 	struct i915_psr psr;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3e140ab..09ca6f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5625,7 +5625,7 @@ void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5633,16 +5633,18 @@ void intel_display_power_get(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	mutex_lock(&power_well->lock);
-	__intel_power_well_get(power_well);
-	mutex_unlock(&power_well->lock);
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	__intel_power_well_get(&power_domains->power_wells[0]);
+	mutex_unlock(&power_domains->lock);
 }
 
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5650,12 +5652,14 @@ void intel_display_power_put(struct drm_device *dev,
 	if (is_always_on_power_domain(dev, domain))
 		return;
 
-	mutex_lock(&power_well->lock);
-	__intel_power_well_put(power_well);
-	mutex_unlock(&power_well->lock);
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	__intel_power_well_put(&power_domains->power_wells[0]);
+	mutex_unlock(&power_domains->lock);
 }
 
-static struct i915_power_well *hsw_pwr;
+static struct i915_power_domains *hsw_pwr;
 
 /* Display audio driver power well request */
 void i915_request_power_well(void)
@@ -5664,7 +5668,7 @@ void i915_request_power_well(void)
 		return;
 
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(hsw_pwr);
+	__intel_power_well_get(&hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5676,7 +5680,7 @@ void i915_release_power_well(void)
 		return;
 
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(hsw_pwr);
+	__intel_power_well_put(&hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5684,12 +5688,15 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 int i915_init_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
-	hsw_pwr = &dev_priv->power_well;
+	mutex_init(&power_domains->lock);
+	hsw_pwr = power_domains;
 
-	hsw_pwr->device = dev;
-	mutex_init(&hsw_pwr->lock);
-	hsw_pwr->count = 0;
+	power_well = &power_domains->power_wells[0];
+	power_well->device = dev;
+	power_well->count = 0;
 
 	return 0;
 }
@@ -5702,7 +5709,8 @@ void i915_remove_power_well(struct drm_device *dev)
 void intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
@@ -5710,8 +5718,9 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	if (!i915_disable_power_well && !enable)
 		return;
 
-	mutex_lock(&power_well->lock);
+	mutex_lock(&power_domains->lock);
 
+	power_well = &power_domains->power_wells[0];
 	/*
 	 * This function will only ever contribute one
 	 * to the power well reference count. i915_request
@@ -5729,20 +5738,24 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 		__intel_power_well_put(power_well);
 
  out:
-	mutex_unlock(&power_well->lock);
+	mutex_unlock(&power_domains->lock);
 }
 
 static void intel_resume_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	struct i915_power_well *power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	mutex_lock(&power_well->lock);
+	mutex_lock(&power_domains->lock);
+
+	power_well = &power_domains->power_wells[0];
 	__intel_set_power_well(dev, power_well->count > 0);
-	mutex_unlock(&power_well->lock);
+
+	mutex_unlock(&power_domains->lock);
 }
 
 /*
-- 
1.8.4

[PATCH v3 2/4] drm/i915: use power get/put instead of set for power on after init

[Imre Deak]

Currently we make sure that all power domains are enabled during driver
init and turn off unneded ones only after the first modeset. Similarly
during suspend we enable all power domains, which will remain on through
the following resume until the first modeset.

This logic is supported by intel_set_power_well() in the power domain
framework. It would be nice to simplify the API, so that we only have
get/put functions and make it more explicit on the higher level how this
"power well on during init" logic works. This will make it also easier
if in the future we want to shorten the time the power wells are on.

For this add a new device private flag tracking whether we have the
power wells on because of init/suspend and use only
intel_display_power_get()/put(). As nothing else uses
intel_set_power_well() we can remove it.

This also fixes

commit 6efdf354ddb186c6604d1692075421e8d2c740e9
Author: Imre Deak <imre.deak@intel.com>
Date:   Wed Oct 16 17:25:52 2013 +0300

    drm/i915: enable only the needed power domains during modeset

where removing intel_set_power_well() resulted in not releasing the
reference on the power well that was taken during init and thus leaving
the power well on all the time. Regression reported by Paulo.

v2:
- move the init_power_on flag to the power_domains struct (Daniel)

v3:
- add note about this being a regression fix too (Paulo)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  8 +++++++-
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 37 +-----------------------------------
 6 files changed, 28 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fd848ef..b722b35 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1710,7 +1710,7 @@ int i915_driver_unload(struct drm_device *dev)
 		/* The i915.ko module is still not prepared to be loaded when
 		 * the power well is not enabled, so just enable it in case
 		 * we're going to unload/reload. */
-		intel_set_power_well(dev, true);
+		intel_display_set_init_power(dev, true);
 		i915_remove_power_well(dev);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 85ae0dc..770c9f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -477,7 +477,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
 	hsw_disable_package_c8(dev_priv);
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7315095..3565db2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -100,6 +100,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP,
 	POWER_DOMAIN_VGA,
+	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
 };
@@ -913,12 +914,17 @@ struct i915_power_well {
 	struct drm_device *device;
 	/* power well enable/disable usage count */
 	int count;
-	int i915_request;
 };
 
 #define I915_MAX_POWER_WELLS 1
 
 struct i915_power_domains {
+	/*
+	 * Power wells needed for initialization at driver init and suspend
+	 * time are on. They are kept on until after the first modeset.
+	 */
+	bool init_power_on;
+
 	struct mutex lock;
 	struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e79a2a..0c2e83c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6573,6 +6573,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
 	return mask;
 }
 
+void intel_display_set_init_power(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (dev_priv->power_domains.init_power_on == enable)
+		return;
+
+	if (enable)
+		intel_display_power_get(dev, POWER_DOMAIN_INIT);
+	else
+		intel_display_power_put(dev, POWER_DOMAIN_INIT);
+
+	dev_priv->power_domains.init_power_on = enable;
+}
+
 static void modeset_update_power_wells(struct drm_device *dev)
 {
 	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
@@ -6604,6 +6619,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
 
 		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
 	}
+
+	intel_display_set_init_power(dev, false);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index af1553c..bf4394a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -692,6 +692,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
 void i915_disable_vga_mem(struct drm_device *dev);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
+void intel_display_set_init_power(struct drm_device *dev, bool enable);
 
 
 /* intel_dp.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 09ca6f9..4c38e28 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5706,41 +5706,6 @@ void i915_remove_power_well(struct drm_device *dev)
 	hsw_pwr = NULL;
 }
 
-void intel_set_power_well(struct drm_device *dev, bool enable)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	struct i915_power_well *power_well;
-
-	if (!HAS_POWER_WELL(dev))
-		return;
-
-	if (!i915_disable_power_well && !enable)
-		return;
-
-	mutex_lock(&power_domains->lock);
-
-	power_well = &power_domains->power_wells[0];
-	/*
-	 * This function will only ever contribute one
-	 * to the power well reference count. i915_request
-	 * is what tracks whether we have or have not
-	 * added the one to the reference count.
-	 */
-	if (power_well->i915_request == enable)
-		goto out;
-
-	power_well->i915_request = enable;
-
-	if (enable)
-		__intel_power_well_get(power_well);
-	else
-		__intel_power_well_put(power_well);
-
- out:
-	mutex_unlock(&power_domains->lock);
-}
-
 static void intel_resume_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5772,7 +5737,7 @@ void intel_init_power_well(struct drm_device *dev)
 		return;
 
 	/* For now, we need the power well to be always enabled. */
-	intel_set_power_well(dev, true);
+	intel_display_set_init_power(dev, true);
 	intel_resume_power_well(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
-- 
1.8.4

[PATCH v3 3/4] drm/i915: remove device field from struct power_well

[Imre Deak]

The only real need for this field was in
i915_{request,release}_power_well, but there we can get at it by a
container_of magic. Also since in the future we'll have multiple power
wells each with its own power_well struct it makes sense to remove the
field from there where it'd be just redundancy.

Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 -
 drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++---------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3565db2..2731fbb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -911,7 +911,6 @@ struct intel_ilk_power_mgmt {
 
 /* Power well structure for haswell */
 struct i915_power_well {
-	struct drm_device *device;
 	/* power well enable/disable usage count */
 	int count;
 };
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4c38e28..4f84a4b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5608,17 +5608,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
-static void __intel_power_well_get(struct i915_power_well *power_well)
+static void __intel_power_well_get(struct drm_device *dev,
+				   struct i915_power_well *power_well)
 {
 	if (!power_well->count++)
-		__intel_set_power_well(power_well->device, true);
+		__intel_set_power_well(dev, true);
 }
 
-static void __intel_power_well_put(struct i915_power_well *power_well)
+static void __intel_power_well_put(struct drm_device *dev,
+				   struct i915_power_well *power_well)
 {
 	WARN_ON(!power_well->count);
 	if (!--power_well->count)
-		__intel_set_power_well(power_well->device, false);
+		__intel_set_power_well(dev, false);
 }
 
 void intel_display_power_get(struct drm_device *dev,
@@ -5636,7 +5638,7 @@ void intel_display_power_get(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_get(&power_domains->power_wells[0]);
+	__intel_power_well_get(dev, &power_domains->power_wells[0]);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5655,7 +5657,7 @@ void intel_display_power_put(struct drm_device *dev,
 	power_domains = &dev_priv->power_domains;
 
 	mutex_lock(&power_domains->lock);
-	__intel_power_well_put(&power_domains->power_wells[0]);
+	__intel_power_well_put(dev, &power_domains->power_wells[0]);
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5664,11 +5666,16 @@ static struct i915_power_domains *hsw_pwr;
 /* Display audio driver power well request */
 void i915_request_power_well(void)
 {
+	struct drm_i915_private *dev_priv;
+
 	if (WARN_ON(!hsw_pwr))
 		return;
 
+	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
+				power_domains);
+
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_get(&hsw_pwr->power_wells[0]);
+	__intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5676,11 +5683,16 @@ EXPORT_SYMBOL_GPL(i915_request_power_well);
 /* Display audio driver power well release */
 void i915_release_power_well(void)
 {
+	struct drm_i915_private *dev_priv;
+
 	if (WARN_ON(!hsw_pwr))
 		return;
 
+	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
+				power_domains);
+
 	mutex_lock(&hsw_pwr->lock);
-	__intel_power_well_put(&hsw_pwr->power_wells[0]);
+	__intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
 	mutex_unlock(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5695,7 +5707,6 @@ int i915_init_power_well(struct drm_device *dev)
 	hsw_pwr = power_domains;
 
 	power_well = &power_domains->power_wells[0];
-	power_well->device = dev;
 	power_well->count = 0;
 
 	return 0;
-- 
1.8.4

[PATCH v3 4/4] drm/i915: rename i915_init_power_well to i915_init_power_domains

[Imre Deak]

Similarly rename the other related functions in the power domain
interface.

Higher level driver code calling these functions knows only about power
domains, not the underlying power wells which may be different on
different platforms. Also these functions really init/cleanup/resume
power domains and only through that all related power wells, so rename
them accordingly.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.c  |  2 +-
 drivers/gpu/drm/i915/intel_drv.h |  6 +++---
 drivers/gpu/drm/i915/intel_pm.c  | 10 +++++-----
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b722b35..75f02e4 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_gem_stolen;
 
-	intel_init_power_well(dev);
+	intel_init_power_domains(dev);
 
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
@@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	if (HAS_POWER_WELL(dev))
-		i915_init_power_well(dev);
+		i915_init_power_domains(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
@@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 out_power_well:
 	if (HAS_POWER_WELL(dev))
-		i915_remove_power_well(dev);
+		i915_remove_power_domains(dev);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
@@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev)
 		 * the power well is not enabled, so just enable it in case
 		 * we're going to unload/reload. */
 		intel_display_set_init_power(dev, true);
-		i915_remove_power_well(dev);
+		i915_remove_power_domains(dev);
 	}
 
 	i915_teardown_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 770c9f8..e4474c6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_init_power_well(dev);
+	intel_init_power_domains(dev);
 
 	i915_restore_state(dev);
 	intel_opregion_setup(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf4394a..ae6d362 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
-int i915_init_power_well(struct drm_device *dev);
-void i915_remove_power_well(struct drm_device *dev);
+int i915_init_power_domains(struct drm_device *dev);
+void i915_remove_power_domains(struct drm_device *dev);
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
-void intel_init_power_well(struct drm_device *dev);
+void intel_init_power_domains(struct drm_device *dev);
 void intel_set_power_well(struct drm_device *dev, bool enable);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f84a4b..9afec23 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5697,7 +5697,7 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
-int i915_init_power_well(struct drm_device *dev)
+int i915_init_power_domains(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
@@ -5712,12 +5712,12 @@ int i915_init_power_well(struct drm_device *dev)
 	return 0;
 }
 
-void i915_remove_power_well(struct drm_device *dev)
+void i915_remove_power_domains(struct drm_device *dev)
 {
 	hsw_pwr = NULL;
 }
 
-static void intel_resume_power_well(struct drm_device *dev)
+static void intel_resume_power_domains(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
@@ -5740,7 +5740,7 @@ static void intel_resume_power_well(struct drm_device *dev)
  * to be enabled, and it will only be disabled if none of the registers is
  * requesting it to be enabled.
  */
-void intel_init_power_well(struct drm_device *dev)
+void intel_init_power_domains(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -5749,7 +5749,7 @@ void intel_init_power_well(struct drm_device *dev)
 
 	/* For now, we need the power well to be always enabled. */
 	intel_display_set_init_power(dev, true);
-	intel_resume_power_well(dev);
+	intel_resume_power_domains(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
-- 
1.8.4

Re: [PATCH v3 2/4] drm/i915: use power get/put instead of set for power on after init

[Paulo Zanoni]

2013/10/25 Imre Deak <imre.deak@intel.com>:
> Currently we make sure that all power domains are enabled during driver
> init and turn off unneded ones only after the first modeset. Similarly
> during suspend we enable all power domains, which will remain on through
> the following resume until the first modeset.
>
> This logic is supported by intel_set_power_well() in the power domain
> framework. It would be nice to simplify the API, so that we only have
> get/put functions and make it more explicit on the higher level how this
> "power well on during init" logic works. This will make it also easier
> if in the future we want to shorten the time the power wells are on.
>
> For this add a new device private flag tracking whether we have the
> power wells on because of init/suspend and use only
> intel_display_power_get()/put(). As nothing else uses
> intel_set_power_well() we can remove it.
>
> This also fixes
>
> commit 6efdf354ddb186c6604d1692075421e8d2c740e9
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Wed Oct 16 17:25:52 2013 +0300
>
>     drm/i915: enable only the needed power domains during modeset
>
> where removing intel_set_power_well() resulted in not releasing the
> reference on the power well that was taken during init and thus leaving
> the power well on all the time. Regression reported by Paulo.
>
> v2:
> - move the init_power_on flag to the power_domains struct (Daniel)
>
> v3:
> - add note about this being a regression fix too (Paulo)

I didn't closely follow the first 5 patches, but this one makes sense.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c      |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  8 +++++++-
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 37 +-----------------------------------
>  6 files changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index fd848ef..b722b35 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1710,7 +1710,7 @@ int i915_driver_unload(struct drm_device *dev)
>                 /* The i915.ko module is still not prepared to be loaded when
>                  * the power well is not enabled, so just enable it in case
>                  * we're going to unload/reload. */
> -               intel_set_power_well(dev, true);
> +               intel_display_set_init_power(dev, true);
>                 i915_remove_power_well(dev);
>         }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 85ae0dc..770c9f8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -477,7 +477,7 @@ static int i915_drm_freeze(struct drm_device *dev)
>         /* We do a lot of poking in a lot of registers, make sure they work
>          * properly. */
>         hsw_disable_package_c8(dev_priv);
> -       intel_set_power_well(dev, true);
> +       intel_display_set_init_power(dev, true);
>
>         drm_kms_helper_poll_disable(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7315095..3565db2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -100,6 +100,7 @@ enum intel_display_power_domain {
>         POWER_DOMAIN_TRANSCODER_C,
>         POWER_DOMAIN_TRANSCODER_EDP,
>         POWER_DOMAIN_VGA,
> +       POWER_DOMAIN_INIT,
>
>         POWER_DOMAIN_NUM,
>  };
> @@ -913,12 +914,17 @@ struct i915_power_well {
>         struct drm_device *device;
>         /* power well enable/disable usage count */
>         int count;
> -       int i915_request;
>  };
>
>  #define I915_MAX_POWER_WELLS 1
>
>  struct i915_power_domains {
> +       /*
> +        * Power wells needed for initialization at driver init and suspend
> +        * time are on. They are kept on until after the first modeset.
> +        */
> +       bool init_power_on;
> +
>         struct mutex lock;
>         struct i915_power_well power_wells[I915_MAX_POWER_WELLS];
>  };
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3e79a2a..0c2e83c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6573,6 +6573,21 @@ static unsigned long get_pipe_power_domains(struct drm_device *dev,
>         return mask;
>  }
>
> +void intel_display_set_init_power(struct drm_device *dev, bool enable)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (dev_priv->power_domains.init_power_on == enable)
> +               return;
> +
> +       if (enable)
> +               intel_display_power_get(dev, POWER_DOMAIN_INIT);
> +       else
> +               intel_display_power_put(dev, POWER_DOMAIN_INIT);
> +
> +       dev_priv->power_domains.init_power_on = enable;
> +}
> +
>  static void modeset_update_power_wells(struct drm_device *dev)
>  {
>         unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> @@ -6604,6 +6619,8 @@ static void modeset_update_power_wells(struct drm_device *dev)
>
>                 crtc->enabled_power_domains = pipe_domains[crtc->pipe];
>         }
> +
> +       intel_display_set_init_power(dev, false);
>  }
>
>  static void haswell_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index af1553c..bf4394a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -692,6 +692,7 @@ bool intel_crtc_active(struct drm_crtc *crtc);
>  void i915_disable_vga_mem(struct drm_device *dev);
>  void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
> +void intel_display_set_init_power(struct drm_device *dev, bool enable);
>
>
>  /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 09ca6f9..4c38e28 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5706,41 +5706,6 @@ void i915_remove_power_well(struct drm_device *dev)
>         hsw_pwr = NULL;
>  }
>
> -void intel_set_power_well(struct drm_device *dev, bool enable)
> -{
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct i915_power_domains *power_domains = &dev_priv->power_domains;
> -       struct i915_power_well *power_well;
> -
> -       if (!HAS_POWER_WELL(dev))
> -               return;
> -
> -       if (!i915_disable_power_well && !enable)
> -               return;
> -
> -       mutex_lock(&power_domains->lock);
> -
> -       power_well = &power_domains->power_wells[0];
> -       /*
> -        * This function will only ever contribute one
> -        * to the power well reference count. i915_request
> -        * is what tracks whether we have or have not
> -        * added the one to the reference count.
> -        */
> -       if (power_well->i915_request == enable)
> -               goto out;
> -
> -       power_well->i915_request = enable;
> -
> -       if (enable)
> -               __intel_power_well_get(power_well);
> -       else
> -               __intel_power_well_put(power_well);
> -
> - out:
> -       mutex_unlock(&power_domains->lock);
> -}
> -
>  static void intel_resume_power_well(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5772,7 +5737,7 @@ void intel_init_power_well(struct drm_device *dev)
>                 return;
>
>         /* For now, we need the power well to be always enabled. */
> -       intel_set_power_well(dev, true);
> +       intel_display_set_init_power(dev, true);
>         intel_resume_power_well(dev);
>
>         /* We're taking over the BIOS, so clear any requests made by it since
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v3 3/4] drm/i915: remove device field from struct power_well

[Paulo Zanoni]

2013/10/25 Imre Deak <imre.deak@intel.com>:
> The only real need for this field was in
> i915_{request,release}_power_well, but there we can get at it by a
> container_of magic. Also since in the future we'll have multiple power
> wells each with its own power_well struct it makes sense to remove the
> field from there where it'd be just redundancy.
>
> Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com>

My original idea was to just move it from i915_power_well to
i915_power_domains, so hsw_pwr (which is the new external static
thing) would still have a pointer to our driver. This way we wouldn't
need the container_of magic. But your solution works too, and saves
4/8 bytes :)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++++++++++++++---------
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3565db2..2731fbb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -911,7 +911,6 @@ struct intel_ilk_power_mgmt {
>
>  /* Power well structure for haswell */
>  struct i915_power_well {
> -       struct drm_device *device;
>         /* power well enable/disable usage count */
>         int count;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4c38e28..4f84a4b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5608,17 +5608,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>         }
>  }
>
> -static void __intel_power_well_get(struct i915_power_well *power_well)
> +static void __intel_power_well_get(struct drm_device *dev,
> +                                  struct i915_power_well *power_well)
>  {
>         if (!power_well->count++)
> -               __intel_set_power_well(power_well->device, true);
> +               __intel_set_power_well(dev, true);
>  }
>
> -static void __intel_power_well_put(struct i915_power_well *power_well)
> +static void __intel_power_well_put(struct drm_device *dev,
> +                                  struct i915_power_well *power_well)
>  {
>         WARN_ON(!power_well->count);
>         if (!--power_well->count)
> -               __intel_set_power_well(power_well->device, false);
> +               __intel_set_power_well(dev, false);
>  }
>
>  void intel_display_power_get(struct drm_device *dev,
> @@ -5636,7 +5638,7 @@ void intel_display_power_get(struct drm_device *dev,
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> -       __intel_power_well_get(&power_domains->power_wells[0]);
> +       __intel_power_well_get(dev, &power_domains->power_wells[0]);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5655,7 +5657,7 @@ void intel_display_power_put(struct drm_device *dev,
>         power_domains = &dev_priv->power_domains;
>
>         mutex_lock(&power_domains->lock);
> -       __intel_power_well_put(&power_domains->power_wells[0]);
> +       __intel_power_well_put(dev, &power_domains->power_wells[0]);
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5664,11 +5666,16 @@ static struct i915_power_domains *hsw_pwr;
>  /* Display audio driver power well request */
>  void i915_request_power_well(void)
>  {
> +       struct drm_i915_private *dev_priv;
> +
>         if (WARN_ON(!hsw_pwr))
>                 return;
>
> +       dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> +                               power_domains);
> +
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_get(&hsw_pwr->power_wells[0]);
> +       __intel_power_well_get(dev_priv->dev, &hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
> @@ -5676,11 +5683,16 @@ EXPORT_SYMBOL_GPL(i915_request_power_well);
>  /* Display audio driver power well release */
>  void i915_release_power_well(void)
>  {
> +       struct drm_i915_private *dev_priv;
> +
>         if (WARN_ON(!hsw_pwr))
>                 return;
>
> +       dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> +                               power_domains);
> +
>         mutex_lock(&hsw_pwr->lock);
> -       __intel_power_well_put(&hsw_pwr->power_wells[0]);
> +       __intel_power_well_put(dev_priv->dev, &hsw_pwr->power_wells[0]);
>         mutex_unlock(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
> @@ -5695,7 +5707,6 @@ int i915_init_power_well(struct drm_device *dev)
>         hsw_pwr = power_domains;
>
>         power_well = &power_domains->power_wells[0];
> -       power_well->device = dev;
>         power_well->count = 0;
>
>         return 0;
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v3 4/4] drm/i915: rename i915_init_power_well to i915_init_power_domains

[Paulo Zanoni]

2013/10/25 Imre Deak <imre.deak@intel.com>:
> Similarly rename the other related functions in the power domain
> interface.
>
> Higher level driver code calling these functions knows only about power
> domains, not the underlying power wells which may be different on
> different platforms. Also these functions really init/cleanup/resume
> power domains and only through that all related power wells, so rename
> them accordingly.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I agree with the "_domains" rename, I think it's an improvement, but
since you're already renaming things, I have to drop my bikeshed: we
have intel_init_power_{well,domains} and
i915_init_power_{well,domains}. IMHO this is really super annoyingly
confusing, because they sound like they do the same thing. I know it's
not your fault, but while you're at it, could you please propose names
to unconfuse this?

i915_init_power_well takes care of initializing the structs and
pointers, while intel_init_power_well is the only one that touches
hardware. A possible suggestion:

- i915_init_power_well becomes intel_init_power_domains (just because
I don't like the "i915_" prefix, since the PM code uses "intel_" for
everything).
- i915_remove_power_well becomes intel_remove_power_domains (to match
the one above)
- intel_init_power_well becomes intel_init_power_domains_hardware or
intel_init_power_wells (since on the HW these things are actually
called "power wells") or intel_init_hw_power_wells (to combine both
suggestions)

Thanks,
Paulo

> ---
>  drivers/gpu/drm/i915/i915_dma.c  |  8 ++++----
>  drivers/gpu/drm/i915/i915_drv.c  |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h |  6 +++---
>  drivers/gpu/drm/i915/intel_pm.c  | 10 +++++-----
>  4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b722b35..75f02e4 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>         if (ret)
>                 goto cleanup_gem_stolen;
>
> -       intel_init_power_well(dev);
> +       intel_init_power_domains(dev);
>
>         /* Important: The output setup functions called by modeset_init need
>          * working irqs for e.g. gmbus and dp aux transfers. */
> @@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         }
>
>         if (HAS_POWER_WELL(dev))
> -               i915_init_power_well(dev);
> +               i915_init_power_domains(dev);
>
>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>                 ret = i915_load_modeset_init(dev);
> @@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
>  out_power_well:
>         if (HAS_POWER_WELL(dev))
> -               i915_remove_power_well(dev);
> +               i915_remove_power_domains(dev);
>         drm_vblank_cleanup(dev);
>  out_gem_unload:
>         if (dev_priv->mm.inactive_shrinker.scan_objects)
> @@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev)
>                  * the power well is not enabled, so just enable it in case
>                  * we're going to unload/reload. */
>                 intel_display_set_init_power(dev, true);
> -               i915_remove_power_well(dev);
> +               i915_remove_power_domains(dev);
>         }
>
>         i915_teardown_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 770c9f8..e4474c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>                 mutex_unlock(&dev->struct_mutex);
>         }
>
> -       intel_init_power_well(dev);
> +       intel_init_power_domains(dev);
>
>         i915_restore_state(dev);
>         intel_opregion_setup(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf4394a..ae6d362 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev);
>  void intel_update_fbc(struct drm_device *dev);
>  void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  void intel_gpu_ips_teardown(void);
> -int i915_init_power_well(struct drm_device *dev);
> -void i915_remove_power_well(struct drm_device *dev);
> +int i915_init_power_domains(struct drm_device *dev);
> +void i915_remove_power_domains(struct drm_device *dev);
>  bool intel_display_power_enabled(struct drm_device *dev,
>                                  enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_device *dev,
>                              enum intel_display_power_domain domain);
> -void intel_init_power_well(struct drm_device *dev);
> +void intel_init_power_domains(struct drm_device *dev);
>  void intel_set_power_well(struct drm_device *dev, bool enable);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f84a4b..9afec23 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5697,7 +5697,7 @@ void i915_release_power_well(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> -int i915_init_power_well(struct drm_device *dev)
> +int i915_init_power_domains(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> @@ -5712,12 +5712,12 @@ int i915_init_power_well(struct drm_device *dev)
>         return 0;
>  }
>
> -void i915_remove_power_well(struct drm_device *dev)
> +void i915_remove_power_domains(struct drm_device *dev)
>  {
>         hsw_pwr = NULL;
>  }
>
> -static void intel_resume_power_well(struct drm_device *dev)
> +static void intel_resume_power_domains(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> @@ -5740,7 +5740,7 @@ static void intel_resume_power_well(struct drm_device *dev)
>   * to be enabled, and it will only be disabled if none of the registers is
>   * requesting it to be enabled.
>   */
> -void intel_init_power_well(struct drm_device *dev)
> +void intel_init_power_domains(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -5749,7 +5749,7 @@ void intel_init_power_well(struct drm_device *dev)
>
>         /* For now, we need the power well to be always enabled. */
>         intel_display_set_init_power(dev, true);
> -       intel_resume_power_well(dev);
> +       intel_resume_power_domains(dev);
>
>         /* We're taking over the BIOS, so clear any requests made by it since
>          * the driver is in charge now. */
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v3 4/4] drm/i915: rename i915_init_power_well to i915_init_power_domains

[Daniel Vetter]

On Fri, Oct 25, 2013 at 06:10:31PM -0200, Paulo Zanoni wrote:
> 2013/10/25 Imre Deak <imre.deak@intel.com>:
> > Similarly rename the other related functions in the power domain
> > interface.
> >
> > Higher level driver code calling these functions knows only about power
> > domains, not the underlying power wells which may be different on
> > different platforms. Also these functions really init/cleanup/resume
> > power domains and only through that all related power wells, so rename
> > them accordingly.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> I agree with the "_domains" rename, I think it's an improvement, but
> since you're already renaming things, I have to drop my bikeshed: we
> have intel_init_power_{well,domains} and
> i915_init_power_{well,domains}. IMHO this is really super annoyingly
> confusing, because they sound like they do the same thing. I know it's
> not your fault, but while you're at it, could you please propose names
> to unconfuse this?
> 
> i915_init_power_well takes care of initializing the structs and
> pointers, while intel_init_power_well is the only one that touches
> hardware. A possible suggestion:
> 
> - i915_init_power_well becomes intel_init_power_domains (just because
> I don't like the "i915_" prefix, since the PM code uses "intel_" for
> everything).
> - i915_remove_power_well becomes intel_remove_power_domains (to match
> the one above)
> - intel_init_power_well becomes intel_init_power_domains_hardware or
> intel_init_power_wells (since on the HW these things are actually
> called "power wells") or intel_init_hw_power_wells (to combine both
> suggestions)

The pattern we have thus far is $prefix_$block_init and
$prefix_$block_init_hw. Occasionally the $block is at the end, but for
cases where we have both an init and an init_hw function I think putting
the init[_hw] at the end to make it stick out more is better.

Just my 2 bikesheds ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v3 3/4] drm/i915: remove device field from struct power_well

[Daniel Vetter]

On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote:
> 2013/10/25 Imre Deak <imre.deak@intel.com>:
> > The only real need for this field was in
> > i915_{request,release}_power_well, but there we can get at it by a
> > container_of magic. Also since in the future we'll have multiple power
> > wells each with its own power_well struct it makes sense to remove the
> > field from there where it'd be just redundancy.
> >
> > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com>
> 
> My original idea was to just move it from i915_power_well to
> i915_power_domains, so hsw_pwr (which is the new external static
> thing) would still have a pointer to our driver. This way we wouldn't
> need the container_of magic. But your solution works too, and saves
> 4/8 bytes :)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

First 3 patches merged, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v4] drm/i915: rename i915_init_power_well to init_power_domains_init

[Imre Deak]

Similarly rename the other related functions in the power domain
interface.

Higher level driver code calling these functions knows only about power
domains, not the underlying power wells which may be different on
different platforms. Also these functions really init/cleanup/resume
power domains and only through that all related power wells, so rename
them accordingly.

Note that I left i915_{request,release}_power_well as is, since that
really changes the state only of a single power well (and is HSW
specific). It should also get a better name once we make it more
generic by controlling things through a new audio power domain.

v4:
- use intel prefix instead of i915 everywhere (Paulo)
- use a $prefix_$block_$action format (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.c  |  2 +-
 drivers/gpu/drm/i915/intel_drv.h |  6 +++---
 drivers/gpu/drm/i915/intel_pm.c  | 10 +++++-----
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b722b35..0cab2d0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_gem_stolen;
 
-	intel_init_power_well(dev);
+	intel_power_domains_init_hw(dev);
 
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
@@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	if (HAS_POWER_WELL(dev))
-		i915_init_power_well(dev);
+		intel_power_domains_init(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		ret = i915_load_modeset_init(dev);
@@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 out_power_well:
 	if (HAS_POWER_WELL(dev))
-		i915_remove_power_well(dev);
+		intel_power_domains_remove(dev);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
 	if (dev_priv->mm.inactive_shrinker.scan_objects)
@@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev)
 		 * the power well is not enabled, so just enable it in case
 		 * we're going to unload/reload. */
 		intel_display_set_init_power(dev, true);
-		i915_remove_power_well(dev);
+		intel_power_domains_remove(dev);
 	}
 
 	i915_teardown_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 770c9f8..a0804fa 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	intel_init_power_well(dev);
+	intel_power_domains_init_hw(dev);
 
 	i915_restore_state(dev);
 	intel_opregion_setup(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf4394a..9d2624f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
-int i915_init_power_well(struct drm_device *dev);
-void i915_remove_power_well(struct drm_device *dev);
+int intel_power_domains_init(struct drm_device *dev);
+void intel_power_domains_remove(struct drm_device *dev);
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
-void intel_init_power_well(struct drm_device *dev);
+void intel_power_domains_init_hw(struct drm_device *dev);
 void intel_set_power_well(struct drm_device *dev, bool enable);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d2a640c..a0c907f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5677,7 +5677,7 @@ void i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
-int i915_init_power_well(struct drm_device *dev)
+int intel_power_domains_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
@@ -5692,12 +5692,12 @@ int i915_init_power_well(struct drm_device *dev)
 	return 0;
 }
 
-void i915_remove_power_well(struct drm_device *dev)
+void intel_power_domains_remove(struct drm_device *dev)
 {
 	hsw_pwr = NULL;
 }
 
-static void intel_resume_power_well(struct drm_device *dev)
+static void intel_power_domains_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
@@ -5720,7 +5720,7 @@ static void intel_resume_power_well(struct drm_device *dev)
  * to be enabled, and it will only be disabled if none of the registers is
  * requesting it to be enabled.
  */
-void intel_init_power_well(struct drm_device *dev)
+void intel_power_domains_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -5729,7 +5729,7 @@ void intel_init_power_well(struct drm_device *dev)
 
 	/* For now, we need the power well to be always enabled. */
 	intel_display_set_init_power(dev, true);
-	intel_resume_power_well(dev);
+	intel_power_domains_resume(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
-- 
1.8.4

Re: [PATCH v3 3/4] drm/i915: remove device field from struct power_well

[Paulo Zanoni]

2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote:
>> 2013/10/25 Imre Deak <imre.deak@intel.com>:
>> > The only real need for this field was in
>> > i915_{request,release}_power_well, but there we can get at it by a
>> > container_of magic. Also since in the future we'll have multiple power
>> > wells each with its own power_well struct it makes sense to remove the
>> > field from there where it'd be just redundancy.
>> >
>> > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com>
>>
>> My original idea was to just move it from i915_power_well to
>> i915_power_domains, so hsw_pwr (which is the new external static
>> thing) would still have a pointer to our driver. This way we wouldn't
>> need the container_of magic. But your solution works too, and saves
>> 4/8 bytes :)
>>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> First 3 patches merged, thanks.

I just realized that at some point we accidentally killed the
i915.disable_power_well option... I see the i915_disable_power_well
variable is not being used anywhere. Imre, can you please investigate
that?

Thanks,
Paulo


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH v3 3/4] drm/i915: remove device field from struct power_well

[Imre Deak]

On Mon, 2013-10-28 at 15:41 -0200, Paulo Zanoni wrote:
> 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Oct 25, 2013 at 05:50:18PM -0200, Paulo Zanoni wrote:
> >> 2013/10/25 Imre Deak <imre.deak@intel.com>:
> >> > The only real need for this field was in
> >> > i915_{request,release}_power_well, but there we can get at it by a
> >> > container_of magic. Also since in the future we'll have multiple power
> >> > wells each with its own power_well struct it makes sense to remove the
> >> > field from there where it'd be just redundancy.
> >> >
> >> > Suggested-by: Paulo Zanoni <paulo.zanoni@intel.com>
> >>
> >> My original idea was to just move it from i915_power_well to
> >> i915_power_domains, so hsw_pwr (which is the new external static
> >> thing) would still have a pointer to our driver. This way we wouldn't
> >> need the container_of magic. But your solution works too, and saves
> >> 4/8 bytes :)
> >>
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > First 3 patches merged, thanks.
> 
> I just realized that at some point we accidentally killed the
> i915.disable_power_well option... I see the i915_disable_power_well
> variable is not being used anywhere. Imre, can you please investigate
> that?

Yep, that got removed by mistake in

commit 6efdf354ddb186c6604d1692075421e8d2c740e9
Author: Imre Deak <imre.deak@intel.com>
Date:   Wed Oct 16 17:25:52 2013 +0300

    drm/i915: enable only the needed power domains during modeset

I'll follow up with a fix.

--Imre

Re: [PATCH v4] drm/i915: rename i915_init_power_well to init_power_domains_init

[Paulo Zanoni]

2013/10/28 Imre Deak <imre.deak@intel.com>:
> Similarly rename the other related functions in the power domain
> interface.
>
> Higher level driver code calling these functions knows only about power
> domains, not the underlying power wells which may be different on
> different platforms. Also these functions really init/cleanup/resume
> power domains and only through that all related power wells, so rename
> them accordingly.
>
> Note that I left i915_{request,release}_power_well as is, since that
> really changes the state only of a single power well (and is HSW
> specific). It should also get a better name once we make it more
> generic by controlling things through a new audio power domain.
>
> v4:
> - use intel prefix instead of i915 everywhere (Paulo)
> - use a $prefix_$block_$action format (Daniel)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c  |  8 ++++----
>  drivers/gpu/drm/i915/i915_drv.c  |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h |  6 +++---
>  drivers/gpu/drm/i915/intel_pm.c  | 10 +++++-----
>  4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b722b35..0cab2d0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1311,7 +1311,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>         if (ret)
>                 goto cleanup_gem_stolen;
>
> -       intel_init_power_well(dev);
> +       intel_power_domains_init_hw(dev);
>
>         /* Important: The output setup functions called by modeset_init need
>          * working irqs for e.g. gmbus and dp aux transfers. */
> @@ -1640,7 +1640,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         }
>
>         if (HAS_POWER_WELL(dev))
> -               i915_init_power_well(dev);
> +               intel_power_domains_init(dev);
>
>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>                 ret = i915_load_modeset_init(dev);
> @@ -1668,7 +1668,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
>  out_power_well:
>         if (HAS_POWER_WELL(dev))
> -               i915_remove_power_well(dev);
> +               intel_power_domains_remove(dev);
>         drm_vblank_cleanup(dev);
>  out_gem_unload:
>         if (dev_priv->mm.inactive_shrinker.scan_objects)
> @@ -1711,7 +1711,7 @@ int i915_driver_unload(struct drm_device *dev)
>                  * the power well is not enabled, so just enable it in case
>                  * we're going to unload/reload. */
>                 intel_display_set_init_power(dev, true);
> -               i915_remove_power_well(dev);
> +               intel_power_domains_remove(dev);
>         }
>
>         i915_teardown_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 770c9f8..a0804fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -597,7 +597,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>                 mutex_unlock(&dev->struct_mutex);
>         }
>
> -       intel_init_power_well(dev);
> +       intel_power_domains_init_hw(dev);
>
>         i915_restore_state(dev);
>         intel_opregion_setup(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf4394a..9d2624f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -823,15 +823,15 @@ bool intel_fbc_enabled(struct drm_device *dev);
>  void intel_update_fbc(struct drm_device *dev);
>  void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  void intel_gpu_ips_teardown(void);
> -int i915_init_power_well(struct drm_device *dev);
> -void i915_remove_power_well(struct drm_device *dev);
> +int intel_power_domains_init(struct drm_device *dev);
> +void intel_power_domains_remove(struct drm_device *dev);
>  bool intel_display_power_enabled(struct drm_device *dev,
>                                  enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_device *dev,
>                              enum intel_display_power_domain domain);
> -void intel_init_power_well(struct drm_device *dev);
> +void intel_power_domains_init_hw(struct drm_device *dev);
>  void intel_set_power_well(struct drm_device *dev, bool enable);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d2a640c..a0c907f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5677,7 +5677,7 @@ void i915_release_power_well(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>
> -int i915_init_power_well(struct drm_device *dev)
> +int intel_power_domains_init(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> @@ -5692,12 +5692,12 @@ int i915_init_power_well(struct drm_device *dev)
>         return 0;
>  }
>
> -void i915_remove_power_well(struct drm_device *dev)
> +void intel_power_domains_remove(struct drm_device *dev)
>  {
>         hsw_pwr = NULL;
>  }
>
> -static void intel_resume_power_well(struct drm_device *dev)
> +static void intel_power_domains_resume(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct i915_power_domains *power_domains = &dev_priv->power_domains;
> @@ -5720,7 +5720,7 @@ static void intel_resume_power_well(struct drm_device *dev)
>   * to be enabled, and it will only be disabled if none of the registers is
>   * requesting it to be enabled.
>   */
> -void intel_init_power_well(struct drm_device *dev)
> +void intel_power_domains_init_hw(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -5729,7 +5729,7 @@ void intel_init_power_well(struct drm_device *dev)
>
>         /* For now, we need the power well to be always enabled. */
>         intel_display_set_init_power(dev, true);
> -       intel_resume_power_well(dev);
> +       intel_power_domains_resume(dev);
>
>         /* We're taking over the BIOS, so clear any requests made by it since
>          * the driver is in charge now. */
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v4] drm/i915: rename i915_init_power_well to init_power_domains_init

[Daniel Vetter]

On Mon, Oct 28, 2013 at 04:51:52PM -0200, Paulo Zanoni wrote:
> 2013/10/28 Imre Deak <imre.deak@intel.com>:
> > Similarly rename the other related functions in the power domain
> > interface.
> >
> > Higher level driver code calling these functions knows only about power
> > domains, not the underlying power wells which may be different on
> > different platforms. Also these functions really init/cleanup/resume
> > power domains and only through that all related power wells, so rename
> > them accordingly.
> >
> > Note that I left i915_{request,release}_power_well as is, since that
> > really changes the state only of a single power well (and is HSW
> > specific). It should also get a better name once we make it more
> > generic by controlling things through a new audio power domain.
> >
> > v4:
> > - use intel prefix instead of i915 everywhere (Paulo)
> > - use a $prefix_$block_$action format (Daniel)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch