[PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[Manasi Navare]

Intel_dp_link_is_valid() function reads the Link status registers
and returns a boolean to indicate link is valid or not.
If the link has lost lock and is not valid any more, link
training is performed outside the function else previously trained link
is retained.
This gives us flexibility of checking whether link is valid and training
it independently.

v2:
* Changed the function name from intel_dp_check_link_status()
to intel_dp_link_is_valid()  (Lukas Wunner)
* Checks for CRTC and active CRTC are moved outside the
intel_dp_link_is_valid() function (Rodrigo Vivi)

Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 364db90..891147d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3881,36 +3881,33 @@ go_again:
 	return -EINVAL;
 }
 
-static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+static bool
+intel_dp_link_is_valid(struct intel_dp *intel_dp)
 {
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	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;
+	/* 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_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
+		return false;
+	}
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
+	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:
@@ -3928,6 +3925,8 @@ static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
@@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
+	/* Do not train the link if there is no crtc */
+	if (!intel_encoder->base.crtc)
+		return true;
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return true;
+
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-	intel_dp_check_link_status(intel_dp);
+	if (!intel_dp_link_is_valid(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;
@@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 * check links status, there has been known issues of
 		 * link loss triggerring long pulse!!!!
 		 */
+		/* Do not train the link if there is no crtc */
+		if (!intel_encoder->base.crtc)
+			goto out;
+		if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+			goto out;
+
 		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-		intel_dp_check_link_status(intel_dp);
+		if (!intel_dp_link_is_valid(intel_dp)) {
+			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

Re: [PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[Pandiyan, Dhinakaran]

On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote:
> Intel_dp_link_is_valid() function reads the Link status registers
> and returns a boolean to indicate link is valid or not.
> If the link has lost lock and is not valid any more, link
> training is performed outside the function else previously trained link
> is retained.
> This gives us flexibility of checking whether link is valid and training
> it independently.
> 
> v2:
> * Changed the function name from intel_dp_check_link_status()
> to intel_dp_link_is_valid()  (Lukas Wunner)
> * Checks for CRTC and active CRTC are moved outside the
> intel_dp_link_is_valid() function (Rodrigo Vivi)
> 
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 364db90..891147d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3881,36 +3881,33 @@ go_again:
>  	return -EINVAL;
>  }
>  
> -static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> +static bool
> +intel_dp_link_is_valid(struct intel_dp *intel_dp)
>  {
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
>  	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;
> +	/* 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_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
drm_dp_channel_eq_ok() does not check for CR. Should we just say
"Channel EQ not ok" to preempt ambiguity while debugging ?

> +		return false;
> +	}
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> +	DRM_DEBUG_KMS("Link is good, no need to retrain\n");
The caller does not expect us to link train anymore, I don't think we
have to explicitly state "no need to retrain". Also, do we need debug
messages if the link is good?

> +	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:
> @@ -3928,6 +3925,8 @@ static bool
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> +	/* Do not train the link if there is no crtc */
> +	if (!intel_encoder->base.crtc)
> +		return true;
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return true;
> +
I might be completely off base here. Shouldn't we keep the link valid
irrespective of whether there is an active crtc? I thought that is what
the refactoring is supposed to enable. Does intel_dp_short_pulse() get
called when there is a link loss during upfront link training? And in
that case, shouldn't we retrain even without a crtc? 

Besides that, how about using just one return?

struct drm_crtc *crtc = intel_encoder->base.crtc;

if (crtc == NULL || !to_intel_crtc(crtc)->active)
	return true;


>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -	intel_dp_check_link_status(intel_dp);
> +	if (!intel_dp_link_is_valid(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;
> @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 * check links status, there has been known issues of
>  		 * link loss triggerring long pulse!!!!
>  		 */
> +		/* Do not train the link if there is no crtc */
> +		if (!intel_encoder->base.crtc)
> +			goto out;
> +		if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +			goto out;
> +
>  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -		intel_dp_check_link_status(intel_dp);
> +		if (!intel_dp_link_is_valid(intel_dp)) {
> +			intel_dp_start_link_train(intel_dp);
> +			intel_dp_stop_link_train(intel_dp);
> +		}
>  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  		goto out;
>  	}

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for drm/i915: intel_dp_link_is_valid() should only return status of link (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: intel_dp_link_is_valid() should only return status of link (rev2)
URL   : https://patchwork.freedesktop.org/series/9737/
State : failure

== Summary ==

Series 9737v2 drm/i915: intel_dp_link_is_valid() should only return status of link
http://patchwork.freedesktop.org/api/1.0/series/9737/revisions/2/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)
Test gem_flink_basic:
        Subgroup bad-open:
                pass       -> INCOMPLETE (fi-skl-i7-6700k)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-varying-size:
                pass       -> FAIL       (ro-ilk1-i5-650)
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (ro-byt-n2820)
        Subgroup read-crc-pipe-b-frame-sequence:
                fail       -> PASS       (ro-ivb2-i7-3770)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:107  pass:91   dwarn:0   dfail:0   fail:0   skip:15 
fi-kbl-qkkr      total:244  pass:186  dwarn:28  dfail:0   fail:3   skip:27 
fi-skl-i5-6260u  total:244  pass:224  dwarn:4   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:112  pass:87   dwarn:1   dfail:0   fail:0   skip:23 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:2   dfail:0   fail:2   skip:18 
ro-bdw-i7-5600u  total:240  pass:206  dwarn:1   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:193  dwarn:0   dfail:0   fail:4   skip:43 
ro-byt-n2820     total:240  pass:197  dwarn:1   dfail:0   fail:2   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:223  dwarn:0   dfail:0   fail:3   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1843/

4a26251 drm-intel-nightly: 2016y-08m-11d-16h-12m-42s UTC integration manifest
3d6ab56 drm/i915: intel_dp_link_is_valid() should only return status of link

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[Manasi Navare]

On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote:
> > Intel_dp_link_is_valid() function reads the Link status registers
> > and returns a boolean to indicate link is valid or not.
> > If the link has lost lock and is not valid any more, link
> > training is performed outside the function else previously trained link
> > is retained.
> > This gives us flexibility of checking whether link is valid and training
> > it independently.
> > 
> > v2:
> > * Changed the function name from intel_dp_check_link_status()
> > to intel_dp_link_is_valid()  (Lukas Wunner)
> > * Checks for CRTC and active CRTC are moved outside the
> > intel_dp_link_is_valid() function (Rodrigo Vivi)
> > 
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++--------------
> >  1 file changed, 37 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 364db90..891147d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3881,36 +3881,33 @@ go_again:
> >  	return -EINVAL;
> >  }
> >  
> > -static void
> > -intel_dp_check_link_status(struct intel_dp *intel_dp)
> > +static bool
> > +intel_dp_link_is_valid(struct intel_dp *intel_dp)
> >  {
> > -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >  	u8 link_status[DP_LINK_STATUS_SIZE];
> >  
> >  	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;
> > +	/* 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_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
> drm_dp_channel_eq_ok() does not check for CR. Should we just say
> "Channel EQ not ok" to preempt ambiguity while debugging ?

Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as:
#define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE |           \
                            DP_LANE_CHANNEL_EQ_DONE |   \
                            DP_LANE_SYMBOL_LOCKED)
So it includes checking for Channel EQ and Clock Recovery CR bits


> 
> > +		return false;
> > +	}
> >  
> > -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > -		return;
> > +	DRM_DEBUG_KMS("Link is good, no need to retrain\n");
> The caller does not expect us to link train anymore, I don't think we
> have to explicitly state "no need to retrain". Also, do we need debug
> messages if the link is good?

I agree , maybe this is not needed. I will remove this

> 
> > +	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:
> > @@ -3928,6 +3925,8 @@ static bool
> >  intel_dp_short_pulse(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> >  	u8 sink_irq_vector = 0;
> >  	u8 old_sink_count = intel_dp->sink_count;
> >  	bool ret;
> > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> >  	}
> >  
> > +	/* Do not train the link if there is no crtc */
> > +	if (!intel_encoder->base.crtc)
> > +		return true;
> > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > +		return true;
> > +
> I might be completely off base here. Shouldn't we keep the link valid
> irrespective of whether there is an active crtc? I thought that is what
> the refactoring is supposed to enable. Does intel_dp_short_pulse() get
> called when there is a link loss during upfront link training? And in
> that case, shouldn't we retrain even without a crtc? 

We cannot ever retrain without a CRTC. This check is more for making sure that the clocks
are set up befofe we try to retrain else we will see AUX channel failures.
If I track this back in the kernel tree, this check was added to avoid the lock up issues on some
platforms.

> 
> Besides that, how about using just one return?
> 
> struct drm_crtc *crtc = intel_encoder->base.crtc;
> 
> if (crtc == NULL || !to_intel_crtc(crtc)->active)
> 	return true;
> 
>

The only problem with doing both these checks together is that if crtc is NULL
then we are trying to dereference a NULL pointer in the second check.
So it should be seuqential, check if crtc is active only if there is crtc available.

Manasi
 
> >  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -	intel_dp_check_link_status(intel_dp);
> > +	if (!intel_dp_link_is_valid(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;
> > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  		 * check links status, there has been known issues of
> >  		 * link loss triggerring long pulse!!!!
> >  		 */
> > +		/* Do not train the link if there is no crtc */
> > +		if (!intel_encoder->base.crtc)
> > +			goto out;
> > +		if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > +			goto out;
> > +
> >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > -		intel_dp_check_link_status(intel_dp);
> > +		if (!intel_dp_link_is_valid(intel_dp)) {
> > +			intel_dp_start_link_train(intel_dp);
> > +			intel_dp_stop_link_train(intel_dp);
> > +		}
> >  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >  		goto out;
> >  	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[Pandiyan, Dhinakaran]

On Fri, 2016-08-12 at 10:56 -0700, Manasi Navare wrote:
> On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote:
> > On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote:
> > > Intel_dp_link_is_valid() function reads the Link status registers
> > > and returns a boolean to indicate link is valid or not.
> > > If the link has lost lock and is not valid any more, link
> > > training is performed outside the function else previously trained link
> > > is retained.
> > > This gives us flexibility of checking whether link is valid and training
> > > it independently.
> > > 
> > > v2:
> > > * Changed the function name from intel_dp_check_link_status()
> > > to intel_dp_link_is_valid()  (Lukas Wunner)
> > > * Checks for CRTC and active CRTC are moved outside the
> > > intel_dp_link_is_valid() function (Rodrigo Vivi)
> > > 
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++--------------
> > >  1 file changed, 37 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 364db90..891147d 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3881,36 +3881,33 @@ go_again:
> > >  	return -EINVAL;
> > >  }
> > >  
> > > -static void
> > > -intel_dp_check_link_status(struct intel_dp *intel_dp)
> > > +static bool
> > > +intel_dp_link_is_valid(struct intel_dp *intel_dp)
> > >  {
> > > -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > >  	u8 link_status[DP_LINK_STATUS_SIZE];
> > >  
> > >  	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;
> > > +	/* 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_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
> > drm_dp_channel_eq_ok() does not check for CR. Should we just say
> > "Channel EQ not ok" to preempt ambiguity while debugging ?
> 
> Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as:
> #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE |           \
>                             DP_LANE_CHANNEL_EQ_DONE |   \
>                             DP_LANE_SYMBOL_LOCKED)
> So it includes checking for Channel EQ and Clock Recovery CR bits
> 
> 

Thank you, I should have looked hard. I will leave this to you. 

> > 
> > > +		return false;
> > > +	}
> > >  
> > > -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > -		return;
> > > +	DRM_DEBUG_KMS("Link is good, no need to retrain\n");
> > The caller does not expect us to link train anymore, I don't think we
> > have to explicitly state "no need to retrain". Also, do we need debug
> > messages if the link is good?
> 
> I agree , maybe this is not needed. I will remove this
> 
> > 
> > > +	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:
> > > @@ -3928,6 +3925,8 @@ static bool
> > >  intel_dp_short_pulse(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > >  	u8 sink_irq_vector = 0;
> > >  	u8 old_sink_count = intel_dp->sink_count;
> > >  	bool ret;
> > > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> > >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> > >  	}
> > >  
> > > +	/* Do not train the link if there is no crtc */
> > > +	if (!intel_encoder->base.crtc)
> > > +		return true;
> > > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > +		return true;
> > > +
> > I might be completely off base here. Shouldn't we keep the link valid
> > irrespective of whether there is an active crtc? I thought that is what
> > the refactoring is supposed to enable. Does intel_dp_short_pulse() get
> > called when there is a link loss during upfront link training? And in
> > that case, shouldn't we retrain even without a crtc? 
> 
> We cannot ever retrain without a CRTC. This check is more for making sure that the clocks
> are set up befofe we try to retrain else we will see AUX channel failures.
> If I track this back in the kernel tree, this check was added to avoid the lock up issues on some
> platforms.

So, crtc will be active by the time we get short pulse for upfront link
training failures ?

> > 
> > Besides that, how about using just one return?
> > 
> > struct drm_crtc *crtc = intel_encoder->base.crtc;
> > 
> > if (crtc == NULL || !to_intel_crtc(crtc)->active)
> > 	return true;
> > 
> >
> 
> The only problem with doing both these checks together is that if crtc is NULL
> then we are trying to dereference a NULL pointer in the second check.
> So it should be seuqential, check if crtc is active only if there is crtc available.
> 
> Manasi
>  

afaik the second check won't be evaluated if the first is True.

> > >  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > -	intel_dp_check_link_status(intel_dp);
> > > +	if (!intel_dp_link_is_valid(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;
> > > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >  		 * check links status, there has been known issues of
> > >  		 * link loss triggerring long pulse!!!!
> > >  		 */
> > > +		/* Do not train the link if there is no crtc */
> > > +		if (!intel_encoder->base.crtc)
> > > +			goto out;
> > > +		if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > +			goto out;
> > > +
> > >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > -		intel_dp_check_link_status(intel_dp);
> > > +		if (!intel_dp_link_is_valid(intel_dp)) {
> > > +			intel_dp_start_link_train(intel_dp);
> > > +			intel_dp_stop_link_train(intel_dp);
> > > +		}
> > >  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > >  		goto out;
> > >  	}
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: intel_dp_link_is_valid() should only return status of link

[Manasi Navare]

On Fri, Aug 12, 2016 at 02:50:58PM -0700, Pandiyan, Dhinakaran wrote:
> On Fri, 2016-08-12 at 10:56 -0700, Manasi Navare wrote:
> > On Thu, Aug 11, 2016 at 08:18:54PM -0700, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2016-08-11 at 15:23 -0700, Manasi Navare wrote:
> > > > Intel_dp_link_is_valid() function reads the Link status registers
> > > > and returns a boolean to indicate link is valid or not.
> > > > If the link has lost lock and is not valid any more, link
> > > > training is performed outside the function else previously trained link
> > > > is retained.
> > > > This gives us flexibility of checking whether link is valid and training
> > > > it independently.
> > > > 
> > > > v2:
> > > > * Changed the function name from intel_dp_check_link_status()
> > > > to intel_dp_link_is_valid()  (Lukas Wunner)
> > > > * Checks for CRTC and active CRTC are moved outside the
> > > > intel_dp_link_is_valid() function (Rodrigo Vivi)
> > > > 
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++++--------------
> > > >  1 file changed, 37 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 364db90..891147d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3881,36 +3881,33 @@ go_again:
> > > >  	return -EINVAL;
> > > >  }
> > > >  
> > > > -static void
> > > > -intel_dp_check_link_status(struct intel_dp *intel_dp)
> > > > +static bool
> > > > +intel_dp_link_is_valid(struct intel_dp *intel_dp)
> > > >  {
> > > > -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > > >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > >  	u8 link_status[DP_LINK_STATUS_SIZE];
> > > >  
> > > >  	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;
> > > > +	/* 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_DEBUG_KMS("Channel EQ or CR not ok, need to retrain\n");
> > > drm_dp_channel_eq_ok() does not check for CR. Should we just say
> > > "Channel EQ not ok" to preempt ambiguity while debugging ?
> > 
> > Actually this macro checks for DP_CHANNEL_EQ_BITS which is defined as:
> > #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE |           \
> >                             DP_LANE_CHANNEL_EQ_DONE |   \
> >                             DP_LANE_SYMBOL_LOCKED)
> > So it includes checking for Channel EQ and Clock Recovery CR bits
> > 
> > 
> 
> Thank you, I should have looked hard. I will leave this to you. 
> 
> > > 
> > > > +		return false;
> > > > +	}
> > > >  
> > > > -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > > -		return;
> > > > +	DRM_DEBUG_KMS("Link is good, no need to retrain\n");
> > > The caller does not expect us to link train anymore, I don't think we
> > > have to explicitly state "no need to retrain". Also, do we need debug
> > > messages if the link is good?
> > 
> > I agree , maybe this is not needed. I will remove this
> > 
> > > 
> > > > +	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:
> > > > @@ -3928,6 +3925,8 @@ static bool
> > > >  intel_dp_short_pulse(struct intel_dp *intel_dp)
> > > >  {
> > > >  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > >  	u8 sink_irq_vector = 0;
> > > >  	u8 old_sink_count = intel_dp->sink_count;
> > > >  	bool ret;
> > > > @@ -3968,8 +3967,18 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> > > >  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> > > >  	}
> > > >  
> > > > +	/* Do not train the link if there is no crtc */
> > > > +	if (!intel_encoder->base.crtc)
> > > > +		return true;
> > > > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > > +		return true;
> > > > +
> > > I might be completely off base here. Shouldn't we keep the link valid
> > > irrespective of whether there is an active crtc? I thought that is what
> > > the refactoring is supposed to enable. Does intel_dp_short_pulse() get
> > > called when there is a link loss during upfront link training? And in
> > > that case, shouldn't we retrain even without a crtc? 
> > 
> > We cannot ever retrain without a CRTC. This check is more for making sure that the clocks
> > are set up befofe we try to retrain else we will see AUX channel failures.
> > If I track this back in the kernel tree, this check was added to avoid the lock up issues on some
> > platforms.
> 
> So, crtc will be active by the time we get short pulse for upfront link
> training failures ?

So the way locks are taken by upfront link train, it would have enabled the crtc before it can handle link loss
related short pulses.

 
> 
> > > 
> > > Besides that, how about using just one return?
> > > 
> > > struct drm_crtc *crtc = intel_encoder->base.crtc;
> > > 
> > > if (crtc == NULL || !to_intel_crtc(crtc)->active)
> > > 	return true;
> > > 
> > >
> > 
> > The only problem with doing both these checks together is that if crtc is NULL
> > then we are trying to dereference a NULL pointer in the second check.
> > So it should be seuqential, check if crtc is active only if there is crtc available.
> > 
> > Manasi
> >  
> 
> afaik the second check won't be evaluated if the first is True.
>

Yup, makes sense. I will change that

 
> > > >  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > > -	intel_dp_check_link_status(intel_dp);
> > > > +	if (!intel_dp_link_is_valid(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;
> > > > @@ -4298,8 +4307,17 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > >  		 * check links status, there has been known issues of
> > > >  		 * link loss triggerring long pulse!!!!
> > > >  		 */
> > > > +		/* Do not train the link if there is no crtc */
> > > > +		if (!intel_encoder->base.crtc)
> > > > +			goto out;
> > > > +		if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > > > +			goto out;
> > > > +
> > > >  		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > > > -		intel_dp_check_link_status(intel_dp);
> > > > +		if (!intel_dp_link_is_valid(intel_dp)) {
> > > > +			intel_dp_start_link_train(intel_dp);
> > > > +			intel_dp_stop_link_train(intel_dp);
> > > > +		}
> > > >  		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > > >  		goto out;
> > > >  	}
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx