On Thu, Apr 16, 2015 at 02:22:11PM +0530, Animesh Manna wrote:
> From: Suketu Shah <suketu.j.shah@xxxxxxxxx>
>
> Add triggers for DC6 as per details provided in skl_enable_dc6
> and skl_disable_dc6 implementations.
>
> Also Call POSTING_READ for every write to a register to ensure
> it is written to immediately
>
> v1: Remove POSTING_READ and intel_prepare_ddi calls as they've been added in previous patches.
>
> v2:
> 1] Remove check for backlight disabled as it should be the case by that time.
> 2] Mark DC5 as disabled when enabling DC6.
> 3] Return from DC5-disabling function early if DC5 is already be disabled which can happen
> due to DC6-enabling earlier.
> 3] Ensure CSR firmware is loaded after resume from DC6 as corresponding memory contents won't
> be retained after runtime-suspend.
> 4] Ensure that CSR isn't identified as loaded before CSR-loading program is called during
> runtime-resume.
>
> v3: Rebase to latest
> Modified as per review comments from Imre and after discussion with Art:
> 1] DC6 should be preferably enabled when PG2 is disabled by SW as the check for PG1 being
> disabled is taken of by HW to enter DC6, and disabled when PG2 is enabled respectively.
> This helps save more power, especially in the case when display is disabled but GT is
> enabled. Accordingly, replacing DC5 trigger sequence with DC6 for SKL.
> 2] DC6 could be enabled from intel_runtime_suspend() function, if DC5 is already enabled.
> 3] Move CSR-load-status setting code from intel_runtime_suspend function to a new function.
>
> v4:
> 1] Enable/disable DC6 only when toggling the power-well using a newly defined macro ENABLE_DC6.
>
> v5:
> 1] Load CSR on system resume too as firmware may be lost on system suspend preventing
> enabling DC5, DC6.
> 2] DDI buffers shouldn't be programmed during driver-load/resume as it's already done
> during modeset initialization then and also that the encoder list is still uninitialized by
> then. Therefore, call intel_prepare_ddi function right after disabling DC6 but outside
> skl_disable_dc6 function and not during driver-load/resume.
>
> v6:
> 1] Rebase to latest.
> 2] Move SKL_ENABLE_DC6 macro definition from intel_display.c to intel_runtime_pm.c.
>
> v7:
> 1) Refactored the code for removing the warning got from checkpatch.
> 2) After adding dmc ver 1.0 support rebased on top of nightly. (Animesh)
>
> v8:
> - Reverted the changes done in v7.
> - Removed the condition check in skl_prepare_resune(). (Animesh)
>
> Issue: VIZ-2819
> Signed-off-by: A.Sunil Kamath <sunil.kamath@xxxxxxxxx>
> Signed-off-by: Suketu Shah <suketu.j.shah@xxxxxxxxx>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 30 +++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_runtime_pm.c | 43 +++++++++++++++++++++++++++------
> 2 files changed, 66 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index acd0e2b..0f590e3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -594,6 +594,8 @@ static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> static int intel_suspend_complete(struct drm_i915_private *dev_priv);
> static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
> bool rpm_resume);
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv);
> +
>
> static int i915_drm_suspend(struct drm_device *dev)
> {
> @@ -808,6 +810,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
>
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> hsw_disable_pc8(dev_priv);
> + else if (IS_SKYLAKE(dev_priv))
> + ret = skl_resume_prepare(dev_priv);
>
> intel_uncore_sanitize(dev);
> intel_power_domains_init_hw(dev_priv);
> @@ -1022,6 +1026,19 @@ static int i915_pm_resume(struct device *dev)
> return i915_drm_resume(drm_dev);
> }
>
> +static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> +{
> + /* Enabling DC6 is not a hard requirement to enter runtime D3 */
> +
> + /*
> + * This is to ensure that CSR isn't identified as loaded before
> + * CSR-loading program is called during runtime-resume.
> + */
> + intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> +
> + return 0;
> +}
> +
> static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
> {
> hsw_enable_pc8(dev_priv);
> @@ -1029,6 +1046,15 @@ static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> +static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> +{
> + struct drm_device *dev = dev_priv->dev;
> +
> + intel_csr_load_program(dev);
> +
> + return 0;
> +}
> +
> /*
> * Save all Gunit registers that may be lost after a D3 and a subsequent
> * S0i[R123] transition. The list of registers needing a save/restore is
> @@ -1487,6 +1513,8 @@ static int intel_runtime_resume(struct device *device)
>
> if (IS_GEN6(dev_priv))
> intel_init_pch_refclk(dev);
> + else if (IS_SKYLAKE(dev))
> + ret = skl_resume_prepare(dev_priv);
I resolved the conflicts with bxt support here assuming that there's no
functional conflicts since dc6 is skl-only. But please double-check just
to make sure.
> else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> hsw_disable_pc8(dev_priv);
> else if (IS_VALLEYVIEW(dev_priv))
> @@ -1519,6 +1547,8 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv)
> struct drm_device *dev = dev_priv->dev;
> int ret;
>
> + if (IS_SKYLAKE(dev))
> + ret = skl_suspend_complete(dev_priv);
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> ret = hsw_suspend_complete(dev_priv);
> else if (IS_VALLEYVIEW(dev))
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index da8c18d..7e6908e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,7 +49,8 @@
> * present for a given platform.
> */
>
> -#define GEN9_ENABLE_DC5(dev) (IS_SKYLAKE(dev))
> +#define GEN9_ENABLE_DC5(dev) 0
> +#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
>
> #define for_each_power_well(i, power_well, domain_mask, power_domains) \
> for (i = 0; \
> @@ -397,6 +398,16 @@ static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
> POSTING_READ(DC_STATE_EN);
> }
>
> +static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +{
> + /* TODO: Implementation to be done. */
> +}
> +
> +static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +{
> + /* TODO: Implementation to be done. */
> +}
> +
> static void skl_set_power_well(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well, bool enable)
> {
> @@ -444,9 +455,21 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> !I915_READ(HSW_PWR_WELL_BIOS),
> "Invalid for power well status to be enabled, unless done by the BIOS, \
> when request is to disable!\n");
> - if (GEN9_ENABLE_DC5(dev) &&
> - power_well->data == SKL_DISP_PW_2)
> - gen9_disable_dc5(dev_priv);
> + if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> + power_well->data == SKL_DISP_PW_2) {
> + if (SKL_ENABLE_DC6(dev)) {
> + skl_disable_dc6(dev_priv);
> + /*
> + * DDI buffer programming unnecessary during driver-load/resume
> + * as it's already done during modeset initialization then.
> + * It's also invalid here as encoder list is still uninitialized.
> + */
> + if (!dev_priv->power_domains.initializing)
> + intel_prepare_ddi(dev);
> + } else {
> + gen9_disable_dc5(dev_priv);
> + }
> + }
> I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> }
>
> @@ -464,17 +487,23 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> POSTING_READ(HSW_PWR_WELL_DRIVER);
> DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>
> - if (GEN9_ENABLE_DC5(dev) &&
> + if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> power_well->data == SKL_DISP_PW_2) {
> enum csr_state state;
> -
> + /* TODO: wait for a completion event or
> + * similar here instead of busy
> + * waiting using wait_for function.
> + */
> wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> FW_UNINITIALIZED, 1000);
> if (state != FW_LOADED)
> DRM_ERROR("CSR firmware not ready (%d)\n",
> state);
> else
> - gen9_enable_dc5(dev_priv);
> + if (SKL_ENABLE_DC6(dev))
> + skl_enable_dc6(dev_priv);
> + else
> + gen9_enable_dc5(dev_priv);
Yep, skl_set_power_well is definitely going over the top. Rule of thumb is
that you shouldn't use more than 3 tabs in a function, maybe 4 for an if
(error_condition) baile_out; check.
-Daniel
> }
> }
> }
> --
> 2.0.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
