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.
I also think the patch evolved to be considerably neater than the
original. What do you think?
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
