[Intel-gfx] [PATCH 6/7] drm/i915: Implement PHY lane power gating for CHV
Deepak S
deepak.s at linux.intel.com
Tue May 12 20:19:47 PDT 2015
On Monday 11 May 2015 05:13 PM, Ville Syrjälä wrote:
> On Sat, May 09, 2015 at 11:05:27AM +0530, Deepak S wrote:
>>
>> On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
>>> On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
>>>> On Friday 10 April 2015 08:51 PM, ville.syrjala at linux.intel.com wrote:
>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>
>>>>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
>>>>> are needed always, but for DP we can enable only the needed lanes. And
>>>>> when the port is not used all lanes can be power gated. This could
>>>>> reduce power consumption a bit when only a subset of the lanes in the
>>>>> PHY are required.
>>>>>
>>>>> A bit of extra care is needed to reconstruct the initial state of the
>>>>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
>>>>> actual lane status from the DPLL/PHY_STATUS registers and use that to
>>>>> determine which lanes ought to be powergated initially.
>>>>>
>>>>> Also sprinkle a few debug prints around so that we can monitor the
>>>>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
>>>>> it.
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_reg.h | 2 +
>>>>> drivers/gpu/drm/i915/intel_dp.c | 8 ++++
>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 5 +++
>>>>> drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>>>>> 5 files changed, 82 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 977bad6..34c366a 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>>>>> #define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
>>>>> #define DPLL_PORTD_READY_MASK (0xf)
>>>>> #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>>>> +#define PHY_CH_POWER_DOWN_OVRD_EN(phy, ch) (1 << (2*(phy)+(ch)+27))
>>>>> #define PHY_LDO_DELAY_0NS 0x0
>>>>> #define PHY_LDO_DELAY_200NS 0x1
>>>>> #define PHY_LDO_DELAY_600NS 0x2
>>>>> #define PHY_LDO_SEQ_DELAY(delay, phy) ((delay) << (2*(phy)+23))
>>>>> +#define PHY_CH_POWER_DOWN_OVRD(mask, phy, ch) ((mask) << (8*(phy)+4*(ch)+11))
>>>>> #define PHY_CH_SU_PSR 0x1
>>>>> #define PHY_CH_DEEP_PSR 0x7
>>>>> #define PHY_CH_POWER_MODE(mode, phy, ch) ((mode) << (6*(phy)+3*(ch)+2))
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index ac38fd8..0b43f99 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>>>>>
>>>>> intel_dp_link_down(intel_dp);
>>>>>
>>>>> + chv_powergate_phy_lanes(encoder, 0xf);
>>>>> +
>>>>> mutex_lock(&dev_priv->dpio_lock);
>>>>>
>>>>> /* Propagate soft reset to data lane reset */
>>>>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>>> if (IS_VALLEYVIEW(dev))
>>>>> vlv_init_panel_power_sequencer(intel_dp);
>>>>>
>>>>> + if (IS_CHERRYVIEW(dev)) {
>>>>> + /* FIXME deal with lane reversal */
>>>>> + lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
>>>>> + chv_powergate_phy_lanes(encoder, lane_mask);
>>>>> + }
>>>>> +
>>>>> intel_dp_enable_port(intel_dp);
>>>>>
>>>>> edp_panel_vdd_on(intel_dp);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 3ec829a..54bcca8 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>>>>
>>>>> void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>>>>>
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
>>>>> +
>>>>> /* intel_pm.c */
>>>>> void intel_init_clock_gating(struct drm_device *dev);
>>>>> void intel_suspend_hw(struct drm_device *dev);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> index 24b0aa1..f5842c3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>>> I915_WRITE(intel_hdmi->hdmi_reg, temp);
>>>>> POSTING_READ(intel_hdmi->hdmi_reg);
>>>>> }
>>>>> +
>>>>> + if (IS_CHERRYVIEW(dev))
>>>>> + chv_powergate_phy_lanes(encoder, 0xf);
>>>>> }
>>>>>
>>>>> static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>>>>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>>>>> intel_crtc->config->has_hdmi_sink,
>>>>> adjusted_mode);
>>>>>
>>>>> + chv_powergate_phy_lanes(encoder, 0x0);
>>>>> +
>>>>> intel_enable_hdmi(encoder);
>>>>>
>>>>> vlv_wait_port_ready(dev_priv, dport, 0x0);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> index 5cd8a51..d9e00d3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>>>
>>>>> dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>>>> I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> + DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> + phy, dev_priv->chv_phy_control);
>>>>> }
>>>>>
>>>>> static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>>
>>>>> if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>>>>> phy = DPIO_PHY0;
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>>>>> assert_pll_disabled(dev_priv, PIPE_A);
>>>>> assert_pll_disabled(dev_priv, PIPE_B);
>>>>> } else {
>>>>> phy = DPIO_PHY1;
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>>>>> assert_pll_disabled(dev_priv, PIPE_C);
>>>>> }
>>>>>
>>>>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>> I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>>
>>>>> vlv_set_power_well(dev_priv, power_well, false);
>>>>> +
>>>>> + DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> + phy, dev_priv->chv_phy_control);
>>>>> +}
>>>>> +
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
>>>>> +{
>>>>> + struct drm_device *dev = encoder->base.dev;
>>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>>>> + enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
>>>>> + enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
>>>>> +
>>>>> + dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
>>>>> + dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
>>>>> + I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> + DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> + phy, ch, mask, dev_priv->chv_phy_control);
>>>>> }
>>>>>
>>>> Hi Ville,
>>>>
>>>> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
>>>> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
>>> Doh. Nice catch. Looks like the dynamic power down bits default to off.
>>> I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
>>> I'll cook up a patch to flip them and run a few tests to see if stuff
>>> still works.
>> Ok Thanks
>> One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?
> I've taken a stab at this, and sadly its making things quite a bit more
> complicated. Whenever the lane is power gated we also lose the relevant
> registers, so if we power gate the unused lanes before programming all
> the PCS/TX registers, we need to change the code so that it doesn't
> try to program the registers in the power gated lanes.
>
> Also I'm not entirely sure how LRC etc. works when some/all lanes power
> gated. There are some notes that sort of make me think the hw internally
> powers up everything regardless of the input signals to guarantee that
> things are properly calibrated, but if we lose the registers does that
> mean we lose the calibrationresults as well?
>
> And on the other hand there are a bunch of other notes that make me think
> they assumed we would only powergate the unused lanes after link training
> has finished, which again suggests that we should train the link with max
> lanes initially and drop the unused lanes afterwards. But then it's not
> clear what happens if we need to increase the number of lanes afterwards
> without doing a full PHY reinit.
>
> I also had the machine hard hang if I enabled the CL1 PSR power down bit
> in the single channel PHY, whereas setting it in the dual channel PHY is
> apparently OK.
>
> So looks like this topic needs a bit more study.
Yes ville. I agree we need some more study. I will also do some
experiments to find the HW behavior in different scenario.
>>>> Thanks
>>>> Deepak
>>>>
>>>>> static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>>>>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>>> * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>>>> * workaround never ever read DISPLAY_PHY_CONTROL, and
>>>>> * instead maintain a shadow copy ourselves. Use the actual
>>>>> - * power well state to reconstruct the expected initial
>>>>> - * value.
>>>>> + * power well state and lane status to reconstruct the
>>>>> + * expected initial value.
>>>>> */
>>>>> dev_priv->chv_phy_control =
>>>>> + PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
>>>>> + PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
>>>>> + PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>>>>> PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>>>> PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>>> PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>>> PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>>> PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>>>> - if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>>>> +
>>>>> + if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
>>>>> + uint32_t status = I915_READ(DPLL(PIPE_A));
>>>>> + unsigned int mask;
>>>>> +
>>>>> + mask = status & DPLL_PORTB_READY_MASK;
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
>>>>> + mask = (status & DPLL_PORTC_READY_MASK) >> 4;
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
>>>>> +
>>>>> dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>>>> - if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>>>> + } else {
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
>>>>> + }
>>>>> +
>>>>> + if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
>>>>> + uint32_t status = I915_READ(DPIO_PHY_STATUS);
>>>>> + unsigned int mask;
>>>>> +
>>>>> + mask = status & DPLL_PORTD_READY_MASK;
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
>>>>> +
>>>>> dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>>>> + } else {
>>>>> + dev_priv->chv_phy_control |=
>>>>> + PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
>>>>> + }
>>>>> +
>>>>> + DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
>>>>> + dev_priv->chv_phy_control);
>>>>> }
>>>>>
>>>>> static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list