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

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

On 10/5/2021 1:34 PM, Jani Nikula wrote:
> On Tue, 05 Oct 2021, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >   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