On Wed, Nov 4, 2015 at 6:53 PM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Tue, Nov 03, 2015 at 01:31:12PM +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 but will be configurable in a later patch.
>>
>> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@xxxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 6 --
>> drivers/gpu/drm/i915/intel_display.c | 6 ++
>> drivers/gpu/drm/i915/intel_runtime_pm.c | 107 +++++++++++++++++++++-----------
>> 3 files changed, 78 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 37319b0..e190237 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1069,9 +1069,6 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>> {
>> skl_uninit_cdclk(dev_priv);
>>
>> - if (dev_priv->csr.dmc_payload)
>> - skl_enable_dc6(dev_priv);
>> -
>> return 0;
>> }
>>
>> @@ -1116,9 +1113,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);
>> -
>> skl_init_cdclk(dev_priv);
>> intel_csr_load_program(dev_priv);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c6d60b8..e401871 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13296,6 +13296,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);
>> @@ -13319,6 +13322,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 c901b19..042d92f 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 && \
>> @@ -336,10 +333,15 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>> SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS | \
>> BIT(POWER_DOMAIN_PLLS) | \
>> BIT(POWER_DOMAIN_INIT))
>> +#define SKL_DISPLAY_DC_OFF_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_1_POWER_DOMAINS | \
>> SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
>> - SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) | \
>> + SKL_DISPLAY_MISC_IO_POWER_DOMAINS | \
>> + SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) | \
>> BIT(POWER_DOMAIN_INIT))
>>
>> #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS ( \
>> @@ -511,20 +513,6 @@ static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>> POSTING_READ(DC_STATE_EN);
>> }
>>
>> -static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
>> -{
>> - uint32_t val;
>> -
>> - assert_can_disable_dc5(dev_priv);
>> -
>> - DRM_DEBUG_KMS("Disabling DC5\n");
>> -
>> - val = I915_READ(DC_STATE_EN);
>> - val &= ~DC_STATE_EN_UPTO_DC5;
>> - I915_WRITE(DC_STATE_EN, val);
>> - POSTING_READ(DC_STATE_EN);
>> -}
>> -
>> static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
>> {
>> struct drm_device *dev = dev_priv->dev;
>> @@ -553,6 +541,21 @@ 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)
>> +{
>> + uint32_t val;
>> +
>> + assert_can_disable_dc5(dev_priv);
>> + assert_can_disable_dc6(dev_priv);
>> +
>> + DRM_DEBUG_KMS("Disabling DC5 and DC6\n");
>> +
>> + val = I915_READ(DC_STATE_EN);
>> + val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
>> + I915_WRITE(DC_STATE_EN, val);
>> + POSTING_READ(DC_STATE_EN);
>> +}
>> +
>> void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> {
>> uint32_t val;
>> @@ -632,17 +635,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);
>> }
>> @@ -658,17 +657,13 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>> } else {
>> if (enable_requested) {
>> if (IS_SKYLAKE(dev) &&
>> - (power_well->data == SKL_DISP_PW_1))
>> + (power_well->data == SKL_DISP_PW_1)) {
>> DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
>> - else {
>> + } else {
>> 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);
>> }
>> }
>>
>> @@ -743,6 +738,36 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>> skl_set_power_well(dev_priv, power_well, false);
>> }
>>
>> +static bool skl_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_DC6);
>
> This doesn't handle DC5...
>
>> +}
>> +
>> +static void skl_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 skl_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);
>
> ...whereas this one tries to handle DC5.
>
> Assuming we want to handle DC5 and DC6 with a single power well (to make
> it easier to change between DC5 and DC6 using a module parameter on SKL,
> skl_dc_off_power_well_enabled() should probably just be:
>
> {
> return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> }
Thanks, that slipped me by. Checking DC5 and DC6 like you do above is
what we want.
>
> Otherwise the DC off power well bits look all right to me. The rest
> presumably needs some coordination with Imre's work since the PW1 and
> MISC IO stuff should be going away from here.
Yes, I'm syncing this with Imre.
>
>> +}
>> +
>> +static void skl_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
>> + struct i915_power_well *power_well)
>> +{
>> + if (power_well->count > 0)
>> + skl_dc_off_power_well_enable(dev_priv, power_well);
>> + else
>> + skl_dc_off_power_well_disable(dev_priv, power_well);
>> +}
>> +
>> static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>> struct i915_power_well *power_well)
>> {
>> @@ -1574,6 +1599,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 skl_dc_off_power_well_ops = {
>> + .sync_hw = skl_dc_off_power_well_sync_hw,
>> + .enable = skl_dc_off_power_well_enable,
>> + .disable = skl_dc_off_power_well_disable,
>> + .is_enabled = skl_dc_off_power_well_enabled,
>> +};
>> +
>> static struct i915_power_well hsw_power_wells[] = {
>> {
>> .name = "always-on",
>> @@ -1738,6 +1770,11 @@ static struct i915_power_well skl_power_wells[] = {
>> .data = SKL_DISP_PW_1,
>> },
>> {
>> + .name = "DC off",
>> + .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
>> + .ops = &skl_dc_off_power_well_ops,
>> + },
>> + {
>> .name = "MISC IO power well",
>> .domains = SKL_DISPLAY_MISC_IO_POWER_DOMAINS,
>> .ops = &skl_power_well_ops,
>> --
>> 2.1.4
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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
