[PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Use the same coding pattern as we use in the other functions of the
same file: just call lookup_power_well() directly in the only caller.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e209edbc561d..e0947f662361 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,9 +49,6 @@
  * present for a given platform.
  */
 
-bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
-					 enum i915_power_well_id power_well_id);
-
 static struct i915_power_well *
 lookup_power_well(struct drm_i915_private *dev_priv,
 		  enum i915_power_well_id power_well_id);
@@ -678,8 +675,9 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
 
 static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 {
-	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
-					SKL_DISP_PW_2);
+	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
+							SKL_DISP_PW_2);
+	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
 
 	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
 
@@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc chv_power_wells[] = {
 	},
 };
 
-bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
-					 enum i915_power_well_id power_well_id)
-{
-	struct i915_power_well *power_well;
-	bool ret;
-
-	power_well = lookup_power_well(dev_priv, power_well_id);
-	ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
-
-	return ret;
-}
-
 static const struct i915_power_well_desc skl_power_wells[] = {
 	{
 		.name = "always-on",
-- 
2.14.4

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

[PATCH 2/4] drm/i915: BUG() if we can't lookup_power_well()

[Paulo Zanoni]

None of the current lookup_power_well() callers are actually checking
for NULL return values, they all just use the pointer right away.
Replacing these theoretical segfaults with BUG()s at least makes our
code a little more explicit to the reader.

We can only hit this NULL/BUG() condition if we try to lookup a power
well that isn't defined on a given platform. If that ever happens, we
have to fix our code, making it lookup the correct power well. Because
of this, I don't think it's worth trying to implement error checking
in every caller. Improving our CI system will be a better use of our
time once a bug is found in the wild.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e0947f662361..2fdb1f4125c2 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1120,7 +1120,8 @@ lookup_power_well(struct drm_i915_private *dev_priv,
 			return power_well;
 	}
 
-	return NULL;
+	/* Tried to lookup a power well that doesn't exist for the platform. */
+	BUG();
 }
 
 #define BITS_SET(val, bits) (((val) & (bits)) == (bits))
-- 
2.14.4

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

[PATCH 3/4] drm/i915: use for_each_power_well in lookup_power_well()

[Paulo Zanoni]

Use the nice helper function to make the implementation simpler.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2fdb1f4125c2..eabf98d153f7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1109,16 +1109,11 @@ static struct i915_power_well *
 lookup_power_well(struct drm_i915_private *dev_priv,
 		  enum i915_power_well_id power_well_id)
 {
-	struct i915_power_domains *power_domains = &dev_priv->power_domains;
-	int i;
-
-	for (i = 0; i < power_domains->power_well_count; i++) {
-		struct i915_power_well *power_well;
+	struct i915_power_well *power_well;
 
-		power_well = &power_domains->power_wells[i];
+	for_each_power_well(dev_priv, power_well)
 		if (power_well->desc->id == power_well_id)
 			return power_well;
-	}
 
 	/* Tried to lookup a power well that doesn't exist for the platform. */
 	BUG();
-- 
2.14.4

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

[PATCH 4/4] drm/i915: move lookup_power_well() up

[Paulo Zanoni]

There's no need for that forward declaration.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index eabf98d153f7..d9c7062ffc95 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,10 +49,6 @@
  * present for a given platform.
  */
 
-static struct i915_power_well *
-lookup_power_well(struct drm_i915_private *dev_priv,
-		  enum i915_power_well_id power_well_id);
-
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain)
 {
@@ -673,6 +669,20 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
 	WARN_ONCE(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
 }
 
+static struct i915_power_well *
+lookup_power_well(struct drm_i915_private *dev_priv,
+		  enum i915_power_well_id power_well_id)
+{
+	struct i915_power_well *power_well;
+
+	for_each_power_well(dev_priv, power_well)
+		if (power_well->desc->id == power_well_id)
+			return power_well;
+
+	/* Tried to lookup a power well that doesn't exist for the platform. */
+	BUG();
+}
+
 static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
@@ -1105,20 +1115,6 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 
 #define POWER_DOMAIN_MASK (GENMASK_ULL(POWER_DOMAIN_NUM - 1, 0))
 
-static struct i915_power_well *
-lookup_power_well(struct drm_i915_private *dev_priv,
-		  enum i915_power_well_id power_well_id)
-{
-	struct i915_power_well *power_well;
-
-	for_each_power_well(dev_priv, power_well)
-		if (power_well->desc->id == power_well_id)
-			return power_well;
-
-	/* Tried to lookup a power well that doesn't exist for the platform. */
-	BUG();
-}
-
 #define BITS_SET(val, bits) (((val) & (bits)) == (bits))
 
 static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
-- 
2.14.4

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

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Souza, Jose]

On Wed, 2018-08-08 at 15:16 -0700, Paulo Zanoni wrote:
> Use the same coding pattern as we use in the other functions of the
> same file: just call lookup_power_well() directly in the only caller.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e209edbc561d..e0947f662361 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,9 +49,6 @@
>   * present for a given platform.
>   */
>  
> -bool intel_display_power_well_is_enabled(struct drm_i915_private
> *dev_priv,
> -					 enum i915_power_well_id
> power_well_id);
> -
>  static struct i915_power_well *
>  lookup_power_well(struct drm_i915_private *dev_priv,
>  		  enum i915_power_well_id power_well_id);
> @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> drm_i915_private *dev_priv)
>  
>  static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
>  {
> -	bool pg2_enabled =
> intel_display_power_well_is_enabled(dev_priv,
> -					SKL_DISP_PW_2);
> +	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> +							SKL_DISP_PW_2);
> +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);

Why not trust our sync with hardware and use pg2->hw_enabled?

>  
>  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
>  
> @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> chv_power_wells[] = {
>  	},
>  };
>  
> -bool intel_display_power_well_is_enabled(struct drm_i915_private
> *dev_priv,
> -					 enum i915_power_well_id
> power_well_id)
> -{
> -	struct i915_power_well *power_well;
> -	bool ret;
> -
> -	power_well = lookup_power_well(dev_priv, power_well_id);
> -	ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> -
> -	return ret;
> -}
> -
>  static const struct i915_power_well_desc skl_power_wells[] = {
>  	{
>  		.name = "always-on",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()
URL   : https://patchwork.freedesktop.org/series/47908/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled()
dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well()
-:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124:
+	BUG();

total: 0 errors, 1 warnings, 0 checks, 9 lines checked
e5298551c379 drm/i915: use for_each_power_well in lookup_power_well()
942f90859025 drm/i915: move lookup_power_well() up
-:40: WARNING:AVOID_BUG: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
#40: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:683:
+	BUG();

total: 0 errors, 1 warnings, 0 checks, 50 lines checked

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()
URL   : https://patchwork.freedesktop.org/series/47908/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4635 -> Patchwork_9892 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47908/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      {fi-kbl-8809g}:     NOTRUN -> FAIL (fdo#107341)

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_workarounds:
      fi-whl-u:           PASS -> DMESG-FAIL (fdo#107292)
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    {igt@kms_psr@primary_page_flip}:
      fi-skl-6600u:       PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_basic@userptr}:
      {fi-kbl-8809g}:     INCOMPLETE (fdo#107402) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-cnl-psr:         DMESG-FAIL (fdo#106560) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS

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

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107402 https://bugs.freedesktop.org/show_bug.cgi?id=107402


== Participating hosts (50 -> 47) ==

  Additional (1): fi-hsw-peppy 
  Missing    (4): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4635 -> Patchwork_9892

  CI_DRM_4635: a9f34f7e3bab765e2b1320a1b154a76be38602a7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4589: 779d2d42f9db6a2797d1ef50036af6fac4e62e73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9892: 942f90859025412da524cfe6b53b2b5b05e84b9c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

942f90859025 drm/i915: move lookup_power_well() up
e5298551c379 drm/i915: use for_each_power_well in lookup_power_well()
dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well()
94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9892/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: use for_each_power_well in lookup_power_well()

[Souza, Jose]

On Wed, 2018-08-08 at 15:16 -0700, Paulo Zanoni wrote:
> Use the nice helper function to make the implementation simpler.
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2fdb1f4125c2..eabf98d153f7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1109,16 +1109,11 @@ static struct i915_power_well *
>  lookup_power_well(struct drm_i915_private *dev_priv,
>  		  enum i915_power_well_id power_well_id)
>  {
> -	struct i915_power_domains *power_domains = &dev_priv-
> >power_domains;
> -	int i;
> -
> -	for (i = 0; i < power_domains->power_well_count; i++) {
> -		struct i915_power_well *power_well;
> +	struct i915_power_well *power_well;
>  
> -		power_well = &power_domains->power_wells[i];
> +	for_each_power_well(dev_priv, power_well)
>  		if (power_well->desc->id == power_well_id)
>  			return power_well;
> -	}
>  
>  	/* Tried to lookup a power well that doesn't exist for the
> platform. */
>  	BUG();
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Qua, 2018-08-08 às 15:22 -0700, Souza, Jose escreveu:
> On Wed, 2018-08-08 at 15:16 -0700, Paulo Zanoni wrote:
> > Use the same coding pattern as we use in the other functions of the
> > same file: just call lookup_power_well() directly in the only
> > caller.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
> >  1 file changed, 3 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e209edbc561d..e0947f662361 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,9 +49,6 @@
> >   * present for a given platform.
> >   */
> >  
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -					 enum i915_power_well_id
> > power_well_id);
> > -
> >  static struct i915_power_well *
> >  lookup_power_well(struct drm_i915_private *dev_priv,
> >  		  enum i915_power_well_id power_well_id);
> > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > drm_i915_private *dev_priv)
> >  
> >  static void assert_can_enable_dc5(struct drm_i915_private
> > *dev_priv)
> >  {
> > -	bool pg2_enabled =
> > intel_display_power_well_is_enabled(dev_priv,
> > -					SKL_DISP_PW_2);
> > +	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> > +							SKL_DISP_P
> > W_2);
> > +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > pg2);
> 
> Why not trust our sync with hardware and use pg2->hw_enabled?

While that's a very reasonable thing and it sounds like something we
should do (as far as I have investigated in 5 minutes), it's a
functional change with a potential downside and should be in a separate
patch IMHO.

> 
> >  
> >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > DC5.\n");
> >  
> > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > chv_power_wells[] = {
> >  	},
> >  };
> >  
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -					 enum i915_power_well_id
> > power_well_id)
> > -{
> > -	struct i915_power_well *power_well;
> > -	bool ret;
> > -
> > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > power_well);
> > -
> > -	return ret;
> > -}
> > -
> >  static const struct i915_power_well_desc skl_power_wells[] = {
> >  	{
> >  		.name = "always-on",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu:
> == Series Details ==
> 
> Series: series starting with [1/4] drm/i915: kill
> intel_display_power_well_is_enabled()
> URL   : https://patchwork.freedesktop.org/series/47908/
> State : warning
> 
> == Summary ==
> 
> $ dim checkpatch origin/drm-tip
> 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled()
> dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well()
> -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using
> WARN_ON & recovery code rather than BUG() or BUG_ON()
> #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124:
> +	BUG();

See the commit message of patch 2, Mr. Robot. I don't think it's worth
adding checking code in the callers, and replacing the current null
pointer dereference with a BUG() is an improvement IMHO.

If anybody disagrees with this, please say so.

> 
> total: 0 errors, 1 warnings, 0 checks, 9 lines checked
> e5298551c379 drm/i915: use for_each_power_well in lookup_power_well()
> 942f90859025 drm/i915: move lookup_power_well() up
> -:40: WARNING:AVOID_BUG: Avoid crashing the kernel - try using
> WARN_ON & recovery code rather than BUG() or BUG_ON()
> #40: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:683:
> +	BUG();
> 
> total: 0 errors, 1 warnings, 0 checks, 50 lines checked
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()
URL   : https://patchwork.freedesktop.org/series/47908/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4635_full -> Patchwork_9892_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@perf_pmu@rc6:
      shard-kbl:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> INCOMPLETE (fdo#106886, fdo#105411)

    igt@gem_render_linear_blits@basic:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_universal_plane@cursor-fb-leak-pipe-b:
      shard-apl:          PASS -> FAIL (fdo#107241)

    
    ==== Possible fixes ====

    igt@gem_exec_await@wide-contexts:
      shard-apl:          FAIL (fdo#105900, fdo#106680) -> PASS

    igt@gem_exec_reuse@single:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107241 https://bugs.freedesktop.org/show_bug.cgi?id=107241


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4635 -> Patchwork_9892

  CI_DRM_4635: a9f34f7e3bab765e2b1320a1b154a76be38602a7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4589: 779d2d42f9db6a2797d1ef50036af6fac4e62e73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9892: 942f90859025412da524cfe6b53b2b5b05e84b9c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9892/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Michal Wajdeczko]

On Thu, 09 Aug 2018 00:58:53 +0200, Paulo Zanoni  
<paulo.r.zanoni@intel.com> wrote:

> Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu:
>> == Series Details ==
>>
>> Series: series starting with [1/4] drm/i915: kill
>> intel_display_power_well_is_enabled()
>> URL   : https://patchwork.freedesktop.org/series/47908/
>> State : warning
>>
>> == Summary ==
>>
>> $ dim checkpatch origin/drm-tip
>> 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled()
>> dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well()
>> -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using
>> WARN_ON & recovery code rather than BUG() or BUG_ON()
>> #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124:
>> +	BUG();
>
> See the commit message of patch 2, Mr. Robot. I don't think it's worth
> adding checking code in the callers, and replacing the current null
> pointer dereference with a BUG() is an improvement IMHO.
>
> If anybody disagrees with this, please say so.

other option could be to use WARN_ON to make Mr. Robot happy and
then return first/last/default power well to make callers happy

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

Re: ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Qui, 2018-08-09 às 08:29 +0200, Michal Wajdeczko escreveu:
> On Thu, 09 Aug 2018 00:58:53 +0200, Paulo Zanoni  
> <paulo.r.zanoni@intel.com> wrote:
> 
> > Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu:
> > > == Series Details ==
> > > 
> > > Series: series starting with [1/4] drm/i915: kill
> > > intel_display_power_well_is_enabled()
> > > URL   : https://patchwork.freedesktop.org/series/47908/
> > > State : warning
> > > 
> > > == Summary ==
> > > 
> > > $ dim checkpatch origin/drm-tip
> > > 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled()
> > > dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well()
> > > -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using
> > > WARN_ON & recovery code rather than BUG() or BUG_ON()
> > > #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124:
> > > +	BUG();
> > 
> > See the commit message of patch 2, Mr. Robot. I don't think it's
> > worth
> > adding checking code in the callers, and replacing the current null
> > pointer dereference with a BUG() is an improvement IMHO.
> > 
> > If anybody disagrees with this, please say so.
> 
> other option could be to use WARN_ON to make Mr. Robot happy and
> then return first/last/default power well to make callers happy

Yeah, that's probably an improvement. I'll submit a new version.
Thanks!

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

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Imre Deak]

On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> Use the same coding pattern as we use in the other functions of the
> same file: just call lookup_power_well() directly in the only caller.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e209edbc561d..e0947f662361 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,9 +49,6 @@
>   * present for a given platform.
>   */
>  
> -bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> -					 enum i915_power_well_id power_well_id);
> -
>  static struct i915_power_well *
>  lookup_power_well(struct drm_i915_private *dev_priv,
>  		  enum i915_power_well_id power_well_id);
> @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  
>  static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
>  {
> -	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
> -					SKL_DISP_PW_2);
> +	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> +							SKL_DISP_PW_2);
> +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
>  
>  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
>  
> @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc chv_power_wells[] = {
>  	},
>  };
>  
> -bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> -					 enum i915_power_well_id power_well_id)
> -{
> -	struct i915_power_well *power_well;
> -	bool ret;
> -
> -	power_well = lookup_power_well(dev_priv, power_well_id);
> -	ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> -
> -	return ret;
> -}
> -

Or rather export a locked version of it and use that in intel_hdcp.c to
better hide the internals?

>  static const struct i915_power_well_desc skl_power_wells[] = {
>  	{
>  		.name = "always-on",
> -- 
> 2.14.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > Use the same coding pattern as we use in the other functions of the
> > same file: just call lookup_power_well() directly in the only
> > caller.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-----------------
> >  1 file changed, 3 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e209edbc561d..e0947f662361 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,9 +49,6 @@
> >   * present for a given platform.
> >   */
> >  
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -					 enum i915_power_well_id
> > power_well_id);
> > -
> >  static struct i915_power_well *
> >  lookup_power_well(struct drm_i915_private *dev_priv,
> >  		  enum i915_power_well_id power_well_id);
> > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > drm_i915_private *dev_priv)
> >  
> >  static void assert_can_enable_dc5(struct drm_i915_private
> > *dev_priv)
> >  {
> > -	bool pg2_enabled =
> > intel_display_power_well_is_enabled(dev_priv,
> > -					SKL_DISP_PW_2);
> > +	struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> > +							SKL_DISP_P
> > W_2);
> > +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > pg2);
> >  
> >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > DC5.\n");
> >  
> > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > chv_power_wells[] = {
> >  	},
> >  };
> >  
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -					 enum i915_power_well_id
> > power_well_id)
> > -{
> > -	struct i915_power_well *power_well;
> > -	bool ret;
> > -
> > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > power_well);
> > -
> > -	return ret;
> > -}
> > -
> 
> Or rather export a locked version of it and use that in intel_hdcp.c
> to
> better hide the internals?

That should probably be combined with José's idea of using ->enabled so
we trust the hardware sync.

Thanks for the suggestions.

> 
> >  static const struct i915_power_well_desc skl_power_wells[] = {
> >  	{
> >  		.name = "always-on",
> > -- 
> > 2.14.4
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > Use the same coding pattern as we use in the other functions of
> > > the
> > > same file: just call lookup_power_well() directly in the only
> > > caller.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--------------
> > > ---
> > >  1 file changed, 3 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index e209edbc561d..e0947f662361 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -49,9 +49,6 @@
> > >   * present for a given platform.
> > >   */
> > >  
> > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > -					 enum i915_power_well_id
> > > power_well_id);
> > > -
> > >  static struct i915_power_well *
> > >  lookup_power_well(struct drm_i915_private *dev_priv,
> > >  		  enum i915_power_well_id power_well_id);
> > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > drm_i915_private *dev_priv)
> > >  
> > >  static void assert_can_enable_dc5(struct drm_i915_private
> > > *dev_priv)
> > >  {
> > > -	bool pg2_enabled =
> > > intel_display_power_well_is_enabled(dev_priv,
> > > -					SKL_DISP_PW_2);
> > > +	struct i915_power_well *pg2 =
> > > lookup_power_well(dev_priv,
> > > +							SKL_DISP
> > > _P
> > > W_2);
> > > +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > > pg2);
> > >  
> > >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > DC5.\n");
> > >  
> > > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > > chv_power_wells[] = {
> > >  	},
> > >  };
> > >  
> > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > -					 enum i915_power_well_id
> > > power_well_id)
> > > -{
> > > -	struct i915_power_well *power_well;
> > > -	bool ret;
> > > -
> > > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > > power_well);
> > > -
> > > -	return ret;
> > > -}
> > > -
> > 
> > Or rather export a locked version of it and use that in
> > intel_hdcp.c
> > to
> > better hide the internals?
> 
> That should probably be combined with José's idea of using ->enabled
> so
> we trust the hardware sync.
> 
> Thanks for the suggestions.

After further analysis, I wonder if intel_hdcp.c should really be
checking for enabled power wells or if it should be doing something
else, such as actually grabbing power domain references to make sure
we're able to enable/disable HDCP whenever we need. Most of our code
should not be checking for power wells/domains being enabled/disabled
(except for HW readout), it should actually be requesting those
resources to make sure we have them when we need them.

CCing Ramaligam for that.

> 
> > 
> > >  static const struct i915_power_well_desc skl_power_wells[] = {
> > >  	{
> > >  		.name = "always-on",
> > > -- 
> > > 2.14.4
> > > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Imre Deak]

On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > Use the same coding pattern as we use in the other functions of
> > > > the
> > > > same file: just call lookup_power_well() directly in the only
> > > > caller.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--------------
> > > > ---
> > > >  1 file changed, 3 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index e209edbc561d..e0947f662361 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -49,9 +49,6 @@
> > > >   * present for a given platform.
> > > >   */
> > > >  
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -					 enum i915_power_well_id
> > > > power_well_id);
> > > > -
> > > >  static struct i915_power_well *
> > > >  lookup_power_well(struct drm_i915_private *dev_priv,
> > > >  		  enum i915_power_well_id power_well_id);
> > > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > > drm_i915_private *dev_priv)
> > > >  
> > > >  static void assert_can_enable_dc5(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > > -	bool pg2_enabled =
> > > > intel_display_power_well_is_enabled(dev_priv,
> > > > -					SKL_DISP_PW_2);
> > > > +	struct i915_power_well *pg2 =
> > > > lookup_power_well(dev_priv,
> > > > +							SKL_DISP
> > > > _P
> > > > W_2);
> > > > +	bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > > > pg2);
> > > >  
> > > >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > > DC5.\n");
> > > >  
> > > > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > > > chv_power_wells[] = {
> > > >  	},
> > > >  };
> > > >  
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -					 enum i915_power_well_id
> > > > power_well_id)
> > > > -{
> > > > -	struct i915_power_well *power_well;
> > > > -	bool ret;
> > > > -
> > > > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > > > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > > > power_well);
> > > > -
> > > > -	return ret;
> > > > -}
> > > > -
> > > 
> > > Or rather export a locked version of it and use that in
> > > intel_hdcp.c
> > > to
> > > better hide the internals?
> > 
> > That should probably be combined with José's idea of using ->enabled
> > so
> > we trust the hardware sync.
> > 
> > Thanks for the suggestions.
> 
> After further analysis, I wonder if intel_hdcp.c should really be
> checking for enabled power wells or if it should be doing something
> else, such as actually grabbing power domain references to make sure
> we're able to enable/disable HDCP whenever we need. Most of our code
> should not be checking for power wells/domains being enabled/disabled
> (except for HW readout), it should actually be requesting those
> resources to make sure we have them when we need them.
> 
> CCing Ramaligam for that.

There is no separate power resource for HDCP, it just uses the power
wells the encoder already uses. Those are guaranteed to be on, since
intel_hdcp_enable/disable are called from the encoder enable/disable
hooks. As such hdcp_key_loadable() is just an assert. Defining a new
power domain for this would be a bit overkill imo and as PW#1 is handled
automatically by HW (and so not the usual driver get/put ops via power
domain handles) we would have to special case it.

> 
> > 
> > > 
> > > >  static const struct i915_power_well_desc skl_power_wells[] = {
> > > >  	{
> > > >  		.name = "always-on",
> > > > -- 
> > > > 2.14.4
> > > > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Paulo Zanoni]

Em Ter, 2018-08-21 às 14:12 +0300, Imre Deak escreveu:
> On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> > Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > > Use the same coding pattern as we use in the other functions
> > > > > of
> > > > > the
> > > > > same file: just call lookup_power_well() directly in the only
> > > > > caller.
> > > > > 
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++----------
> > > > > ----
> > > > > ---
> > > > >  1 file changed, 3 insertions(+), 17 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index e209edbc561d..e0947f662361 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -49,9 +49,6 @@
> > > > >   * present for a given platform.
> > > > >   */
> > > > >  
> > > > > -bool intel_display_power_well_is_enabled(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > -					 enum
> > > > > i915_power_well_id
> > > > > power_well_id);
> > > > > -
> > > > >  static struct i915_power_well *
> > > > >  lookup_power_well(struct drm_i915_private *dev_priv,
> > > > >  		  enum i915_power_well_id power_well_id);
> > > > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  
> > > > >  static void assert_can_enable_dc5(struct drm_i915_private
> > > > > *dev_priv)
> > > > >  {
> > > > > -	bool pg2_enabled =
> > > > > intel_display_power_well_is_enabled(dev_priv,
> > > > > -					SKL_DISP_PW_2);
> > > > > +	struct i915_power_well *pg2 =
> > > > > lookup_power_well(dev_priv,
> > > > > +							SKL_
> > > > > DISP
> > > > > _P
> > > > > W_2);
> > > > > +	bool pg2_enabled = pg2->desc->ops-
> > > > > >is_enabled(dev_priv,
> > > > > pg2);
> > > > >  
> > > > >  	WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > > > DC5.\n");
> > > > >  
> > > > > @@ -2302,18 +2300,6 @@ static const struct
> > > > > i915_power_well_desc
> > > > > chv_power_wells[] = {
> > > > >  	},
> > > > >  };
> > > > >  
> > > > > -bool intel_display_power_well_is_enabled(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > -					 enum
> > > > > i915_power_well_id
> > > > > power_well_id)
> > > > > -{
> > > > > -	struct i915_power_well *power_well;
> > > > > -	bool ret;
> > > > > -
> > > > > -	power_well = lookup_power_well(dev_priv,
> > > > > power_well_id);
> > > > > -	ret = power_well->desc->ops->is_enabled(dev_priv,
> > > > > power_well);
> > > > > -
> > > > > -	return ret;
> > > > > -}
> > > > > -
> > > > 
> > > > Or rather export a locked version of it and use that in
> > > > intel_hdcp.c
> > > > to
> > > > better hide the internals?
> > > 
> > > That should probably be combined with José's idea of using
> > > ->enabled
> > > so
> > > we trust the hardware sync.
> > > 
> > > Thanks for the suggestions.
> > 
> > After further analysis, I wonder if intel_hdcp.c should really be
> > checking for enabled power wells or if it should be doing something
> > else, such as actually grabbing power domain references to make
> > sure
> > we're able to enable/disable HDCP whenever we need. Most of our
> > code
> > should not be checking for power wells/domains being
> > enabled/disabled
> > (except for HW readout), it should actually be requesting those
> > resources to make sure we have them when we need them.
> > 
> > CCing Ramaligam for that.
> 
> There is no separate power resource for HDCP, it just uses the power
> wells the encoder already uses. Those are guaranteed to be on, since
> intel_hdcp_enable/disable are called from the encoder enable/disable
> hooks. As such hdcp_key_loadable() is just an assert.

But then in this case, an assertion wouldn't make sense at all, since
if the condition for the assert was not valid, everything before that
call would have been broken too. If this were the only problem I would
just vote to remove the assertion.

The problem here is that we have a work function that also runs the
assert, and since it's a work function we really have no guarantees
about the power wells being held when it runs.

intel_hdcp_check_work() -> intel_hdcp_check_link() ->
_intel_hdcp_enable() -> hdcp_key_loadable().

I'm not sure what's the best thing to do here, but perhaps preventing
the hardware from going away when this work is scheduled would help.

>  Defining a new
> power domain for this would be a bit overkill imo and as PW#1 is
> handled
> automatically by HW (and so not the usual driver get/put ops via
> power
> domain handles) we would have to special case it.

It only gets automatically handled by the HW once we drop a well
defined set of power domains. If we grab those power domains we prevent
automatic hardware handling.

> 
> > 
> > > 
> > > > 
> > > > >  static const struct i915_power_well_desc skl_power_wells[] =
> > > > > {
> > > > >  	{
> > > > >  		.name = "always-on",
> > > > > -- 
> > > > > 2.14.4
> > > > > 
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()

[Imre Deak]

On Tue, Aug 21, 2018 at 12:54:12PM -0700, Paulo Zanoni wrote:
> Em Ter, 2018-08-21 às 14:12 +0300, Imre Deak escreveu:
> > On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> > > Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > > > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > > > [...]
> > > > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> > > > > > -					 enum i915_power_well_id power_well_id)
> > > > > > -{
> > > > > > -	struct i915_power_well *power_well;
> > > > > > -	bool ret;
> > > > > > -
> > > > > > -	power_well = lookup_power_well(dev_priv, power_well_id);
> > > > > > -	ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> > > > > > -
> > > > > > -	return ret;
> > > > > > -}
> > > > > > -
> > > > > 
> > > > > Or rather export a locked version of it and use that in intel_hdcp.c
> > > > > to better hide the internals?
> > > > 
> > > > That should probably be combined with José's idea of using
> > > > ->enabled so we trust the hardware sync.
> > > > 
> > > > Thanks for the suggestions.
> > > 
> > > After further analysis, I wonder if intel_hdcp.c should really be
> > > checking for enabled power wells or if it should be doing
> > > something else, such as actually grabbing power domain references
> > > to make sure we're able to enable/disable HDCP whenever we need.
> > > Most of our code should not be checking for power wells/domains
> > > being enabled/disabled (except for HW readout), it should actually
> > > be requesting those resources to make sure we have them when we
> > > need them.
> > > 
> > > CCing Ramaligam for that.
> > 
> > There is no separate power resource for HDCP, it just uses the power
> > wells the encoder already uses. Those are guaranteed to be on, since
> > intel_hdcp_enable/disable are called from the encoder enable/disable
> > hooks. As such hdcp_key_loadable() is just an assert.
> 
> But then in this case, an assertion wouldn't make sense at all, since
> if the condition for the assert was not valid, everything before that
> call would have been broken too. If this were the only problem I would
> just vote to remove the assertion.

It is an assertion, hdcp_key_loadable() always returns true, unless
there is a bug somewhere. I think it also makes sense to have an
assertion past the point where the condition should be true, close to a
place where you logically depend on this condition. For instance for
documentation or to make sure some bug in between didn't cause the
condition to become false. In this case it looks like it was added to
match the conditions listed by BSpec.

> The problem here is that we have a work function that also runs the
> assert, and since it's a work function we really have no guarantees
> about the power wells being held when it runs.
> 
> intel_hdcp_check_work() -> intel_hdcp_check_link() ->
> _intel_hdcp_enable() -> hdcp_key_loadable().
> 
> I'm not sure what's the best thing to do here, but perhaps preventing
> the hardware from going away when this work is scheduled would help.

The work is cancelled in intel_hdcp_disable(), so it's guarateed to run
only while the encoder is active (and hence that the required power
wells are enabled).

> > Defining a new power domain for this would be a bit overkill imo
> > and as PW#1 is handled automatically by HW (and so not the usual
> > driver get/put ops via power domain handles) we would have to
> > special case it.
> 
> It only gets automatically handled by the HW once we drop a well
> defined set of power domains. If we grab those power domains we
> prevent automatic hardware handling.

Sure, but the assert is specifically about PW#1, not about some other
indirect dependency that should cause PW#1 to be enabled. (That is
should unless there is a bug somewhere.)

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