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
