Hello Jani,
Thank you for the review.
> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Wednesday, October 9, 2024 8:47 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
> Cc: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@xxxxxxxxx>
> Subject: Re: [v2] drm/i915/dp: Add FEC Enable Retry mechanism
>
> On Tue, 08 Oct 2024, Chaitanya Kumar Borah
> <chaitanya.kumar.borah@xxxxxxxxx> 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.
> >
> > 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>
>
> What you have here is really hard to understand.
>
I will try to rephrase this.
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 79 +++++++++++++++++----
> > drivers/gpu/drm/i915/display/intel_ddi.h | 2 +-
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +-
> > 3 files changed, 67 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index fe1ded6707f9..047816a427d5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2256,30 +2256,74 @@ 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 void retry_fec_enable(struct intel_encoder *encoder,
> > + const struct intel_crtc_state *crtc_state,
> > + struct drm_dp_aux *aux)
>
> Probably shouldn't pass aux around.
>
ack
> > +{
> > + struct drm_i915_private *i915 = to_i915(aux->drm_dev);
>
> struct intel_display for new code please.
>
> > + int ret = 0;
>
> Unnecessary initialization.
>
Ack
> > +
> > + ret = intel_de_wait_for_clear(i915, dp_tp_status_reg(encoder,
> crtc_state),
> > + DP_TP_STATUS_FEC_ENABLE_LIVE, 1);
> > +
> > + if (ret)
> > + drm_err(&i915->drm,
> > + "Timeout waiting for FEC live state to get disabled
> during
> > +retry\n");
> > +
> > + /* 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(struct intel_encoder *encoder,
> > + const struct intel_crtc_state *crtc_state,
> > + struct drm_dp_aux *aux, bool enabled, bool retry)
>
> Multiple bool parameters make for a crappy interface.
>
Ack
> > {
> > struct drm_i915_private *i915 = to_i915(aux->drm_dev);
> > int mask = enabled ? DP_FEC_DECODE_EN_DETECTED :
> DP_FEC_DECODE_DIS_DETECTED;
> > int status;
> > int err;
> > + int max_retries = retry ? 3 : 1;
> >
> > - err = readx_poll_timeout(read_fec_detected_status, aux, status,
> > - status & mask || status < 0,
> > - 10000, 200000);
> > + for (int retrycount = 0; retrycount < max_retries; retrycount++) {
> > + err = readx_poll_timeout(read_fec_detected_status, aux,
> status,
> > + status & mask || status < 0,
> > + retry ? 500 : 10000,
> > + retry ? 5000 : 200000);
> >
> > - if (!err && status >= 0)
> > - return;
> > + if (!err && status >= 0)
> > + return;
> >
> > - 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);
> > + if (err == -ETIMEDOUT) {
> > + drm_dbg_kms(&i915->drm,
> > + "Timeout waiting for FEC %s to get
> detected, retrying (%d/%d)\n",
> > + str_enabled_disabled(enabled), retrycount
> + 1, max_retries);
> > +
> > + if (retry && enabled)
> > + retry_fec_enable(encoder, crtc_state, aux);
>
> A function whose name is "wait for fec detected" really should *not* retry
> itself. The point is to report status. The callers are supposed to act on that. It's
> really hard to follow what's going on.
>
ack
> > + } else {
> > + drm_dbg_kms(&i915->drm, "FEC detected status read
> error: %d\n", status);
> > + return;
> > + }
> > + }
> > +
> > + drm_err(&i915->drm, "FEC %s Failed after %d attempts\n",
> > + str_enabled_disabled(enabled), max_retries);
> > }
> >
> > void intel_ddi_wait_for_fec_status(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state,
> > - bool enabled)
> > + bool enabled, bool retry)
> > {
> > struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); @@ -2305,7
> > +2349,7 @@ void intel_ddi_wait_for_fec_status(struct intel_encoder
> *encoder,
> > * FEC decoding disabling so skip waiting for that.
> > */
> > if (enabled)
> > - wait_for_fec_detected(&intel_dp->aux, enabled);
> > + wait_for_fec_detected(encoder, crtc_state, &intel_dp->aux,
> enabled,
> > +retry);
> > }
> >
> > static void intel_ddi_enable_fec(struct intel_encoder *encoder, @@
> > -2318,6 +2362,9 @@ static void intel_ddi_enable_fec(struct
> > intel_encoder *encoder,
> >
> > intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state),
> > 0, DP_TP_CTL_FEC_ENABLE);
> > +
> > + if (DISPLAY_VER(dev_priv) >= 30)
> > + intel_ddi_wait_for_fec_status(encoder, crtc_state, true, true);
>
> Ugh. So I was trying to say that I don't want duplication of fec enable. But you
> still have that. Both intel_ddi_enable_fec() and
> retry_fec_enable() have it.
>
> It's probably intel_ddi_enable_fec() that should try multiple times for display
> version 30+. Right here.
>
> Adding the retries in "wait for status" is odd.
>
> Add building blocks for doing parts of what needs to be done, and then drive
> them at the high level.
>
Just to confirm that I understand the comment correctly, are we talking about something like this
static void intel_ddi_enable_fec(struct intel_encoder *encoder,
const struct intel_crtc_state *crtc_state)
{
...
intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state),
0, DP_TP_CTL_FEC_ENABLE);
if (DISPLAY_VER(dev_priv) < 30)
return;
for (i = 0; i < 3; i++) {
/* Wait for FEC status */
ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, true);
/* Return if FEC is enabled */
if(!ret)
return;
...
/* Disable FEC */
intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state),
DP_TP_CTL_FEC_ENABLE, 0);
/* Wait for FEC disabled */
ret = intel_ddi_wait_for_fec_status(encoder, crtc_state, false);
...
/* Enable FEC */
intel_de_rmw(dev_priv, dp_tp_ctl_reg(encoder, crtc_state),
0, DP_TP_CTL_FEC_ENABLE);
}
}
If so, we have to make two changes to the current code. Let me know if they work.
1. Change return type of intel_ddi_wait_for_fec_status and wait_for_fec_detected to int.
2. I could not find the reason for it (may be Imre knows) but currently the timeout for reading FEC DPCD register is set as 200ms with every read made in an interval of 5ms. However, the spec says that sink should acknowledge FEC enable within 5ms.
Considering that the initial values were selected based on some empirical evidence, can we have different timeout (5ms or 10ms) for the retry case depending on the Display version?
> > }
> >
> > static void intel_ddi_disable_fec(struct intel_encoder *encoder, @@
> > -3010,7 +3057,7 @@ static void intel_disable_ddi_buf(struct intel_encoder
> *encoder,
> > disable_ddi_buf(encoder, crtc_state);
> > }
> >
> > - intel_ddi_wait_for_fec_status(encoder, crtc_state, false);
> > + intel_ddi_wait_for_fec_status(encoder, crtc_state, false, false);
> > }
> >
> > static void intel_ddi_post_disable_dp(struct intel_atomic_state
> > *state, @@ -3383,6 +3430,7 @@ static void intel_enable_ddi(struct
> intel_atomic_state *state,
> > const struct intel_crtc_state *crtc_state,
> > const struct drm_connector_state *conn_state) {
> > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > struct intel_display *display = to_intel_display(encoder);
> > struct intel_crtc *pipe_crtc;
> > int i;
> > @@ -3394,7 +3442,8 @@ static void intel_enable_ddi(struct
> > intel_atomic_state *state,
> >
> > intel_enable_transcoder(crtc_state);
> >
> > - intel_ddi_wait_for_fec_status(encoder, crtc_state, true);
> > + if (DISPLAY_VER(i915) < 30)
> > + intel_ddi_wait_for_fec_status(encoder, crtc_state, true, false);
>
> Presumably there's no harm in waiting on all platforms here. It gets tricky
> with all the display version checks.
>
Ack
> >
> > for_each_pipe_crtc_modeset_enable(display, pipe_crtc, crtc_state, i) {
> > const struct intel_crtc_state *pipe_crtc_state = diff --git
> > a/drivers/gpu/drm/i915/display/intel_ddi.h
> > b/drivers/gpu/drm/i915/display/intel_ddi.h
> > index 6d85422bdefe..981e7702e11e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.h
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.h
> > @@ -65,7 +65,7 @@ void intel_ddi_enable_transcoder_clock(struct
> > intel_encoder *encoder, 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);
> > + bool enabled, bool retry);
> > 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); diff --git
> > a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 732d7543ad06..226ac9a73a55 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -1289,8 +1289,8 @@ static void intel_mst_enable_dp(struct
> > intel_atomic_state *state,
> >
> > wait_for_act_sent(encoder, pipe_config);
> >
> > - if (first_mst_stream)
> > - intel_ddi_wait_for_fec_status(encoder, pipe_config, true);
> > + if (first_mst_stream && DISPLAY_VER(dev_priv) < 30)
> > + intel_ddi_wait_for_fec_status(encoder, pipe_config, true,
> false);
>
> Ditto.
Ack
Regards
Chaitanya
>
> >
> > ret = drm_dp_add_payload_part2(&intel_dp->mst_mgr,
> >
> drm_atomic_get_mst_payload_state(mst_state,
>
> --
> Jani Nikula, Intel
