[PATCH 1/2] drm/i915: Sanitize display INIT power domain enabling

[PATCH 1/2] drm/i915: Sanitize display INIT power domain enabling

[Imre Deak]

For consistency move the INIT power domain enabling to happen at the
same call-stack level everywhere. So far we didn't do this enabling
during:
- on GEN9 big-core when resuming from system freeze
- on VLV on the i915_drm_suspend_late() error path

Fortunately neither of these depended on display power wells being
enabled until proper references are taken by the modeset code. (This
also means the we can probably remove the INIT power domain get/put
from these places as a follow-up, after some more auditing.)

The current unpaired enable/disable was noticed by Ville during some
earlier review.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 6 ++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 --
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6d9944a..6b10e37 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -592,6 +592,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	intel_update_rawclk(dev_priv);
 
 	intel_power_domains_init_hw(dev_priv, false);
+	/* Enable the INIT power domain wells for HW initialization. */
+	intel_display_set_init_power(dev_priv, true);
 
 	intel_csr_ucode_init(dev_priv);
 
@@ -1559,6 +1561,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 		if (!fw_csr)
 			intel_power_domains_init_hw(dev_priv, true);
 
+		intel_display_set_init_power(dev_priv, true);
+
 		goto out;
 	}
 
@@ -1768,6 +1772,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	if (IS_GEN9_LP(dev_priv) ||
 	    !(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload))
 		intel_power_domains_init_hw(dev_priv, true);
+	/* Enable the INIT power domain wells for HW initialization. */
+	intel_display_set_init_power(dev_priv, true);
 
 	i915_gem_sanitize(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 012bc35..ecc43c6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2715,8 +2715,6 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		mutex_unlock(&power_domains->lock);
 	}
 
-	/* For now, we need the power well to be always enabled. */
-	intel_display_set_init_power(dev_priv, true);
 	/* Disable power support if the user asked so. */
 	if (!i915.disable_power_well)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
-- 
2.5.0

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

[PATCH 2/2] drm/i915: Sanitize display INIT power domain disabling

[Imre Deak]

For consistency move the INIT power domain disabling to the same
call-stack level with the corresponding enabling.

This change will remove the disable call via intel_finish_reset() and
intel_lid_notify(), but this is fine: we didn't enable the INIT power
domain on these paths to begin with, so the disable call was just a NOP.
(Enabling the INIT power domain on these paths is not needed either
 since we access the HW directly only for old HW without RPM support, or
 do a full modeset restore, which knows how to enable any required power
 domain.)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 4 ++++
 drivers/gpu/drm/i915/intel_display.c    | 3 ---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 7 +++++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6b10e37..13fa585 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -609,6 +609,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
+	intel_display_set_init_power(dev_priv, false);
+
 	intel_uc_init_fw(dev_priv);
 
 	ret = i915_gem_init(dev_priv);
@@ -1670,6 +1672,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	intel_display_resume(dev);
 
+	intel_display_set_init_power(dev_priv, false);
+
 	drm_kms_helper_poll_enable(dev);
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a28a89..0920291 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15610,9 +15610,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 		if (WARN_ON(put_domains))
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
-	intel_display_set_init_power(dev_priv, false);
-
-	intel_power_domains_verify_state(dev_priv);
 
 	intel_fbc_init_pipe_state(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ecc43c6..0fb5bfb 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -270,6 +270,13 @@ void intel_display_set_init_power(struct drm_i915_private *dev_priv,
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
 	dev_priv->power_domains.init_power_on = enable;
+
+	if (!enable)
+		/*
+		 * By this point all enabled power wells must have a reference
+		 * and all unneeded power wells must be disabled; verify this.
+		 */
+		intel_power_domains_verify_state(dev_priv);
 }
 
 /*
-- 
2.5.0

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Sanitize display INIT power domain enabling

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Sanitize display INIT power domain enabling
URL   : https://patchwork.freedesktop.org/series/21827/
State : failure

== Summary ==

Series 21827v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21827/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test core_prop_blob:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-subslice-total:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> INCOMPLETE (fi-byt-j1900)
                pass       -> INCOMPLETE (fi-byt-n2820)
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup create-close:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup create-fd-close:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_ctx_basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-files:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_ctx_exec:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_ctx_param:
        Subgroup basic:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-default-heavy:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
Test gem_exec_basic:
        Subgroup basic-blt:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-default:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup basic-render:
                pass       -> SKIP       (fi-byt-j1900)
                pass       -> SKIP       (fi-byt-n2820)
        Subgroup gtt-blt:
WARNING: Long output truncated
fi-bdw-gvtdvm failed to collect. IGT log at Patchwork_4293/fi-bdw-gvtdvm/igt.log

fd27e1e4c9b5dc11966b4953432bd6e0510da308 drm-tip: 2017y-03m-24d-08h-39m-20s UTC integration manifest
eff2a71 drm/i915: Sanitize display INIT power domain disabling
f6f4eaa drm/i915: Sanitize display INIT power domain enabling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4293/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Sanitize display INIT power domain enabling

[Imre Deak]

On Fri, Mar 24, 2017 at 02:36:54PM +0200, Imre Deak wrote:
> For consistency move the INIT power domain enabling to happen at the
> same call-stack level everywhere. So far we didn't do this enabling
> during:
> - on GEN9 big-core when resuming from system freeze
> - on VLV on the i915_drm_suspend_late() error path
> 
> Fortunately neither of these depended on display power wells being
> enabled until proper references are taken by the modeset code. (This
> also means the we can probably remove the INIT power domain get/put
> from these places as a follow-up, after some more auditing.)
> 
> The current unpaired enable/disable was noticed by Ville during some
> earlier review.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 6 ++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 --
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6d9944a..6b10e37 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -592,6 +592,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	intel_update_rawclk(dev_priv);
>  
>  	intel_power_domains_init_hw(dev_priv, false);
> +	/* Enable the INIT power domain wells for HW initialization. */
> +	intel_display_set_init_power(dev_priv, true);

Arr, this will result in HPD IRQs inited too early on VLV. Need to
rethink this.

>  
>  	intel_csr_ucode_init(dev_priv);
>  
> @@ -1559,6 +1561,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  		if (!fw_csr)
>  			intel_power_domains_init_hw(dev_priv, true);
>  
> +		intel_display_set_init_power(dev_priv, true);
> +
>  		goto out;
>  	}
>  
> @@ -1768,6 +1772,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
>  	if (IS_GEN9_LP(dev_priv) ||
>  	    !(dev_priv->suspended_to_idle && dev_priv->csr.dmc_payload))
>  		intel_power_domains_init_hw(dev_priv, true);
> +	/* Enable the INIT power domain wells for HW initialization. */
> +	intel_display_set_init_power(dev_priv, true);
>  
>  	i915_gem_sanitize(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 012bc35..ecc43c6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2715,8 +2715,6 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
>  		mutex_unlock(&power_domains->lock);
>  	}
>  
> -	/* For now, we need the power well to be always enabled. */
> -	intel_display_set_init_power(dev_priv, true);
>  	/* Disable power support if the user asked so. */
>  	if (!i915.disable_power_well)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx