Re: [Intel-gfx] [PATCH v2 08/12] drm/i915/dp: Convert link training error to debug message on disconnected sink

[Imre Deak <imre.deak@intel.com>]

On Fri, May 05, 2023 at 11:22:36AM +0300, Govindapillai, Vinod wrote:
> On Thu, 2023-05-04 at 02:10 +0300, Imre Deak wrote:
> > If a sink is disconnected it's expected that link training actions will
> > fail on it, so downgrade the error messages about such actions to be a
> > debug message. Such - expected - link training failures are more
> > frequent after a follow up patch, after which an active TypeC link is
> > reset after the sink is disconnected which also involves a link
> > training.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c               |  2 +-
> >  drivers/gpu/drm/i915/display/intel_dp.h               |  1 +
> >  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 11 +++++++++--
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 4361c1ac65c3a..1942a05719776 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4179,7 +4179,7 @@ static int intel_dp_prep_link_retrain(struct intel_dp *intel_dp,
> >         return ret;
> >  }
> >
> > -static bool intel_dp_is_connected(struct intel_dp *intel_dp)
> > +bool intel_dp_is_connected(struct intel_dp *intel_dp)
> >  {
> >         struct intel_connector *connector = intel_dp->attached_connector;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > index ef39e4f7a329e..488da392fafe5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -42,6 +42,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >                               int link_rate, int lane_count);
> >  int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >                                             int link_rate, u8 lane_count);
> > +bool intel_dp_is_connected(struct intel_dp *intel_dp);
> >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> >                           struct drm_modeset_acquire_ctx *ctx);
> >  void intel_dp_set_power(struct intel_dp *intel_dp, u8 mode);
> > 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 f83944eaac128..2fe076e2d64b3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -33,6 +33,7 @@ static void lt_msg(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy,
> >         struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >         struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >         struct intel_connector *connector = intel_dp->attached_connector;
> > +       const char *discon_str = "";
> >         struct va_format vaf;
> >         va_list args;
> >
> > @@ -43,6 +44,11 @@ static void lt_msg(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy,
> >         vaf.fmt = format;
> >         vaf.va = &args;
> >
> > +       if (is_error && !intel_dp_is_connected(intel_dp)) {
> > +               discon_str = " (sink disconnected)";
> > +               is_error = false;
> > +       }
> > +
> Hi imre,
> 
> If the ignore long HPD env variable is set, in case of spurious HPDs
> we will ignore the long HPDs and the connector status remains
> unchanged. So don't we need to downgrade the error messages to debug
> messages if dev_priv->display.hotplug.ignore_long_hpd is set?

Yes, if the connector state is forced in general, we should check here
the actual HPD state instead (as reported by intel_digital_port_connected()).

> 
> BR
> vinod
> 
> >         if (is_error)
> >                 drm_err(&i915->drm, "[CONNECTOR:%d:%s][ENCODER:%d:%s][%s] %pV\n",
> >                         connector->base.base.id, connector->base.name,
> > @@ -50,11 +56,12 @@ static void lt_msg(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy,
> >                         drm_dp_phy_name(dp_phy),
> >                         &vaf);
> >         else
> > -               drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s][ENCODER:%d:%s][%s] %pV\n",
> > +               drm_dbg_kms(&i915->drm, "[CONNECTOR:%d:%s][ENCODER:%d:%s][%s] %pV%s\n",
> >                             connector->base.base.id, connector->base.name,
> >                             encoder->base.base.id, encoder->base.name,
> >                             drm_dp_phy_name(dp_phy),
> > -                           &vaf);
> > +                           &vaf,
> > +                           discon_str);
> >  }
> >
> >  #define lt_err(intel_dp, dp_phy, format, ...) \
>