| Message ID | 20240206104759.2079133-2-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DP link training failure fallback | expand |
On Tue, 2024-02-06 at 16:17 +0530, Arun R Murthy wrote: > Fallback mandates on DP link training failure. This patch just covers > the DP2.0 fallback sequence. > > TODO: Need to implement the DP1.4 fallback. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 92 ++++++++++++++++++++++- > -- > 1 file changed, 82 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 10ec231acd98..82d354a6b0cd 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, > 15}; > */ > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > +/* DL Link Rates */ > +#define UHBR20 2000000 > +#define UHBR13P5 1350000 Do we have 13.5G support? Plz check Specs:55726 Note 6 Thanks Khaled > +#define UHBR10 1000000 > +#define HBR3 810000 > +#define HBR2 540000 > +#define HBR 270000 > +#define RBR 162000 > + > +/* DP Lane Count */ > +#define LANE_COUNT_4 4 > +#define LANE_COUNT_2 2 > +#define LANE_COUNT_1 1 > + > +/* DP2.0 fallback values */ > +struct dp_fallback { > + u32 link_rate; > + u8 lane_count; > +}; > + > +struct dp_fallback dp2dot0_fallback[] = { > + {UHBR20, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_4}, > + {UHBR20, LANE_COUNT_2}, > + {UHBR10, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_2}, > + {HBR3, LANE_COUNT_4}, > + {UHBR20, LANE_COUNT_1}, > + {UHBR10, LANE_COUNT_2}, > + {HBR2, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_1}, > + {HBR3, LANE_COUNT_2}, > + {UHBR10, LANE_COUNT_1}, > + {HBR2, LANE_COUNT_2}, > + {HBR, LANE_COUNT_4}, > + {HBR3, LANE_COUNT_1}, > + {RBR, LANE_COUNT_4}, > + {HBR2, LANE_COUNT_1}, > + {HBR, LANE_COUNT_2}, > + {RBR, LANE_COUNT_2}, > + {HBR, LANE_COUNT_1}, > + {RBR, LANE_COUNT_1}, > +}; > + > /** > * intel_dp_is_edp - is the given port attached to an eDP panel > (either CPU or PCH) > * @intel_dp: DP struct > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const > struct intel_dp *intel_dp, > intel_dp->num_common_rates, > max_rate); > } > > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, > u32 link_rate) > +{ > + u8 i; > + > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > + if (intel_dp->common_rates[i] == link_rate) > + return true; > + else > + continue; > + } > + return false; > +} > + > static int intel_dp_common_rate(struct intel_dp *intel_dp, int > index) > { > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > @@ -671,15 +728,6 @@ int > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > int index; > > - /* > - * TODO: Enable fallback on MST links once MST link compute can > handle > - * the fallback params. > - */ > - if (intel_dp->is_mst) { > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > - return -1; > - } > - > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > drm_dbg_kms(&i915->drm, > "Retrying Link training for eDP with max > parameters\n"); > @@ -687,6 +735,31 @@ int > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > return 0; > } > > + /* DP fallback values */ > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & > DP_CAP_ANSI_128B132B) { > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); > index++) { > + if (link_rate == > dp2dot0_fallback[index].link_rate && > + lane_count == > dp2dot0_fallback[index].lane_count) { > + for(index += 1; index < > ARRAY_SIZE(dp2dot0_fallback); index++) { > + if > (intel_dp_link_rate_supported(intel_dp, > + dp2dot0_fallbac > k[index].link_rate)) { > + intel_dp_set_link_param > s(intel_dp, > + d > p2dot0_fallback[index].link_rate, > + d > p2dot0_fallback[index].lane_count); > + drm_dbg_kms(&i915->drm, > + "Retrying > Link training with link rate %d and lane count %d\n", > + dp2dot0_fal > lback[index].link_rate, > + dp2dot0_fal > lback[index].lane_count); > + return 0; > + } > + } > + } > + } > + /* Report failure and fail link training */ > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); > + return -1; > + } > + > index = intel_dp_rate_index(intel_dp->common_rates, > intel_dp->num_common_rates, > link_rate); > @@ -716,7 +789,6 @@ int > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > return -1; > } > - > return 0; > } >
> -----Original Message----- > From: Almahallawy, Khaled <khaled.almahallawy@intel.com> > Sent: Wednesday, February 7, 2024 4:11 AM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Shankar, Uma <uma.shankar@intel.com>; Nikula, Jani > <jani.nikula@intel.com>; Deak, Imre <imre.deak@intel.com>; Syrjala, Ville > <ville.syrjala@intel.com> > Subject: Re: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT > > On Tue, 2024-02-06 at 16:17 +0530, Arun R Murthy wrote: > > Fallback mandates on DP link training failure. This patch just covers > > the DP2.0 fallback sequence. > > > > TODO: Need to implement the DP1.4 fallback. > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 92 ++++++++++++++++++++++- > > -- > > 1 file changed, 82 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 10ec231acd98..82d354a6b0cd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, > > 15}; > > */ > > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > > > +/* DL Link Rates */ > > +#define UHBR20 2000000 > > +#define UHBR13P5 1350000 > > Do we have 13.5G support? > Plz check Specs:55726 Note 6 > Long back had posted a patch to remove UHBR13.5(https://patchwork.freedesktop.org/series/119555/), but was rejected. Hence added this. Thanks and Regards, Arun R Murthy -------------------- > Thanks > Khaled > > > +#define UHBR10 1000000 > > +#define HBR3 810000 > > +#define HBR2 540000 > > +#define HBR 270000 > > +#define RBR 162000 > > + > > +/* DP Lane Count */ > > +#define LANE_COUNT_4 4 > > +#define LANE_COUNT_2 2 > > +#define LANE_COUNT_1 1 > > + > > +/* DP2.0 fallback values */ > > +struct dp_fallback { > > + u32 link_rate; > > + u8 lane_count; > > +}; > > + > > +struct dp_fallback dp2dot0_fallback[] = { > > + {UHBR20, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_4}, > > + {UHBR20, LANE_COUNT_2}, > > + {UHBR10, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_2}, > > + {HBR3, LANE_COUNT_4}, > > + {UHBR20, LANE_COUNT_1}, > > + {UHBR10, LANE_COUNT_2}, > > + {HBR2, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_1}, > > + {HBR3, LANE_COUNT_2}, > > + {UHBR10, LANE_COUNT_1}, > > + {HBR2, LANE_COUNT_2}, > > + {HBR, LANE_COUNT_4}, > > + {HBR3, LANE_COUNT_1}, > > + {RBR, LANE_COUNT_4}, > > + {HBR2, LANE_COUNT_1}, > > + {HBR, LANE_COUNT_2}, > > + {RBR, LANE_COUNT_2}, > > + {HBR, LANE_COUNT_1}, > > + {RBR, LANE_COUNT_1}, > > +}; > > + > > /** > > * intel_dp_is_edp - is the given port attached to an eDP panel > > (either CPU or PCH) > > * @intel_dp: DP struct > > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const > > struct intel_dp *intel_dp, > > intel_dp->num_common_rates, max_rate); > } > > > > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, > > u32 link_rate) > > +{ > > + u8 i; > > + > > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > > + if (intel_dp->common_rates[i] == link_rate) > > + return true; > > + else > > + continue; > > + } > > + return false; > > +} > > + > > static int intel_dp_common_rate(struct intel_dp *intel_dp, int > > index) > > { > > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > > @@ -671,15 +728,6 @@ int > > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > int index; > > > > - /* > > - * TODO: Enable fallback on MST links once MST link compute can > > handle > > - * the fallback params. > > - */ > > - if (intel_dp->is_mst) { > > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > - return -1; > > - } > > - > > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > > drm_dbg_kms(&i915->drm, > > "Retrying Link training for eDP with max > parameters\n"); @@ > > -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct > > intel_dp *intel_dp, > > return 0; > > } > > > > + /* DP fallback values */ > > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & > > DP_CAP_ANSI_128B132B) { > > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); > > index++) { > > + if (link_rate == > > dp2dot0_fallback[index].link_rate && > > + lane_count == > > dp2dot0_fallback[index].lane_count) { > > + for(index += 1; index < > > ARRAY_SIZE(dp2dot0_fallback); index++) { > > + if > > (intel_dp_link_rate_supported(intel_dp, > > + dp2dot0_fallbac > > k[index].link_rate)) { > > + intel_dp_set_link_param > > s(intel_dp, > > + d > > p2dot0_fallback[index].link_rate, > > + d > > p2dot0_fallback[index].lane_count); > > + drm_dbg_kms(&i915->drm, > > + "Retrying > > Link training with link rate %d and lane count %d\n", > > + dp2dot0_fal > > lback[index].link_rate, > > + dp2dot0_fal > > lback[index].lane_count); > > + return 0; > > + } > > + } > > + } > > + } > > + /* Report failure and fail link training */ > > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > + return -1; > > + } > > + > > index = intel_dp_rate_index(intel_dp->common_rates, > > intel_dp->num_common_rates, > > link_rate); > > @@ -716,7 +789,6 @@ int > > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > return -1; > > } > > - > > return 0; > > } > >
On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > Fallback mandates on DP link training failure. This patch just covers > the DP2.0 fallback sequence. > > TODO: Need to implement the DP1.4 fallback. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 92 ++++++++++++++++++++++--- > 1 file changed, 82 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 10ec231acd98..82d354a6b0cd 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15}; > */ > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > +/* DL Link Rates */ > +#define UHBR20 2000000 > +#define UHBR13P5 1350000 > +#define UHBR10 1000000 > +#define HBR3 810000 > +#define HBR2 540000 > +#define HBR 270000 > +#define RBR 162000 > + > +/* DP Lane Count */ > +#define LANE_COUNT_4 4 > +#define LANE_COUNT_2 2 > +#define LANE_COUNT_1 1 > + > +/* DP2.0 fallback values */ > +struct dp_fallback { > + u32 link_rate; > + u8 lane_count; > +}; > + > +struct dp_fallback dp2dot0_fallback[] = { > + {UHBR20, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_4}, > + {UHBR20, LANE_COUNT_2}, > + {UHBR10, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_2}, > + {HBR3, LANE_COUNT_4}, > + {UHBR20, LANE_COUNT_1}, > + {UHBR10, LANE_COUNT_2}, > + {HBR2, LANE_COUNT_4}, > + {UHBR13P5, LANE_COUNT_1}, > + {HBR3, LANE_COUNT_2}, > + {UHBR10, LANE_COUNT_1}, > + {HBR2, LANE_COUNT_2}, > + {HBR, LANE_COUNT_4}, > + {HBR3, LANE_COUNT_1}, > + {RBR, LANE_COUNT_4}, > + {HBR2, LANE_COUNT_1}, > + {HBR, LANE_COUNT_2}, > + {RBR, LANE_COUNT_2}, > + {HBR, LANE_COUNT_1}, > + {RBR, LANE_COUNT_1}, > +}; > + > /** > * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or PCH) > * @intel_dp: DP struct > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const struct intel_dp *intel_dp, > intel_dp->num_common_rates, max_rate); > } > > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, u32 link_rate) > +{ > + u8 i; > + > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > + if (intel_dp->common_rates[i] == link_rate) > + return true; > + else > + continue; > + } > + return false; > +} > + > static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) > { > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > @@ -671,15 +728,6 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > int index; > > - /* > - * TODO: Enable fallback on MST links once MST link compute can handle > - * the fallback params. > - */ > - if (intel_dp->is_mst) { > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > - return -1; > - } > - By removing this, the claim is both 8b/10b and 128b/132b DP MST link training fallbacks work... > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > drm_dbg_kms(&i915->drm, > "Retrying Link training for eDP with max parameters\n"); > @@ -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > return 0; > } > > + /* DP fallback values */ > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) { ...but this only addresses 128b/132b, and the 8b/10b MST drops to the existing SST fallback path. And with the current code, DP_CAP_ANSI_128B132B does not decide whether we use DP MST or not. So this will also cover 8b/10b fallback for displays that support 128b/132b but have DP_MSTM_CAP == 0. > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); index++) { > + if (link_rate == dp2dot0_fallback[index].link_rate && > + lane_count == dp2dot0_fallback[index].lane_count) { > + for(index += 1; index < ARRAY_SIZE(dp2dot0_fallback); index++) { I honestly do not understand the double looping here, and how index is managed. > + if (intel_dp_link_rate_supported(intel_dp, > + dp2dot0_fallback[index].link_rate)) { > + intel_dp_set_link_params(intel_dp, > + dp2dot0_fallback[index].link_rate, > + dp2dot0_fallback[index].lane_count); intel_dp_set_link_params() is supposed to be called in the DP encoder (pre-)enable paths to set the link rates. If you do it here, the subsequent enable will just overwrite whatever you did here. The mechanism in this function should be to to adjust intel_dp->max_link_rate and intel_dp->max_link_lane_count, and then the caller will send an uevent to have the userspace do everything again, but with reduced max values. This is all very convoluted. And I admit the existing code is also complex, but this makes it *much* harder to understand. BR, Jani. > + drm_dbg_kms(&i915->drm, > + "Retrying Link training with link rate %d and lane count %d\n", > + dp2dot0_fallback[index].link_rate, > + dp2dot0_fallback[index].lane_count); > + return 0; > + } > + } > + } > + } > + /* Report failure and fail link training */ > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); > + return -1; > + } > + > index = intel_dp_rate_index(intel_dp->common_rates, > intel_dp->num_common_rates, > link_rate); > @@ -716,7 +789,6 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > return -1; > } > - > return 0; > }
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Tuesday, February 13, 2024 11:41 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; > Shankar, Uma <uma.shankar@intel.com>; Murthy, Arun R > <arun.r.murthy@intel.com> > Subject: Re: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT > > On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > > Fallback mandates on DP link training failure. This patch just covers > > the DP2.0 fallback sequence. > > > > TODO: Need to implement the DP1.4 fallback. > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 92 > > ++++++++++++++++++++++--- > > 1 file changed, 82 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > b/drivers/gpu/drm/i915/display/intel_dp.c > > index 10ec231acd98..82d354a6b0cd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15}; > > */ > > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > > > > +/* DL Link Rates */ > > +#define UHBR20 2000000 > > +#define UHBR13P5 1350000 > > +#define UHBR10 1000000 > > +#define HBR3 810000 > > +#define HBR2 540000 > > +#define HBR 270000 > > +#define RBR 162000 > > + > > +/* DP Lane Count */ > > +#define LANE_COUNT_4 4 > > +#define LANE_COUNT_2 2 > > +#define LANE_COUNT_1 1 > > + > > +/* DP2.0 fallback values */ > > +struct dp_fallback { > > + u32 link_rate; > > + u8 lane_count; > > +}; > > + > > +struct dp_fallback dp2dot0_fallback[] = { > > + {UHBR20, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_4}, > > + {UHBR20, LANE_COUNT_2}, > > + {UHBR10, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_2}, > > + {HBR3, LANE_COUNT_4}, > > + {UHBR20, LANE_COUNT_1}, > > + {UHBR10, LANE_COUNT_2}, > > + {HBR2, LANE_COUNT_4}, > > + {UHBR13P5, LANE_COUNT_1}, > > + {HBR3, LANE_COUNT_2}, > > + {UHBR10, LANE_COUNT_1}, > > + {HBR2, LANE_COUNT_2}, > > + {HBR, LANE_COUNT_4}, > > + {HBR3, LANE_COUNT_1}, > > + {RBR, LANE_COUNT_4}, > > + {HBR2, LANE_COUNT_1}, > > + {HBR, LANE_COUNT_2}, > > + {RBR, LANE_COUNT_2}, > > + {HBR, LANE_COUNT_1}, > > + {RBR, LANE_COUNT_1}, > > +}; > > + > > /** > > * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or > PCH) > > * @intel_dp: DP struct > > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const > struct intel_dp *intel_dp, > > intel_dp->num_common_rates, max_rate); > } > > > > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, > > +u32 link_rate) { > > + u8 i; > > + > > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > > + if (intel_dp->common_rates[i] == link_rate) > > + return true; > > + else > > + continue; > > + } > > + return false; > > +} > > + > > static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) > > { > > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > > @@ -671,15 +728,6 @@ int intel_dp_get_link_train_fallback_values(struct > intel_dp *intel_dp, > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > int index; > > > > - /* > > - * TODO: Enable fallback on MST links once MST link compute can > handle > > - * the fallback params. > > - */ > > - if (intel_dp->is_mst) { > > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > - return -1; > > - } > > - > > By removing this, the claim is both 8b/10b and 128b/132b DP MST link training > fallbacks work... Yes! This series focuses on the fallback mandates mentioned in DP2.1 spec and doesn't fallback from MST to SST or vicecersa. Hence if it is MST the fallback will be within MST and if its SST the fallback will be within SST. > > > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > > drm_dbg_kms(&i915->drm, > > "Retrying Link training for eDP with max > parameters\n"); @@ > > -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct > intel_dp *intel_dp, > > return 0; > > } > > > > + /* DP fallback values */ > > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & > > +DP_CAP_ANSI_128B132B) { > > ...but this only addresses 128b/132b, and the 8b/10b MST drops to the existing > SST fallback path. Yes! As said above this fallback is based on fallback mandates mentioned in DP2.1 spec in Table 3.31 and Figure 3-52 which focuses on reducing the link rate/lance count and nothing to with MST/SST > > And with the current code, DP_CAP_ANSI_128B132B does not decide whether > we use DP MST or not. So this will also cover 8b/10b fallback for displays that > support 128b/132b but have DP_MSTM_CAP == 0. Yes, the series doent depend on MST and SST and doest fallback from MST to SST or viceversa. > > > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); index++) > { > > + if (link_rate == dp2dot0_fallback[index].link_rate && > > + lane_count == > dp2dot0_fallback[index].lane_count) { > > + for(index += 1; index < > ARRAY_SIZE(dp2dot0_fallback); index++) { > > I honestly do not understand the double looping here, and how index is > managed. The first loop is to find the present link rate and lane count in the fallback table. Once we find this, we will have to traverse from that index below to get the next fallback link rate and lane count. The second loop is now to traverse from this index to see the supported link rate and lane count. For ex: if the link rate is 10Gbps and lane count is 4. First loop is to find this in the fallback table, index would be 3. Then next loop is to traverse from this index 3 to find the fallback values. This would essentially be UHBR13P5 lane count 2. But MTL doesn' support this. Hence will have to move index by 1 to get UHBR10 lane count 2. This second loop will be used for this purpose. > > > + if > (intel_dp_link_rate_supported(intel_dp, > > + > dp2dot0_fallback[index].link_rate)) { > > + > intel_dp_set_link_params(intel_dp, > > + > dp2dot0_fallback[index].link_rate, > > + > dp2dot0_fallback[index].lane_count); > > intel_dp_set_link_params() is supposed to be called in the DP encoder (pre- > )enable paths to set the link rates. If you do it here, the subsequent enable will > just overwrite whatever you did here. This is taken care of so as to not override and retain this fallback value. > > The mechanism in this function should be to to adjust intel_dp->max_link_rate > and intel_dp->max_link_lane_count, and then the caller will send an uevent to > have the userspace do everything again, but with reduced max values. > If falling back within UHBR rate, so with a mode that supports the new fallback link rate then we don't essentially have to send uevent to user and new modeset may not be required. For Ex: the link rate is 20Gbps with mode 6k, Link training fails. So with the new fallback linkrate falling within UHBR need not do a modeset. Only if the fallback link rate falls to HBR rate for which 6k is not supported, only then uevent will be sent to user. > This is all very convoluted. And I admit the existing code is also complex, but > this makes it *much* harder to understand. > Hopefully upon cleaning up some redundant code and re-arranging this implementation with a formal patch traversing the fallback code might become a little simple. Thanks and Regards, Arun R Murthy -------------------- > BR, > Jani. > > > + drm_dbg_kms(&i915->drm, > > + "Retrying Link > training with link rate %d and lane count %d\n", > > + > dp2dot0_fallback[index].link_rate, > > + > dp2dot0_fallback[index].lane_count); > > + return 0; > > + } > > + } > > + } > > + } > > + /* Report failure and fail link training */ > > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > + return -1; > > + } > > + > > index = intel_dp_rate_index(intel_dp->common_rates, > > intel_dp->num_common_rates, > > link_rate); > > @@ -716,7 +789,6 @@ int intel_dp_get_link_train_fallback_values(struct > intel_dp *intel_dp, > > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > return -1; > > } > > - > > return 0; > > } > > -- > Jani Nikula, Intel
On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani <jani.nikula@intel.com> >> Sent: Tuesday, February 13, 2024 11:41 PM >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org >> Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; >> Shankar, Uma <uma.shankar@intel.com>; Murthy, Arun R >> <arun.r.murthy@intel.com> >> Subject: Re: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT >> >> On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: >> > Fallback mandates on DP link training failure. This patch just covers >> > the DP2.0 fallback sequence. >> > >> > TODO: Need to implement the DP1.4 fallback. >> > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp.c | 92 >> > ++++++++++++++++++++++--- >> > 1 file changed, 82 insertions(+), 10 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >> > b/drivers/gpu/drm/i915/display/intel_dp.c >> > index 10ec231acd98..82d354a6b0cd 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15}; >> > */ >> > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; >> > >> > +/* DL Link Rates */ >> > +#define UHBR20 2000000 >> > +#define UHBR13P5 1350000 >> > +#define UHBR10 1000000 >> > +#define HBR3 810000 >> > +#define HBR2 540000 >> > +#define HBR 270000 >> > +#define RBR 162000 >> > + >> > +/* DP Lane Count */ >> > +#define LANE_COUNT_4 4 >> > +#define LANE_COUNT_2 2 >> > +#define LANE_COUNT_1 1 >> > + >> > +/* DP2.0 fallback values */ >> > +struct dp_fallback { >> > + u32 link_rate; >> > + u8 lane_count; >> > +}; >> > + >> > +struct dp_fallback dp2dot0_fallback[] = { >> > + {UHBR20, LANE_COUNT_4}, >> > + {UHBR13P5, LANE_COUNT_4}, >> > + {UHBR20, LANE_COUNT_2}, >> > + {UHBR10, LANE_COUNT_4}, >> > + {UHBR13P5, LANE_COUNT_2}, >> > + {HBR3, LANE_COUNT_4}, >> > + {UHBR20, LANE_COUNT_1}, >> > + {UHBR10, LANE_COUNT_2}, >> > + {HBR2, LANE_COUNT_4}, >> > + {UHBR13P5, LANE_COUNT_1}, >> > + {HBR3, LANE_COUNT_2}, >> > + {UHBR10, LANE_COUNT_1}, >> > + {HBR2, LANE_COUNT_2}, >> > + {HBR, LANE_COUNT_4}, >> > + {HBR3, LANE_COUNT_1}, >> > + {RBR, LANE_COUNT_4}, >> > + {HBR2, LANE_COUNT_1}, >> > + {HBR, LANE_COUNT_2}, >> > + {RBR, LANE_COUNT_2}, >> > + {HBR, LANE_COUNT_1}, >> > + {RBR, LANE_COUNT_1}, >> > +}; >> > + >> > /** >> > * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or >> PCH) >> > * @intel_dp: DP struct >> > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const >> struct intel_dp *intel_dp, >> > intel_dp->num_common_rates, max_rate); >> } >> > >> > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, >> > +u32 link_rate) { >> > + u8 i; >> > + >> > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { >> > + if (intel_dp->common_rates[i] == link_rate) >> > + return true; >> > + else >> > + continue; >> > + } >> > + return false; >> > +} >> > + >> > static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) >> > { >> > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, >> > @@ -671,15 +728,6 @@ int intel_dp_get_link_train_fallback_values(struct >> intel_dp *intel_dp, >> > struct drm_i915_private *i915 = dp_to_i915(intel_dp); >> > int index; >> > >> > - /* >> > - * TODO: Enable fallback on MST links once MST link compute can >> handle >> > - * the fallback params. >> > - */ >> > - if (intel_dp->is_mst) { >> > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); >> > - return -1; >> > - } >> > - >> >> By removing this, the claim is both 8b/10b and 128b/132b DP MST link training >> fallbacks work... > Yes! This series focuses on the fallback mandates mentioned in DP2.1 spec and doesn't fallback from MST to SST or vicecersa. > Hence if it is MST the fallback will be within MST and if its SST the fallback will be within SST. > >> >> > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { >> > drm_dbg_kms(&i915->drm, >> > "Retrying Link training for eDP with max >> parameters\n"); @@ >> > -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct >> intel_dp *intel_dp, >> > return 0; >> > } >> > >> > + /* DP fallback values */ >> > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & >> > +DP_CAP_ANSI_128B132B) { >> >> ...but this only addresses 128b/132b, and the 8b/10b MST drops to the existing >> SST fallback path. > Yes! As said above this fallback is based on fallback mandates mentioned in DP2.1 spec in Table 3.31 and Figure 3-52 which focuses on reducing the link rate/lance count and nothing to with MST/SST > >> >> And with the current code, DP_CAP_ANSI_128B132B does not decide whether >> we use DP MST or not. So this will also cover 8b/10b fallback for displays that >> support 128b/132b but have DP_MSTM_CAP == 0. > > Yes, the series doent depend on MST and SST and doest fallback from MST to SST or viceversa. What I'm saying is, this changes the way 8b/10b link training fallback is handled. First, it starts handling 8b/10b MST link training fallback. Second, it changes the way 8b/10b *and* 128b/132b *and* SST *and* MST link training fallback is handled for all displays that support 128b/132b channel coding. That's *wildly* too much in one patch. It also duplicates the existing code in the same function, with a different mechanism. We don't want to have two different ways to do this, and of all things based on sink's 128b/132b cap. Just one. > >> >> > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); index++) >> { >> > + if (link_rate == dp2dot0_fallback[index].link_rate && >> > + lane_count == >> dp2dot0_fallback[index].lane_count) { >> > + for(index += 1; index < >> ARRAY_SIZE(dp2dot0_fallback); index++) { >> >> I honestly do not understand the double looping here, and how index is >> managed. > The first loop is to find the present link rate and lane count in the fallback table. Once we find this, we will have to traverse from that index below to get the next fallback link rate and lane count. The second loop is now to traverse from this index to see the supported link rate and lane count. > For ex: if the link rate is 10Gbps and lane count is 4. First loop is to find this in the fallback table, index would be 3. Then next loop is to traverse from this index 3 to find the fallback values. This would essentially be UHBR13P5 lane count 2. But MTL doesn' support this. Hence will have to move index by 1 to get UHBR10 lane count 2. This second loop will be used for this purpose. Needs abstractions i.e. more functions instead of trying to make it all happen in one loop. > >> >> > + if >> (intel_dp_link_rate_supported(intel_dp, >> > + >> dp2dot0_fallback[index].link_rate)) { >> > + >> intel_dp_set_link_params(intel_dp, >> > + >> dp2dot0_fallback[index].link_rate, >> > + >> dp2dot0_fallback[index].lane_count); >> >> intel_dp_set_link_params() is supposed to be called in the DP encoder (pre- >> )enable paths to set the link rates. If you do it here, the subsequent enable will >> just overwrite whatever you did here. > This is taken care of so as to not override and retain this fallback value. I don't understand. > >> >> The mechanism in this function should be to to adjust intel_dp->max_link_rate >> and intel_dp->max_link_lane_count, and then the caller will send an uevent to >> have the userspace do everything again, but with reduced max values. >> > If falling back within UHBR rate, so with a mode that supports the new fallback link rate then we don't essentially have to send uevent to user and new modeset may not be required. > For Ex: the link rate is 20Gbps with mode 6k, Link training fails. So with the new fallback linkrate falling within UHBR need not do a modeset. Only if the fallback link rate falls to HBR rate for which 6k is not supported, only then uevent will be sent to user. For SST paths we'll always choose the optimal link parameters, and the mode will not fit if we have to reduce the parameters. And as I just explained, your changes impact SST paths as well. For MST we'll start with max parameters, so yeah there's a possibility we could reduce the link parameters without having to reduce the mode. However, I'm inclined to always go through userspace here, using the same tested paths for link training failures. This will also give userspace some form of transparency into what is going on, and why an additional MST stream might not fit when it should. >> This is all very convoluted. And I admit the existing code is also complex, but >> this makes it *much* harder to understand. >> > Hopefully upon cleaning up some redundant code and re-arranging this implementation with a formal patch traversing the fallback code might become a little simple. If we want to use a list for the parameters, I think the first step should be to modify the existing code to use the list. No additional changes, no functional changes. BR, Jani. > > Thanks and Regards, > Arun R Murthy > -------------------- >> BR, >> Jani. >> >> > + drm_dbg_kms(&i915->drm, >> > + "Retrying Link >> training with link rate %d and lane count %d\n", >> > + >> dp2dot0_fallback[index].link_rate, >> > + >> dp2dot0_fallback[index].lane_count); >> > + return 0; >> > + } >> > + } >> > + } >> > + } >> > + /* Report failure and fail link training */ >> > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); >> > + return -1; >> > + } >> > + >> > index = intel_dp_rate_index(intel_dp->common_rates, >> > intel_dp->num_common_rates, >> > link_rate); >> > @@ -716,7 +789,6 @@ int intel_dp_get_link_train_fallback_values(struct >> intel_dp *intel_dp, >> > drm_err(&i915->drm, "Link Training Unsuccessful\n"); >> > return -1; >> > } >> > - >> > return 0; >> > } >> >> -- >> Jani Nikula, Intel
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Wednesday, February 14, 2024 4:54 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; > Shankar, Uma <uma.shankar@intel.com> > Subject: RE: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT > > On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: > >> -----Original Message----- > >> From: Nikula, Jani <jani.nikula@intel.com> > >> Sent: Tuesday, February 13, 2024 11:41 PM > >> To: Murthy, Arun R <arun.r.murthy@intel.com>; > >> intel-gfx@lists.freedesktop.org > >> Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville > >> <ville.syrjala@intel.com>; Shankar, Uma <uma.shankar@intel.com>; > >> Murthy, Arun R <arun.r.murthy@intel.com> > >> Subject: Re: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT > >> > >> On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > >> > Fallback mandates on DP link training failure. This patch just > >> > covers the DP2.0 fallback sequence. > >> > > >> > TODO: Need to implement the DP1.4 fallback. > >> > > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 92 > >> > ++++++++++++++++++++++--- > >> > 1 file changed, 82 insertions(+), 10 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> > b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index 10ec231acd98..82d354a6b0cd 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, > 15}; > >> > */ > >> > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > >> > > >> > +/* DL Link Rates */ > >> > +#define UHBR20 2000000 > >> > +#define UHBR13P5 1350000 > >> > +#define UHBR10 1000000 > >> > +#define HBR3 810000 > >> > +#define HBR2 540000 > >> > +#define HBR 270000 > >> > +#define RBR 162000 > >> > + > >> > +/* DP Lane Count */ > >> > +#define LANE_COUNT_4 4 > >> > +#define LANE_COUNT_2 2 > >> > +#define LANE_COUNT_1 1 > >> > + > >> > +/* DP2.0 fallback values */ > >> > +struct dp_fallback { > >> > + u32 link_rate; > >> > + u8 lane_count; > >> > +}; > >> > + > >> > +struct dp_fallback dp2dot0_fallback[] = { > >> > + {UHBR20, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_4}, > >> > + {UHBR20, LANE_COUNT_2}, > >> > + {UHBR10, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_2}, > >> > + {HBR3, LANE_COUNT_4}, > >> > + {UHBR20, LANE_COUNT_1}, > >> > + {UHBR10, LANE_COUNT_2}, > >> > + {HBR2, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_1}, > >> > + {HBR3, LANE_COUNT_2}, > >> > + {UHBR10, LANE_COUNT_1}, > >> > + {HBR2, LANE_COUNT_2}, > >> > + {HBR, LANE_COUNT_4}, > >> > + {HBR3, LANE_COUNT_1}, > >> > + {RBR, LANE_COUNT_4}, > >> > + {HBR2, LANE_COUNT_1}, > >> > + {HBR, LANE_COUNT_2}, > >> > + {RBR, LANE_COUNT_2}, > >> > + {HBR, LANE_COUNT_1}, > >> > + {RBR, LANE_COUNT_1}, > >> > +}; > >> > + > >> > /** > >> > * intel_dp_is_edp - is the given port attached to an eDP panel > >> > (either CPU or > >> PCH) > >> > * @intel_dp: DP struct > >> > @@ -299,6 +343,19 @@ static int > >> > intel_dp_common_len_rate_limit(const > >> struct intel_dp *intel_dp, > >> > intel_dp->num_common_rates, > >> > max_rate); > >> } > >> > > >> > +static bool intel_dp_link_rate_supported(struct intel_dp > >> > +*intel_dp, > >> > +u32 link_rate) { > >> > + u8 i; > >> > + > >> > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > >> > + if (intel_dp->common_rates[i] == link_rate) > >> > + return true; > >> > + else > >> > + continue; > >> > + } > >> > + return false; > >> > +} > >> > + > >> > static int intel_dp_common_rate(struct intel_dp *intel_dp, int > >> > index) { > >> > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > >> > @@ -671,15 +728,6 @@ int > >> > intel_dp_get_link_train_fallback_values(struct > >> intel_dp *intel_dp, > >> > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > >> > int index; > >> > > >> > - /* > >> > - * TODO: Enable fallback on MST links once MST link compute can > >> handle > >> > - * the fallback params. > >> > - */ > >> > - if (intel_dp->is_mst) { > >> > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > >> > - return -1; > >> > - } > >> > - > >> > >> By removing this, the claim is both 8b/10b and 128b/132b DP MST link > >> training fallbacks work... > > Yes! This series focuses on the fallback mandates mentioned in DP2.1 spec and > doesn't fallback from MST to SST or vicecersa. > > Hence if it is MST the fallback will be within MST and if its SST the fallback > will be within SST. > > > >> > >> > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > >> > drm_dbg_kms(&i915->drm, > >> > "Retrying Link training for eDP with max > >> parameters\n"); @@ > >> > -687,6 +735,31 @@ int > >> > intel_dp_get_link_train_fallback_values(struct > >> intel_dp *intel_dp, > >> > return 0; > >> > } > >> > > >> > + /* DP fallback values */ > >> > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & > >> > +DP_CAP_ANSI_128B132B) { > >> > >> ...but this only addresses 128b/132b, and the 8b/10b MST drops to the > >> existing SST fallback path. > > Yes! As said above this fallback is based on fallback mandates > > mentioned in DP2.1 spec in Table 3.31 and Figure 3-52 which focuses on > > reducing the link rate/lance count and nothing to with MST/SST > > > >> > >> And with the current code, DP_CAP_ANSI_128B132B does not decide > >> whether we use DP MST or not. So this will also cover 8b/10b fallback > >> for displays that support 128b/132b but have DP_MSTM_CAP == 0. > > > > Yes, the series doent depend on MST and SST and doest fallback from MST to > SST or viceversa. > > What I'm saying is, this changes the way 8b/10b link training fallback is > handled. > The first loop has a if condition for 128/132b and is executed only if its 128/132b and if not falls to the existing code. i.e 8/10b link training fallback sequence. > First, it starts handling 8b/10b MST link training fallback. > As far as I see, at the entry of this function 128/132b is checked and link training fallback values for this obtained and if not link training fallback values for 8/10b is obtained. Have taken care as to not modify the existing 8/10b fallback. > Second, it changes the way 8b/10b *and* 128b/132b *and* SST *and* MST link > training fallback is handled for all displays that support 128b/132b channel > coding. > MST/SST configuration and then the link training happens. This link training by writing to dpcd registers is done over here by sending certain patterns. The fallback in this RFC is done only in this small link training sequence. On failure the handler doesn't return back instead retry from starting of link training is done. MST/SST configuration is not touched upon, if any required for this as part of fallback can be taken up in the next step. This RFC is aiming to achieve fallback for the link training sequence only. > That's *wildly* too much in one patch. > Will surely break this into multiple patches based on the functionality. > It also duplicates the existing code in the same function, with a different > mechanism. We don't want to have two different ways to do this, and of all > things based on sink's 128b/132b cap. Just one. > The way for obtaining link training fallback values for 128/132b is done and the same code will be utilized for 8/10b as well but with a different table. If the RFC is approved then will work on getting this done in a cleaner and optimized way. > > > >> > >> > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); > >> > + index++) > >> { > >> > + if (link_rate == dp2dot0_fallback[index].link_rate && > >> > + lane_count == > >> dp2dot0_fallback[index].lane_count) { > >> > + for(index += 1; index < > >> ARRAY_SIZE(dp2dot0_fallback); index++) { > >> > >> I honestly do not understand the double looping here, and how index > >> is managed. > > The first loop is to find the present link rate and lane count in the fallback > table. Once we find this, we will have to traverse from that index below to get > the next fallback link rate and lane count. The second loop is now to traverse > from this index to see the supported link rate and lane count. > > For ex: if the link rate is 10Gbps and lane count is 4. First loop is to find this in > the fallback table, index would be 3. Then next loop is to traverse from this > index 3 to find the fallback values. This would essentially be UHBR13P5 lane > count 2. But MTL doesn' support this. Hence will have to move index by 1 to get > UHBR10 lane count 2. This second loop will be used for this purpose. > > Needs abstractions i.e. more functions instead of trying to make it all happen in > one loop. Sure will work on this and will float the patch. > > > > >> > >> > + if > >> (intel_dp_link_rate_supported(intel_dp, > >> > + > >> dp2dot0_fallback[index].link_rate)) { > >> > + > >> intel_dp_set_link_params(intel_dp, > >> > + > >> dp2dot0_fallback[index].link_rate, > >> > + > >> dp2dot0_fallback[index].lane_count); > >> > >> intel_dp_set_link_params() is supposed to be called in the DP encoder > >> (pre- )enable paths to set the link rates. If you do it here, the > >> subsequent enable will just overwrite whatever you did here. > > This is taken care of so as to not override and retain this fallback value. > > I don't understand. > With the existing code the driver sends uevent and a new modeset along with dp_init is done and the values will be overwritten. In this RFC we don't send uevent for all the fallback values instead re-iterate only the link training part without touching the dp enable sequence. > > > >> > >> The mechanism in this function should be to to adjust > >> intel_dp->max_link_rate and intel_dp->max_link_lane_count, and then > >> the caller will send an uevent to have the userspace do everything again, but > with reduced max values. > >> > > If falling back within UHBR rate, so with a mode that supports the new > fallback link rate then we don't essentially have to send uevent to user and new > modeset may not be required. > > For Ex: the link rate is 20Gbps with mode 6k, Link training fails. So with the > new fallback linkrate falling within UHBR need not do a modeset. Only if the > fallback link rate falls to HBR rate for which 6k is not supported, only then > uevent will be sent to user. > > For SST paths we'll always choose the optimal link parameters, and the mode > will not fit if we have to reduce the parameters. And as I just explained, your > changes impact SST paths as well. > > For MST we'll start with max parameters, so yeah there's a possibility we could > reduce the link parameters without having to reduce the mode. However, I'm > inclined to always go through userspace here, using the same tested paths for > link training failures. This will also give userspace some form of transparency > into what is going on, and why an additional MST stream might not fit when it > should. > > >> This is all very convoluted. And I admit the existing code is also > >> complex, but this makes it *much* harder to understand. > >> > > Hopefully upon cleaning up some redundant code and re-arranging this > implementation with a formal patch traversing the fallback code might become > a little simple. > > If we want to use a list for the parameters, I think the first step should be to > modify the existing code to use the list. No additional changes, no functional > changes. > Sure will ensure that would be the first patch in this series before touching upon anything on the 128/132b fallback. Thanks and Regards, Arun R Murthy ------------------- > BR, > Jani. > > > > > Thanks and Regards, > > Arun R Murthy > > -------------------- > >> BR, > >> Jani. > >> > >> > + drm_dbg_kms(&i915->drm, > >> > + "Retrying > >> > + Link > >> training with link rate %d and lane count %d\n", > >> > + > >> dp2dot0_fallback[index].link_rate, > >> > + > >> dp2dot0_fallback[index].lane_count); > >> > + return 0; > >> > + } > >> > + } > >> > + } > >> > + } > >> > + /* Report failure and fail link training */ > >> > + drm_err(&i915->drm, "Link Training Unsuccessful\n"); > >> > + return -1; > >> > + } > >> > + > >> > index = intel_dp_rate_index(intel_dp->common_rates, > >> > intel_dp->num_common_rates, > >> > link_rate); @@ -716,7 +789,6 @@ int > >> > intel_dp_get_link_train_fallback_values(struct > >> intel_dp *intel_dp, > >> > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > >> > return -1; > >> > } > >> > - > >> > return 0; > >> > } > >> > >> -- > >> Jani Nikula, Intel > > -- > Jani Nikula, Intel
On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> >> And with the current code, DP_CAP_ANSI_128B132B does not decide >> >> whether we use DP MST or not. So this will also cover 8b/10b fallback >> >> for displays that support 128b/132b but have DP_MSTM_CAP == 0. >> > >> > Yes, the series doent depend on MST and SST and doest fallback from MST to >> SST or viceversa. >> >> What I'm saying is, this changes the way 8b/10b link training fallback is >> handled. >> > The first loop has a if condition for 128/132b and is executed only if its 128/132b and if not falls to the existing code. i.e 8/10b link training fallback sequence. You check for sink 128b/132b capability. Please take the time to consider what this means. I've tried to explain this a few times now. >> First, it starts handling 8b/10b MST link training fallback. >> > As far as I see, at the entry of this function 128/132b is checked and link training fallback values for this obtained and if not link training fallback values for 8/10b is obtained. Have taken care as to not modify the existing 8/10b fallback. Same as above. >> Second, it changes the way 8b/10b *and* 128b/132b *and* SST *and* MST link >> training fallback is handled for all displays that support 128b/132b channel >> coding. >> > MST/SST configuration and then the link training happens. This link training by writing to dpcd registers is done over here by sending certain patterns. The fallback in this RFC is done only in this small link training sequence. On failure the handler doesn't return back instead retry from starting of link training is done. MST/SST configuration is not touched upon, if any required for this as part of fallback can be taken up in the next step. > This RFC is aiming to achieve fallback for the link training sequence only. To be clear, I don't want to change the way link training failure fallback is handled in general. It should go via userspace. Please let's just not go there, at all. Changing it does not help us right now, it just adds another complication, and another code path to test. But regardless of that, I don't think you rightly appreciate what implications your changes actually have. The code does not do what you claim it does. I don't know what else to say. >> >> intel_dp_set_link_params() is supposed to be called in the DP encoder >> >> (pre- )enable paths to set the link rates. If you do it here, the >> >> subsequent enable will just overwrite whatever you did here. >> > This is taken care of so as to not override and retain this fallback value. >> >> I don't understand. >> > With the existing code the driver sends uevent and a new modeset along with dp_init is done and the values will be overwritten. In this RFC we don't send uevent for all the fallback values instead re-iterate only the link training part without touching the dp enable sequence. I any patch series, no matter what you're working on, each patch in the series must stand on its own merits. Patches can't depend on something that may or may not be done later in subsequent patches. BR, Jani.
On Wed, Feb 14, 2024 at 01:23:42PM +0200, Jani Nikula wrote: > On Wed, 14 Feb 2024, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: > >> -----Original Message----- > >> From: Nikula, Jani <jani.nikula@intel.com> > >> Sent: Tuesday, February 13, 2024 11:41 PM > >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > >> Cc: Deak, Imre <imre.deak@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; > >> Shankar, Uma <uma.shankar@intel.com>; Murthy, Arun R > >> <arun.r.murthy@intel.com> > >> Subject: Re: [RFC 1/4] drm/i915/display/dp: Add DP fallback on LT > >> > >> On Tue, 06 Feb 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > >> > Fallback mandates on DP link training failure. This patch just covers > >> > the DP2.0 fallback sequence. > >> > > >> > TODO: Need to implement the DP1.4 fallback. > >> > > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 92 > >> > ++++++++++++++++++++++--- > >> > 1 file changed, 82 insertions(+), 10 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> > b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index 10ec231acd98..82d354a6b0cd 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15}; > >> > */ > >> > static const u8 valid_dsc_slicecount[] = {1, 2, 4}; > >> > > >> > +/* DL Link Rates */ > >> > +#define UHBR20 2000000 > >> > +#define UHBR13P5 1350000 > >> > +#define UHBR10 1000000 > >> > +#define HBR3 810000 > >> > +#define HBR2 540000 > >> > +#define HBR 270000 > >> > +#define RBR 162000 > >> > + > >> > +/* DP Lane Count */ > >> > +#define LANE_COUNT_4 4 > >> > +#define LANE_COUNT_2 2 > >> > +#define LANE_COUNT_1 1 > >> > + > >> > +/* DP2.0 fallback values */ > >> > +struct dp_fallback { > >> > + u32 link_rate; > >> > + u8 lane_count; > >> > +}; > >> > + > >> > +struct dp_fallback dp2dot0_fallback[] = { > >> > + {UHBR20, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_4}, > >> > + {UHBR20, LANE_COUNT_2}, > >> > + {UHBR10, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_2}, > >> > + {HBR3, LANE_COUNT_4}, > >> > + {UHBR20, LANE_COUNT_1}, > >> > + {UHBR10, LANE_COUNT_2}, > >> > + {HBR2, LANE_COUNT_4}, > >> > + {UHBR13P5, LANE_COUNT_1}, > >> > + {HBR3, LANE_COUNT_2}, > >> > + {UHBR10, LANE_COUNT_1}, > >> > + {HBR2, LANE_COUNT_2}, > >> > + {HBR, LANE_COUNT_4}, > >> > + {HBR3, LANE_COUNT_1}, > >> > + {RBR, LANE_COUNT_4}, > >> > + {HBR2, LANE_COUNT_1}, > >> > + {HBR, LANE_COUNT_2}, > >> > + {RBR, LANE_COUNT_2}, > >> > + {HBR, LANE_COUNT_1}, > >> > + {RBR, LANE_COUNT_1}, > >> > +}; > >> > + > >> > /** > >> > * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or > >> PCH) > >> > * @intel_dp: DP struct > >> > @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const > >> struct intel_dp *intel_dp, > >> > intel_dp->num_common_rates, max_rate); > >> } > >> > > >> > +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, > >> > +u32 link_rate) { > >> > + u8 i; > >> > + > >> > + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { > >> > + if (intel_dp->common_rates[i] == link_rate) > >> > + return true; > >> > + else > >> > + continue; > >> > + } > >> > + return false; > >> > +} > >> > + > >> > static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) > >> > { > >> > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > >> > @@ -671,15 +728,6 @@ int intel_dp_get_link_train_fallback_values(struct > >> intel_dp *intel_dp, > >> > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > >> > int index; > >> > > >> > - /* > >> > - * TODO: Enable fallback on MST links once MST link compute can > >> handle > >> > - * the fallback params. > >> > - */ > >> > - if (intel_dp->is_mst) { > >> > - drm_err(&i915->drm, "Link Training Unsuccessful\n"); > >> > - return -1; > >> > - } > >> > - > >> > >> By removing this, the claim is both 8b/10b and 128b/132b DP MST link training > >> fallbacks work... > > Yes! This series focuses on the fallback mandates mentioned in DP2.1 spec and doesn't fallback from MST to SST or vicecersa. > > Hence if it is MST the fallback will be within MST and if its SST the fallback will be within SST. > > > >> > >> > if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { > >> > drm_dbg_kms(&i915->drm, > >> > "Retrying Link training for eDP with max > >> parameters\n"); @@ > >> > -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct > >> intel_dp *intel_dp, > >> > return 0; > >> > } > >> > > >> > + /* DP fallback values */ > >> > + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & > >> > +DP_CAP_ANSI_128B132B) { > >> > >> ...but this only addresses 128b/132b, and the 8b/10b MST drops to the existing > >> SST fallback path. > > Yes! As said above this fallback is based on fallback mandates mentioned in DP2.1 spec in Table 3.31 and Figure 3-52 which focuses on reducing the link rate/lance count and nothing to with MST/SST > > > >> > >> And with the current code, DP_CAP_ANSI_128B132B does not decide whether > >> we use DP MST or not. So this will also cover 8b/10b fallback for displays that > >> support 128b/132b but have DP_MSTM_CAP == 0. > > > > Yes, the series doent depend on MST and SST and doest fallback from MST to SST or viceversa. > > What I'm saying is, this changes the way 8b/10b link training fallback > is handled. > > First, it starts handling 8b/10b MST link training fallback. > > Second, it changes the way 8b/10b *and* 128b/132b *and* SST *and* MST > link training fallback is handled for all displays that support > 128b/132b channel coding. > > That's *wildly* too much in one patch. > > It also duplicates the existing code in the same function, with a > different mechanism. We don't want to have two different ways to do > this, and of all things based on sink's 128b/132b cap. Just one. > > > > >> > >> > + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); index++) > >> { > >> > + if (link_rate == dp2dot0_fallback[index].link_rate && > >> > + lane_count == > >> dp2dot0_fallback[index].lane_count) { > >> > + for(index += 1; index < > >> ARRAY_SIZE(dp2dot0_fallback); index++) { > >> > >> I honestly do not understand the double looping here, and how index is > >> managed. > > The first loop is to find the present link rate and lane count in the fallback table. Once we find this, we will have to traverse from that index below to get the next fallback link rate and lane count. The second loop is now to traverse from this index to see the supported link rate and lane count. > > For ex: if the link rate is 10Gbps and lane count is 4. First loop is to find this in the fallback table, index would be 3. Then next loop is to traverse from this index 3 to find the fallback values. This would essentially be UHBR13P5 lane count 2. But MTL doesn' support this. Hence will have to move index by 1 to get UHBR10 lane count 2. This second loop will be used for this purpose. > > Needs abstractions i.e. more functions instead of trying to make it all > happen in one loop. > > > > >> > >> > + if > >> (intel_dp_link_rate_supported(intel_dp, > >> > + > >> dp2dot0_fallback[index].link_rate)) { > >> > + > >> intel_dp_set_link_params(intel_dp, > >> > + > >> dp2dot0_fallback[index].link_rate, > >> > + > >> dp2dot0_fallback[index].lane_count); > >> > >> intel_dp_set_link_params() is supposed to be called in the DP encoder (pre- > >> )enable paths to set the link rates. If you do it here, the subsequent enable will > >> just overwrite whatever you did here. > > This is taken care of so as to not override and retain this fallback value. > > I don't understand. > > > > >> > >> The mechanism in this function should be to to adjust intel_dp->max_link_rate > >> and intel_dp->max_link_lane_count, and then the caller will send an uevent to > >> have the userspace do everything again, but with reduced max values. > >> > > If falling back within UHBR rate, so with a mode that supports the new fallback link rate then we don't essentially have to send uevent to user and new modeset may not be required. > > For Ex: the link rate is 20Gbps with mode 6k, Link training fails. So with the new fallback linkrate falling within UHBR need not do a modeset. Only if the fallback link rate falls to HBR rate for which 6k is not supported, only then uevent will be sent to user. > > For SST paths we'll always choose the optimal link parameters, and the > mode will not fit if we have to reduce the parameters. And as I just > explained, your changes impact SST paths as well. > > For MST we'll start with max parameters, so yeah there's a possibility > we could reduce the link parameters without having to reduce the > mode. However, I'm inclined to always go through userspace here, using > the same tested paths for link training failures. This will also give > userspace some form of transparency into what is going on, and why an > additional MST stream might not fit when it should. Sudden loss of link -> try a blind retrain: This is rather sketchy as we don't go through the full modeset sequence. Probably should replace this by either just always punting to userspace, or by just doing a proper atomic commit with the current parameters from a work. I'm not sure all userspace/fb-helper handle everything correctly so might still want to keep this in kernel... If link training fails then reduce link parms and punt to userspace: This could in theory also try a blind modeset in kernel first and if that fails then punt to userspace. Again the concern might be that not all userspace is perhaps very good, but I might be wrong about that. Anyways all of that is kinda orthogonal stuff to just getting MST to reduce its link rate. For that I think we should just need: a) remove the MST check from the fallback stuff b) handle all the MST connectors on the same link in the retry work
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 10ec231acd98..82d354a6b0cd 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -104,6 +104,50 @@ static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15}; */ static const u8 valid_dsc_slicecount[] = {1, 2, 4}; +/* DL Link Rates */ +#define UHBR20 2000000 +#define UHBR13P5 1350000 +#define UHBR10 1000000 +#define HBR3 810000 +#define HBR2 540000 +#define HBR 270000 +#define RBR 162000 + +/* DP Lane Count */ +#define LANE_COUNT_4 4 +#define LANE_COUNT_2 2 +#define LANE_COUNT_1 1 + +/* DP2.0 fallback values */ +struct dp_fallback { + u32 link_rate; + u8 lane_count; +}; + +struct dp_fallback dp2dot0_fallback[] = { + {UHBR20, LANE_COUNT_4}, + {UHBR13P5, LANE_COUNT_4}, + {UHBR20, LANE_COUNT_2}, + {UHBR10, LANE_COUNT_4}, + {UHBR13P5, LANE_COUNT_2}, + {HBR3, LANE_COUNT_4}, + {UHBR20, LANE_COUNT_1}, + {UHBR10, LANE_COUNT_2}, + {HBR2, LANE_COUNT_4}, + {UHBR13P5, LANE_COUNT_1}, + {HBR3, LANE_COUNT_2}, + {UHBR10, LANE_COUNT_1}, + {HBR2, LANE_COUNT_2}, + {HBR, LANE_COUNT_4}, + {HBR3, LANE_COUNT_1}, + {RBR, LANE_COUNT_4}, + {HBR2, LANE_COUNT_1}, + {HBR, LANE_COUNT_2}, + {RBR, LANE_COUNT_2}, + {HBR, LANE_COUNT_1}, + {RBR, LANE_COUNT_1}, +}; + /** * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -299,6 +343,19 @@ static int intel_dp_common_len_rate_limit(const struct intel_dp *intel_dp, intel_dp->num_common_rates, max_rate); } +static bool intel_dp_link_rate_supported(struct intel_dp *intel_dp, u32 link_rate) +{ + u8 i; + + for (i = 0; i < ARRAY_SIZE(intel_dp->common_rates); i++) { + if (intel_dp->common_rates[i] == link_rate) + return true; + else + continue; + } + return false; +} + static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) { if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, @@ -671,15 +728,6 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, struct drm_i915_private *i915 = dp_to_i915(intel_dp); int index; - /* - * TODO: Enable fallback on MST links once MST link compute can handle - * the fallback params. - */ - if (intel_dp->is_mst) { - drm_err(&i915->drm, "Link Training Unsuccessful\n"); - return -1; - } - if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) { drm_dbg_kms(&i915->drm, "Retrying Link training for eDP with max parameters\n"); @@ -687,6 +735,31 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, return 0; } + /* DP fallback values */ + if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) { + for(index = 0; index < ARRAY_SIZE(dp2dot0_fallback); index++) { + if (link_rate == dp2dot0_fallback[index].link_rate && + lane_count == dp2dot0_fallback[index].lane_count) { + for(index += 1; index < ARRAY_SIZE(dp2dot0_fallback); index++) { + if (intel_dp_link_rate_supported(intel_dp, + dp2dot0_fallback[index].link_rate)) { + intel_dp_set_link_params(intel_dp, + dp2dot0_fallback[index].link_rate, + dp2dot0_fallback[index].lane_count); + drm_dbg_kms(&i915->drm, + "Retrying Link training with link rate %d and lane count %d\n", + dp2dot0_fallback[index].link_rate, + dp2dot0_fallback[index].lane_count); + return 0; + } + } + } + } + /* Report failure and fail link training */ + drm_err(&i915->drm, "Link Training Unsuccessful\n"); + return -1; + } + index = intel_dp_rate_index(intel_dp->common_rates, intel_dp->num_common_rates, link_rate); @@ -716,7 +789,6 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, drm_err(&i915->drm, "Link Training Unsuccessful\n"); return -1; } - return 0; }
Fallback mandates on DP link training failure. This patch just covers the DP2.0 fallback sequence. TODO: Need to implement the DP1.4 fallback. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 92 ++++++++++++++++++++++--- 1 file changed, 82 insertions(+), 10 deletions(-)