| Message ID | 20240119101024.1060812-7-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Panel replay selective update support | expand |
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Friday, January 19, 2024 3:40 PM > To: intel-gfx@lists.freedesktop.org > Cc: Manna, Animesh <animesh.manna@intel.com>; Hogander, Jouni > <jouni.hogander@intel.com> > 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@intel.com> > --- > 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))) > 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. > 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. 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
On Fri, 2024-02-02 at 08:10 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Friday, January 19, 2024 3:40 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Manna, Animesh <animesh.manna@intel.com>; Hogander, Jouni > > <jouni.hogander@intel.com> > > 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@intel.com> > > --- > > 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. 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 >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Friday, February 2, 2024 1:50 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > 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@intel.com> > > > Sent: Friday, January 19, 2024 3:40 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Manna, Animesh <animesh.manna@intel.com>; Hogander, Jouni > > > <jouni.hogander@intel.com> > > > 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@intel.com> > > > --- > > > 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@intel.com> > > 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 > >
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)) 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)) { 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) 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);
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@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)