Re: [PATCH 1/3] drm/i915/dp: Fix link retraining comment in intel_dp_long_pulse()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 06 Sep 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> Comment claims link needs to be retrained because the connected sink raised
> a long pulse to indicate link loss. If the sink did so,
> intel_dp_hotplug() would have handled link retraining. Looking at the
> logs in Bugzilla referenced in commit '3cf71bc9904d ("drm/i915: Re-apply
> Perform link quality check, unconditionally during long pulse"")', the
> issue is that the sink does not trigger an interrupt. We want the
> ->detect() from user space to check link status and retrain. Ville's
> review for the original patch also indicates the same root cause.
>
> I have also renamed long_pulse() to full_detect().

Since intel_dp_detect() doesn't actually do *anything* except skip
detect sometimes, I think "full detect" is a misnomer.

Arguably you could merge intel_dp_long_pulse() into its only caller,
intel_dp_detect(), and do an early return on skipping detect. You have
to look at the functions, and wonder if the current split is the right
one.

Please let others chime in too before making changes.

BR,
Jani.


>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jan-Marek Glogowski <glogow@fbihome.de>
> References: 3cf71bc9904d ("drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse"")
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 436c22de33b6..befdaa076d8c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5012,7 +5012,7 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  }
>  
>  static int
> -intel_dp_long_pulse(struct intel_connector *connector,
> +intel_dp_full_detect(struct intel_connector *connector,
>  		    struct drm_modeset_acquire_ctx *ctx)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -5074,16 +5074,8 @@ intel_dp_long_pulse(struct intel_connector *connector,
>  		goto out;
>  	} else {
>  		/*
> -		 * If display is now connected check links status,
> -		 * there has been known issues of link loss triggering
> -		 * long pulse.
> -		 *
> -		 * Some sinks (eg. ASUS PB287Q) seem to perform some
> -		 * weird HPD ping pong during modesets. So we can apparently
> -		 * end up with HPD going low during a modeset, and then
> -		 * going back up soon after. And once that happens we must
> -		 * retrain the link to get a picture. That's in case no
> -		 * userspace component reacted to intermittent HPD dip.
> +		 * Check link status in case the link went out of
> +		 * synchronization and the sink did not tell us.
>  		 */
>  		struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  
> @@ -5149,7 +5141,8 @@ intel_dp_detect(struct drm_connector *connector,
>  				return ret;
>  		}
>  
> -		status = intel_dp_long_pulse(intel_dp->attached_connector, ctx);
> +		status = intel_dp_full_detect(intel_dp->attached_connector,
> +					      ctx);
>  	}
>  
>  	intel_dp->detect_done = false;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx