Re: [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Jul 01, 2016 at 05:35:25PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 1, 2016 at 3:47 PM, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > Intel_dp_check_link_status() function reads the Link status registers
> > and returns a boolean to indicate link is good or bad.
> > If the link is bad, it is retrained outside the function based
> > on the return value.
> >
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++-----------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6d586b7..c795c9e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3863,7 +3863,7 @@ go_again:
> >         return -EINVAL;
> >  }
> >
> > -static void
> > +static bool
> >  intel_dp_check_link_status(struct intel_dp *intel_dp)
> >  {
> >         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > @@ -3873,26 +3873,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >         WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >
> >         if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > -               DRM_ERROR("Failed to get link status\n");
> > -               return;
> > +               DRM_DEBUG_KMS("Failed to get link status\n");
> > +               return false;
> >         }
> >
> > -       if (!intel_encoder->base.crtc)
> > -               return;
> 
> where did this check go? why don't we need this anymore?
>

This function is being refactored to handle upront link training independent 
of modeset and according to the spec, this function should only read the DPCD registers
to check if the link is valid. In case of upfront link training, we do not care about
the crtc or if the crtc is active hence this and the next check for crtc acrive are
not needed.

 
> > +       /* Check if the link is valid by reading the bits of Link status
> > +        * registers
> > +        */
> > +       if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count) ||
> > +           !drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > +               DRM_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
> > +               return false;
> > +       }
> >
> > -       if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > -               return;
> 
> what happens if it is not active now? are we going to attemtp the
> retraing in this case?
> in other words: we really don't need this check anymore as the one above?
> 
> Thanks,
> Rodrigo.
> 
> > +       DRM_DEBUG_KMS("Link is good, no need to retrain\n");
> > +       return true;
> >
> > -       /* if link training is requested we should perform it always */
> > -       if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> > -           (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> > -               DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> > -                             intel_encoder->base.name);
> > -               intel_dp_start_link_train(intel_dp);
> > -               intel_dp_stop_link_train(intel_dp);
> > -       }
> >  }
> >
> > +
> >  /*
> >   * According to DP spec
> >   * 5.1.2:
> > @@ -3950,7 +3949,11 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> >         }
> >
> >         drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -       intel_dp_check_link_status(intel_dp);
> > +       if (!intel_dp_check_link_status(intel_dp) ||
> > +           intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) {
> > +               intel_dp_start_link_train(intel_dp);
> > +               intel_dp_stop_link_train(intel_dp);
> > +       }
> >         drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >
> >         return true;
> > @@ -4271,7 +4274,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >                  * link loss triggerring long pulse!!!!
> >                  */
> >                 drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -               intel_dp_check_link_status(intel_dp);
> > +               if (!intel_dp_check_link_status(intel_dp) ||
> > +                   intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) {
> > +                       intel_dp_start_link_train(intel_dp);
> > +                       intel_dp_stop_link_train(intel_dp);
> > +               }
> >                 drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >                 goto out;
> >         }
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx