[Intel-gfx] [PATCH 14/15] drm/i915: Add some CHV DPIO lane power state asserts

[Deepak]

On 08/27/2015 04:32 PM, Ville Syrjälä wrote:
> On Thu, Aug 27, 2015 at 10:06:09AM +0530, Deepak wrote:
>>
>> On 07/09/2015 02:16 AM, ville.syrjala at linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> Add some checks that the state of the DPIO lanes is more or less what we
>>> expect based on the overrides.
>>>
>>> The hardware only provides two bits per channel indicating whether all
>>> or some of the lanes are powered down, so we can't do an exact check.
>>>
>>> Additionally, CL2 powering down before we can check it adds another
>>> twist. To work around this we simply check for the 0 value of the
>>> CL2 register (which is what we get when it's powered down) and
>>> adjust our expectations.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_reg.h         |  8 +++++
>>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 54 +++++++++++++++++++++++++++++++++
>>>    2 files changed, 62 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 395f556..586a0f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1089,6 +1089,12 @@ enum skl_disp_power_wells {
>>>    #define  DPIO_CHV_INT_LOCK_THRESHOLD_SEL_COARSE	1 /* 1: coarse & 0 : fine  */
>>>    #define CHV_PLL_DW9(ch) _PIPE(ch, _CHV_PLL_DW9_CH0, _CHV_PLL_DW9_CH1)
>>>    
>>> +#define _CHV_CMN_DW0_CH0               0x8100
>>> +#define   DPIO_ALLDL_POWERDOWN_SHIFT_CH0	19
>>> +#define   DPIO_ANYDL_POWERDOWN_SHIFT_CH0	18
>>> +#define   DPIO_ALLDL_POWERDOWN			(1 << 1)
>>> +#define   DPIO_ANYDL_POWERDOWN			(1 << 0)
>>> +
>>>    #define _CHV_CMN_DW5_CH0               0x8114
>>>    #define   CHV_BUFRIGHTENA1_DISABLE	(0 << 20)
>>>    #define   CHV_BUFRIGHTENA1_NORMAL	(1 << 20)
>>> @@ -1125,6 +1131,8 @@ enum skl_disp_power_wells {
>>>    
>>>    #define _CHV_CMN_DW19_CH0		0x814c
>>>    #define _CHV_CMN_DW6_CH1		0x8098
>>> +#define   DPIO_ALLDL_POWERDOWN_SHIFT_CH1	30 /* CL2 DW6 only */
>>> +#define   DPIO_ANYDL_POWERDOWN_SHIFT_CH1	29 /* CL2 DW6 only */
>>>    #define   DPIO_DYNPWRDOWNEN_CH1		(1 << 28) /* CL2 DW6 only */
>>>    #define   CHV_CMN_USEDCLKCHANNEL	(1 << 13)
>>>    
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index 002b78f..a1d9676 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -1026,6 +1026,58 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    		      phy, dev_priv->chv_phy_control);
>>>    }
>>>    
>>> +static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>>> +				     enum dpio_channel ch, bool override, unsigned int mask)
>>> +{
>>> +	enum pipe pipe = phy == DPIO_PHY0 ? PIPE_A : PIPE_C;
>> Why not PIPE B? I think PIPE B can also be used to drive using PHY0?
> Here we only pass 'pipe' to vlv_dpio_read() and it will just use
> 'DPIO_PHY(pipe)' to turn it back into an enum dpio_phy :) Since pipe A
> and B are hooked up the same phy we don't need pipe B here.
>
> I've been meaning to change vlv_dpio_read/write() to take enum dpio_phy
> instead of enum pipe to get rid of this kind of sillyness, but haven't
> gotten around to it yet. There may also be a few other functions that
> could use the same treatment.
>
Thanks for the clarification
Reviewed-by: Deepak S <deepak.s at linux.intel.com>
>>> +	u32 reg, val, expected, actual;
>>> +
>>> +	if (ch == DPIO_CH0)
>>> +		reg = _CHV_CMN_DW0_CH0;
>>> +	else
>>> +		reg = _CHV_CMN_DW6_CH1;
>>> +
>>> +	mutex_lock(&dev_priv->sb_lock);
>>> +	val = vlv_dpio_read(dev_priv, pipe, reg);
>>> +	mutex_unlock(&dev_priv->sb_lock);
>>> +
>>> +	/*
>>> +	 * This assumes !override is only used when the port is disabled.
>>> +	 * All lanes should power down even without the override when
>>> +	 * the port is disabled.
>>> +	 */
>>> +	if (!override || mask == 0xf) {
>>> +		expected = DPIO_ALLDL_POWERDOWN | DPIO_ANYDL_POWERDOWN;
>>> +		/*
>>> +		 * If CH1 common lane is not active anymore
>>> +		 * (eg. for pipe B DPLL) the entire channel will
>>> +		 * shut down, which causes the common lane registers
>>> +		 * to read as 0. That means we can't actually check
>>> +		 * the lane power down status bits, but as the entire
>>> +		 * register reads as 0 it's a good indication that the
>>> +		 * channel is indeed entirely powered down.
>>> +		 */
>>> +		if (ch == DPIO_CH1 && val == 0)
>>> +			expected = 0;
>>> +	} else if (mask != 0x0) {
>>> +		expected = DPIO_ANYDL_POWERDOWN;
>>> +	} else {
>>> +		expected = 0;
>>> +	}
>>> +
>>> +	if (ch == DPIO_CH0)
>>> +		actual = val >> DPIO_ANYDL_POWERDOWN_SHIFT_CH0;
>>> +	else
>>> +		actual = val >> DPIO_ANYDL_POWERDOWN_SHIFT_CH1;
>>> +	actual &= DPIO_ALLDL_POWERDOWN | DPIO_ANYDL_POWERDOWN;
>>> +
>>> +	WARN(actual != expected,
>>> +	     "Unexpected DPIO lane power down: all %d, any %d. Expected: all %d, any %d. (0x%x = 0x%08x)\n",
>>> +	     !!(actual & DPIO_ALLDL_POWERDOWN), !!(actual & DPIO_ANYDL_POWERDOWN),
>>> +	     !!(expected & DPIO_ALLDL_POWERDOWN), !!(expected & DPIO_ANYDL_POWERDOWN),
>>> +	     reg, val);
>>> +}
>>> +
>>>    bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>>>    			  enum dpio_channel ch, bool override)
>>>    {
>>> @@ -1078,6 +1130,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
>>>    	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (PHY_CONTROL=0x%08x)\n",
>>>    		      phy, ch, mask, dev_priv->chv_phy_control);
>>>    
>>> +	assert_chv_phy_powergate(dev_priv, phy, ch, override, mask);
>>> +
>>>    	mutex_unlock(&power_domains->lock);
>>>    }
>>>    
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx