Re: [Intel-gfx] [PATCH v2] drm/i915/display: Increase AUX timeout for Type-C

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

On Mon, 17 Apr 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
> drm/i915/display: Increase AUX timeout
>> for Type-C
>> 
>> On Mon, 17 Apr 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> >> -----Original Message-----
>> >> From: Jani Nikula <jani.nikula@linux.intel.com>
>> >> Sent: Monday, April 17, 2023 4:04 PM
>> >> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
>> >> gfx@lists.freedesktop.org
>> >> Subject: RE: [Intel-gfx] [PATCH v2] drm/i915/display: Increase AUX
>> >> timeout for Type-C
>> >>
>> >> On Mon, 17 Apr 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> >> >> On Wed, 05 Apr 2023, Suraj Kandpal <suraj.kandpal@intel.com>
>> wrote:
>> >> >> > Type-C PHYs are taking longer than expected for Aux IO Power
>> Enabling.
>> >> >> > Workaround: Increase the timeout.
>> >> >> >
>> >> >> > WA_14017248603: adlp
>> >> >> > Bspec: 55480
>> >> >> >
>> >> >> > ---v2
>> >> >> > -change style on how we mention WA [Ankit] -fix bat error
>> >> >> >
>> >> >> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >> >> > ---
>> >> >> >  .../i915/display/intel_display_power_well.c   | 30
>> ++++++++++++++++++-
>> >> >> >  1 file changed, 29 insertions(+), 1 deletion(-)
>> >> >> >
>> >> >> > diff --git
>> >> >> > a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> >> > b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> >> > index 62bafcbc7937..52f595929a18 100644
>> >> >> > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> >> > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> >> >> > @@ -489,6 +489,34 @@ static void icl_tc_cold_exit(struct
>> >> >> drm_i915_private *i915)
>> >> >> >  		    "succeeded");
>> >> >> >  }
>> >> >> >
>> >> >> > +static void
>> >> >> > +adl_aux_wait_for_power_well_enable(struct drm_i915_private
>> *i915,
>> >> >> > +				   struct i915_power_well
>> *power_well,
>> >> >> > +				   bool timeout_expected)
>> >> >> > +{
>> >> >> > +	const struct i915_power_well_regs *regs =
>> >> >> > +power_well->desc->ops-
>> >> >> >regs;
>> >> >> > +	enum phy phy = icl_aux_pw_to_phy(i915, power_well);
>> >> >> > +	int pw_idx = i915_power_well_instance(power_well)-
>> >hsw.idx;
>> >> >> > +
>> >> >> > +	/*
>> >> >> > +	 * WA_14017248603: adlp
>> >> >> > +	 * Type-C Phy are taking longer than expected for AUX IO
>> Power
>> >> >> Enabling.
>> >> >> > +	 * Increase timeout to 500ms.
>> >> >> > +	 */
>> >> >> > +	if (IS_ALDERLAKE_P(i915) && intel_phy_is_tc(i915, phy)) {
>> >> >> > +		if (intel_de_wait_for_set(i915, regs->driver,
>> >> >> > +
>> >> >> HSW_PWR_WELL_CTL_STATE(pw_idx), 500)) {
>> >> >> > +			drm_dbg_kms(&i915->drm, "%s power well
>> enable
>> >> >> timeout\n",
>> >> >> > +
>> intel_power_well_name(power_well));
>> >> >> > +
>> >> >> > +			drm_WARN_ON(&i915->drm,
>> !timeout_expected);
>> >> >> > +		}
>> >> >> > +		return;
>> >> >> > +	}
>> >> >> > +
>> >> >> > +	hsw_wait_for_power_well_enable(i915, power_well,
>> >> >> timeout_expected);
>> >> >> > +}
>> >> >>
>> >> >> Please don't duplicate the function and the wait like this.
>> >> >>
>> >> >> Something like this is sufficient:
>> >> >>
>> >> >>
>> >> >> @@ -252,7 +252,9 @@ static void
>> >> hsw_wait_for_power_well_enable(struct
>> >> >> drm_i915_private *dev_priv,
>> >> >>  					   bool timeout_expected)
>> >> >>  {
>> >> >>  	const struct i915_power_well_regs *regs = power_well->desc->ops-
>> >> >> >regs;
>> >> >> +	enum phy phy = icl_aux_pw_to_phy(i915, power_well);
>> >> >>  	int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
>> >> >> +	int timeout = 1;
>> >> >>
>> >> >>  	/*
>> >> >>  	 * For some power wells we're not supposed to watch the status
>> >> >> bit for @@ -264,9 +266,13 @@ static void
>> >> >> hsw_wait_for_power_well_enable(struct drm_i915_private *dev_priv,
>> >> >>  		return;
>> >> >>  	}
>> >> >>
>> >> >> +	/* WA_14017248603: adlp */
>> >> >> +	if (IS_ALDERLAKE_P(i915) && intel_phy_is_tc(i915, phy))
>> >> >
>> >> > I did try this but it ends up throwing a kernel null pointer error
>> >> > at intel_phy_is_tc which made me float the version of code I did.
>> >>
>> >> Please explain what causes it, and how your version avoids it.
>> >>
>> >
>> > icl_aux_pw_to_phy is called is hsw_wait_for_power_well is called
>> > everywhere where its not possible for aux_pw to phy conversion is not
>> possible with this change we only get the phy for adls version.
>> > Will address this is next version.
>> 
>> Cc: Imre for input
>> 
>> Maybe pass the timeout to hsw_wait_for_power_well_enable() as
>> parameter?
>> Other ideas?
>
> I did think of that but the idea of disturbing other functions stopped me.
> If that doesn't seem like an issue I could do that.

Please let Imre chime in first.

But in general we don't shy away from changes touching other places if
it makes sense, and keeps the code clean.

BR,
Jani.



>
> Regards,
> Suraj Kandpal
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Regards,
>> > Suraj Kandpal
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >> >
>> >> > Regards,
>> >> > Suraj Kandpal
>> >> >> +		timeout = 500;
>> >> >> +
>> >> >>  	/* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */
>> >> >>  	if (intel_de_wait_for_set(dev_priv, regs->driver,
>> >> >> -				  HSW_PWR_WELL_CTL_STATE(pw_idx), 1)) {
>> >> >> +				  HSW_PWR_WELL_CTL_STATE(pw_idx),
>> >> >> timeout)) {
>> >> >>  		drm_dbg_kms(&dev_priv->drm, "%s power well enable
>> >> timeout\n",
>> >> >>  			    intel_power_well_name(power_well));
>> >> >>
>> >> >>
>> >> >> > +
>> >> >
>> >> >> >  static void
>> >> >> >  icl_tc_phy_aux_power_well_enable(struct drm_i915_private
>> *dev_priv,
>> >> >> >  				 struct i915_power_well
>> *power_well) @@ -
>> >> >> 517,7 +545,7 @@
>> >> >> > icl_tc_phy_aux_power_well_enable(struct drm_i915_private
>> *dev_priv,
>> >> >> >  	if (DISPLAY_VER(dev_priv) == 11 &&
>> >> >> intel_tc_cold_requires_aux_pw(dig_port))
>> >> >> >  		icl_tc_cold_exit(dev_priv);
>> >> >> >
>> >> >> > -	hsw_wait_for_power_well_enable(dev_priv, power_well,
>> >> >> timeout_expected);
>> >> >> > +	adl_aux_wait_for_power_well_enable(dev_priv, power_well,
>> >> >> > +timeout_expected);
>> >> >>
>> >> >> A function prefixed adl_ should indicate it's only needed for adl+.
>> >> >> This change is misleading.
>> >> >>
>> >> >> BR,
>> >> >> Jani.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >  	if (DISPLAY_VER(dev_priv) >= 12 && !is_tbt) {
>> >> >> >  		enum tc_port tc_port;
>> >> >>
>> >> >> --
>> >> >> Jani Nikula, Intel Open Source Graphics Center
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Graphics Center
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center