[PATCH 1/2] drm: add retries for lspcon status check

[PATCH 1/2] drm: add retries for lspcon status check

[Shashank Sharma]

It's an observation during some CI tests that few LSPCON chips
respond slow while system is under load, and need some delay
while reading current mode status using i2c-over-aux channel.

This patch:
- Adds few retries and delays before declaring a read
  failure from LSPCON hardware.
- Changes the debug level of the print from ERROR->DEBUG
  whereas another patch in I915 will add an ERROR message
  from the caller when we have timed out all our limits.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
index 80e62f6..c63eac8 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
 			enum drm_lspcon_mode *mode)
 {
 	u8 data;
+	u8 retry = 5;
 	int ret = 0;
 
 	if (!mode) {
@@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
 	}
 
 	/* Read Status: i2c over aux */
-	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
-				    &data, sizeof(data));
+	do {
+		ret = drm_dp_dual_mode_read(adapter,
+					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+					    &data, sizeof(data));
+		if (!ret || !retry--)
+			break;
+		usleep_range(500, 1000);
+	} while (1);
+
 	if (ret < 0) {
-		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
+		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
 		return -EFAULT;
 	}
 
-- 
2.7.4

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

[PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Shashank Sharma]

Our current logic to read LSPCON's current mode, stops retries and
breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
core function. This doesn't allow us to try reading the mode again.

This patch removes this condition and allows retries reading
the currnt mode until timeout.

This also fixes/prevents some of the noise in form of debug messages
while running IGT CI test cases.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 8413a4c..e64a336 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -118,14 +118,13 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
 	enum drm_lspcon_mode current_mode;
 
 	current_mode = lspcon_get_current_mode(lspcon);
-	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
+	if (current_mode == mode)
 		goto out;
 
 	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
 		      lspcon_mode_name(mode));
 
-	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
-		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
+	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);
 	if (current_mode != mode)
 		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
 
-- 
2.7.4

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm: add retries for lspcon status check

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm: add retries for lspcon status check
URL   : https://patchwork.freedesktop.org/series/28684/
State : success

== Summary ==

Series 28684v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/28684/revisions/1/mbox/

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:450s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:440s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:357s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:541s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:526s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:520s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:515s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:608s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:444s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:423s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:508s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:580s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:596s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:526s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:463s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:482s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:434s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:477s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:551s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:414s

fbb8288699ef622bbfc6e10bdca6773a16f93fac drm-tip: 2017y-08m-11d-09h-03m-47s UTC integration manifest
0462ff55be8c drm/i915: Don't give up waiting on INVALID_MODE
1965948a4bdd drm: add retries for lspcon status check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5382/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: add retries for lspcon status check

[Pandiyan, Dhinakaran]

On Fri, 2017-08-11 at 18:58 +0530, Shashank Sharma wrote:
> It's an observation during some CI tests that few LSPCON chips
> respond slow while system is under load, and need some delay

Thanks for the patch. Can you please explain what you mean by load here?
I can't follow why an external chip would respond slow if the system is
under load.

> while reading current mode status using i2c-over-aux channel.
> 

Is there a fdo bug where I can see more CI logs?

The only log I saw has this pattern around the time where this error
occurs. I am not sure how GPU hangs affect AUX transactions. But, I
think we should investigate/fix the underlying problem instead of adding
delays.


[   78.758510] i915 0000:00:02.0: Resetting vcs0 after gpu hang
[   78.758922] [drm:i915_gem_reset_engine [i915]] context
kms_busy[1372]/0 marked guilty (score 19) banned? no
[   78.758941] [drm:i915_gem_reset_engine [i915]] resetting vcs0 to
restart from tail of request 0x62
[   78.758990] [drm:gen8_init_common_ring [i915]] Execlists enabled for
vcs0
[   78.759014] [drm:gen8_init_common_ring [i915]] Restarting vcs0:0 from
0x62
[   78.759482] [drm:intel_power_well_enable [i915]] enabling always-on
[   78.759499] [drm:intel_power_well_enable [i915]] enabling DC off
[   78.759908] [drm:gen9_set_dc_state [i915]] Setting DC state from 02
to 00
[   78.759932] [drm:intel_power_well_enable [i915]] enabling power well
2
[   78.759993] [drm:intel_atomic_commit_tail [i915]] [ENCODER:57:DDI B]
[   78.760021] [drm:intel_atomic_commit_tail [i915]] [ENCODER:59:DP-MST
A]
[   78.760079] [drm:intel_atomic_commit_tail [i915]] [ENCODER:60:DP-MST
B]
[   78.760107] [drm:intel_atomic_commit_tail [i915]] [ENCODER:61:DP-MST
C]
[   78.760136] [drm:intel_atomic_commit_tail [i915]] [ENCODER:64:DDI C]
[   78.760163] [drm:intel_atomic_commit_tail [i915]] [ENCODER:66:DP-MST
A]
[   78.760191] [drm:intel_atomic_commit_tail [i915]] [ENCODER:67:DP-MST
B]
[   78.760218] [drm:intel_atomic_commit_tail [i915]] [ENCODER:68:DP-MST
C]
[   78.760246] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 0
[   78.760275] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 1
[   78.760304] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 2
[   78.760332] [drm:verify_single_dpll_state.isra.70 [i915]] DPLL 3
[   78.760362] [drm:intel_enable_shared_dpll [i915]] enable DPLL 1
(active 1, on? 0) for crtc 36
[   78.760390] [drm:intel_enable_shared_dpll [i915]] enabling DPLL 1
[   78.762529] [drm:intel_power_well_enable [i915]] enabling DDI B IO
power well
[   78.763146] [drm:drm_dp_i2c_do_msg] native defer
[   78.764268] [drm:drm_dp_i2c_do_msg] native defer
[   78.765735] [drm:drm_dp_i2c_do_msg] native defer
[   78.766794] [drm:drm_dp_i2c_do_msg] native defer
[   78.767853] [drm:drm_dp_i2c_do_msg] native defer
[   78.768982] [drm:drm_dp_i2c_do_msg] native defer
[   78.770112] [drm:drm_dp_i2c_do_msg] native defer
[   78.771240] [drm:drm_dp_i2c_do_msg] native defer
[   78.772371] [drm:drm_dp_i2c_do_msg] native defer
[   78.773068] [drm:drm_dp_i2c_do_msg] too many retries, giving up
[   78.773507] [drm:drm_dp_i2c_do_msg] native defer
[   78.774531] [drm:drm_dp_i2c_do_msg] native defer
[   78.775629] [drm:drm_dp_i2c_do_msg] native defer
[   78.776668] [drm:drm_dp_i2c_do_msg] native defer
[   78.777710] [drm:drm_dp_i2c_do_msg] native defer
[   78.778810] [drm:drm_dp_i2c_do_msg] native defer
[   78.779907] [drm:drm_dp_i2c_do_msg] native defer
[   78.780585] [drm:drm_dp_i2c_do_msg] too many retries, giving up
[   78.780589] [drm:drm_lspcon_get_mode] *ERROR* LSPCON read(0x80, 0x41)
failed
[   78.780621] [drm:lspcon_wait_mode [i915]] *ERROR* Error reading
LSPCON mode
[   78.780646] [drm:lspcon_wait_mode [i915]] Current LSPCON mode INVALID



> This patch:
> - Adds few retries and delays before declaring a read
>   failure from LSPCON hardware.
> - Changes the debug level of the print from ERROR->DEBUG
>   whereas another patch in I915 will add an ERROR message
>   from the caller when we have timed out all our limits.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> index 80e62f6..c63eac8 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  			enum drm_lspcon_mode *mode)
>  {
>  	u8 data;
> +	u8 retry = 5;
>  	int ret = 0;
>  
>  	if (!mode) {
> @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	}
>  
>  	/* Read Status: i2c over aux */
> -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> -				    &data, sizeof(data));
> +	do {
> +		ret = drm_dp_dual_mode_read(adapter,
> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +					    &data, sizeof(data));
> +		if (!ret || !retry--)
> +			break;
> +		usleep_range(500, 1000);

Are these numbers from a spec? I think it'd be pertinent to mention the
source.

> +	} while (1);
> +
>  	if (ret < 0) {
> -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>  		return -EFAULT;
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Pandiyan, Dhinakaran]

On Fri, 2017-08-11 at 18:58 +0530, Shashank Sharma wrote:
> Our current logic to read LSPCON's current mode, stops retries and
> breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
> core function. This doesn't allow us to try reading the mode again.
> 
> This patch removes this condition and allows retries reading
> the currnt mode until timeout.
> 
> This also fixes/prevents some of the noise in form of debug messages
> while running IGT CI test cases.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lspcon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 8413a4c..e64a336 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -118,14 +118,13 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
>  	enum drm_lspcon_mode current_mode;
>  
>  	current_mode = lspcon_get_current_mode(lspcon);
> -	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
> +	if (current_mode == mode)
>  		goto out;
>  
>  	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
>  		      lspcon_mode_name(mode));
>  
> -	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
> -		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
> +	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);

Doesn't this do the job of retries that you are adding in patch 1/2 ?

>  	if (current_mode != mode)
>  		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: add retries for lspcon status check

[Imre Deak]

On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
> It's an observation during some CI tests that few LSPCON chips
> respond slow while system is under load, and need some delay
> while reading current mode status using i2c-over-aux channel.
> 
> This patch:
> - Adds few retries and delays before declaring a read
>   failure from LSPCON hardware.
> - Changes the debug level of the print from ERROR->DEBUG
>   whereas another patch in I915 will add an ERROR message
>   from the caller when we have timed out all our limits.

Hm yea, I was wondering if this is the same issue we saw earlier due to
HPD not being asserted. But looking at the logs it's not the case. After
HPD is asserted it really takes this much time (~36ms) for the adaptor
to respond. This is against the DP 1.4 spec which specifies a 20ms
maximum wake up time, but what can you do.

> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> index 80e62f6..c63eac8 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  			enum drm_lspcon_mode *mode)
>  {
>  	u8 data;
> +	u8 retry = 5;

Nitpick: no reason for a specific type so just int?

>  	int ret = 0;
>  
>  	if (!mode) {
> @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	}
>  
>  	/* Read Status: i2c over aux */
> -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> -				    &data, sizeof(data));
> +	do {

Nitpick: a for loop whenever possible is generally clearer.

With the above optionally changed:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> +		ret = drm_dp_dual_mode_read(adapter,
> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +					    &data, sizeof(data));
> +		if (!ret || !retry--)
> +			break;
> +		usleep_range(500, 1000);
> +	} while (1);
> +
>  	if (ret < 0) {
> -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>  		return -EFAULT;
>  	}
>  
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Imre Deak]

On Fri, Aug 11, 2017 at 06:58:27PM +0530, Shashank Sharma wrote:
> Our current logic to read LSPCON's current mode, stops retries and
> breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
> core function. This doesn't allow us to try reading the mode again.
> 
> This patch removes this condition and allows retries reading
> the currnt mode until timeout.
> 
> This also fixes/prevents some of the noise in form of debug messages
> while running IGT CI test cases.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lspcon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 8413a4c..e64a336 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -118,14 +118,13 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
>  	enum drm_lspcon_mode current_mode;
>  
>  	current_mode = lspcon_get_current_mode(lspcon);
> -	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
> +	if (current_mode == mode)
>  		goto out;
>  
>  	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
>  		      lspcon_mode_name(mode));
>  
> -	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
> -		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
> +	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);
>  	if (current_mode != mode)
>  		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
>  
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Imre Deak]

On Tue, Aug 15, 2017 at 12:21:14AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-08-11 at 18:58 +0530, Shashank Sharma wrote:
> > Our current logic to read LSPCON's current mode, stops retries and
> > breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
> > core function. This doesn't allow us to try reading the mode again.
> > 
> > This patch removes this condition and allows retries reading
> > the currnt mode until timeout.
> > 
> > This also fixes/prevents some of the noise in form of debug messages
> > while running IGT CI test cases.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lspcon.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 8413a4c..e64a336 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -118,14 +118,13 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
> >  	enum drm_lspcon_mode current_mode;
> >  
> >  	current_mode = lspcon_get_current_mode(lspcon);
> > -	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
> > +	if (current_mode == mode)
> >  		goto out;
> >  
> >  	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
> >  		      lspcon_mode_name(mode));
> >  
> > -	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
> > -		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
> > +	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);
> 
> Doesn't this do the job of retries that you are adding in patch 1/2 ?

No that's a workaround for a different issue, where the adaptor is
initially in LS mode, but switches on its own to PCON mode after some
delay.

> 
> >  	if (current_mode != mode)
> >  		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
> >  
> _______________________________________________
> 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 1/2] drm: add retries for lspcon status check

[Sharma, Shashank]

Thanks for the review, Imre.

My comments, inline.

Regards
Shashank
On 8/16/2017 7:35 PM, Imre Deak wrote:
> On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
>> It's an observation during some CI tests that few LSPCON chips
>> respond slow while system is under load, and need some delay
>> while reading current mode status using i2c-over-aux channel.
>>
>> This patch:
>> - Adds few retries and delays before declaring a read
>>    failure from LSPCON hardware.
>> - Changes the debug level of the print from ERROR->DEBUG
>>    whereas another patch in I915 will add an ERROR message
>>    from the caller when we have timed out all our limits.
> Hm yea, I was wondering if this is the same issue we saw earlier due to
> HPD not being asserted. But looking at the logs it's not the case. After
> HPD is asserted it really takes this much time (~36ms) for the adaptor
> to respond. This is against the DP 1.4 spec which specifies a 20ms
> maximum wake up time, but what can you do.
>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> index 80e62f6..c63eac8 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>   			enum drm_lspcon_mode *mode)
>>   {
>>   	u8 data;
>> +	u8 retry = 5;
> Nitpick: no reason for a specific type so just int?
Actually, was trying to save 3 bytes, as I knew max value for retry 
would be 5 < 255, but might be too much optimization ?
>>   	int ret = 0;
>>   
>>   	if (!mode) {
>> @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>   	}
>>   
>>   	/* Read Status: i2c over aux */
>> -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> -				    &data, sizeof(data));
>> +	do {
> Nitpick: a for loop whenever possible is generally clearer.
Sorry, I dint understand this comment, little more help required :) ?
- Shashank
>
> With the above optionally changed:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
>> +		ret = drm_dp_dual_mode_read(adapter,
>> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> +					    &data, sizeof(data));
>> +		if (!ret || !retry--)
>> +			break;
>> +		usleep_range(500, 1000);
>> +	} while (1);
>> +
>>   	if (ret < 0) {
>> -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>> +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>>   		return -EFAULT;
>>   	}
>>   
>> -- 
>> 2.7.4
>>

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

Re: [PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Sharma, Shashank]

Regards

Shashank


On 8/15/2017 5:51 AM, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-08-11 at 18:58 +0530, Shashank Sharma wrote:
>> Our current logic to read LSPCON's current mode, stops retries and
>> breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
>> core function. This doesn't allow us to try reading the mode again.
>>
>> This patch removes this condition and allows retries reading
>> the currnt mode until timeout.
>>
>> This also fixes/prevents some of the noise in form of debug messages
>> while running IGT CI test cases.
>>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lspcon.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> index 8413a4c..e64a336 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -118,14 +118,13 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
>>   	enum drm_lspcon_mode current_mode;
>>   
>>   	current_mode = lspcon_get_current_mode(lspcon);
>> -	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
>> +	if (current_mode == mode)
>>   		goto out;
>>   
>>   	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
>>   		      lspcon_mode_name(mode));
>>   
>> -	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
>> -		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
>> +	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);
> Doesn't this do the job of retries that you are adding in patch 1/2 ?
I think Imre has responded on this comment already, and explained better 
than me :)
- Shashank
>>   	if (current_mode != mode)
>>   		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
>>   

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

Re: [PATCH 1/2] drm: add retries for lspcon status check

[Imre Deak]

On Wed, Aug 16, 2017 at 09:18:58PM +0530, Sharma, Shashank wrote:
> Thanks for the review, Imre.
> 
> My comments, inline.
> 
> Regards
> Shashank
> On 8/16/2017 7:35 PM, Imre Deak wrote:
> > On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
> > > It's an observation during some CI tests that few LSPCON chips
> > > respond slow while system is under load, and need some delay
> > > while reading current mode status using i2c-over-aux channel.
> > > 
> > > This patch:
> > > - Adds few retries and delays before declaring a read
> > >    failure from LSPCON hardware.
> > > - Changes the debug level of the print from ERROR->DEBUG
> > >    whereas another patch in I915 will add an ERROR message
> > >    from the caller when we have timed out all our limits.
> > Hm yea, I was wondering if this is the same issue we saw earlier due to
> > HPD not being asserted. But looking at the logs it's not the case. After
> > HPD is asserted it really takes this much time (~36ms) for the adaptor
> > to respond. This is against the DP 1.4 spec which specifies a 20ms
> > maximum wake up time, but what can you do.
> > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > index 80e62f6..c63eac8 100644
> > > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   			enum drm_lspcon_mode *mode)
> > >   {
> > >   	u8 data;
> > > +	u8 retry = 5;
> > Nitpick: no reason for a specific type so just int?
> Actually, was trying to save 3 bytes, as I knew max value for retry would be
> 5 < 255, but might be too much optimization ?

Yes, imo it's odd to use u8 for simple loop counter. The compiler may
even generate some extra code to zero extend or truncate the result.

> > >   	int ret = 0;
> > >   	if (!mode) {
> > > @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   	}
> > >   	/* Read Status: i2c over aux */
> > > -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > -				    &data, sizeof(data));
> > > +	do {
> > Nitpick: a for loop whenever possible is generally clearer.
> Sorry, I dint understand this comment, little more help required :) ?

for (retry = 0; retry < 5; retry++) ...

would be clearer imo.

--Imre

> - Shashank
> > 
> > With the above optionally changed:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > > +		ret = drm_dp_dual_mode_read(adapter,
> > > +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > +					    &data, sizeof(data));
> > > +		if (!ret || !retry--)
> > > +			break;
> > > +		usleep_range(500, 1000);
> > > +	} while (1);
> > > +
> > >   	if (ret < 0) {
> > > -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> > > +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
> > >   		return -EFAULT;
> > >   	}
> > > -- 
> > > 2.7.4
> > > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: add retries for lspcon status check

[Sharma, Shashank]

Regards

Shashank


On 8/16/2017 9:42 PM, Imre Deak wrote:
> On Wed, Aug 16, 2017 at 09:18:58PM +0530, Sharma, Shashank wrote:
>> Thanks for the review, Imre.
>>
>> My comments, inline.
>>
>> Regards
>> Shashank
>> On 8/16/2017 7:35 PM, Imre Deak wrote:
>>> On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
>>>> It's an observation during some CI tests that few LSPCON chips
>>>> respond slow while system is under load, and need some delay
>>>> while reading current mode status using i2c-over-aux channel.
>>>>
>>>> This patch:
>>>> - Adds few retries and delays before declaring a read
>>>>     failure from LSPCON hardware.
>>>> - Changes the debug level of the print from ERROR->DEBUG
>>>>     whereas another patch in I915 will add an ERROR message
>>>>     from the caller when we have timed out all our limits.
>>> Hm yea, I was wondering if this is the same issue we saw earlier due to
>>> HPD not being asserted. But looking at the logs it's not the case. After
>>> HPD is asserted it really takes this much time (~36ms) for the adaptor
>>> to respond. This is against the DP 1.4 spec which specifies a 20ms
>>> maximum wake up time, but what can you do.
>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
>>>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> index 80e62f6..c63eac8 100644
>>>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>>>    			enum drm_lspcon_mode *mode)
>>>>    {
>>>>    	u8 data;
>>>> +	u8 retry = 5;
>>> Nitpick: no reason for a specific type so just int?
>> Actually, was trying to save 3 bytes, as I knew max value for retry would be
>> 5 < 255, but might be too much optimization ?
> Yes, imo it's odd to use u8 for simple loop counter. The compiler may
> even generate some extra code to zero extend or truncate the result.
cool, so int it is.
>>>>    	int ret = 0;
>>>>    	if (!mode) {
>>>> @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>>>    	}
>>>>    	/* Read Status: i2c over aux */
>>>> -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>>> -				    &data, sizeof(data));
>>>> +	do {
>>> Nitpick: a for loop whenever possible is generally clearer.
>> Sorry, I dint understand this comment, little more help required :) ?
> for (retry = 0; retry < 5; retry++) ...
>
> would be clearer imo.
Sure, will do it.
> --Imre
>
>> - Shashank
>>> With the above optionally changed:
>>> Reviewed-by: Imre Deak <imre.deak@intel.com>
>>>
>>>> +		ret = drm_dp_dual_mode_read(adapter,
>>>> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>>> +					    &data, sizeof(data));
>>>> +		if (!ret || !retry--)
>>>> +			break;
>>>> +		usleep_range(500, 1000);
>>>> +	} while (1);
>>>> +
>>>>    	if (ret < 0) {
>>>> -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>>>> +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>>>>    		return -EFAULT;
>>>>    	}
>>>> -- 
>>>> 2.7.4
>>>>

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

[PATCH 2/2] drm/i915: Don't give up waiting on INVALID_MODE

[Shashank Sharma]

Our current logic to read LSPCON's current mode, stops retries and
breaks wait-loop, if it gets LSPCON_MODE_INVALID as return from the
core function. This doesn't allow us to try reading the mode again.

This patch removes this condition and allows retries reading
the currnt mode until timeout.

This also fixes/prevents some of the noise in form of debug messages
while running IGT CI test cases.

V2: rebase, added r-b
V2: changed some debug message levels from debug->error and
    error->debug in lspcon_get_current_mode function.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Mahesh Kumar <Mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 8413a4c..0a61c0d 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -106,7 +106,7 @@ static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
 	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
 
 	if (drm_lspcon_get_mode(adapter, &current_mode)) {
-		DRM_ERROR("Error reading LSPCON mode\n");
+		DRM_DEBUG_KMS("Error reading LSPCON mode\n");
 		return DRM_LSPCON_MODE_INVALID;
 	}
 	return current_mode;
@@ -118,16 +118,15 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
 	enum drm_lspcon_mode current_mode;
 
 	current_mode = lspcon_get_current_mode(lspcon);
-	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
+	if (current_mode == mode)
 		goto out;
 
 	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
 		      lspcon_mode_name(mode));
 
-	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
-		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
+	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 100);
 	if (current_mode != mode)
-		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
+		DRM_ERROR("LSPCON mode hasn't settled\n");
 
 out:
 	DRM_DEBUG_KMS("Current LSPCON mode %s\n",
-- 
2.7.4

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