Re: [Intel-gfx] [PATCH] drm/i915/display: Remove check for low voltage sku for max dp source rate

[Jani Nikula <jani.nikula@intel.com>]

Cc: Imre, I think you were involved in adding the checks.

BR,
Jani.

On Tue, 05 Oct 2021, "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> On 10/5/2021 1:34 PM, Jani Nikula wrote:
>> On Tue, 05 Oct 2021, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>>> The low voltage sku check can be ignored as OEMs need to consider that
>>> when designing the board and then put any limits in VBT.
>> "can" or "must"?
>>
>> VBT has been notoriously buggy over the years, and we need to safeguard
>> against that. Are there any cases where having these checks are wrong?
>
> Hi Jani,
>
> Bspec page for Combo PHY PLL frequencies now says "OEM must use VBT to 
> specify a maximum that is tolerated by the board design" for the rates 
> above 5.4G.
>
> Earlier it was mentioned that rates > 5.4G were supported on SKUs with 
> Higher I/O Voltage.
>
> There was an instance where on an ADL-S board, where VBT was showing as 
> HBR3 supporting for a combo phy port,  but we were reading the IO 
> voltage as 0.85V in is_low_voltage_sku()
>
> (Specifically, we were reading Register_PORT_COMP_DW3 bits 24-25 as 0) 
> for a combo PHY port, and therefore we were limiting the BW to 5.4Gbps
>
> Due to this, 8k@60 mode was getting pruned on the board for that combo 
> phy port. On removing the low_voltage_sku( ) the mode was able to be set 
> properly.
>
> Incidentally, with Windows 8k@60 was also coming up on the same board on 
> same port.
>
> So I had checked with HW team and GOP/VBT team if driver should consider 
> the low voltage sku check.  As per their response we 'can' ignore the 
> check and rely on the VBT, as OEM should limit the rate as per board 
> design. The Bspec was also updated to reflect the same.
>
> So IMHO we need not limit the rate as per is_low_voltage_sku check, as 
> this limiting of the rate through VBT is a must for the OEMs.
>
> I should perhaps change the wording of the commit message to convey the 
> same.
>
>
> Thanks & Regards,
>
> Ankit
>
>
>>
>> BR,
>> Jani.
>>
>>> Same is now changed in Bspec (53720).
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_dp.c | 32 +++----------------------
>>>   1 file changed, 3 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 74a657ae131a..75c364c3c88e 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -297,23 +297,13 @@ static int dg2_max_source_rate(struct intel_dp *intel_dp)
>>>   	return intel_dp_is_edp(intel_dp) ? 810000 : 1350000;
>>>   }
>>>   
>>> -static bool is_low_voltage_sku(struct drm_i915_private *i915, enum phy phy)
>>> -{
>>> -	u32 voltage;
>>> -
>>> -	voltage = intel_de_read(i915, ICL_PORT_COMP_DW3(phy)) & VOLTAGE_INFO_MASK;
>>> -
>>> -	return voltage == VOLTAGE_INFO_0_85V;
>>> -}
>>> -
>>>   static int icl_max_source_rate(struct intel_dp *intel_dp)
>>>   {
>>>   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>>   	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>>>   	enum phy phy = intel_port_to_phy(dev_priv, dig_port->base.port);
>>>   
>>> -	if (intel_phy_is_combo(dev_priv, phy) &&
>>> -	    (is_low_voltage_sku(dev_priv, phy) || !intel_dp_is_edp(intel_dp)))
>>> +	if (intel_phy_is_combo(dev_priv, phy) && !intel_dp_is_edp(intel_dp))
>>>   		return 540000;
>>>   
>>>   	return 810000;
>>> @@ -321,23 +311,7 @@ static int icl_max_source_rate(struct intel_dp *intel_dp)
>>>   
>>>   static int ehl_max_source_rate(struct intel_dp *intel_dp)
>>>   {
>>> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> -	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>>> -	enum phy phy = intel_port_to_phy(dev_priv, dig_port->base.port);
>>> -
>>> -	if (intel_dp_is_edp(intel_dp) || is_low_voltage_sku(dev_priv, phy))
>>> -		return 540000;
>>> -
>>> -	return 810000;
>>> -}
>>> -
>>> -static int dg1_max_source_rate(struct intel_dp *intel_dp)
>>> -{
>>> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> -	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>>> -	enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
>>> -
>>> -	if (intel_phy_is_combo(i915, phy) && is_low_voltage_sku(i915, phy))
>>> +	if (intel_dp_is_edp(intel_dp))
>>>   		return 540000;
>>>   
>>>   	return 810000;
>>> @@ -380,7 +354,7 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp)
>>>   			max_rate = dg2_max_source_rate(intel_dp);
>>>   		else if (IS_ALDERLAKE_P(dev_priv) || IS_ALDERLAKE_S(dev_priv) ||
>>>   			 IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv))
>>> -			max_rate = dg1_max_source_rate(intel_dp);
>>> +			max_rate = 810000;
>>>   		else if (IS_JSL_EHL(dev_priv))
>>>   			max_rate = ehl_max_source_rate(intel_dp);
>>>   		else

-- 
Jani Nikula, Intel Open Source Graphics Center