From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: update link training for 128b/132b
Date: Thu, 7 Oct 2021 13:46:11 +0300 [thread overview]
Message-ID: <YV7P88TJwDaErW0V@intel.com> (raw)
In-Reply-To: <20211007103108.7707-1-jani.nikula@intel.com>
On Thu, Oct 07, 2021 at 01:31:08PM +0300, Jani Nikula wrote:
> The 128b/132b channel coding link training uses more straightforward TX
> FFE preset values. Reuse voltage tries and max vswing for retry logic.
>
> The delays for 128b/132b are still all wrong, but this is regardless a
> step forward.
>
> v2: Fix UHBR rate checks, use intel_dp_is_uhbr() helper
>
> v3:
> - Rebase
> - Modify intel_dp_adjust_request_changed() and
> intel_dp_link_max_vswing_reached() to take 128b/132b into
> account. (Ville)
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 18 ++-
> .../drm/i915/display/intel_dp_link_training.c | 124 +++++++++++++-----
> 2 files changed, 106 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 3f7bbeb3e3cd..59428ce4f8c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -1338,13 +1338,20 @@ static int translate_signal_level(struct intel_dp *intel_dp,
> return 0;
> }
>
> -static int intel_ddi_dp_level(struct intel_dp *intel_dp, int lane)
> +static int intel_ddi_dp_level(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state,
> + int lane)
> {
> u8 train_set = intel_dp->train_set[lane];
> - u8 signal_levels = train_set & (DP_TRAIN_VOLTAGE_SWING_MASK |
> - DP_TRAIN_PRE_EMPHASIS_MASK);
>
> - return translate_signal_level(intel_dp, signal_levels);
> + if (intel_dp_is_uhbr(crtc_state)) {
> + return train_set & DP_TX_FFE_PRESET_VALUE_MASK;
BTW I just noticed dg2_get_snps_buf_trans() does not check for
HDMI vs. DP. Also it isn't using intel_dp_is_uhbr() and has
the > vs. >= bug in the open coded check.
> + } else {
> + u8 signal_levels = train_set & (DP_TRAIN_VOLTAGE_SWING_MASK |
> + DP_TRAIN_PRE_EMPHASIS_MASK);
> +
> + return translate_signal_level(intel_dp, signal_levels);
> + }
> }
>
> int intel_ddi_level(struct intel_encoder *encoder,
> @@ -1362,7 +1369,8 @@ int intel_ddi_level(struct intel_encoder *encoder,
> if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> level = intel_ddi_hdmi_level(encoder, trans);
> else
> - level = intel_ddi_dp_level(enc_to_intel_dp(encoder), lane);
> + level = intel_ddi_dp_level(enc_to_intel_dp(encoder), crtc_state,
> + lane);
>
> if (drm_WARN_ON_ONCE(&i915->drm, level >= n_entries))
> level = n_entries - 1;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 1a943ae38a6b..c54b7df56c9f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -304,11 +304,31 @@ static bool has_per_lane_signal_levels(struct intel_dp *intel_dp,
> return !intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy);
> }
>
> -static u8 intel_dp_get_lane_adjust_train(struct intel_dp *intel_dp,
> - const struct intel_crtc_state *crtc_state,
> - enum drm_dp_phy dp_phy,
> - const u8 link_status[DP_LINK_STATUS_SIZE],
> - int lane)
> +
> +static u8 intel_dp_get_lane_adjust_train_128b132b(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state,
> + enum drm_dp_phy dp_phy,
> + const u8 link_status[DP_LINK_STATUS_SIZE],
> + int lane)
> +{
> + u8 tx_ffe = 0;
> +
> + if (has_per_lane_signal_levels(intel_dp, dp_phy)) {
> + lane = min(lane, crtc_state->lane_count - 1);
> + tx_ffe = drm_dp_get_adjust_tx_ffe_preset(link_status, lane);
> + } else {
> + for (lane = 0; lane < crtc_state->lane_count; lane++)
> + tx_ffe = max(tx_ffe, drm_dp_get_adjust_tx_ffe_preset(link_status, lane));
> + }
> +
> + return tx_ffe;
> +}
> +
> +static u8 intel_dp_get_lane_adjust_train_8b10b(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state,
> + enum drm_dp_phy dp_phy,
> + const u8 link_status[DP_LINK_STATUS_SIZE],
> + int lane)
> {
> u8 v = 0;
> u8 p = 0;
> @@ -340,6 +360,20 @@ static u8 intel_dp_get_lane_adjust_train(struct intel_dp *intel_dp,
> return v | p;
> }
>
> +static u8 intel_dp_get_lane_adjust_train(struct intel_dp *intel_dp,
> + const struct intel_crtc_state *crtc_state,
> + enum drm_dp_phy dp_phy,
> + const u8 link_status[DP_LINK_STATUS_SIZE],
> + int lane)
> +{
> + if (intel_dp_is_uhbr(crtc_state))
> + return intel_dp_get_lane_adjust_train_128b132b(intel_dp, crtc_state,
> + dp_phy, link_status, lane);
> + else
> + return intel_dp_get_lane_adjust_train_8b10b(intel_dp, crtc_state,
> + dp_phy, link_status, lane);
> +}
> +
> #define TRAIN_REQ_FMT "%d/%d/%d/%d"
> #define _TRAIN_REQ_VSWING_ARGS(link_status, lane) \
> (drm_dp_get_adjust_request_voltage((link_status), (lane)) >> DP_TRAIN_VOLTAGE_SWING_SHIFT)
^ those would need doing too
> @@ -464,6 +498,13 @@ intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
> _TRAIN_SET_PREEMPH_ARGS((train_set)[1]), \
> _TRAIN_SET_PREEMPH_ARGS((train_set)[2]), \
> _TRAIN_SET_PREEMPH_ARGS((train_set)[3])
> +#define _TRAIN_SET_TX_FFE_ARGS(train_set) \
> + ((train_set) & DP_TX_FFE_PRESET_VALUE_MASK), ""
Sure, why not.
> +#define TRAIN_SET_TX_FFE_ARGS(train_set) \
> + _TRAIN_SET_TX_FFE_ARGS((train_set)[0]), \
> + _TRAIN_SET_TX_FFE_ARGS((train_set)[1]), \
> + _TRAIN_SET_TX_FFE_ARGS((train_set)[2]), \
> + _TRAIN_SET_TX_FFE_ARGS((train_set)[3])
>
> void intel_dp_set_signal_levels(struct intel_dp *intel_dp,
> const struct intel_crtc_state *crtc_state,
> @@ -473,14 +514,23 @@ void intel_dp_set_signal_levels(struct intel_dp *intel_dp,
> struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> char phy_name[10];
>
> - drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s][%s] lanes: %d, "
> - "vswing levels: " TRAIN_SET_FMT ", "
> - "pre-emphasis levels: " TRAIN_SET_FMT "\n",
> - encoder->base.base.id, encoder->base.name,
> - intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name)),
> - crtc_state->lane_count,
> - TRAIN_SET_VSWING_ARGS(intel_dp->train_set),
> - TRAIN_SET_PREEMPH_ARGS(intel_dp->train_set));
> + if (intel_dp_is_uhbr(crtc_state)) {
> + drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s][%s] lanes: %d, "
> + "128b/132b TX FFE presets: " TRAIN_SET_FMT "\n",
> + encoder->base.base.id, encoder->base.name,
> + intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name)),
> + crtc_state->lane_count,
> + TRAIN_SET_TX_FFE_ARGS(intel_dp->train_set));
> + } else {
> + drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s][%s] lanes: %d, "
> + "vswing levels: " TRAIN_SET_FMT ", "
> + "pre-emphasis levels: " TRAIN_SET_FMT "\n",
> + encoder->base.base.id, encoder->base.name,
> + intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name)),
> + crtc_state->lane_count,
> + TRAIN_SET_VSWING_ARGS(intel_dp->train_set),
> + TRAIN_SET_PREEMPH_ARGS(intel_dp->train_set));
Wonder if we should say "8b/10b" here just to be super clear?
> + }
>
> if (intel_dp_phy_is_downstream_of_source(intel_dp, dp_phy))
> encoder->set_signal_levels(encoder, crtc_state);
> @@ -529,16 +579,22 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> * 2+1 or 3+0 depending on whether vswing level 3 is supported or not.
> */
> for (lane = 0; lane < crtc_state->lane_count; lane++) {
> - u8 v = (intel_dp->train_set[lane] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> - DP_TRAIN_VOLTAGE_SWING_SHIFT;
> - u8 p = (intel_dp->train_set[lane] & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> - DP_TRAIN_PRE_EMPHASIS_SHIFT;
> -
> - if ((intel_dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) == 0)
> - return false;
> -
> - if (v + p != 3)
> - return false;
> + if (intel_dp_is_uhbr(crtc_state)) {
> + if ((intel_dp->train_set[lane] & DP_TX_FFE_PRESET_VALUE_MASK) !=
> + DP_TX_FFE_PRESET_VALUE_MASK)
> + return false;
Seems sane. Maybe we shuld suck these little things into
128b/132b vs. 8b/10b specific helper function just to
declutter intel_dp_link_max_vswing_reached() a bit?
> + } else {
> + u8 v = (intel_dp->train_set[lane] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
> + u8 p = (intel_dp->train_set[lane] & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> + DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +
> + if ((intel_dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) == 0)
> + return false;
> +
> + if (v + p != 3)
> + return false;
> + }
> }
>
> return true;
> @@ -601,17 +657,24 @@ static void intel_dp_link_training_clock_recovery_delay(struct intel_dp *intel_d
> drm_dp_lttpr_link_train_clock_recovery_delay();
> }
>
> -static bool intel_dp_adjust_request_changed(int lane_count,
> +static bool intel_dp_adjust_request_changed(const struct intel_crtc_state *crtc_state,
> const u8 old_link_status[DP_LINK_STATUS_SIZE],
> const u8 new_link_status[DP_LINK_STATUS_SIZE])
> {
> int lane;
>
> - for (lane = 0; lane < lane_count; lane++) {
> - u8 old = drm_dp_get_adjust_request_voltage(old_link_status, lane) |
> - drm_dp_get_adjust_request_pre_emphasis(old_link_status, lane);
> - u8 new = drm_dp_get_adjust_request_voltage(new_link_status, lane) |
> - drm_dp_get_adjust_request_pre_emphasis(new_link_status, lane);
> + for (lane = 0; lane < crtc_state->lane_count; lane++) {
> + u8 old, new;
> +
> + if (intel_dp_is_uhbr(crtc_state)) {
> + old = drm_dp_get_adjust_tx_ffe_preset(old_link_status, lane);
> + new = drm_dp_get_adjust_tx_ffe_preset(new_link_status, lane);
> + } else {
> + old = drm_dp_get_adjust_request_voltage(old_link_status, lane) |
> + drm_dp_get_adjust_request_pre_emphasis(old_link_status, lane);
> + new = drm_dp_get_adjust_request_voltage(new_link_status, lane) |
> + drm_dp_get_adjust_request_pre_emphasis(new_link_status, lane);
> + }
I like it.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> if (old != new)
> return true;
> @@ -721,8 +784,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
> return false;
> }
>
> - if (!intel_dp_adjust_request_changed(crtc_state->lane_count,
> - old_link_status, link_status))
> + if (!intel_dp_adjust_request_changed(crtc_state, old_link_status, link_status))
> ++voltage_tries;
> else
> voltage_tries = 1;
> --
> 2.30.2
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2021-10-07 10:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-07 10:31 [Intel-gfx] [PATCH] drm/i915/dg2: update link training for 128b/132b Jani Nikula
2021-10-07 10:46 ` Ville Syrjälä [this message]
2021-10-07 11:43 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/dg2: update link training for 128b/132b (rev2) Patchwork
2021-10-07 12:17 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2021-10-01 10:02 [Intel-gfx] [PATCH] drm/i915/dg2: update link training for 128b/132b Jani Nikula
2021-10-01 15:38 ` Ville Syrjälä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YV7P88TJwDaErW0V@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.