| Message ID | 20250321145626.94101-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915: Fix DP MST SB message timeouts due to PPS delays | expand |
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. > + } > + > 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
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? > > + } > > + > > 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
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
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 <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? The latter, i.e. #define INTEL_WAKEREF_NONE ((intel_wakeref_t)0) ? > > > > + } > > > > + > > > > 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
On Fri, 21 Mar 2025, Imre Deak <imre.deak@intel.com> 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 <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? > > 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/874izibtvx.fsf@intel.com > > ? > >> > > > + } >> > > > + >> > > > 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
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; + } + 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);
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(+)