[Intel-gfx] [PATCH v4 1/6] drm/i915/dp_link_training: Add a final failing state to link training fallback

[Gil Dekel]

On Fri, Sep 1, 2023 at 2:57 PM Rodrigo Vivi <rodrigo.vivi at intel.com> 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 at chromium.org>
> > ---
> >  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