On Fri, 21 Mar 2025, Imre Deak <[email protected]> wrote: > On Fri, Mar 21, 2025 at 08:44:22PM +0200, Ville Syrjälä wrote: >> 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 <[email protected]> >> > > > --- >> > > > 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? > > The latter, i.e. > > #define INTEL_WAKEREF_NONE ((intel_wakeref_t)0)
I've been leaning towards making it the pointer it actually is, i.e. struct ref_tracker *. See the new intel_display_rpm.[ch]. But I have much stronger objections to patches 1 and 2 than this [1]. BR, Jani. [1] https://lore.kernel.org/r/[email protected] > > ? > >> > > > + } >> > > > + >> > > > 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 -- Jani Nikula, Intel
