> -----Original Message----- > From: Deak, Imre <imre.deak@xxxxxxxxx> > Sent: Wednesday, July 24, 2024 4:57 PM > To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 09/14] drm/i915/dp_mst: Reduce the link parameters in > BW order after LT failures > > On Wed, Jul 24, 2024 at 09:43:55AM +0300, Kandpal, Suraj wrote: > > > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf > > > Of Imre Deak > > > Sent: Monday, July 22, 2024 10:25 PM > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Subject: [PATCH 09/14] drm/i915/dp_mst: Reduce the link parameters > > > in BW order after LT failures > > > > > > On MST links - at least for some MST branch devices - the list of > > > modes returned to users on an enabled link depends on the current > > > link rate/lane count parameters (besides the DPRX link capabilities, > > > any MST branch BW limit and the maximum link parameters reduced > > > after LT failures). In particular the MST branch BW limit may depend > > > on the link rate/lane count parameters programmed to DPCD. After an > > > LT failure and limiting the maximum link parameters accordingly, > > > users should see a mode list reflecting these new limits. However > > > with the current fallback order this isn't ensured, as the new limit > > > could allow for modes requiring a higher link BW, but these modes will > be filtered out due to the enabled link's lower link BW. > > > > > > Ensure that the mode list changes in a consistent way after a link > > > training failure and reducing the link parameters by changing the > > > fallback order on MST links to happen in BW order. > > > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > --- > > > .../drm/i915/display/intel_display_types.h | 12 ++ > > > drivers/gpu/drm/i915/display/intel_dp.c | 111 ++++++++++++++++++ > > > drivers/gpu/drm/i915/display/intel_dp.h | 2 + > > > .../drm/i915/display/intel_dp_link_training.c | 43 ++++++- > > > 4 files changed, 166 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 3501125c55158..51e2151315977 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -1778,6 +1778,18 @@ struct intel_dp { > > > int common_rates[DP_MAX_SUPPORTED_RATES]; > > > struct { > > > /* TODO: move the rest of link specific fields to here > > > */ > > > + /* common rate,lane_count configs in bw order */ > > > + int num_configs; > > > +#define INTEL_DP_MAX_LANE_COUNT 4 > > > +#define INTEL_DP_MAX_SUPPORTED_LANE_COUNTS > > > (ilog2(INTEL_DP_MAX_LANE_COUNT) + 1) > > > > I feel this name could be a little confusing this define here I am > > assuming the Different lane count configs we can support which would > > be 1,2,4 hence 3, Maybe renaming it as DP_MAX_LANE_COUNT_CONFIG > > Ok, will rename it to INTEL_DP_MAX_SUPPORTED_LANE_CONFIGS. > > > > +#define INTEL_DP_LANE_COUNT_EXP_BITS > > > order_base_2(INTEL_DP_MAX_SUPPORTED_LANE_COUNTS) > > > +#define INTEL_DP_LINK_RATE_IDX_BITS (BITS_PER_TYPE(u8) - > > > INTEL_DP_LANE_COUNT_EXP_BITS) > > > +#define INTEL_DP_MAX_LINK_CONFIGS > > > (DP_MAX_SUPPORTED_RATES * \ > > > + > > > INTEL_DP_MAX_SUPPORTED_LANE_COUNTS) > > > + struct intel_dp_link_config { > > > + u8 link_rate_idx:INTEL_DP_LINK_RATE_IDX_BITS; > > > + u8 > > > lane_count_exp:INTEL_DP_LANE_COUNT_EXP_BITS; > > > + } configs[INTEL_DP_MAX_LINK_CONFIGS]; > > > /* Max lane count for the current link */ > > > int max_lane_count; > > > /* Max rate for the current link */ diff --git > > > a/drivers/gpu/drm/i915/display/intel_dp.c > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > index 0882dddd97206..d3529c5836393 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > @@ -29,6 +29,7 @@ > > > #include <linux/i2c.h> > > > #include <linux/notifier.h> > > > #include <linux/slab.h> > > > +#include <linux/sort.h> > > > #include <linux/string_helpers.h> > > > #include <linux/timekeeping.h> > > > #include <linux/types.h> > > > @@ -634,6 +635,114 @@ int intel_dp_rate_index(const int *rates, int > > > len, int rate) > > > return -1; > > > } > > > > > > +static int intel_dp_link_config_rate(struct intel_dp *intel_dp, > > > + const struct intel_dp_link_config *lc) { > > > + return intel_dp_common_rate(intel_dp, lc->link_rate_idx); } > > > + > > > +static int intel_dp_link_config_lane_count(const struct > > > +intel_dp_link_config *lc) { > > > + return 1 << lc->lane_count_exp; } > > > + > > > +static int intel_dp_link_config_bw(struct intel_dp *intel_dp, > > > + const struct intel_dp_link_config *lc) { > > > + return > > > drm_dp_max_dprx_data_rate(intel_dp_link_config_rate(intel_dp, lc), > > > + > > > +intel_dp_link_config_lane_count(lc)); > > > +} > > > + > > > +static int link_config_cmp_by_bw(const void *a, const void *b, > > > +const void *p) { > > > + struct intel_dp *intel_dp = (struct intel_dp *)p; /* remove const > */ > > > + const struct intel_dp_link_config *lc_a = a; > > > + const struct intel_dp_link_config *lc_b = b; > > > + int bw_a = intel_dp_link_config_bw(intel_dp, lc_a); > > > + int bw_b = intel_dp_link_config_bw(intel_dp, lc_b); > > > + > > > + if (bw_a != bw_b) > > > + return bw_a - bw_b; > > > + > > > + return intel_dp_link_config_rate(intel_dp, lc_a) - > > > + intel_dp_link_config_rate(intel_dp, lc_b); } > > > + > > > +static void link_config_swap(void *a, void *b, int size, const void > > > +* __always_unused p) { > > > + struct intel_dp_link_config *lc_a = a; > > > + struct intel_dp_link_config *lc_b = b; > > > + > > > + swap(*lc_a, *lc_b); > > > +} > > > + > > > +static void intel_dp_link_config_init(struct intel_dp *intel_dp) { > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + struct intel_dp_link_config *lc; > > > + int num_common_lane_counts; > > > + int i; > > > + int j; > > > + > > > + if (drm_WARN_ON(&i915->drm, > > > !is_power_of_2(intel_dp_max_common_lane_count(intel_dp)))) > > > + return; > > > + > > > + num_common_lane_counts = > > > +ilog2(intel_dp_max_common_lane_count(intel_dp)) + 1; > > > > Same thing here can we rename it as num_common_lane_count_config > > Ok, will rename to num_common_lane_configs. > Rest LGTM, Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx> > > Regards, > > Suraj Kandpal > > > + > > > + if (drm_WARN_ON(&i915->drm, intel_dp->num_common_rates * > > > num_common_lane_counts > > > > + ARRAY_SIZE(intel_dp->link.configs))) > > > + return; > > > + > > > + intel_dp->link.num_configs = intel_dp->num_common_rates * > > > +num_common_lane_counts; > > > + > > > + lc = &intel_dp->link.configs[0]; > > > + for (i = 0; i < intel_dp->num_common_rates; i++) { > > > + for (j = 0; j < num_common_lane_counts; j++) { > > > + lc->lane_count_exp = j; > > > + lc->link_rate_idx = i; > > > + > > > + lc++; > > > + } > > > + } > > > + > > > + sort_r(intel_dp->link.configs, intel_dp->link.num_configs, > > > + sizeof(intel_dp->link.configs[0]), > > > + link_config_cmp_by_bw, link_config_swap, > > > + intel_dp); > > > +} > > > + > > > +void intel_dp_link_config_get(struct intel_dp *intel_dp, int idx, > > > +int *link_rate, int *lane_count) { > > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > > + const struct intel_dp_link_config *lc; > > > + > > > + if (drm_WARN_ON(&i915->drm, idx < 0 || idx >= intel_dp- > > > >link.num_configs)) > > > + idx = 0; > > > + > > > + lc = &intel_dp->link.configs[idx]; > > > + > > > + *link_rate = intel_dp_link_config_rate(intel_dp, lc); > > > + *lane_count = intel_dp_link_config_lane_count(lc); > > > +} > > > + > > > +int intel_dp_link_config_index(struct intel_dp *intel_dp, int > > > +link_rate, int lane_count) { > > > + int link_rate_idx = > > > +intel_dp_rate_index(intel_dp->common_rates, > > > intel_dp->num_common_rates, > > > + link_rate); > > > + int lane_count_exp = ilog2(lane_count); > > > + int i; > > > + > > > + for (i = 0; i < intel_dp->link.num_configs; i++) { > > > + const struct intel_dp_link_config *lc = &intel_dp- > > > >link.configs[i]; > > > + > > > + if (lc->lane_count_exp == lane_count_exp && > > > + lc->link_rate_idx == link_rate_idx) > > > + return i; > > > + } > > > + > > > + return -1; > > > +} > > > + > > > static void intel_dp_set_common_rates(struct intel_dp *intel_dp) { > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); @@ > > > -652,6 > > > +761,8 @@ static void intel_dp_set_common_rates(struct intel_dp > > > *intel_dp) > > > intel_dp->common_rates[0] = 162000; > > > intel_dp->num_common_rates = 1; > > > } > > > + > > > + intel_dp_link_config_init(intel_dp); > > > } > > > > > > static bool intel_dp_link_params_valid(struct intel_dp *intel_dp, > > > int link_rate, diff --git a/drivers/gpu/drm/i915/display/intel_dp.h > > > b/drivers/gpu/drm/i915/display/intel_dp.h > > > index 9be539edf817b..1b9aaddd8c35c 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp.h > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h > > > @@ -107,6 +107,8 @@ int intel_dp_max_common_rate(struct intel_dp > > > *intel_dp); int intel_dp_max_common_lane_count(struct intel_dp > > > *intel_dp); int intel_dp_common_rate(struct intel_dp *intel_dp, int > > > index); int intel_dp_rate_index(const int *rates, int len, int > > > rate); > > > +int intel_dp_link_config_index(struct intel_dp *intel_dp, int > > > +link_rate, int lane_count); void intel_dp_link_config_get(struct > > > +intel_dp *intel_dp, int idx, int *link_rate, int *lane_count); > > > void intel_dp_update_sink_caps(struct intel_dp *intel_dp); void > > > intel_dp_reset_link_params(struct intel_dp *intel_dp); > > > > > > 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 214c8858b8a94..0c8e0d6437b5b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > @@ -1170,6 +1170,41 @@ static bool > > > intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp, > > > return true; > > > } > > > > > > +static bool reduce_link_params_in_bw_order(struct intel_dp *intel_dp, > > > + const struct > > > +intel_crtc_state > > > *crtc_state, > > > + int *new_link_rate, int > > > *new_lane_count) { > > > + int link_rate; > > > + int lane_count; > > > + int i; > > > + > > > + i = intel_dp_link_config_index(intel_dp, > > > + crtc_state->port_clock, > > > crtc_state->lane_count); > > > + for (i--; i >= 0; i--) { > > > + intel_dp_link_config_get(intel_dp, i, &link_rate, > > > &lane_count); > > > + > > > + if ((intel_dp->link.force_rate && > > > + intel_dp->link.force_rate != link_rate) || > > > + (intel_dp->link.force_lane_count && > > > + intel_dp->link.force_lane_count != lane_count)) > > > + continue; > > > + > > > + /* TODO: Make switching from UHBR to non-UHBR rates > > > work. */ > > > + if (drm_dp_is_uhbr_rate(crtc_state->port_clock) != > > > + drm_dp_is_uhbr_rate(link_rate)) > > > + continue; > > > + > > > + break; > > > + } > > > + > > > + if (i < 0) > > > + return false; > > > + > > > + *new_link_rate = link_rate; > > > + *new_lane_count = lane_count; > > > + > > > + return true; > > > +} > > > + > > > static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate) { > > > int rate_index; > > > @@ -1231,8 +1266,12 @@ static bool > > > reduce_link_params_in_rate_lane_order(struct intel_dp *intel_dp, > > > static bool reduce_link_params(struct intel_dp *intel_dp, const > > > struct intel_crtc_state *crtc_state, > > > int *new_link_rate, int *new_lane_count) { > > > - return reduce_link_params_in_rate_lane_order(intel_dp, crtc_state, > > > - new_link_rate, > > > new_lane_count); > > > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) > > > + return reduce_link_params_in_bw_order(intel_dp, > > > crtc_state, > > > + new_link_rate, > > > new_lane_count); > > > + else > > > + return reduce_link_params_in_rate_lane_order(intel_dp, > > > crtc_state, > > > + > > > + new_link_rate, > > > new_lane_count); > > > } > > > > > > static int intel_dp_get_link_train_fallback_values(struct intel_dp > > > *intel_dp, > > > -- > > > 2.44.2 > >