>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Thursday, September 13, 2018 1:14 PM
>To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Manna, Animesh
><animesh.manna@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>; Ausmus,
>James <james.ausmus@xxxxxxxxx>
>Subject: Re: [PATCH] drm/i915/icl: Enable DC9 as lowest possible state during
>screen-off
>
>On Thu, Sep 13, 2018 at 12:31:09PM -0700, Anusha Srivatsa wrote:
>> From: Animesh Manna <animesh.manna@xxxxxxxxx>
>>
>> ICL supports DC5, DC6, and DC9. Enable DC9 during screen-off, and
>> enable
>> DC5/6 when appropriate.
>>
>> v2: (James Ausmus)
>> - Also handle ICL as GEN9_LP in i915_drm_suspend_late and
>> i915_drm_suspend_early
>> - Add DC9 to gen9_dc_mask for ICL
>> - Re-order GEN checks for newest platform first
>> - Use INTEL_GEN instead of INTEL_INFO->gen
>> - Use INTEL_GEN >= 11 instead of IS_ICELAKE
>> - Consolidate GEN checks
>>
>> v3: (James Ausmus)
>> - Also allow DC6 for ICL (Imre, Art)
>> - Simplify !(GEN >= 11) to GEN < 11 (Imre)
>>
>> v4: (James Ausmus)
>> - Don't call intel_power_sequencer_reset after DC9 for Gen11+, as the
>> PPS regs are Always On
>> - Rebase against upstream changes
>>
>> v5: (Anusha Srivatsa)
>> - rebased against the latest upstream changes.
>
>First concern with this patch is regarding the tests...
>How is this getting tested? Are you able to see DC6 and DC9?
>
>>
>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
>> Signed-off-by: James Ausmus <james.ausmus@xxxxxxxxx>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 20 ++++++++++++++---
>> drivers/gpu/drm/i915/intel_drv.h | 3 +++
>> drivers/gpu/drm/i915/intel_runtime_pm.c | 29
>> +++++++++++++++----------
>> 3 files changed, 37 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c index 2ddf8538cb47..86a83e0a7ef2
>> 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1855,7 +1855,7 @@ static int i915_drm_resume_early(struct
>> drm_device *dev)
>>
>> intel_uncore_resume_early(dev_priv);
>>
>> - if (IS_GEN9_LP(dev_priv)) {
>> + if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
>> gen9_sanitize_dc_state(dev_priv);
>> bxt_disable_dc9(dev_priv);
>> } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { @@
>> -2622,7 +2622,10 @@ static int intel_runtime_suspend(struct device *kdev)
>> intel_uncore_suspend(dev_priv);
>>
>> ret = 0;
>> - if (IS_GEN9_LP(dev_priv)) {
>> + if (IS_ICELAKE(dev_priv)) {
>> + icl_display_core_uninit(dev_priv);
>> + bxt_enable_dc9(dev_priv);
>> + } else if (IS_GEN9_LP(dev_priv)) {
>> bxt_display_core_uninit(dev_priv);
>> bxt_enable_dc9(dev_priv);
>> } else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) { @@
>> -2707,7 +2710,18 @@ static int intel_runtime_resume(struct device *kdev)
>> if (intel_uncore_unclaimed_mmio(dev_priv))
>> DRM_DEBUG_DRIVER("Unclaimed access during suspend,
>bios?\n");
>>
>> - if (IS_GEN9_LP(dev_priv)) {
>> + if (IS_ICELAKE(dev_priv)) {
>
>commit message mention the use of INTEL_GEN instead of ICELAKE, but it seems
>we are missing some replacements here....
Actually double checked with the internal version of this patch, seems like IS_ICELAKE was what was used in the code...
Maybe change the commit log?
>
>> + bxt_disable_dc9(dev_priv);
>> + icl_display_core_init(dev_priv, true);
>> + if (dev_priv->csr.dmc_payload) {
>> + if (dev_priv->csr.allowed_dc_mask &
>> + DC_STATE_EN_UPTO_DC6)
>> + skl_enable_dc6(dev_priv);
>> + else if (dev_priv->csr.allowed_dc_mask &
>> + DC_STATE_EN_UPTO_DC5)
>> + gen9_enable_dc5(dev_priv);
>> + }
>> + } else if (IS_GEN9_LP(dev_priv)) {
>> bxt_disable_dc9(dev_priv);
>> bxt_display_core_init(dev_priv, true);
>> if (dev_priv->csr.dmc_payload &&
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index bf1c38728a59..f0385fe5bb15 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1627,6 +1627,7 @@ void bxt_enable_dc9(struct drm_i915_private
>> *dev_priv); void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>> void gen9_enable_dc5(struct drm_i915_private *dev_priv); unsigned int
>> skl_cdclk_get_vco(unsigned int freq);
>> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
>> void intel_dp_get_m_n(struct intel_crtc *crtc,
>> struct intel_crtc_state *pipe_config); void
>> intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n); @@
>> -1966,6 +1967,8 @@ int intel_power_domains_init(struct
>> drm_i915_private *); void intel_power_domains_cleanup(struct
>> drm_i915_private *dev_priv); void intel_power_domains_init_hw(struct
>> drm_i915_private *dev_priv, bool resume); void
>> intel_power_domains_fini_hw(struct drm_i915_private *dev_priv);
>> +void icl_display_core_init(struct drm_i915_private *dev_priv, bool
>> +resume); void icl_display_core_uninit(struct drm_i915_private
>> +*dev_priv);
>> void intel_power_domains_enable(struct drm_i915_private *dev_priv);
>> void intel_power_domains_disable(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 480dadb1047b..3e2c936217f8 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -551,7 +551,9 @@ static u32 gen9_dc_mask(struct drm_i915_private
>*dev_priv)
>> u32 mask;
>>
>> mask = DC_STATE_EN_UPTO_DC5;
>> - if (IS_GEN9_LP(dev_priv))
>> + if (INTEL_GEN(dev_priv) >= 11)
>> + mask |= DC_STATE_EN_UPTO_DC6 | DC_STATE_EN_DC9;
>> + else if (IS_GEN9_LP(dev_priv))
>> mask |= DC_STATE_EN_DC9;
>> else
>> mask |= DC_STATE_EN_UPTO_DC6;
>> @@ -624,8 +626,8 @@ void bxt_enable_dc9(struct drm_i915_private
>*dev_priv)
>> assert_can_enable_dc9(dev_priv);
>>
>> DRM_DEBUG_KMS("Enabling DC9\n");
>> -
>> - intel_power_sequencer_reset(dev_priv);
>> + if (INTEL_GEN(dev_priv) < 11)
>> + intel_power_sequencer_reset(dev_priv);
>> gen9_set_dc_state(dev_priv, DC_STATE_EN_DC9); }
>>
>> @@ -707,7 +709,7 @@ static void assert_can_enable_dc6(struct
>drm_i915_private *dev_priv)
>> assert_csr_loaded(dev_priv);
>> }
>>
>> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> {
>> assert_can_enable_dc6(dev_priv);
>>
>> @@ -2968,17 +2970,20 @@ static uint32_t get_allowed_dc_mask(const struct
>drm_i915_private *dev_priv,
>> int requested_dc;
>> int max_dc;
>>
>> - if (IS_GEN9_BC(dev_priv) || INTEL_INFO(dev_priv)->gen >= 10) {
>> - max_dc = 2;
>> - mask = 0;
>> - } else if (IS_GEN9_LP(dev_priv)) {
>> - max_dc = 1;
>> + if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
>> + if (INTEL_GEN(dev_priv) >= 11)
>> + max_dc = 2;
>
>This is supper confusing... Could we create a new full block for INTEL_GEN >= 11?
Yes...that sounds good.
Anusha
>
>> + else
>> + max_dc = 1;
>> /*
>> * DC9 has a separate HW flow from the rest of the DC states,
>> * not depending on the DMC firmware. It's needed by system
>> * suspend/resume, so allow it unconditionally.
>> */
>> mask = DC_STATE_EN_DC9;
>> + } else if (IS_GEN10(dev_priv) || IS_GEN9_BC(dev_priv)) {
>> + max_dc = 2;
>> + mask = 0;
>> } else {
>> max_dc = 0;
>> mask = 0;
>> @@ -3513,8 +3518,8 @@ static void cnl_display_core_uninit(struct
>drm_i915_private *dev_priv)
>> I915_WRITE(CHICKEN_MISC_2, val);
>> }
>>
>> -static void icl_display_core_init(struct drm_i915_private *dev_priv,
>> - bool resume)
>> +void icl_display_core_init(struct drm_i915_private *dev_priv,
>> + bool resume)
>> {
>> struct i915_power_domains *power_domains = &dev_priv-
>>power_domains;
>> struct i915_power_well *well;
>> @@ -3565,7 +3570,7 @@ static void icl_display_core_init(struct
>drm_i915_private *dev_priv,
>> icl_mbus_init(dev_priv);
>> }
>>
>> -static void icl_display_core_uninit(struct drm_i915_private
>> *dev_priv)
>> +void icl_display_core_uninit(struct drm_i915_private *dev_priv)
>> {
>> struct i915_power_domains *power_domains = &dev_priv-
>>power_domains;
>> struct i915_power_well *well;
>> --
>> 2.17.1
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
