On Mon, Sep 21, 2015 at 11:26:06AM +0300, Jani Nikula wrote: > On Mon, 21 Sep 2015, Patrik Jakobsson <[email protected]> > 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 <[email protected]> > >> > --- > >> > 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. > > Are you referring to stuff that is merged? Am I missing something?
It's not merged afaik. Ville's patches are at: https://github.com/vsyrjala/linux/commits/aux_gmbus_power_domains > >> > >> That's of course assuming that AUX is the only reason why we need to > >> keep DC6 disabled here. > >> > > > > The upside with having get/put around bigger aux transfers is that we > > don't get tons of enable/disable lines in the log. My vote is that we > > keep this but also have your fine-grained get/puts. > > If it works without (and everything Ville is referring to above is > merged), I'd split these to a separate patch, and we can judge the > improvement on its own. Yes, and there are a few other places that I'd like to group so I'll split that out. > BR, > Jani. > > > > > > We also have an extra enable disable print in skl_disable_dc6() / > > skl_enable_dc6() which I think should be removed unless various DC on/off > > hacks > > are required outside of the power wells framework. > > > >> > 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 > > > > I don't know why DC6 is required for PW2 and at this point I don't see why > > the > > ordering should matter. Could you please explain? > > > >> > /* > >> > * 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. > > > > From what I've heard we don't need to care about DC5 atm. But I also think > > that > > a power well for DC5 is the way to go. Need to check with Damien what the > > requirements for DC5 are. > > > >> > } > >> > } > >> > } > >> > @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct > >> > drm_i915_private *dev_priv, > >> > skl_set_power_well(dev_priv, power_well, false); > >> > } > >> > > >> > +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv, > >> > + struct i915_power_well *power_well) > >> > +{ > >> > + /* Return true if disabling of DC6 is enabled */ > >> > + return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6); > >> > +} > >> > + > >> > +static void skl_dc6_state_on(struct drm_i915_private *dev_priv, > >> > + struct i915_power_well *power_well) > >> > +{ > >> > + skl_enable_dc6(dev_priv); > >> > +} > >> > + > >> > +static void skl_dc6_state_off(struct drm_i915_private *dev_priv, > >> > + struct i915_power_well *power_well) > >> > +{ > >> > + skl_disable_dc6(dev_priv); > >> > +} > >> > + > >> > +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv, > >> > + struct i915_power_well *power_well) > >> > +{ > >> > + if (power_well->count > 0) > >> > + skl_disable_dc6(dev_priv); > >> > + else > >> > + skl_enable_dc6(dev_priv); > >> > +} > >> > >> Nit: Looks like we usuall have these in the following order > >> sync_hw > >> enable > >> disable > >> > >> 'enabled' is a bit all over the place usually. I guess before or after the > >> rest is fine. > > > > Yes, better keep the current order. > > > >> I'm not really sure how I would name these. The current naming doesn't > >> really tell me they're power well ops. Maybe > >> skl_dc6_off_power_well_{enable,disable,sync_hw,enabled}() ? > > > > I agree, better make it clear that they are pw ops. > > > >> > + > >> > static void i9xx_always_on_power_well_noop(struct drm_i915_private > >> > *dev_priv, > >> > struct i915_power_well > >> > *power_well) > >> > { > >> > @@ -1546,6 +1570,14 @@ 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_state_ops = { > >> > >> _dc6_, and naming to match how the function are finally named of > >> course. > >> > >> > + .sync_hw = skl_dc6_sync_hw, > >> > + /* To enable power we turn the DC state off */ > >> > + .enable = skl_dc6_state_off, > >> > + .disable = skl_dc6_state_on, > >> > + .is_enabled = skl_dc6_state_enabled, > >> > +}; > >> > + > >> > static struct i915_power_well hsw_power_wells[] = { > >> > { > >> > .name = "always-on", > >> > @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = > >> > { > >> > .ops = &skl_power_well_ops, > >> > .data = SKL_DISP_PW_DDI_D, > >> > }, > >> > + { > >> > + .name = "DC6 state off", > >> > >> Just "DC6 off" perhaps? > >> > >> I wasn't able to think of a nice way to name this so that it would make > >> more sense in the logs. With this we would get > >> 'enabling DC6 off' and 'disabling DC6 off' which is a bit confusing. > >> Maybe we should at least put quotes around the power well name in the > >> debug message to make it a bit less funky? Probably not worth it to > >> add support for sully customized enable/disable log message. > > > > Agreed > > > >> > + .domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS, > >> > + .ops = &skl_power_well_ops, > >> > >> Surely you want to use the new ops you created? :) > > > > Oops :) > > > >> And here we need to be careful where in the list we insert the power > >> well. Based on what we saw earlier, the right place should be just > >> before PW2. That way DC6 gets disabled before PW2 is enabled, and > >> PW2 gets disabled before DC6 gets enabled. > > > > Yes, regardless of the ordering requirements it makes sense to move it up. > > > > Thanks for having a look > > -Patrik > > > >> > + }, > >> > }; > >> > > >> > static struct i915_power_well bxt_power_wells[] = { > >> > -- > >> > 2.1.4 > >> > > >> > _______________________________________________ > >> > Intel-gfx mailing list > >> > [email protected] > >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > >> -- > >> Ville Syrjälä > >> Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
