[Intel-gfx] [RFC PATCH] drm/i915/skl: Add DC6 disabling as a power well
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Sep 17 04:14:37 PDT 2015
On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > We need to be able to control if DC6 is allowed or not. Much like
> > requesting power to a specific piece of the hardware we need to be able
> > to request that we don't enter DC6 during certain hw access.
> >
> > To solve this without introducing too much infrastructure I'm hooking
> > into the power well / power domain framework. DC6 prevention is modeled
> > much like an enabled power well. Thus I'm using the terminology on/off
> > for DC states instead of enable/disable.
> >
> > The problem that started this work is the need for DC6 to be disabled
> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > patch.
> >
> > This is posted as an RFC since DMC and DC state handling is being
> > reworked and will possibly affect the outcome of this patch. The patch
> > has known warnings.
> >
> > Signed-off-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 9 +++++
> > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > 3 files changed, 64 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4823184..c2c1ad2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >
> > + intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
>
> These I think shouldn't be necessary with my
> intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> itself grab the appropriate power domain.
>
> That's of course assuming that AUX is the only reason why we need to
> keep DC6 disabled here.
>
> > intel_dp_set_link_params(intel_dp, crtc->config);
> >
> > intel_ddi_init_dp_buf_reg(intel_encoder);
> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > intel_dp_complete_link_train(intel_dp);
> > if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> > intel_dp_stop_link_train(intel_dp);
> > +
> > + intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > } else if (type == INTEL_OUTPUT_HDMI) {
> > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >
> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >
> > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > + intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > intel_edp_panel_vdd_on(intel_dp);
> > intel_edp_panel_off(intel_dp);
> > +
> > + intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > }
> >
> > if (IS_SKYLAKE(dev))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..82489ad 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> > bool override, unsigned int mask);
> > bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> > enum dpio_channel ch, bool override);
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >
> >
> > /* intel_pm.c */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3f682a1..e30c9a6 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -335,6 +335,10 @@ 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_DC6_OFF_POWER_DOMAINS ( \
> > + SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> > + BIT(POWER_DOMAIN_AUX_A))
> > +
> > #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS ( \
> > (POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS | \
> > SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> > "DC6 already programmed to be disabled.\n");
> > }
> >
> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > {
> > uint32_t val;
> >
> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > POSTING_READ(DC_STATE_EN);
> > }
> >
> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > {
> > uint32_t val;
> >
> > @@ -628,10 +632,8 @@ 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) || SKL_ENABLE_DC6(dev)) &&
> > - power_well->data == SKL_DISP_PW_2) {
> > + if (power_well->data == SKL_DISP_PW_2) {
> > if (SKL_ENABLE_DC6(dev)) {
> > - skl_disable_dc6(dev_priv);
>
> Hmm. So the ordering needs to be
> disable dc6 -> enable pw2
>
> > /*
> > * DDI buffer programming unnecessary during driver-load/resume
> > * as it's already done during modeset initialization then.
> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > */
> > 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);
> > }
> >
> > @@ -660,8 +661,7 @@ 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) || SKL_ENABLE_DC6(dev)) &&
> > - power_well->data == SKL_DISP_PW_2) {
> > + if (power_well->data == SKL_DISP_PW_2) {
> > enum csr_state state;
> > /* TODO: wait for a completion event or
> > * similar here instead of busy
> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > */
> > wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> > FW_UNINITIALIZED, 1000);
> > - if (state != FW_LOADED)
> > + if (state != FW_LOADED) {
> > DRM_ERROR("CSR firmware not ready (%d)\n",
> > - state);
> > - else
> > - if (SKL_ENABLE_DC6(dev))
> > - skl_enable_dc6(dev_priv);
> > - else
> > - gen9_enable_dc5(dev_priv);
> > + state);
> > + }
>
> and here we need
> disable pw2 -> enable dc6
>
> That's symmetric which is great for using power wells here since we walk
> the power wells array forward when enabling, and backwards when
> disabling.
>
> I'm thinking that we could also move the dc5 stuff into a power well.
> Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> what the requirements wrt. dc5 are. If they are the same as for dc6,
> then a single dc power well would do, otherwise we would need two, each
> with its own domains.
BTW after a bit more look, I think we could probably simplify things
quite a bit with this move. We could perhaps then set power_well->data
to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
convert the enable/disable dc5/6 into somehting like:
gen9_enable_dc_state(dev_priv, uint32_t state)
{
// csr wait and the debugmask thing could go here
val = I915_READ(DC_STATE_EN);
val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
val |= state;
I915_WRITE(DC_STATE_EN, val);
POSTING_READ(DC_STATE_EN);
}
gen9_disable_dc_state(dev_priv, uint32_t val)
{
uint32_t val;
val = I915_READ(DC_STATE_EN);
val &= ~state;
I915_WRITE(DC_STATE_EN, val);
POSTING_READ(DC_STATE_EN);
}
We could even put these directly in the power well hooks, and share
those for DC5 and DC6. But that would perhaps mean losing the
can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
But perhaps it would be cleaners to have separate power well ops for dc5
and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
macros which I supposed we'd need to check in the power well hooks.
But this unification could be a separate patch. First we can just
introduce the new power wells using the existing dc5/dc6 enable/disable
functions we have.
I didn't look at the dc9 stuff yet, so not sure if that could be handled
in a similar fashion.
Also I think currently we just disable runtime PM when the firmware
hasn't yet been loaded. But I think we would need to change that to hold
a DC5/DC5 references. I guess to do this properly we should a new power
domain for this purpose, but I'm not sure that's really worth it for a
single user use case.
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list