| Message ID | 20241024061002.4085137-1-chaitanya.kumar.borah@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] drm/i915/dp: Add FEC Enable Retry mechanism | expand |
On Thu, 24 Oct 2024, Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> wrote: > Currently, even though there is a bit to control FEC enable/disable > individually, the FEC Decode Enable sequence is sent by the SOC only > once TRANS_CONF enable is set. This ties the FEC enabling too tightly > to modeset and therefore cannot be re-issued (in case of failure) > without a modeset. > > 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 a modeset. Hence allowing us to have a retry > mechanism in case the DPRX does not respond with an FEC_ENABLE > within certain amount of time. > > While at it, replace struct drm_i915_private with struct intel_display > > v4: > - More code refactor [Jani] > - use struct intel_display [Jani] > - Optimize logging [Jani] > > v3: > - Make the commit message more legible [Jani] > - Refactor code to re-use existing code [Jani] > - Do away with platform dependent FEC enable checks [Jani] > > v2: > - Refactor code to avoid duplication and improve readability [Jani] > - In case of PTL, wait for FEC status directly after FEC enable [Srikanth] > - Wait for FEC_ENABLE_LIVE_STATUS to be cleared before > re-enabling FEC [Srikanth] > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 81 +++++++++++++++++------- > drivers/gpu/drm/i915/display/intel_ddi.h | 6 +- > 2 files changed, 61 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index fe1ded6707f9..ce95263d74ea 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -2256,9 +2256,9 @@ static int read_fec_detected_status(struct drm_dp_aux *aux) > return status; > } > > -static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) > +static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) > { > - struct drm_i915_private *i915 = to_i915(aux->drm_dev); > + struct intel_display *display = to_intel_display(aux->drm_dev); > int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED; > int status; > int err; > @@ -2267,57 +2267,92 @@ static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) > status & mask || status < 0, > 10000, 200000); > > - if (!err && status >= 0) > - return; > + if (err || status < 0) { > + drm_dbg_kms(display->drm, > + "Failed waiting for FEC %s to get detected: %d (status %d)\n", > + str_enabled_disabled(enabled), err, status); > + return err ? err : status; > + } > > - if (err == -ETIMEDOUT) > - drm_dbg_kms(&i915->drm, "Timeout waiting for FEC %s to get detected\n", > - str_enabled_disabled(enabled)); > - else > - drm_dbg_kms(&i915->drm, "FEC detected status read error: %d\n", status); > + return 0; > } > > -void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > - const struct intel_crtc_state *crtc_state, > - bool enabled) > +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + bool enabled) > { > - struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > + struct intel_display *display = to_intel_display(encoder); > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > int ret; > > if (!crtc_state->fec_enable) > - return; > + return 0; > > if (enabled) > - ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, crtc_state), > + ret = intel_de_wait_for_set(display, dp_tp_status_reg(encoder, crtc_state), > DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > else > - ret = intel_de_wait_for_clear(i915, dp_tp_status_reg(encoder, crtc_state), > + ret = intel_de_wait_for_clear(display, dp_tp_status_reg(encoder, crtc_state), > DP_TP_STATUS_FEC_ENABLE_LIVE, 1); > > - if (ret) > - drm_err(&i915->drm, > + if (ret) { > + drm_err(display->drm, > "Timeout waiting for FEC live state to get %s\n", > str_enabled_disabled(enabled)); > - > + return ret; > + } So the functional change here is that we skip the below wait if we timed out above. I think it's probably fine. I also think the patch evolved to be considerably neater than the original. What do you think? Reviewed-by: Jani Nikula <jani.nikula@intel.com> > /* > * 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) { > + ret = wait_for_fec_detected(&intel_dp->aux, enabled); > + if (ret) > + return ret; > + } > + > + return 0; > } > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state) > { > - struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_display *display = to_intel_display(encoder); > + int i; > + int ret; > > if (!crtc_state->fec_enable) > return; > > - intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > 0, DP_TP_CTL_FEC_ENABLE); > + > + if (DISPLAY_VER(display) < 30) > + return; > + > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > + if (!ret) > + return; > + > + for (i = 0; i < 3; i++) { > + drm_dbg_kms(display->drm, "Retry FEC enabling\n"); > + > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > + DP_TP_CTL_FEC_ENABLE, 0); > + > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, false); > + if (ret) > + continue; > + > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > + 0, DP_TP_CTL_FEC_ENABLE); > + > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > + if (!ret) > + return; > + } > + > + drm_err(display->drm, "Failed to enable FEC after retries\n"); > } > > static void intel_ddi_disable_fec(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h > index 6d85422bdefe..640851d46b1b 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.h > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h > @@ -63,9 +63,9 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > void intel_ddi_enable_transcoder_clock(struct intel_encoder *encoder, > const struct intel_crtc_state *crtc_state); > void intel_ddi_disable_transcoder_clock(const struct intel_crtc_state *crtc_state); > -void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > - const struct intel_crtc_state *crtc_state, > - bool enabled); > +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + bool enabled); > void intel_ddi_set_dp_msa(const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state); > bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
Hello Jani, > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Thursday, October 24, 2024 3:00 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; intel- > gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org > Cc: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>; Deak, Imre > <imre.deak@intel.com> > Subject: Re: [v4] drm/i915/dp: Add FEC Enable Retry mechanism > > On Thu, 24 Oct 2024, Chaitanya Kumar Borah > <chaitanya.kumar.borah@intel.com> wrote: > > Currently, even though there is a bit to control FEC enable/disable > > individually, the FEC Decode Enable sequence is sent by the SOC only > > once TRANS_CONF enable is set. This ties the FEC enabling too tightly > > to modeset and therefore cannot be re-issued (in case of failure) > > without a modeset. > > > > 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 a modeset. Hence allowing us to have a retry > > mechanism in case the DPRX does not respond with an FEC_ENABLE within > > certain amount of time. > > > > While at it, replace struct drm_i915_private with struct intel_display > > > > v4: > > - More code refactor [Jani] > > - use struct intel_display [Jani] > > - Optimize logging [Jani] > > > > v3: > > - Make the commit message more legible [Jani] > > - Refactor code to re-use existing code [Jani] > > - Do away with platform dependent FEC enable checks [Jani] > > > > v2: > > - Refactor code to avoid duplication and improve readability [Jani] > > - In case of PTL, wait for FEC status directly after FEC enable > > [Srikanth] > > - Wait for FEC_ENABLE_LIVE_STATUS to be cleared before > > re-enabling FEC [Srikanth] > > > > Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 81 > > +++++++++++++++++------- drivers/gpu/drm/i915/display/intel_ddi.h | > > 6 +- > > 2 files changed, 61 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index fe1ded6707f9..ce95263d74ea 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -2256,9 +2256,9 @@ static int read_fec_detected_status(struct > drm_dp_aux *aux) > > return status; > > } > > > > -static void wait_for_fec_detected(struct drm_dp_aux *aux, bool > > enabled) > > +static int wait_for_fec_detected(struct drm_dp_aux *aux, bool > > +enabled) > > { > > - struct drm_i915_private *i915 = to_i915(aux->drm_dev); > > + struct intel_display *display = to_intel_display(aux->drm_dev); > > int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : > DP_FEC_DECODE_DIS_DETECTED; > > int status; > > int err; > > @@ -2267,57 +2267,92 @@ static void wait_for_fec_detected(struct > drm_dp_aux *aux, bool enabled) > > status & mask || status < 0, > > 10000, 200000); > > > > - if (!err && status >= 0) > > - return; > > + if (err || status < 0) { > > + drm_dbg_kms(display->drm, > > + "Failed waiting for FEC %s to get detected: %d > (status %d)\n", > > + str_enabled_disabled(enabled), err, status); > > + return err ? err : status; > > + } > > > > - if (err == -ETIMEDOUT) > > - drm_dbg_kms(&i915->drm, "Timeout waiting for FEC %s to > get detected\n", > > - str_enabled_disabled(enabled)); > > - else > > - drm_dbg_kms(&i915->drm, "FEC detected status read error: > %d\n", status); > > + return 0; > > } > > > > -void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > > - const struct intel_crtc_state *crtc_state, > > - bool enabled) > > +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + bool enabled) > > { > > - struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); > > + struct intel_display *display = to_intel_display(encoder); > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > int ret; > > > > if (!crtc_state->fec_enable) > > - return; > > + return 0; > > > > if (enabled) > > - ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, > crtc_state), > > + ret = intel_de_wait_for_set(display, > dp_tp_status_reg(encoder, > > +crtc_state), > > DP_TP_STATUS_FEC_ENABLE_LIVE, > 1); > > else > > - ret = intel_de_wait_for_clear(i915, dp_tp_status_reg(encoder, > crtc_state), > > + ret = intel_de_wait_for_clear(display, > dp_tp_status_reg(encoder, > > +crtc_state), > > DP_TP_STATUS_FEC_ENABLE_LIVE, > 1); > > > > - if (ret) > > - drm_err(&i915->drm, > > + if (ret) { > > + drm_err(display->drm, > > "Timeout waiting for FEC live state to get %s\n", > > str_enabled_disabled(enabled)); > > - > > + return ret; > > + } > > So the functional change here is that we skip the below wait if we timed out > above. I think it's probably fine. > Since we're no longer using the crash-and-burn approach and making the functions return values now, I thought it would be good to do the same here. > I also think the patch evolved to be considerably neater than the original. > What do you think? > Agreed. It gives me framework for the future too. Thanks a lot for the review. > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > /* > > * 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) { > > + ret = wait_for_fec_detected(&intel_dp->aux, enabled); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > } > > > > static void intel_ddi_enable_fec(struct intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state) { > > - struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + struct intel_display *display = to_intel_display(encoder); > > + int i; > > + int ret; > > > > if (!crtc_state->fec_enable) > > return; > > > > - intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), > > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > > 0, DP_TP_CTL_FEC_ENABLE); > > + > > + if (DISPLAY_VER(display) < 30) > > + return; > > + > > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > > + if (!ret) > > + return; > > + > > + for (i = 0; i < 3; i++) { > > + drm_dbg_kms(display->drm, "Retry FEC enabling\n"); > > + > > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > > + DP_TP_CTL_FEC_ENABLE, 0); > > + > > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, false); > > + if (ret) > > + continue; > > + > > + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), > > + 0, DP_TP_CTL_FEC_ENABLE); > > + > > + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); > > + if (!ret) > > + return; > > + } > > + > > + drm_err(display->drm, "Failed to enable FEC after retries\n"); > > } > > > > static void intel_ddi_disable_fec(struct intel_encoder *encoder, diff > > --git a/drivers/gpu/drm/i915/display/intel_ddi.h > > b/drivers/gpu/drm/i915/display/intel_ddi.h > > index 6d85422bdefe..640851d46b1b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.h > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h > > @@ -63,9 +63,9 @@ void intel_ddi_disable_transcoder_func(const struct > > intel_crtc_state *crtc_state void intel_ddi_enable_transcoder_clock(struct > intel_encoder *encoder, > > const struct intel_crtc_state *crtc_state); > void > > intel_ddi_disable_transcoder_clock(const struct intel_crtc_state > > *crtc_state); -void intel_ddi_wait_for_fec_status(struct intel_encoder > *encoder, > > - const struct intel_crtc_state *crtc_state, > > - bool enabled); > > +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, > > + const struct intel_crtc_state *crtc_state, > > + bool enabled); > > void intel_ddi_set_dp_msa(const struct intel_crtc_state *crtc_state, > > const struct drm_connector_state *conn_state); > bool > > intel_ddi_connector_get_hw_state(struct intel_connector > > *intel_connector); > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index fe1ded6707f9..ce95263d74ea 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -2256,9 +2256,9 @@ static int read_fec_detected_status(struct drm_dp_aux *aux) return status; } -static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) +static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) { - struct drm_i915_private *i915 = to_i915(aux->drm_dev); + struct intel_display *display = to_intel_display(aux->drm_dev); int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED; int status; int err; @@ -2267,57 +2267,92 @@ static void wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled) status & mask || status < 0, 10000, 200000); - if (!err && status >= 0) - return; + if (err || status < 0) { + drm_dbg_kms(display->drm, + "Failed waiting for FEC %s to get detected: %d (status %d)\n", + str_enabled_disabled(enabled), err, status); + return err ? err : status; + } - if (err == -ETIMEDOUT) - drm_dbg_kms(&i915->drm, "Timeout waiting for FEC %s to get detected\n", - str_enabled_disabled(enabled)); - else - drm_dbg_kms(&i915->drm, "FEC detected status read error: %d\n", status); + return 0; } -void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, - const struct intel_crtc_state *crtc_state, - bool enabled) +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + bool enabled) { - struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); + struct intel_display *display = to_intel_display(encoder); struct intel_dp *intel_dp = enc_to_intel_dp(encoder); int ret; if (!crtc_state->fec_enable) - return; + return 0; if (enabled) - ret = intel_de_wait_for_set(i915, dp_tp_status_reg(encoder, crtc_state), + ret = intel_de_wait_for_set(display, dp_tp_status_reg(encoder, crtc_state), DP_TP_STATUS_FEC_ENABLE_LIVE, 1); else - ret = intel_de_wait_for_clear(i915, dp_tp_status_reg(encoder, crtc_state), + ret = intel_de_wait_for_clear(display, dp_tp_status_reg(encoder, crtc_state), DP_TP_STATUS_FEC_ENABLE_LIVE, 1); - if (ret) - drm_err(&i915->drm, + if (ret) { + drm_err(display->drm, "Timeout waiting for FEC live state to get %s\n", str_enabled_disabled(enabled)); - + return ret; + } /* * 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) { + ret = wait_for_fec_detected(&intel_dp->aux, enabled); + if (ret) + return ret; + } + + return 0; } static void intel_ddi_enable_fec(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state) { - struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_display *display = to_intel_display(encoder); + int i; + int ret; if (!crtc_state->fec_enable) return; - intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), 0, DP_TP_CTL_FEC_ENABLE); + + if (DISPLAY_VER(display) < 30) + return; + + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); + if (!ret) + return; + + for (i = 0; i < 3; i++) { + drm_dbg_kms(display->drm, "Retry FEC enabling\n"); + + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), + DP_TP_CTL_FEC_ENABLE, 0); + + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, false); + if (ret) + continue; + + intel_de_rmw(display, dp_tp_ctl_reg(encoder, crtc_state), + 0, DP_TP_CTL_FEC_ENABLE); + + ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true); + if (!ret) + return; + } + + drm_err(display->drm, "Failed to enable FEC after retries\n"); } static void intel_ddi_disable_fec(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h index 6d85422bdefe..640851d46b1b 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.h +++ b/drivers/gpu/drm/i915/display/intel_ddi.h @@ -63,9 +63,9 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state void intel_ddi_enable_transcoder_clock(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state); void intel_ddi_disable_transcoder_clock(const struct intel_crtc_state *crtc_state); -void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, - const struct intel_crtc_state *crtc_state, - bool enabled); +int intel_ddi_wait_for_fec_status(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + bool enabled); void intel_ddi_set_dp_msa(const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state); bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
Currently, even though there is a bit to control FEC enable/disable individually, the FEC Decode Enable sequence is sent by the SOC only once TRANS_CONF enable is set. This ties the FEC enabling too tightly to modeset and therefore cannot be re-issued (in case of failure) without a modeset. 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 a modeset. Hence allowing us to have a retry mechanism in case the DPRX does not respond with an FEC_ENABLE within certain amount of time. While at it, replace struct drm_i915_private with struct intel_display v4: - More code refactor [Jani] - use struct intel_display [Jani] - Optimize logging [Jani] v3: - Make the commit message more legible [Jani] - Refactor code to re-use existing code [Jani] - Do away with platform dependent FEC enable checks [Jani] v2: - Refactor code to avoid duplication and improve readability [Jani] - In case of PTL, wait for FEC status directly after FEC enable [Srikanth] - Wait for FEC_ENABLE_LIVE_STATUS to be cleared before re-enabling FEC [Srikanth] Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 81 +++++++++++++++++------- drivers/gpu/drm/i915/display/intel_ddi.h | 6 +- 2 files changed, 61 insertions(+), 26 deletions(-)