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 <[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. > > > > 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.
I just had the idea that POWER_DOMAIN_INIT might be a nice fit for this, but that would also keep the DDI power wells on even though we don't need the firmware for those. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
