On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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.
My feeling is that the complexity of DC vs PW is only going to grow so I'd
seperate DC5 and DC6. Not much overhead if we do as you say above.
Those macros seems a bit excessive to me. Can we drop them? The powerwell is
only available if we explicitly say so.
> 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.
We don't do anything special for DC9 since GEN9_ENABLE_DC5(dev) is always zero.
We can probably handle it similarly when needed. At least lets assume we don't
have a problem just yet :)
> 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.
I like the simplicity with the POWER_DOMAIN_INIT approach you describe in the
other mail. Not sure what we aim at providing wrt powersaving when no firmware
is loaded.
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
