| Message ID | 1446553874-22986-7-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > --- > 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; } 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. > +} > + > +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
On ti, 2015-11-03 at 13:31 +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@linux.intel.com> > --- > 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); > +} > + > +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); > +} > + > +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, We need to set a unique ID in .data, since we use it to look up power wells. I have a patch to fix up the other places missing an ID, but you could fix this one up already now. > + }, > + { > .name = "MISC IO power well", > .domains = SKL_DISPLAY_MISC_IO_POWER_DOMAINS, > .ops = &skl_power_well_ops,
On Wed, Nov 4, 2015 at 6:53 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com> >> --- >> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On 3 November 2015 at 12:31, Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote: > 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); > } If it's safe to shift the modeset_put_power_domains call to after post_plane_update, you might as well just put POWER_DOMAIN_MODESET in there, saving a call. (But see the comment on the other patch ...) Cheers, Daniel
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); +} + +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); +} + +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,
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@linux.intel.com> --- 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(-)