[PATCH 0/3] Add retries for DP dual mode devices

[PATCH 0/3] Add retries for DP dual mode devices

[Shashank Sharma]

Some of the DP dual mode devices (like LSPCON) need some time
to settle down, on specific platfomrs. Its been observed during
IGT CI runs that some of the KBL boards show some i2c-over-aux
failures when driver module is re-inserted or re-loaded.

This patch series adds some retries at various dp_dual_mode
helper functions, to allow these specofic devices to settle
down.

First two patches were separately reviewed here:
https://patchwork.freedesktop.org/series/28684/

Re-publishing them here to make this series with
an agenda :)

Shashank Sharma (3):
  drm: Add retries for lspcon status check
  drm/i915: Don't give up waiting on INVALID_MODE
  drm: Add retries for dp dual mode read

 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 23 +++++++++++++++++++----
 drivers/gpu/drm/i915/intel_lspcon.c       |  5 ++---
 2 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] 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.

V2: addressed review comments from Imre, and added r-b
    - use int instead of u8 for counter
    - use for loop instead of do...while();
V3: Rebase

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@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/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..fc8c7ac 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;
+	int retry;
 	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));
+	for (retry = 5; ; retry--) {
+		ret = drm_dp_dual_mode_read(adapter,
+					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
+					    &data, sizeof(data));
+		if (!ret || !retry)
+			break;
+		usleep_range(500, 1000);
+	}
+
 	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/3] 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

[PATCH 3/3] drm: Add retries for dp dual mode read

[Shashank Sharma]

From the CI builds, its been observed that during a driver
reload/insert, dp dual mode read function sometimes fails to
read from dual mode devices (like LSPCON) over i2c-over-aux
channel. This patch adds some delay and few retries, allowing
a scope for these devices to settle down.

V3: Introduced this patch

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
index fc8c7ac..bb34ff8 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -75,8 +75,15 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
 		},
 	};
 	int ret;
+	int retry;
+
+	for (retry = 5; ; retry--) {
+		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+		if (ret > 0 || !retry)
+			break;
+		usleep_range(500, 1000);
+	}
 
-	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
 	if (ret < 0)
 		return ret;
 	if (ret != ARRAY_SIZE(msgs))
-- 
2.7.4

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

Re: [PATCH 1/3] drm: Add retries for lspcon status check

[Imre Deak]

On Tue, Aug 22, 2017 at 08:23:49PM +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.
> 
> V2: addressed review comments from Imre, and added r-b
>     - use int instead of u8 for counter
>     - use for loop instead of do...while();
> V3: Rebase
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@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/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..fc8c7ac 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;
> +	int retry;
>  	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));
> +	for (retry = 5; ; retry--) {
> +		ret = drm_dp_dual_mode_read(adapter,
> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +					    &data, sizeof(data));

Why do we still need the retry here with patch 3/3?

> +		if (!ret || !retry)
> +			break;
> +		usleep_range(500, 1000);
> +	}
> +
>  	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/3] drm: Add retries for lspcon status check

[Sharma, Shashank]

Regards

Shashank


On 8/22/2017 8:24 PM, Imre Deak wrote:
> On Tue, Aug 22, 2017 at 08:23:49PM +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.
>>
>> V2: addressed review comments from Imre, and added r-b
>>      - use int instead of u8 for counter
>>      - use for loop instead of do...while();
>> V3: Rebase
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@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/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..fc8c7ac 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;
>> +	int retry;
>>   	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));
>> +	for (retry = 5; ; retry--) {
>> +		ret = drm_dp_dual_mode_read(adapter,
>> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> +					    &data, sizeof(data));
> Why do we still need the retry here with patch 3/3?
The only reason I can think of would be I am paranoid :) ? I was 
thinking of going with this one first and see
the CI results, and then later remove and optimize. But I guess you are 
right, this would be too many retries
here, and I should do it other way around.

- Shashank
>> +		if (!ret || !retry)
>> +			break;
>> +		usleep_range(500, 1000);
>> +	}
>> +
>>   	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

✓ Fi.CI.BAT: success for Add retries for DP dual mode devices

[Patchwork]

== Series Details ==

Series: Add retries for DP dual mode devices
URL   : https://patchwork.freedesktop.org/series/29146/
State : success

== Summary ==

Series 29146v1 Add retries for DP dual mode devices
https://patchwork.freedesktop.org/api/1.0/series/29146/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-kbl-7260u) fdo#102359
Test gem_sync:
        Subgroup basic-store-all:
                incomplete -> PASS       (fi-kbl-7260u) fdo#102360

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#102359 https://bugs.freedesktop.org/show_bug.cgi?id=102359
fdo#102360 https://bugs.freedesktop.org/show_bug.cgi?id=102360

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:454s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:445s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:555s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:252s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:524s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:524s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:517s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:446s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:610s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:452s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:425s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:423s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:562s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:524s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:607s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:627s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:525s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:471s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:484s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:496s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:445s
fi-skl-x1585l    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:513s
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:411s

b9294399405b59609f76fe52af2d3e61f53e9f68 drm-tip: 2017y-08m-22d-13h-44m-53s UTC integration manifest
f07a7b9a2e43 drm: Add retries for dp dual mode read
718a10c7d844 drm/i915: Don't give up waiting on INVALID_MODE
198105976bb9 drm: Add retries for lspcon status check

== Logs ==

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

Re: [PATCH 1/3] drm: Add retries for lspcon status check

[Jani Nikula]

On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma@intel.com> 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.
>
> V2: addressed review comments from Imre, and added r-b
>     - use int instead of u8 for counter
>     - use for loop instead of do...while();
> V3: Rebase
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@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/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..fc8c7ac 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;
> +	int retry;
>  	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));
> +	for (retry = 5; ; retry--) {
> +		ret = drm_dp_dual_mode_read(adapter,
> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> +					    &data, sizeof(data));
> +		if (!ret || !retry)
> +			break;
> +		usleep_range(500, 1000);
> +	}

Sorry, but that looks like a programming quiz to me. At most how many
time does drm_dp_dual_mode_read() get called?

With this, for example, the C programmer's spine tells you "six times"
for the paradigm for loop before you even really stop to think about it:

	for (try = 0; try < 6; try++) {
        	if (try)
                	usleep_range(500, 1000);
                ret = drm_dp_dual_mode_read();
                if (!ret)
                	break;
        }

BR,
Jani.


PS. I'm left wondering if "retry = 5" was there to emphasize that
there's one try and five *retries*.



> +
>  	if (ret < 0) {
> -		DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
> +		DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>  		return -EFAULT;
>  	}

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

Re: [PATCH 1/3] drm: Add retries for lspcon status check

[Sharma, Shashank]

Regards

Shashank


On 8/22/2017 8:57 PM, Jani Nikula wrote:
> On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma@intel.com> 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.
>>
>> V2: addressed review comments from Imre, and added r-b
>>      - use int instead of u8 for counter
>>      - use for loop instead of do...while();
>> V3: Rebase
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@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/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..fc8c7ac 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;
>> +	int retry;
>>   	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));
>> +	for (retry = 5; ; retry--) {
>> +		ret = drm_dp_dual_mode_read(adapter,
>> +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> +					    &data, sizeof(data));
>> +		if (!ret || !retry)
>> +			break;
>> +		usleep_range(500, 1000);
>> +	}
> Sorry, but that looks like a programming quiz to me. At most how many
> time does drm_dp_dual_mode_read() get called?
Yes, you are correct, the loop gets called 6 times, in the initial 
version of this code, I had the condition
to be if (!ret || !--retry) break; which was serving the purpose of 6 
calls to drm_dp_dual_mode_read()
but only 5 delays. But later I moved some code, and messed with the logic.

I will modify this.
>
> With this, for example, the C programmer's spine tells you "six times"
> for the paradigm for loop before you even really stop to think about it:
>
> 	for (try = 0; try < 6; try++) {
>          	if (try)
>                  	usleep_range(500, 1000);
>                  ret = drm_dp_dual_mode_read();
>                  if (!ret)
>                  	break;
>          }
>
> BR,
> Jani.
>
>
> PS. I'm left wondering if "retry = 5" was there to emphasize that
> there's one try and five *retries*.
Actually, the aim was to do 6 call to drm_dp_dual_mode_read() but add 
only 5 delays, as I wanted
the first read also in the loop. But as you rightly mention, the latest 
loop was not good enough.

- Shashank
>
>
>> +
>>   	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