Re: [PATCH v3 10/12] drm/i915/gen9: Turn DC handling into a power well

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 16, 2015 at 8:28 PM, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> On ma, 2015-11-16 at 15:01 +0100, Patrik Jakobsson wrote:
>> Handle DC off as a power well where enabling the power well will prevent
>> the DMC to enter selected DC states (required around modesets and Aux
>> A). Disabling the power well will allow DC states again. For now the
>> highest DC state is DC6 for Skylake and DC5 for Broxton but will be
>> configurable for Skylake in a later patch.
>>
>> v2: Check both DC5 and DC6 bits in power well enabled function (Ville)
>> v3:
>> - Remove unneeded DC_OFF case in skl_set_power_well() (Imre)
>> - Add PW2 dependency to DC_OFF (Imre)
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@xxxxxxxxxxxxxxx>
>> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c         |   6 --
>>  drivers/gpu/drm/i915/i915_reg.h         |   1 +
>>  drivers/gpu/drm/i915/intel_display.c    |   6 ++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 111 +++++++++++++++++++++++---------
>>  4 files changed, 89 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 5a63f9a..0c7f435 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev)
>>
>>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>>  {
>> -     if (dev_priv->csr.dmc_payload)
>> -             skl_enable_dc6(dev_priv);
>> -
>>       return 0;
>>  }
>>
>> @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>>
>>  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>>  {
>> -     if (dev_priv->csr.dmc_payload)
>> -             skl_disable_dc6(dev_priv);
>> -
>>       return 0;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 0f3849f..b6a3525 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -638,6 +638,7 @@ enum skl_disp_power_wells {
>>
>>       /* Not actual bit groups. Used as IDs for lookup_power_well() */
>>       SKL_DISP_PW_ALWAYS_ON,
>> +     SKL_DISP_PW_DC_OFF,
>>  };
>>
>>  #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 978b1b9..21385a0 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13323,6 +13323,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>>                       to_intel_crtc_state(crtc->state)->update_pipe;
>>               unsigned long put_domains = 0;
>>
>> +             if (modeset)
>> +                     intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>> +
>>               if (modeset && crtc->state->active) {
>>                       update_scanline_offset(to_intel_crtc(crtc));
>>                       dev_priv->display.crtc_enable(crtc);
>> @@ -13346,6 +13349,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>>                       modeset_put_power_domains(dev_priv, put_domains);
>>
>>               intel_post_plane_update(intel_crtc);
>> +
>> +             if (modeset)
>> +                     intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>>       }
>>
>>       /* FIXME: add subpixel order */
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index edf753e..b6ee48e 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.
>>   */
>>
>> -#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;                                                     \
>>            i < (power_domains)->power_well_count &&                   \
>> @@ -309,9 +306,15 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>>  #define SKL_DISPLAY_DDI_D_POWER_DOMAINS (            \
>>       BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |            \
>>       BIT(POWER_DOMAIN_INIT))
>> +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS (           \
>> +     SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |         \
>> +     BIT(POWER_DOMAIN_MODESET) |                     \
>> +     BIT(POWER_DOMAIN_AUX_A) |                       \
>> +     BIT(POWER_DOMAIN_INIT))
>>  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (                \
>>       (POWER_DOMAIN_MASK & ~(                         \
>> -     SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |       \
>> +     SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |         \
>> +     SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |            \
>>       BIT(POWER_DOMAIN_INIT))
>>
>>  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (              \
>> @@ -339,6 +342,11 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>>       BIT(POWER_DOMAIN_AUX_A) |                       \
>>       BIT(POWER_DOMAIN_PLLS) |                        \
>>       BIT(POWER_DOMAIN_INIT))
>> +#define BXT_DISPLAY_DC_OFF_POWER_DOMAINS (           \
>> +     BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS |         \
>> +     BIT(POWER_DOMAIN_MODESET) |                     \
>> +     BIT(POWER_DOMAIN_AUX_A) |                       \
>> +     BIT(POWER_DOMAIN_INIT))
>>  #define BXT_DISPLAY_ALWAYS_ON_POWER_DOMAINS (                \
>>       (POWER_DOMAIN_MASK & ~(BXT_DISPLAY_POWERWELL_1_POWER_DOMAINS |  \
>>       BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |       \
>> @@ -486,15 +494,6 @@ static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>>       gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
>>  }
>>
>> -static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
>> -{
>> -     assert_can_disable_dc5(dev_priv);
>> -
>> -     DRM_DEBUG_KMS("Disabling DC5\n");
>> -
>> -     gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>> -}
>> -
>>  static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
>>  {
>>       struct drm_device *dev = dev_priv->dev;
>> @@ -522,6 +521,14 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>>                 "DC6 already programmed to be disabled.\n");
>>  }
>>
>> +static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
>> +{
>> +     assert_can_disable_dc5(dev_priv);
>> +     assert_can_disable_dc6(dev_priv);
>> +
>> +     gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>> +}
>> +
>>  void skl_enable_dc6(struct drm_i915_private *dev_priv)
>>  {
>>       assert_can_enable_dc6(dev_priv);
>> @@ -589,17 +596,13 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>                               "Invalid for power well status to be enabled, unless done by the BIOS, \
>>                               when request is to disable!\n");
>>                       if (power_well->data == SKL_DISP_PW_2) {
>> -                             if (GEN9_ENABLE_DC5(dev))
>> -                                     gen9_disable_dc5(dev_priv);
>> -                             if (SKL_ENABLE_DC6(dev)) {
>> -                                     /*
>> -                                      * 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);
>> -                             }
>> +                             /*
>> +                              * 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);
>>                       }
>>                       I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>>               }
>> @@ -617,10 +620,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>                       I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask);
>>                       POSTING_READ(HSW_PWR_WELL_DRIVER);
>>                       DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>> -
>> -                     if (GEN9_ENABLE_DC5(dev) &&
>> -                             power_well->data == SKL_DISP_PW_2)
>> -                                     gen9_enable_dc5(dev_priv);
>>               }
>>       }
>>
>> @@ -695,6 +694,40 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>>       skl_set_power_well(dev_priv, power_well, false);
>>  }
>>
>> +static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
>> +                                       struct i915_power_well *power_well)
>> +{
>> +     return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
>> +}
>> +
>> +static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
>> +                                      struct i915_power_well *power_well)
>> +{
>> +     gen9_disable_dc5_dc6(dev_priv);
>> +}
>> +
>> +static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>> +                                       struct i915_power_well *power_well)
>> +{
>> +     if (IS_SKYLAKE(dev_priv))
>> +             skl_enable_dc6(dev_priv);
>> +     else
>> +             gen9_enable_dc5(dev_priv);
>> +}
>> +
>> +static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
>> +                                       struct i915_power_well *power_well)
>> +{
>> +     if (power_well->count > 0) {
>> +             gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>> +     } else {
>> +             if (IS_SKYLAKE(dev_priv))
>> +                     gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
>> +             else
>> +                     gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
>> +     }
>> +}
>> +
>>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>>                                          struct i915_power_well *power_well)
>>  {
>> @@ -1517,6 +1550,13 @@ static const struct i915_power_well_ops skl_power_well_ops = {
>>       .is_enabled = skl_power_well_enabled,
>>  };
>>
>> +static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
>> +     .sync_hw = gen9_dc_off_power_well_sync_hw,
>> +     .enable = gen9_dc_off_power_well_enable,
>> +     .disable = gen9_dc_off_power_well_disable,
>> +     .is_enabled = gen9_dc_off_power_well_enabled,
>> +};
>> +
>>  static struct i915_power_well hsw_power_wells[] = {
>>       {
>>               .name = "always-on",
>> @@ -1691,6 +1731,12 @@ static struct i915_power_well skl_power_wells[] = {
>>               .data = SKL_DISP_PW_MISC_IO,
>>       },
>>       {
>> +             .name = "DC off",
>> +             .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
>> +             .ops = &gen9_dc_off_power_well_ops,
>> +             .data = SKL_DISP_PW_DC_OFF,
>> +     },
>> +     {
>>               .name = "power well 2",
>>               .domains = SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS,
>>               .ops = &skl_power_well_ops,
>> @@ -1769,7 +1815,14 @@ static struct i915_power_well bxt_power_wells[] = {
>>               .domains = BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS,
>>               .ops = &skl_power_well_ops,
>>               .data = SKL_DISP_PW_2,
>> -     }
>> +     },
>> +     {
>> +             .name = "DC off",
>> +             .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
>> +             .ops = &gen9_dc_off_power_well_ops,
>> +             .data = SKL_DISP_PW_DC_OFF,
>> +     },
>> +
>
> You forgot to move this one. With that fixed the patch looks ok and my
> R-b holds.

Yea sorry about that. There's already a v4 available.

>
>>  };
>>
>>  #define set_power_wells(power_domains, __power_wells) ({             \
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux