Re: [PATCH 2/3] drm/i915/dp_mst: Fix side-band message timeouts due to long PPS delays

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 21, 2025 at 08:38:45PM +0200, Imre Deak wrote:
> On Fri, Mar 21, 2025 at 08:00:29PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 21, 2025 at 04:56:25PM +0200, Imre Deak wrote:
> > > The Panel Power Sequencer lock held on an eDP port (a) blocks a DP AUX
> > > transfer on another port (b), since the PPS lock is device global, thus
> > > shared by all ports. The PPS lock can be held on port (a) for a longer
> > > period due to the various PPS delays (panel/backlight on/off,
> > > power-cycle delays). This in turn can cause an MST down-message request
> > > on port (b) time out, if the above PPS delay defers the handling of the
> > > reply to the request by more than 100ms: the MST branch device sending
> > > the reply (signaling this via the DP_DOWN_REP_MSG_RDY flag in the
> > > DP_DEVICE_SERVICE_IRQ_VECTOR DPCD register) may cancel the reply
> > > (clearing DP_DOWN_REP_MSG_RDY and the reply message buffer) after 110
> > > ms, if the reply is not processed by that time.
> > > 
> > > Avoid MST down-message timeouts described above, by locking the PPS
> > > state for AUX transfers only if this is actually required: on eDP ports,
> > > where the VDD power depends on the PPS state and on all DP and eDP ports
> > > on VLV/CHV, where the PPS is a pipe instance and hence a modeset on any
> > > port possibly affecting the PPS state.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_pps.c | 34 ++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> > > index 3c078fd53fbfa..7d7157983f25e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_pps.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> > > @@ -26,6 +26,11 @@ static void vlv_steal_power_sequencer(struct intel_display *display,
> > >  static void pps_init_delays(struct intel_dp *intel_dp);
> > >  static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd);
> > >  
> > > +static bool intel_pps_is_pipe_instance(struct intel_display *display)
> > > +{
> > > +	return display->platform.valleyview || display->platform.cherryview;
> > > +}
> > > +
> > >  static const char *pps_name(struct intel_dp *intel_dp)
> > >  {
> > >  	struct intel_display *display = to_intel_display(intel_dp);
> > > @@ -955,10 +960,32 @@ void intel_pps_vdd_off(struct intel_dp *intel_dp)
> > >  		intel_pps_vdd_off_unlocked(intel_dp, false);
> > >  }
> > >  
> > > +static bool aux_needs_pps_lock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_display *display = to_intel_display(intel_dp);
> > > +
> > > +	/*
> > > +	 * The PPS state needs to be locked for:
> > > +	 * - eDP on all platforms, since AUX transfers on eDP need VDD power
> > > +	 *   (either forced or via panel power) which depends on the PPS
> > > +	 *   state.
> > > +	 * - non-eDP on platforms where the PPS is a pipe instance (VLV/CHV),
> > > +	 *   since changing the PPS state (via a parallel modeset for
> > > +	 *   instance) may interfere with the AUX transfers on a non-eDP
> > > +	 *   output as well.
> > > +	 */
> > > +	return intel_dp_is_edp(intel_dp) || intel_pps_is_pipe_instance(display);
> > > +}
> > > +
> > >  intel_wakeref_t intel_pps_lock_for_aux(struct intel_dp *intel_dp, bool *vdd_ref)
> > >  {
> > >  	intel_wakeref_t wakeref;
> > >  
> > > +	if (!aux_needs_pps_lock(intel_dp)) {
> > > +		*vdd_ref = false;
> > > +		return NULL;
> > 
> > I was pondering if we need a define for this since intel_wakeref_t
> > doesn't look like a pointer, but apparently we use NULLs elsewhere
> > as well for this stuff.
> 
> Ok, makes sense. It is a bigger a change though, so is it ok to do that
> as a follow up?

I'm not sure what we even should do about it. Should all the
naked NULLs be hidden, or should we make the thing look like the
pointer it actually is?

> 
> > > +	}
> > > +
> > >  	wakeref = intel_pps_lock(intel_dp);
> > >  
> > >  	/*
> > > @@ -976,6 +1003,13 @@ intel_wakeref_t intel_pps_lock_for_aux(struct intel_dp *intel_dp, bool *vdd_ref)
> > >  
> > >  void intel_pps_unlock_for_aux(struct intel_dp *intel_dp, intel_wakeref_t wakeref, bool vdd_ref)
> > >  {
> > > +	struct intel_display *display = to_intel_display(intel_dp);
> > > +
> > > +	if (!wakeref) {
> > > +		drm_WARN_ON(display->drm, vdd_ref || aux_needs_pps_lock(intel_dp));
> > > +		return;
> > > +	}
> > > +
> > >  	if (vdd_ref)
> > >  		intel_pps_vdd_off_unlocked(intel_dp, false);
> > >  
> > > -- 
> > > 2.44.2
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel