> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx> > Sent: Friday, February 2, 2024 1:50 PM > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 06/21] drm/i915/psr: Check possible errors for panel > replay as well > > On Fri, 2024-02-02 at 08:10 +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx> > > > Sent: Friday, January 19, 2024 3:40 PM > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Manna, Animesh <animesh.manna@xxxxxxxxx>; Hogander, Jouni > > > <jouni.hogander@xxxxxxxxx> > > > Subject: [PATCH v3 06/21] drm/i915/psr: Check possible errors for > > > panel replay as well > > > > > > On HPD interrupt we want to check if the reason for HPD was some > > > panel replay error detected by monitor/panel. This is already done > > > for PSR. We want to do this for panel replay as well. Modify > > > intel_psr_short_pulse to support panel replay as well. > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 21 ++++++++++++++++---- > > > - > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 893c72ea8cf1..6d7ef74201d2 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -2959,6 +2959,13 @@ static void > > > psr_capability_changed_check(struct > > > intel_dp *intel_dp) > > > } > > > } > > > > > > +/* > > > + * On common bits: > > > + * DP_PSR_RFB_STORAGE_ERROR == > > > DP_PANEL_REPLAY_RFB_STORAGE_ERROR > > > + * DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR == > > > +DP_PANEL_REPLAY_VSC_SDP_UNCORRECTABLE_ERROR > > > + * DP_PSR_LINK_CRC_ERROR == DP_PANEL_REPLAY_LINK_CRC_ERROR > > > + * this function is relying on PSR definitions */ > > > void intel_psr_short_pulse(struct intel_dp *intel_dp) { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@ > > > - > > > 2968,7 +2975,7 @@ void intel_psr_short_pulse(struct intel_dp > > > *intel_dp) > > > DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR | > > > DP_PSR_LINK_CRC_ERROR; > > > > > > - if (!CAN_PSR(intel_dp)) > > > + if (!CAN_PSR(intel_dp) && !CAN_PANEL_REPLAY(intel_dp)) > > > > I feel here the condition check would be: > > if (!(CAN_PSR(intel_dp) || CAN_PANEL_REPLAY(intel_dp))) > > This is matter of taste. Received some time ago opposing comment. I don't > have strong opinion on this. I.e I can change it. > > > > > > return; > > > > > > mutex_lock(&psr->lock); > > > @@ -2982,12 +2989,14 @@ void intel_psr_short_pulse(struct intel_dp > > > *intel_dp) > > > goto exit; > > > } > > > > > > - if (status == DP_PSR_SINK_INTERNAL_ERROR || (error_status & > > > errors)) { > > > + if ((!psr->panel_replay_enabled && status == > > > DP_PSR_SINK_INTERNAL_ERROR) || > > > + (error_status & errors)) { > > > > This will check only for psr, rt? .. The flag panel_replay_enabled > > will be true and will not check for error status for panel-replay. > > I think DP_PSR_SINK_INTERNAL_ERROR is only in PSR status register. > error_status bits are for both and they are still checked. > > > > > > intel_psr_disable_locked(intel_dp); > > > psr->sink_not_reliable = true; > > > } > > > > > > - if (status == DP_PSR_SINK_INTERNAL_ERROR && !error_status) > > > + if (!psr->panel_replay_enabled && status == > > > DP_PSR_SINK_INTERNAL_ERROR && > > > + !error_status) > > > > Same doubt as above. > > DP_PSR_SINK_INTERNAL_ERROR doesn't exist in Panel Replay status register. > I.e. if panel replay is enabled do not check further for internal error or error > status bits. Ok. LGTM. Reviewed-by: Animesh Manna <animesh.manna@xxxxxxxxx> > > BR, > > Jouni Högander > > > > > Regards, > > Animesh > > > > > drm_dbg_kms(&dev_priv->drm, > > > "PSR sink internal error, disabling > > > PSR\n"); > > > if (error_status & DP_PSR_RFB_STORAGE_ERROR) @@ -3007,8 > > > +3016,10 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp) > > > /* clear status register */ > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, > > > error_status); > > > > > > - psr_alpm_check(intel_dp); > > > - psr_capability_changed_check(intel_dp); > > > + if (!psr->panel_replay_enabled) { > > > + psr_alpm_check(intel_dp); > > > + psr_capability_changed_check(intel_dp); > > > + } > > > > > > exit: > > > mutex_unlock(&psr->lock); > > > -- > > > 2.34.1 > >