Hello Jani, > -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: Thursday, October 24, 2024 3:00 PM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx > Cc: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@xxxxxxxxx>; Deak, Imre > <imre.deak@xxxxxxxxx> > Subject: Re: [v4] drm/i915/dp: Add FEC Enable Retry mechanism > > On Thu, 24 Oct 2024, Chaitanya Kumar Borah > <chaitanya.kumar.borah@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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@xxxxxxxxx> > > > > /* > > * 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