[PATCH 0/4] Fix SKL runtime PM

[PATCH 0/4] Fix SKL runtime PM

[Paulo Zanoni]

Hello

So I discovered that SKL runtime PM doesn't work on drm-intel-nightly and
decided to investigate why. I found this patch in one of Damien's trees and it
fixed the problem I was seeing. I really don't know why the patches were not
sent to the list yet and I can't ask him since he's on vacation. So I decided to
review them, implement my own bikesheds, add R-B tags and submit to the list,
since it's an important feature that's broken and preliminary_hw_support was
removed for SKL.

I hope the reason he didn't submit this is not because there's some bug I failed
to spot. Anyway, he'll tell us in a few days :).

Also notice that we're not passing every pm_rpm subtest now: we're just not
failing 100% of the tests.

Thanks,
Paulo

Damien Lespiau (3):
  drm/i915: Extract a intel_power_well_enable() function
  drm/i915: Extract a intel_power_well_disable() function
  drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini
    sequences

Paulo Zanoni (1):
  drm/i915/skl: send opregion_nofify_adapter(PCI_D1) instead of PCI_D3

 drivers/gpu/drm/i915/i915_drv.c         | 20 +++++------
 drivers/gpu/drm/i915/intel_ddi.c        |  2 --
 drivers/gpu/drm/i915/intel_display.c    |  5 +--
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 59 +++++++++++++++++++++++++++------
 5 files changed, 63 insertions(+), 25 deletions(-)

-- 
2.4.6

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

[PATCH 1/4] drm/i915: Extract a intel_power_well_enable() function

[Paulo Zanoni]

From: Damien Lespiau <damien.lespiau@intel.com>

We need a bit book keeping around power wells' ops->enable(), namely a
nice debug message and updating hw_enabled. Let's introduce a
intel_power_well_enable() function to make sure all the callers do the
same things.

v2 (from Paulo):
  - s/i915_power_well_enable/intel_power_well_enable/ since everything
    else on this file uses intel_ instead of i915_.
  - Fix typo in commit message.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6393b76..a52574d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -68,6 +68,14 @@
 bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
 				    int power_well_id);
 
+static void intel_power_well_enable(struct drm_i915_private *dev_priv,
+				    struct i915_power_well *power_well)
+{
+	DRM_DEBUG_KMS("enabling %s\n", power_well->name);
+	power_well->ops->enable(dev_priv, power_well);
+	power_well->hw_enabled = true;
+}
+
 /*
  * 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
@@ -1104,11 +1112,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
 	mutex_lock(&power_domains->lock);
 
 	for_each_power_well(i, power_well, BIT(domain), power_domains) {
-		if (!power_well->count++) {
-			DRM_DEBUG_KMS("enabling %s\n", power_well->name);
-			power_well->ops->enable(dev_priv, power_well);
-			power_well->hw_enabled = true;
-		}
+		if (!power_well->count++)
+			intel_power_well_enable(dev_priv, power_well);
 	}
 
 	power_domains->domain_use_count[domain]++;
-- 
2.4.6

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

[PATCH 2/4] drm/i915: Extract a intel_power_well_disable() function

[Paulo Zanoni]

From: Damien Lespiau <damien.lespiau@intel.com>

Similar to the ->enable vfunc in commit:

  commit 865720564389b2b19cf58e41ed31701e5f464b9d
  Author: Damien Lespiau <damien.lespiau@intel.com>
  Date:   Wed Jun 3 14:27:05 2015 +0100

      drm/i915: Extract a intel_power_well_enable() function

v2 (from Paulo):
  - Same s/i915_/intel_/ bikeshed as the previous patch.
  - Update the commit hash.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index a52574d..821644d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -76,6 +76,14 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
 	power_well->hw_enabled = true;
 }
 
+static void intel_power_well_disable(struct drm_i915_private *dev_priv,
+				     struct i915_power_well *power_well)
+{
+	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
+	power_well->hw_enabled = false;
+	power_well->ops->disable(dev_priv, power_well);
+}
+
 /*
  * 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
@@ -1147,11 +1155,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
 		WARN_ON(!power_well->count);
 
-		if (!--power_well->count && i915.disable_power_well) {
-			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
-			power_well->hw_enabled = false;
-			power_well->ops->disable(dev_priv, power_well);
-		}
+		if (!--power_well->count && i915.disable_power_well)
+			intel_power_well_disable(dev_priv, power_well);
 	}
 
 	mutex_unlock(&power_domains->lock);
-- 
2.4.6

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

[PATCH 3/4] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Paulo Zanoni]

From: Damien Lespiau <damien.lespiau@intel.com>

Before this patch, we used the intel_display_power_{get,put} functions
to make sure the PW1 and Misc I/O power wells were enabled all the
time while LCPLL was enabled. We called a get() at
intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
we would call put/get at skl_{un,}init_cdclk().

The problem is that skl_uninit_cdclk() is indirectly called by
intel_runtime_suspend(). So it will only release its power well
_after_ we already decided to runtime suspend. But since we only
decide to runtime suspend after all power wells and refcounts are
released, that basically means we will never decide to runtime
suspend.

So what this patch does to fix that problem is move the PW1 + Misc I/O
power well handling out of the runtime PM mechanism: instead of
calling intel_display_power_{get_put} - functions that touch the
refcount -, we'll call the low level intel_power_well_{en,dis}able,
which don't change the refcount. This way, it is now possible for the
refcount to actually reach zero, and we'll now start runtime
suspending/resuming.

v2 (from Paulo):
  - Write a commit message since the original patch left it empty.
  - Rebase after the intel_power_well_{en,dis}able rename.
  - Use lookup_power_well() instead of hardcoded indexes.

Testcase: igt/pm_rpm/rte (and every other rpm test)
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c        |  2 --
 drivers/gpu/drm/i915/intel_display.c    |  5 +++--
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 29 +++++++++++++++++++++++++++++
 4 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9a40bfb..e629ad3 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2931,8 +2931,6 @@ void intel_ddi_pll_init(struct drm_device *dev)
 		dev_priv->skl_boot_cdclk = cdclk_freq;
 		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
 			DRM_ERROR("LCPLL1 is disabled\n");
-		else
-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	} else if (IS_BROXTON(dev)) {
 		broxton_init_cdclk(dev);
 		broxton_ddi_phy_init(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17..34cbe4a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5680,7 +5680,8 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
 		DRM_ERROR("Couldn't disable DPLL0\n");
 
-	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+	/* disable PG1 and Misc I/O */
+	skl_pw1_misc_io_fini(dev_priv);
 }
 
 void skl_init_cdclk(struct drm_i915_private *dev_priv)
@@ -5693,7 +5694,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
 	I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE);
 
 	/* enable PG1 and Misc I/O */
-	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+	skl_pw1_misc_io_init(dev_priv);
 
 	/* DPLL0 already enabed !? */
 	if (I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6..10e8a2f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1325,6 +1325,8 @@ void intel_psr_single_frame_update(struct drm_device *dev,
 int intel_power_domains_init(struct drm_i915_private *);
 void intel_power_domains_fini(struct drm_i915_private *);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
+void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv);
+void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
 
 bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 821644d..d62b030 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1505,6 +1505,35 @@ static struct i915_power_well skl_power_wells[] = {
 	},
 };
 
+void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_well *well;
+
+	if (!IS_SKYLAKE(dev_priv))
+		return;
+
+	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
+	intel_power_well_enable(dev_priv, well);
+
+	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
+	intel_power_well_enable(dev_priv, well);
+}
+
+void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_well *well;
+
+	if (!IS_SKYLAKE(dev_priv))
+		return;
+
+	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
+	intel_power_well_disable(dev_priv, well);
+
+	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
+	intel_power_well_disable(dev_priv, well);
+
+}
+
 static struct i915_power_well bxt_power_wells[] = {
 	{
 		.name = "always-on",
-- 
2.4.6

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

[PATCH 4/4] drm/i915/skl: send opregion_nofify_adapter(PCI_D1) instead of PCI_D3

[Paulo Zanoni]

I was told that the "repurposed D1 definition" is still valid for SKL.
It is BDW that is special due to its hotplug bug, so let's
special-case BDW instead of HSW.

Cc: Kristen Carlson Accardi <kristen@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 151263b..1d88745 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1489,7 +1489,15 @@ static int intel_runtime_suspend(struct device *device)
 	 * FIXME: We really should find a document that references the arguments
 	 * used below!
 	 */
-	if (IS_HASWELL(dev)) {
+	if (IS_BROADWELL(dev)) {
+		/*
+		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
+		 * being detected, and the call we do at intel_runtime_resume()
+		 * won't be able to restore them. Since PCI_D3hot matches the
+		 * actual specification and appears to be working, use it.
+		 */
+		intel_opregion_notify_adapter(dev, PCI_D3hot);
+	} else {
 		/*
 		 * current versions of firmware which depend on this opregion
 		 * notification have repurposed the D1 definition to mean
@@ -1498,16 +1506,6 @@ static int intel_runtime_suspend(struct device *device)
 		 * the suspend path.
 		 */
 		intel_opregion_notify_adapter(dev, PCI_D1);
-	} else {
-		/*
-		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
-		 * being detected, and the call we do at intel_runtime_resume()
-		 * won't be able to restore them. Since PCI_D3hot matches the
-		 * actual specification and appears to be working, use it. Let's
-		 * assume the other non-Haswell platforms will stay the same as
-		 * Broadwell.
-		 */
-		intel_opregion_notify_adapter(dev, PCI_D3hot);
 	}
 
 	assert_forcewakes_inactive(dev_priv);
-- 
2.4.6

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

Re: [PATCH 2/4] drm/i915: Extract a intel_power_well_disable() function

[Jani Nikula]

On Fri, 31 Jul 2015, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> Similar to the ->enable vfunc in commit:
>
>   commit 865720564389b2b19cf58e41ed31701e5f464b9d

That sha won't be the same once it gets applied. Maybe just squash these
two patches into one?

BR,
Jani.

>   Author: Damien Lespiau <damien.lespiau@intel.com>
>   Date:   Wed Jun 3 14:27:05 2015 +0100
>
>       drm/i915: Extract a intel_power_well_enable() function
>
> v2 (from Paulo):
>   - Same s/i915_/intel_/ bikeshed as the previous patch.
>   - Update the commit hash.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index a52574d..821644d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -76,6 +76,14 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>  	power_well->hw_enabled = true;
>  }
>  
> +static void intel_power_well_disable(struct drm_i915_private *dev_priv,
> +				     struct i915_power_well *power_well)
> +{
> +	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> +	power_well->hw_enabled = false;
> +	power_well->ops->disable(dev_priv, power_well);
> +}
> +
>  /*
>   * 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
> @@ -1147,11 +1155,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>  		WARN_ON(!power_well->count);
>  
> -		if (!--power_well->count && i915.disable_power_well) {
> -			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
> -			power_well->hw_enabled = false;
> -			power_well->ops->disable(dev_priv, power_well);
> -		}
> +		if (!--power_well->count && i915.disable_power_well)
> +			intel_power_well_disable(dev_priv, power_well);
>  	}
>  
>  	mutex_unlock(&power_domains->lock);
> -- 
> 2.4.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] Fix SKL runtime PM

[Patrik Jakobsson]

On Thu, Jul 30, 2015 at 06:20:25PM -0300, Paulo Zanoni wrote:
> Hello
> 
> So I discovered that SKL runtime PM doesn't work on drm-intel-nightly and
> decided to investigate why. I found this patch in one of Damien's trees and it
> fixed the problem I was seeing. I really don't know why the patches were not
> sent to the list yet and I can't ask him since he's on vacation. So I decided to
> review them, implement my own bikesheds, add R-B tags and submit to the list,
> since it's an important feature that's broken and preliminary_hw_support was
> removed for SKL.
> 
> I hope the reason he didn't submit this is not because there's some bug I failed
> to spot. Anyway, he'll tell us in a few days :).
> 
> Also notice that we're not passing every pm_rpm subtest now: we're just not
> failing 100% of the tests.

Good find! I also started looking into this and came up with something similar.
The series looks fine to me but I agree with Jani that you can squash
enable/disable into one commit. Now lets hope the bugs uncovered by this series
are trivial to fix.

For the entire patchset:

Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

> 
> Thanks,
> Paulo
> 
> Damien Lespiau (3):
>   drm/i915: Extract a intel_power_well_enable() function
>   drm/i915: Extract a intel_power_well_disable() function
>   drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini
>     sequences
> 
> Paulo Zanoni (1):
>   drm/i915/skl: send opregion_nofify_adapter(PCI_D1) instead of PCI_D3
> 
>  drivers/gpu/drm/i915/i915_drv.c         | 20 +++++------
>  drivers/gpu/drm/i915/intel_ddi.c        |  2 --
>  drivers/gpu/drm/i915/intel_display.c    |  5 +--
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 59 +++++++++++++++++++++++++++------
>  5 files changed, 63 insertions(+), 25 deletions(-)
> 
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Extract a intel_power_well_disable() function

[Paulo Zanoni]

2015-07-31 6:24 GMT-03:00 Jani Nikula <jani.nikula@linux.intel.com>:
> On Fri, 31 Jul 2015, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
>> From: Damien Lespiau <damien.lespiau@intel.com>
>>
>> Similar to the ->enable vfunc in commit:
>>
>>   commit 865720564389b2b19cf58e41ed31701e5f464b9d
>
> That sha won't be the same once it gets applied. Maybe just squash these
> two patches into one?

Or just ask the maintainers to remove that specific line containing
the sha while applying the patch since the commit title is already
listed and that won't change :)
</lazy_mode>

>
> BR,
> Jani.
>
>>   Author: Damien Lespiau <damien.lespiau@intel.com>
>>   Date:   Wed Jun 3 14:27:05 2015 +0100
>>
>>       drm/i915: Extract a intel_power_well_enable() function
>>
>> v2 (from Paulo):
>>   - Same s/i915_/intel_/ bikeshed as the previous patch.
>>   - Update the commit hash.
>>
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index a52574d..821644d 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -76,6 +76,14 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>>       power_well->hw_enabled = true;
>>  }
>>
>> +static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>> +                                  struct i915_power_well *power_well)
>> +{
>> +     DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>> +     power_well->hw_enabled = false;
>> +     power_well->ops->disable(dev_priv, power_well);
>> +}
>> +
>>  /*
>>   * 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
>> @@ -1147,11 +1155,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>       for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>>               WARN_ON(!power_well->count);
>>
>> -             if (!--power_well->count && i915.disable_power_well) {
>> -                     DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>> -                     power_well->hw_enabled = false;
>> -                     power_well->ops->disable(dev_priv, power_well);
>> -             }
>> +             if (!--power_well->count && i915.disable_power_well)
>> +                     intel_power_well_disable(dev_priv, power_well);
>>       }
>>
>>       mutex_unlock(&power_domains->lock);
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 3/4] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Daniel Vetter]

On Thu, Jul 30, 2015 at 06:20:28PM -0300, Paulo Zanoni wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> Before this patch, we used the intel_display_power_{get,put} functions
> to make sure the PW1 and Misc I/O power wells were enabled all the
> time while LCPLL was enabled. We called a get() at
> intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
> we would call put/get at skl_{un,}init_cdclk().
> 
> The problem is that skl_uninit_cdclk() is indirectly called by
> intel_runtime_suspend(). So it will only release its power well
> _after_ we already decided to runtime suspend. But since we only
> decide to runtime suspend after all power wells and refcounts are
> released, that basically means we will never decide to runtime
> suspend.
> 
> So what this patch does to fix that problem is move the PW1 + Misc I/O
> power well handling out of the runtime PM mechanism: instead of
> calling intel_display_power_{get_put} - functions that touch the
> refcount -, we'll call the low level intel_power_well_{en,dis}able,
> which don't change the refcount. This way, it is now possible for the
> refcount to actually reach zero, and we'll now start runtime
> suspending/resuming.
> 
> v2 (from Paulo):
>   - Write a commit message since the original patch left it empty.
>   - Rebase after the intel_power_well_{en,dis}able rename.
>   - Use lookup_power_well() instead of hardcoded indexes.
> 
> Testcase: igt/pm_rpm/rte (and every other rpm test)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

This is imo too much of a hack. If we go with this then we should either
completely remove the pw1 and misc pw from the power well code and just
directly call the relevant functions.

Or we fix up the ordering and stop lcpll before dropping pw1, which
probably means that skl dp aux and gmbus need to adjust their divisors for
every transaction since those change when lcpll changes.

I don't really have clue about which is the right option, but it seems
like dmc will take control of pw1&pw-misc anyway, so probably we don't
need to manage them explicitly on skl anyway.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ddi.c        |  2 --
>  drivers/gpu/drm/i915/intel_display.c    |  5 +++--
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 29 +++++++++++++++++++++++++++++
>  4 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9a40bfb..e629ad3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2931,8 +2931,6 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  		dev_priv->skl_boot_cdclk = cdclk_freq;
>  		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>  			DRM_ERROR("LCPLL1 is disabled\n");
> -		else
> -			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	} else if (IS_BROXTON(dev)) {
>  		broxton_init_cdclk(dev);
>  		broxton_ddi_phy_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43b0f17..34cbe4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5680,7 +5680,8 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>  		DRM_ERROR("Couldn't disable DPLL0\n");
>  
> -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +	/* disable PG1 and Misc I/O */
> +	skl_pw1_misc_io_fini(dev_priv);
>  }
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
> @@ -5693,7 +5694,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  	I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE);
>  
>  	/* enable PG1 and Misc I/O */
> -	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +	skl_pw1_misc_io_init(dev_priv);
>  
>  	/* DPLL0 already enabed !? */
>  	if (I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6..10e8a2f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1325,6 +1325,8 @@ void intel_psr_single_frame_update(struct drm_device *dev,
>  int intel_power_domains_init(struct drm_i915_private *);
>  void intel_power_domains_fini(struct drm_i915_private *);
>  void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv);
> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
>  
>  bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 821644d..d62b030 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1505,6 +1505,35 @@ static struct i915_power_well skl_power_wells[] = {
>  	},
>  };
>  
> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_well *well;
> +
> +	if (!IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> +	intel_power_well_enable(dev_priv, well);
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
> +	intel_power_well_enable(dev_priv, well);
> +}
> +
> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_well *well;
> +
> +	if (!IS_SKYLAKE(dev_priv))
> +		return;
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> +	intel_power_well_disable(dev_priv, well);
> +
> +	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
> +	intel_power_well_disable(dev_priv, well);
> +
> +}
> +
>  static struct i915_power_well bxt_power_wells[] = {
>  	{
>  		.name = "always-on",
> -- 
> 2.4.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Paulo Zanoni]

2015-08-05 5:30 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Jul 30, 2015 at 06:20:28PM -0300, Paulo Zanoni wrote:
>> From: Damien Lespiau <damien.lespiau@intel.com>
>>
>> Before this patch, we used the intel_display_power_{get,put} functions
>> to make sure the PW1 and Misc I/O power wells were enabled all the
>> time while LCPLL was enabled. We called a get() at
>> intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
>> we would call put/get at skl_{un,}init_cdclk().
>>
>> The problem is that skl_uninit_cdclk() is indirectly called by
>> intel_runtime_suspend(). So it will only release its power well
>> _after_ we already decided to runtime suspend. But since we only
>> decide to runtime suspend after all power wells and refcounts are
>> released, that basically means we will never decide to runtime
>> suspend.
>>
>> So what this patch does to fix that problem is move the PW1 + Misc I/O
>> power well handling out of the runtime PM mechanism: instead of
>> calling intel_display_power_{get_put} - functions that touch the
>> refcount -, we'll call the low level intel_power_well_{en,dis}able,
>> which don't change the refcount. This way, it is now possible for the
>> refcount to actually reach zero, and we'll now start runtime
>> suspending/resuming.
>>
>> v2 (from Paulo):
>>   - Write a commit message since the original patch left it empty.
>>   - Rebase after the intel_power_well_{en,dis}able rename.
>>   - Use lookup_power_well() instead of hardcoded indexes.
>>
>> Testcase: igt/pm_rpm/rte (and every other rpm test)
>> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This is imo too much of a hack. If we go with this then we should either
> completely remove the pw1 and misc pw from the power well code and just
> directly call the relevant functions.

What do you mean by "the relevant functions"? Notice that the patch
already moved us outside of the "power domains" framework, but not the
"power wells" framework, since those are actual power wells. I'm still
trying to fully understand what you wanted here.

>
> Or we fix up the ordering and stop lcpll before dropping pw1, which
> probably means that skl dp aux and gmbus need to adjust their divisors for
> every transaction since those change when lcpll changes.
>
> I don't really have clue about which is the right option, but it seems
> like dmc will take control of pw1&pw-misc anyway, so probably we don't
> need to manage them explicitly on skl anyway.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c        |  2 --
>>  drivers/gpu/drm/i915/intel_display.c    |  5 +++--
>>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 29 +++++++++++++++++++++++++++++
>>  4 files changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9a40bfb..e629ad3 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2931,8 +2931,6 @@ void intel_ddi_pll_init(struct drm_device *dev)
>>               dev_priv->skl_boot_cdclk = cdclk_freq;
>>               if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>>                       DRM_ERROR("LCPLL1 is disabled\n");
>> -             else
>> -                     intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>>       } else if (IS_BROXTON(dev)) {
>>               broxton_init_cdclk(dev);
>>               broxton_ddi_phy_init(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 43b0f17..34cbe4a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5680,7 +5680,8 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>>       if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
>>               DRM_ERROR("Couldn't disable DPLL0\n");
>>
>> -     intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>> +     /* disable PG1 and Misc I/O */
>> +     skl_pw1_misc_io_fini(dev_priv);
>>  }
>>
>>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>> @@ -5693,7 +5694,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
>>       I915_WRITE(HSW_NDE_RSTWRN_OPT, val | RESET_PCH_HANDSHAKE_ENABLE);
>>
>>       /* enable PG1 and Misc I/O */
>> -     intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>> +     skl_pw1_misc_io_init(dev_priv);
>>
>>       /* DPLL0 already enabed !? */
>>       if (I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 320c9e6..10e8a2f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1325,6 +1325,8 @@ void intel_psr_single_frame_update(struct drm_device *dev,
>>  int intel_power_domains_init(struct drm_i915_private *);
>>  void intel_power_domains_fini(struct drm_i915_private *);
>>  void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
>> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv);
>> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv);
>>  void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
>>
>>  bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 821644d..d62b030 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -1505,6 +1505,35 @@ static struct i915_power_well skl_power_wells[] = {
>>       },
>>  };
>>
>> +void skl_pw1_misc_io_init(struct drm_i915_private *dev_priv)
>> +{
>> +     struct i915_power_well *well;
>> +
>> +     if (!IS_SKYLAKE(dev_priv))
>> +             return;
>> +
>> +     well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
>> +     intel_power_well_enable(dev_priv, well);
>> +
>> +     well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
>> +     intel_power_well_enable(dev_priv, well);
>> +}
>> +
>> +void skl_pw1_misc_io_fini(struct drm_i915_private *dev_priv)
>> +{
>> +     struct i915_power_well *well;
>> +
>> +     if (!IS_SKYLAKE(dev_priv))
>> +             return;
>> +
>> +     well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
>> +     intel_power_well_disable(dev_priv, well);
>> +
>> +     well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
>> +     intel_power_well_disable(dev_priv, well);
>> +
>> +}
>> +
>>  static struct i915_power_well bxt_power_wells[] = {
>>       {
>>               .name = "always-on",
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 3/4] drm/i915: Make turning on/off PW1 and Misc I/O part of the init/fini sequences

[Daniel Vetter]

On Wed, Aug 05, 2015 at 03:28:54PM -0300, Paulo Zanoni wrote:
> 2015-08-05 5:30 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Jul 30, 2015 at 06:20:28PM -0300, Paulo Zanoni wrote:
> >> From: Damien Lespiau <damien.lespiau@intel.com>
> >>
> >> Before this patch, we used the intel_display_power_{get,put} functions
> >> to make sure the PW1 and Misc I/O power wells were enabled all the
> >> time while LCPLL was enabled. We called a get() at
> >> intel_ddi_pll_init() when we discovered that LCPLL was enabled, then
> >> we would call put/get at skl_{un,}init_cdclk().
> >>
> >> The problem is that skl_uninit_cdclk() is indirectly called by
> >> intel_runtime_suspend(). So it will only release its power well
> >> _after_ we already decided to runtime suspend. But since we only
> >> decide to runtime suspend after all power wells and refcounts are
> >> released, that basically means we will never decide to runtime
> >> suspend.
> >>
> >> So what this patch does to fix that problem is move the PW1 + Misc I/O
> >> power well handling out of the runtime PM mechanism: instead of
> >> calling intel_display_power_{get_put} - functions that touch the
> >> refcount -, we'll call the low level intel_power_well_{en,dis}able,
> >> which don't change the refcount. This way, it is now possible for the
> >> refcount to actually reach zero, and we'll now start runtime
> >> suspending/resuming.
> >>
> >> v2 (from Paulo):
> >>   - Write a commit message since the original patch left it empty.
> >>   - Rebase after the intel_power_well_{en,dis}able rename.
> >>   - Use lookup_power_well() instead of hardcoded indexes.
> >>
> >> Testcase: igt/pm_rpm/rte (and every other rpm test)
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > This is imo too much of a hack. If we go with this then we should either
> > completely remove the pw1 and misc pw from the power well code and just
> > directly call the relevant functions.
> 
> What do you mean by "the relevant functions"? Notice that the patch
> already moved us outside of the "power domains" framework, but not the
> "power wells" framework, since those are actual power wells. I'm still
> trying to fully understand what you wanted here.

The power wells abstraction doesn't buy anything here if we have a power
well that we always enable/disable with something else (device rpm here).
So instead of the lookup_power_well + enable call just remove pw1 and
pw-misc from the list of power wells and call the enable code directly.

Otherwise we just have a bit of abstraction that gets in the way. Also
note that dmc has similar requirements of enable pw1 and lcpll together to
avoid upsetting the firmware.

The overall idea is that abstraction should only be used when it actually
makes sense for a given platform. And I think using power wells
abstraction for pw1 and pw-misc on skl doesn't make sense since we can't
actually use it as an independent power well from the software pov - the
hw itself is different, but that's all managed by dmc firmware for us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx