On Fri, Sep 1, 2023 at 2:57 PM Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > > On Thu, Aug 24, 2023 at 04:50:16PM -0400, Gil Dekel wrote: > > Instead of silently giving up when all link-training fallback values are > > exhausted, this patch modifies the fallback's failure branch to reduces > > both max_link_lane_count and max_link_rate to zero (0) and continues to > > emit uevents until userspace stops attempting to modeset. > > > > By doing so, we ensure the failing connector, which is in > > link-status=Bad, has all its modes pruned (due to effectively having a > > bandwidth of 0Gbps). > > > > It is then the userspace's responsibility to ignore connectors with no > > modes, even if they are marked as connected. > > > > Signed-off-by: Gil Dekel <gildekel@xxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 7067ee3a4bd3..2152ddbab557 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -276,8 +276,12 @@ static int intel_dp_common_len_rate_limit(const struct intel_dp *intel_dp, > > > > static int intel_dp_common_rate(struct intel_dp *intel_dp, int index) > > { > > + /* This occurs when max link rate drops to 0 via link training fallback*/ > > + if (index < 0) > > + return 0; > > I'm not comfortable with handling negative input as normal here > and no warning or msg. > Maybe I'm missing the cases in which this will get to negative and > it would be acceptable? In this case probably better to improve the > commit message. > If we set the max_link_rate to 0, as I am suggesting in this approach, it will eventually reach: int intel_dp_rate_limit_len(intel_dp_rate_limit_len(const int *rates, int len, int max_rate) and since max_rate is == 0, the returned value will be 0. The common use case of int intel_dp_common_rate(struct intel_dp *intel_dp, int index) is: intel_dp_common_rate(intel_dp, len - 1); where len is the position of the last link rate value that was attempted in the connector's array of bit rates. When len == 0, you hit the case where the index becomes -1, which indicates we ran out of possible bitrates in: intel_dp->num_common_rates so we return the bit rate 0Gbps for all invalid cases (index < 0). If this is acceptable, I'll gladly add some details to the commit message. An alternative approach is to introduce 0 link rate at the front of: intel_dp->num_common_rates, which will ensure all connectors have 0 as a last option, and then we can use the normal fallback code path with no special cases here. I hope this makes sense... > > + > > if (drm_WARN_ON(&dp_to_i915(intel_dp)->drm, > > - index < 0 || index >= intel_dp->num_common_rates)) > > + index >= intel_dp->num_common_rates)) > > return 162000; > > > > return intel_dp->common_rates[index]; > > @@ -318,6 +322,9 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp) > > int intel_dp_max_lane_count(struct intel_dp *intel_dp) > > { > > switch (intel_dp->max_link_lane_count) { > > + /* This occurs when max link lane count drops to 0 via link training fallback*/ > > + case 0: > > + return 0; > > case 1: > > case 2: > > case 4: > > @@ -672,7 +679,14 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp, > > intel_dp->max_link_lane_count = lane_count >> 1; > > } else { > > drm_err(&i915->drm, "Link Training Unsuccessful\n"); > > - return -1; > > + /* > > + * Ensure all of the connector's modes are pruned in the next > > + * probe by effectively reducing its bandwidth to 0 so userspace > > + * can ignore it within the next modeset attempt. > > + */ > > + intel_dp->max_link_rate = 0; > > + intel_dp->max_link_lane_count = 0; > > + return 0; > > } > > > > return 0; > > -- > > Gil Dekel, Software Engineer, Google / ChromeOS Display and Graphics Thanks for your reviews and comments! -- Best, Gil Dekel, Software Engineer, Google / ChromeOS Display and Graphics