> -----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
> >
