| Message ID | 20240923045218.1813255-1-chaitanya.kumar.borah@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Add FEC Enable Retry mechanism | expand |
On Mon, 23 Sep 2024, Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> wrote: > From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent > of TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN > sequence without re-doing the whole mode set sequence. This separate > control over FEC_ECODE_EN/DIS sequence enables us to have a retry > mechanism in case the DPRX does not respond with an FEC_ENABLE > within the stipulated 5ms. > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 61 +++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 85e519a21542..589acea9906a 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -78,6 +78,8 @@ > #include "skl_scaler.h" > #include "skl_universal_plane.h" > > +#define FEC_RETRY_COUNT 3 If there's only one user, I wouldn't bother with a macro. > + > static const u8 index_to_dp_signal_levels[] = { > [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0, > [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_1, > @@ -2255,6 +2257,57 @@ static int read_fec_detected_status(struct drm_dp_aux *aux) > return status; > } > > +static void retry_fec_enable(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + struct drm_dp_aux *aux) > +{ > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > + int ret = 0; > + > + /* Clear FEC enable */ > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > + DP_TP_CTL_FEC_ENABLE, 0); > + > + /* Set FEC enable */ > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > + 0, DP_TP_CTL_FEC_ENABLE); > + > + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, crtc_state), > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > + if (!ret) > + drm_dbg_kms(&i915->drm, > + "Timeout waiting for FEC live state to get enabled"); > +} > + > +static void wait_for_fec_detected_with_retry(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + struct drm_dp_aux *aux) > +{ > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > + int status; > + int err; > + int retrycount = 0; > + > + do { > + err = readx_poll_timeout(read_fec_detected_status, aux, status, > + status & DP_FEC_DECODE_EN_DETECTED || status < 0, > + 500, 5000); > + > + if (!err && status >= 0) > + return; > + > + if (err == -ETIMEDOUT) { > + drm_dbg_kms(&i915->drm, > + "Timeout waiting for FEC ENABLE to get detected, retrying\n"); > + retry_fec_enable(encoder, crtc_state, aux); > + } else { > + drm_dbg_kms(&i915->drm, "FEC detected status read error: %d\n", status); > + } > + } while (retrycount++ < FEC_RETRY_COUNT); Please use a regular for loop with for (i = 0; i < N; i++) so even I can look at it and know in an instant how many times it's going to loop. > + > + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); > +} > + There's just way, way too much duplication between the above two functions and the existing intel_ddi_wait_for_fec_status() and wait_for_fec_detected(). We'll want *one* code path with the retry baked in, with no retries for older platforms. BR, Jani. > static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) > { > struct drm_i915_private *i915 = to_i915(aux->drm_dev); > @@ -2303,8 +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > * At least the Synoptics MST hub doesn't set the detected flag for > * FEC decoding disabling so skip waiting for that. > */ > - if (enabled) > - wait_for_fec_detected(&intel_dp->aux, enabled); > + if (enabled) { > + if (DISPLAY_VER(i915) >= 30) > + wait_for_fec_detected_with_retry(encoder, crtc_state, &intel_dp->aux); > + else > + wait_for_fec_detected(&intel_dp->aux, enabled); > + } > } > > static void intel_ddi_enable_fec(struct intel_encoder *encoder,
> -----Original Message----- > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of > Chaitanya Kumar Borah > Sent: Monday, September 23, 2024 10:22 AM > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Subject: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent of > TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN sequence > without re-doing the whole mode set sequence. This separate control over > FEC_ECODE_EN/DIS sequence enables us to have a retry mechanism in case the > DPRX does not respond with an FEC_ENABLE within the stipulated 5ms. > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 61 +++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 85e519a21542..589acea9906a 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -78,6 +78,8 @@ > #include "skl_scaler.h" > #include "skl_universal_plane.h" > > +#define FEC_RETRY_COUNT 3 > + > static const u8 index_to_dp_signal_levels[] = { > [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_0, > [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_1, @@ -2255,6 +2257,57 @@ static int > read_fec_detected_status(struct drm_dp_aux *aux) > return status; > } > > +static void retry_fec_enable(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + struct drm_dp_aux *aux) > +{ > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > + int ret = 0; > + You should also be clearing FEC_ENABLE_LIVE_STATUS in DP_TP_STATUS register by writing 1. > + /* Clear FEC enable */ > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > + DP_TP_CTL_FEC_ENABLE, 0); > + > + /* Set FEC enable */ > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > + 0, DP_TP_CTL_FEC_ENABLE); > + > + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, > crtc_state), > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > + if (!ret) > + drm_dbg_kms(&i915->drm, > + "Timeout waiting for FEC live state to get enabled"); } > + > +static void wait_for_fec_detected_with_retry(struct intel_encoder *encoder, > + const struct intel_crtc_state > *crtc_state, > + struct drm_dp_aux *aux) > +{ > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > + int status; > + int err; > + int retrycount = 0; > + > + do { > + err = readx_poll_timeout(read_fec_detected_status, aux, > status, > + status & > DP_FEC_DECODE_EN_DETECTED || status < 0, > + 500, 5000); > + > + if (!err && status >= 0) > + return; > + > + if (err == -ETIMEDOUT) { > + drm_dbg_kms(&i915->drm, > + "Timeout waiting for FEC ENABLE to get > detected, retrying\n"); > + retry_fec_enable(encoder, crtc_state, aux); > + } else { > + drm_dbg_kms(&i915->drm, "FEC detected status read > error: %d\n", status); > + } > + } while (retrycount++ < FEC_RETRY_COUNT); > + > + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); } > + > static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) { > struct drm_i915_private *i915 = to_i915(aux->drm_dev); @@ -2303,8 > +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder > *encoder, > * At least the Synoptics MST hub doesn't set the detected flag for > * FEC decoding disabling so skip waiting for that. > */ > - if (enabled) > - wait_for_fec_detected(&intel_dp->aux, enabled); > + if (enabled) { > + if (DISPLAY_VER(i915) >= 30) > + wait_for_fec_detected_with_retry(encoder, crtc_state, > &intel_dp->aux); wait_for_fec_detected_with_retry() should be called inside intel_ddi_enable_fec() after configuring DP_TP_CTL register and before transcoder is enabled. wait_for_fec_detected() is called after transcoder is enabled. > + else > + wait_for_fec_detected(&intel_dp->aux, enabled); > + } > } > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > -- > 2.25.1
Hello Srikanth, Thank you for the review. > -----Original Message----- > From: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com> > Sent: Monday, September 23, 2024 4:16 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Subject: RE: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > > > > -----Original Message----- > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of > > Chaitanya Kumar Borah > > Sent: Monday, September 23, 2024 10:22 AM > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > > Subject: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > > > From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent of > > TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN > > sequence without re-doing the whole mode set sequence. This separate > > control over FEC_ECODE_EN/DIS sequence enables us to have a retry > > mechanism in case the DPRX does not respond with an FEC_ENABLE within > the stipulated 5ms. > > > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 61 > > +++++++++++++++++++++++- > > 1 file changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 85e519a21542..589acea9906a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -78,6 +78,8 @@ > > #include "skl_scaler.h" > > #include "skl_universal_plane.h" > > > > +#define FEC_RETRY_COUNT 3 > > + > > static const u8 index_to_dp_signal_levels[] = { > > [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_0, > > [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_1, @@ > > -2255,6 +2257,57 @@ static int read_fec_detected_status(struct > > drm_dp_aux *aux) > > return status; > > } > > > > +static void retry_fec_enable(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + struct drm_dp_aux *aux) > > +{ > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > + int ret = 0; > > + > > You should also be clearing FEC_ENABLE_LIVE_STATUS in DP_TP_STATUS > register by writing 1. > This seems to be a RO bit according to spec. > > + /* Clear FEC enable */ > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > + DP_TP_CTL_FEC_ENABLE, 0); > > + > > > + /* Set FEC enable */ > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > + 0, DP_TP_CTL_FEC_ENABLE); > > + > > + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, > > crtc_state), > > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > > + if (!ret) > > + drm_dbg_kms(&i915->drm, > > + "Timeout waiting for FEC live state to get enabled"); > } > > + > > +static void wait_for_fec_detected_with_retry(struct intel_encoder > *encoder, > > + const struct intel_crtc_state > > *crtc_state, > > + struct drm_dp_aux *aux) > > +{ > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > + int status; > > + int err; > > + int retrycount = 0; > > + > > + do { > > + err = readx_poll_timeout(read_fec_detected_status, aux, > > status, > > + status & > > DP_FEC_DECODE_EN_DETECTED || status < 0, > > + 500, 5000); > > + > > + if (!err && status >= 0) > > + return; > > + > > + if (err == -ETIMEDOUT) { > > + drm_dbg_kms(&i915->drm, > > + "Timeout waiting for FEC ENABLE to get > > detected, retrying\n"); > > + retry_fec_enable(encoder, crtc_state, aux); > > + } else { > > + drm_dbg_kms(&i915->drm, "FEC detected status > read > > error: %d\n", status); > > + } > > + } while (retrycount++ < FEC_RETRY_COUNT); > > + > > + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); } > > + > > static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) { > > struct drm_i915_private *i915 = to_i915(aux->drm_dev); @@ - > 2303,8 > > +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder > > *encoder, > > * At least the Synoptics MST hub doesn't set the detected flag for > > * FEC decoding disabling so skip waiting for that. > > */ > > - if (enabled) > > - wait_for_fec_detected(&intel_dp->aux, enabled); > > + if (enabled) { > > + if (DISPLAY_VER(i915) >= 30) > > + wait_for_fec_detected_with_retry(encoder, > crtc_state, > > &intel_dp->aux); > > wait_for_fec_detected_with_retry() should be called inside > intel_ddi_enable_fec() after configuring DP_TP_CTL register and before > transcoder is enabled. > wait_for_fec_detected() is called after transcoder is enabled. > Agreed. I will make the change. Regards Chaitanya > > + else > > + wait_for_fec_detected(&intel_dp->aux, enabled); > > + } > > } > > > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > > -- > > 2.25.1
Hello Jani, Thank you for the review. > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Monday, September 23, 2024 12:40 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > On Mon, 23 Sep 2024, Chaitanya Kumar Borah > <chaitanya.kumar.borah@intel.com> wrote: > > From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent of > > TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN > > sequence without re-doing the whole mode set sequence. This separate > > control over FEC_ECODE_EN/DIS sequence enables us to have a retry > > mechanism in case the DPRX does not respond with an FEC_ENABLE within > > the stipulated 5ms. > > > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 61 > > +++++++++++++++++++++++- > > 1 file changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 85e519a21542..589acea9906a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -78,6 +78,8 @@ > > #include "skl_scaler.h" > > #include "skl_universal_plane.h" > > > > +#define FEC_RETRY_COUNT 3 > > If there's only one user, I wouldn't bother with a macro. > Ack. > > + > > static const u8 index_to_dp_signal_levels[] = { > > [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_0, > > [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > DP_TRAIN_PRE_EMPH_LEVEL_1, @@ > > -2255,6 +2257,57 @@ static int read_fec_detected_status(struct > drm_dp_aux *aux) > > return status; > > } > > > > +static void retry_fec_enable(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + struct drm_dp_aux *aux) > > +{ > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > + int ret = 0; > > + > > + /* Clear FEC enable */ > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > + DP_TP_CTL_FEC_ENABLE, 0); > > + > > + /* Set FEC enable */ > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > + 0, DP_TP_CTL_FEC_ENABLE); > > + > > + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, > crtc_state), > > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > > + if (!ret) > > + drm_dbg_kms(&i915->drm, > > + "Timeout waiting for FEC live state to get enabled"); > } > > + > > +static void wait_for_fec_detected_with_retry(struct intel_encoder > *encoder, > > + const struct intel_crtc_state > *crtc_state, > > + struct drm_dp_aux *aux) > > +{ > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > + int status; > > + int err; > > + int retrycount = 0; > > + > > + do { > > + err = readx_poll_timeout(read_fec_detected_status, aux, > status, > > + status & > DP_FEC_DECODE_EN_DETECTED || status < 0, > > + 500, 5000); > > + > > + if (!err && status >= 0) > > + return; > > + > > + if (err == -ETIMEDOUT) { > > + drm_dbg_kms(&i915->drm, > > + "Timeout waiting for FEC ENABLE to get > detected, retrying\n"); > > + retry_fec_enable(encoder, crtc_state, aux); > > + } else { > > + drm_dbg_kms(&i915->drm, "FEC detected status > read error: %d\n", status); > > + } > > + } while (retrycount++ < FEC_RETRY_COUNT); > > Please use a regular for loop with for (i = 0; i < N; i++) so even I can look at it > and know in an instant how many times it's going to loop. > > > + > > + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); } > > + > > There's just way, way too much duplication between the above two functions > and the existing intel_ddi_wait_for_fec_status() and wait_for_fec_detected(). > We'll want *one* code path with the retry baked in, with no retries for older > platforms. Agreed. Let me try to cook something up. Regards Chaitanya > > BR, > Jani. > > > > static void wait_for_fec_detected(struct drm_dp_aux *aux, bool > > enabled) { > > struct drm_i915_private *i915 = to_i915(aux->drm_dev); @@ - > 2303,8 > > +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder > *encoder, > > * At least the Synoptics MST hub doesn't set the detected flag for > > * FEC decoding disabling so skip waiting for that. > > */ > > - if (enabled) > > - wait_for_fec_detected(&intel_dp->aux, enabled); > > + if (enabled) { > > + if (DISPLAY_VER(i915) >= 30) > > + wait_for_fec_detected_with_retry(encoder, > crtc_state, &intel_dp->aux); > > + else > > + wait_for_fec_detected(&intel_dp->aux, enabled); > > + } > > } > > > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > > -- > Jani Nikula, Intel
> -----Original Message----- > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > Sent: Wednesday, September 25, 2024 12:02 PM > To: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Subject: RE: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > Hello Srikanth, > > Thank you for the review. > > > -----Original Message----- > > From: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com> > > Sent: Monday, September 23, 2024 4:16 PM > > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > > Subject: RE: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > > > > > > > > -----Original Message----- > > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of > > > Chaitanya Kumar Borah > > > Sent: Monday, September 23, 2024 10:22 AM > > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > > > Subject: [PATCH] drm/i915/dp: Add FEC Enable Retry mechanism > > > > > > From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent > > > of TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN > > > sequence without re-doing the whole mode set sequence. This separate > > > control over FEC_ECODE_EN/DIS sequence enables us to have a retry > > > mechanism in case the DPRX does not respond with an FEC_ENABLE > > > within > > the stipulated 5ms. > > > > > > Signed-off-by: Chaitanya Kumar Borah > > > <chaitanya.kumar.borah@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 61 > > > +++++++++++++++++++++++- > > > 1 file changed, 59 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index 85e519a21542..589acea9906a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -78,6 +78,8 @@ > > > #include "skl_scaler.h" > > > #include "skl_universal_plane.h" > > > > > > +#define FEC_RETRY_COUNT 3 > > > + > > > static const u8 index_to_dp_signal_levels[] = { > > > [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > > DP_TRAIN_PRE_EMPH_LEVEL_0, > > > [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > > DP_TRAIN_PRE_EMPH_LEVEL_1, @@ > > > -2255,6 +2257,57 @@ static int read_fec_detected_status(struct > > > drm_dp_aux *aux) > > > return status; > > > } > > > > > > +static void retry_fec_enable(struct intel_encoder *encoder, > > > + const struct intel_crtc_state *crtc_state, > > > + struct drm_dp_aux *aux) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > > + int ret = 0; > > > + > > > > You should also be clearing FEC_ENABLE_LIVE_STATUS in DP_TP_STATUS > > register by writing 1. > > > > This seems to be a RO bit according to spec. > You are right. But we could wait till FEC_ENABLE_LIVE_STATUS is cleared after clearing FEC_ENABLE in DP_TP_CTL. We could also use FEC_DISABLE_PENDING in DP_TP_CTL also here. > > > + /* Clear FEC enable */ > > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > > + DP_TP_CTL_FEC_ENABLE, 0); > > > + > > > > > + /* Set FEC enable */ > > > + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), > > > + 0, DP_TP_CTL_FEC_ENABLE); > > > + > > > + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, > > > crtc_state), > > > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > > > + if (!ret) > > > + drm_dbg_kms(&i915->drm, > > > + "Timeout waiting for FEC live state to get > enabled"); > > } > > > + > > > +static void wait_for_fec_detected_with_retry(struct intel_encoder > > *encoder, > > > + const struct intel_crtc_state > > > *crtc_state, > > > + struct drm_dp_aux *aux) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > > + int status; > > > + int err; > > > + int retrycount = 0; > > > + > > > + do { > > > + err = readx_poll_timeout(read_fec_detected_status, aux, > > > status, > > > + status & > > > DP_FEC_DECODE_EN_DETECTED || status < 0, > > > + 500, 5000); > > > + > > > + if (!err && status >= 0) > > > + return; > > > + > > > + if (err == -ETIMEDOUT) { > > > + drm_dbg_kms(&i915->drm, > > > + "Timeout waiting for FEC ENABLE to get > > > detected, retrying\n"); > > > + retry_fec_enable(encoder, crtc_state, aux); > > > + } else { > > > + drm_dbg_kms(&i915->drm, "FEC detected status > > read > > > error: %d\n", status); > > > + } > > > + } while (retrycount++ < FEC_RETRY_COUNT); > > > + > > > + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); } > > > + > > > static void wait_for_fec_detected(struct drm_dp_aux *aux, bool > enabled) { > > > struct drm_i915_private *i915 = to_i915(aux->drm_dev); @@ - > > 2303,8 > > > +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder > > > *encoder, > > > * At least the Synoptics MST hub doesn't set the detected flag for > > > * FEC decoding disabling so skip waiting for that. > > > */ > > > - if (enabled) > > > - wait_for_fec_detected(&intel_dp->aux, enabled); > > > + if (enabled) { > > > + if (DISPLAY_VER(i915) >= 30) > > > + wait_for_fec_detected_with_retry(encoder, > > crtc_state, > > > &intel_dp->aux); > > > > wait_for_fec_detected_with_retry() should be called inside > > intel_ddi_enable_fec() after configuring DP_TP_CTL register and before > > transcoder is enabled. > > wait_for_fec_detected() is called after transcoder is enabled. > > > > Agreed. I will make the change. > > Regards > > Chaitanya > > > > + else > > > + wait_for_fec_detected(&intel_dp->aux, enabled); > > > + } > > > } > > > > > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > > > -- > > > 2.25.1
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 85e519a21542..589acea9906a 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -78,6 +78,8 @@ #include "skl_scaler.h" #include "skl_universal_plane.h" +#define FEC_RETRY_COUNT 3 + static const u8 index_to_dp_signal_levels[] = { [0] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0, [1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_1, @@ -2255,6 +2257,57 @@ static int read_fec_detected_status(struct drm_dp_aux *aux) return status; } +static void retry_fec_enable(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + struct drm_dp_aux *aux) +{ + struct drm_i915_private *i915 = to_i915(aux->drm_dev); + int ret = 0; + + /* Clear FEC enable */ + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), + DP_TP_CTL_FEC_ENABLE, 0); + + /* Set FEC enable */ + intel_de_rmw(i915, dp_tp_ctl_reg(encoder, crtc_state), + 0, DP_TP_CTL_FEC_ENABLE); + + ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, crtc_state), + DP_TP_STATUS_FEC_ENABLE_LIVE, 1); + if (!ret) + drm_dbg_kms(&i915->drm, + "Timeout waiting for FEC live state to get enabled"); +} + +static void wait_for_fec_detected_with_retry(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + struct drm_dp_aux *aux) +{ + struct drm_i915_private *i915 = to_i915(aux->drm_dev); + int status; + int err; + int retrycount = 0; + + do { + err = readx_poll_timeout(read_fec_detected_status, aux, status, + status & DP_FEC_DECODE_EN_DETECTED || status < 0, + 500, 5000); + + if (!err && status >= 0) + return; + + if (err == -ETIMEDOUT) { + drm_dbg_kms(&i915->drm, + "Timeout waiting for FEC ENABLE to get detected, retrying\n"); + retry_fec_enable(encoder, crtc_state, aux); + } else { + drm_dbg_kms(&i915->drm, "FEC detected status read error: %d\n", status); + } + } while (retrycount++ < FEC_RETRY_COUNT); + + drm_err(&i915->drm, "FEC enable Failed after Retry\n"); +} + static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) { struct drm_i915_private *i915 = to_i915(aux->drm_dev); @@ -2303,8 +2356,12 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, * At least the Synoptics MST hub doesn't set the detected flag for * FEC decoding disabling so skip waiting for that. */ - if (enabled) - wait_for_fec_detected(&intel_dp->aux, enabled); + if (enabled) { + if (DISPLAY_VER(i915) >= 30) + wait_for_fec_detected_with_retry(encoder, crtc_state, &intel_dp->aux); + else + wait_for_fec_detected(&intel_dp->aux, enabled); + } } static void intel_ddi_enable_fec(struct intel_encoder *encoder,
From PTL, FEC_DECODE_EN sequence can be sent to a DPRX independent of TRANS_CONF enable. This allows us to re-issue an FEC_DECODE_EN sequence without re-doing the whole mode set sequence. This separate control over FEC_ECODE_EN/DIS sequence enables us to have a retry mechanism in case the DPRX does not respond with an FEC_ENABLE within the stipulated 5ms. Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 61 +++++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-)