[PATCH 0/2] Add retries for dp dual mode reads

[PATCH 0/2] Add retries for dp dual mode reads

[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, and adjusts debug levels for failure messages.

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

 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
 drivers/gpu/drm/i915/intel_lspcon.c       |  9 ++++-----
 2 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.7.4

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

[PATCH 1/2] 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 and respond.
- changes one error log's level from ERROR->DEBUG as we want
  to call it an error only after all the retries are exhausted.

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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 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..13f67a36 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; ; ) {
+		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))
@@ -420,7 +427,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
 	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
 				    &data, sizeof(data));
 	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

✓ Fi.CI.BAT: success for Add retries for dp dual mode reads

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads
URL   : https://patchwork.freedesktop.org/series/29155/
State : success

== Summary ==

Series 29155v1 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7260u) fdo#102294
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781

fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:459s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:447s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:360s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:559s
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:532s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:524s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:439s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:618s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:448s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:427s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:556s
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:523s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:482s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:605s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:628s
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:474s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:488s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:493s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:451s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:490s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:555s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:417s

017fec5c2e57672a8c2a350376070e6c6a5ae950 drm-tip: 2017y-08m-22d-16h-23m-11s UTC integration manifest
78c3d541a917 drm/i915: Don't give up waiting on INVALID_MODE
fffb553d68b6 drm: Add retries for dp dual mode read

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5463/
_______________________________________________
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 dp dual mode read

[Jani Nikula]

On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma@intel.com> wrote:
> 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 and respond.
> - changes one error log's level from ERROR->DEBUG as we want
>   to call it an error only after all the retries are exhausted.
>
> 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 | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 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..13f67a36 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; ; ) {

As I said, I think the paradigm for loop (i = 0; i < N; i++) is always
more obvious and faster for humans to parse.

BR,
Jani.

> +		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))
> @@ -420,7 +427,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>  				    &data, sizeof(data));
>  	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

[PATCH v2 1/2] 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 and respond.
- changes one error log's level from ERROR->DEBUG as we want
  to call it an error only after all the retries are exhausted.

V2: Addressed review comments from Jani (for loop for retry)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 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..09bf962 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
 		},
 	};
 	int ret;
+	int retry;
+
+	for (retry = 0; retry < 6; retry++) {
+		if (retry)
+			usleep_range(500, 1000);
+		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+		if (ret > 0)
+			break;
+	}
 
-	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
 	if (ret < 0)
 		return ret;
 	if (ret != ARRAY_SIZE(msgs))
@@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
 	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
 				    &data, sizeof(data));
 	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 reads (rev2)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev2)
URL   : https://patchwork.freedesktop.org/series/29155/
State : success

== Summary ==

Series 29155v2 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/2/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:453s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:442s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:366s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:563s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:253s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:533s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:532s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:530s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:439s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:613s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:450s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:428s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:427s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:559s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:481s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:474s
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:639s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:479s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:480s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:494s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:449s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:490s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:554s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:410s
fi-pnv-d510 failed to connect after reboot

ebd0ddf26a92d346469b3eb6ca9917793e5542b9 drm-tip: 2017y-08m-23d-09h-28m-47s UTC integration manifest
444e42d7e599 drm/i915: Don't give up waiting on INVALID_MODE
89db7936ed6a drm: Add retries for dp dual mode read

== Logs ==

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

Re: [PATCH v2 1/2] drm: Add retries for dp dual mode read

[Imre Deak]

On Wed, Aug 23, 2017 at 06:12:51PM +0530, Shashank Sharma wrote:
> 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 and respond.
> - changes one error log's level from ERROR->DEBUG as we want
>   to call it an error only after all the retries are exhausted.
> 
> V2: Addressed review comments from Jani (for loop for retry)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 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..09bf962 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>  		},
>  	};
>  	int ret;
> +	int retry;
> +
> +	for (retry = 0; retry < 6; retry++) {
> +		if (retry)
> +			usleep_range(500, 1000);
> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +		if (ret > 0)

I think the ret == 0 case should be handled with the rest of
partial-read cases (which we check against further down). So the above
should be
		if (ret >= 0)

With this fixed it looks ok to me, so fwiw:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Note that ideally we wouldn't retry for error returns like
-ENOTSUPPORTED, -ENOMEM, -E2BIG, but it's not a big deal to do that
either. One possibility to solving that and also making this workaround
more generic is to reuse the existing retry logic in __i2c_transfer() by
making it retry in all relevant error cases (timeout/IO) and setting
i2c_adapter::retries for the LSPCON adaptor. But that can be done as a
follow-up.

> +			break;
> +	}
>  
> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>  	if (ret < 0)
>  		return ret;
>  	if (ret != ARRAY_SIZE(msgs))
> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>  				    &data, sizeof(data));
>  	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 v2 1/2] drm: Add retries for dp dual mode read

[Sharma, Shashank]

Regards

Shashank


On 8/24/2017 5:19 PM, Imre Deak wrote:
> On Wed, Aug 23, 2017 at 06:12:51PM +0530, Shashank Sharma wrote:
>>  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 and respond.
>> - changes one error log's level from ERROR->DEBUG as we want
>>    to call it an error only after all the retries are exhausted.
>>
>> V2: Addressed review comments from Jani (for loop for retry)
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 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..09bf962 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>>   		},
>>   	};
>>   	int ret;
>> +	int retry;
>> +
>> +	for (retry = 0; retry < 6; retry++) {
>> +		if (retry)
>> +			usleep_range(500, 1000);
>> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>> +		if (ret > 0)
> I think the ret == 0 case should be handled with the rest of
> partial-read cases (which we check against further down). So the above
> should be
> 		if (ret >= 0)
My thinking for this case was, that, partial error cases also should be 
given retries, and try again.
IN this way, there is a possibility that in the next attempt there would 
be proper read.
but if after the retries too, we see partial read, then we should call 
it an error.

Does it sound like a good thing to do ?
- Shashank
> With this fixed it looks ok to me, so fwiw:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
> Note that ideally we wouldn't retry for error returns like
> -ENOTSUPPORTED, -ENOMEM, -E2BIG, but it's not a big deal to do that
> either. One possibility to solving that and also making this workaround
> more generic is to reuse the existing retry logic in __i2c_transfer() by
> making it retry in all relevant error cases (timeout/IO) and setting
> i2c_adapter::retries for the LSPCON adaptor. But that can be done as a
> follow-up.
>
>> +			break;
>> +	}
>>   
>> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>>   	if (ret < 0)
>>   		return ret;
>>   	if (ret != ARRAY_SIZE(msgs))
>> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>   	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>   				    &data, sizeof(data));
>>   	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 v2 1/2] drm: Add retries for dp dual mode read

[Imre Deak]

On Thu, Aug 24, 2017 at 05:40:32PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 8/24/2017 5:19 PM, Imre Deak wrote:
> > On Wed, Aug 23, 2017 at 06:12:51PM +0530, Shashank Sharma wrote:
> > >  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 and respond.
> > > - changes one error log's level from ERROR->DEBUG as we want
> > >    to call it an error only after all the retries are exhausted.
> > > 
> > > V2: Addressed review comments from Jani (for loop for retry)
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
> > >   1 file changed, 10 insertions(+), 2 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..09bf962 100644
> > > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> > >   		},
> > >   	};
> > >   	int ret;
> > > +	int retry;
> > > +
> > > +	for (retry = 0; retry < 6; retry++) {
> > > +		if (retry)
> > > +			usleep_range(500, 1000);
> > > +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > > +		if (ret > 0)
> > I think the ret == 0 case should be handled with the rest of
> > partial-read cases (which we check against further down). So the above
> > should be
> > 		if (ret >= 0)
> My thinking for this case was, that, partial error cases also should be
> given retries, and try again.

Well, that would've been then

		if (ret == ARRAY_SIZE(msgs))
			break;

then, not the way you have written no?

But I don't think we shold do that. We only retry here to wait for the
HW to wake up. Any other error afterwards is really unexpected. This is
also how the __i2c_transfer() retry logic works.

> IN this way, there is a possibility that in the next attempt there would be
> proper read.
> but if after the retries too, we see partial read, then we should call it an
> error.
> 
> Does it sound like a good thing to do ?
> - Shashank
> > With this fixed it looks ok to me, so fwiw:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > Note that ideally we wouldn't retry for error returns like
> > -ENOTSUPPORTED, -ENOMEM, -E2BIG, but it's not a big deal to do that
> > either. One possibility to solving that and also making this workaround
> > more generic is to reuse the existing retry logic in __i2c_transfer() by
> > making it retry in all relevant error cases (timeout/IO) and setting
> > i2c_adapter::retries for the LSPCON adaptor. But that can be done as a
> > follow-up.
> > 
> > > +			break;
> > > +	}
> > > -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > >   	if (ret < 0)
> > >   		return ret;
> > >   	if (ret != ARRAY_SIZE(msgs))
> > > @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > >   				    &data, sizeof(data));
> > >   	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 v2 1/2] drm: Add retries for dp dual mode read

[Sharma, Shashank]

Regards

Shashank


On 8/24/2017 5:49 PM, Imre Deak wrote:
> On Thu, Aug 24, 2017 at 05:40:32PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 8/24/2017 5:19 PM, Imre Deak wrote:
>>> On Wed, Aug 23, 2017 at 06:12:51PM +0530, Shashank Sharma wrote:
>>>>   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 and respond.
>>>> - changes one error log's level from ERROR->DEBUG as we want
>>>>     to call it an error only after all the retries are exhausted.
>>>>
>>>> V2: Addressed review comments from Jani (for loop for retry)
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>>>>    1 file changed, 10 insertions(+), 2 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..09bf962 100644
>>>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>>>> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>>>>    		},
>>>>    	};
>>>>    	int ret;
>>>> +	int retry;
>>>> +
>>>> +	for (retry = 0; retry < 6; retry++) {
>>>> +		if (retry)
>>>> +			usleep_range(500, 1000);
>>>> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>>>> +		if (ret > 0)
>>> I think the ret == 0 case should be handled with the rest of
>>> partial-read cases (which we check against further down). So the above
>>> should be
>>> 		if (ret >= 0)
>> My thinking for this case was, that, partial error cases also should be
>> given retries, and try again.
> Well, that would've been then
>
> 		if (ret == ARRAY_SIZE(msgs))
> 			break;
>
> then, not the way you have written no?
>
> But I don't think we shold do that. We only retry here to wait for the
> HW to wake up. Any other error afterwards is really unexpected. This is
> also how the __i2c_transfer() retry logic works.
Ok then, I will add this part.
Shashank
>> IN this way, there is a possibility that in the next attempt there would be
>> proper read.
>> but if after the retries too, we see partial read, then we should call it an
>> error.
>>
>> Does it sound like a good thing to do ?
>> - Shashank
>>> With this fixed it looks ok to me, so fwiw:
>>> Reviewed-by: Imre Deak <imre.deak@intel.com>
>>>
>>> Note that ideally we wouldn't retry for error returns like
>>> -ENOTSUPPORTED, -ENOMEM, -E2BIG, but it's not a big deal to do that
>>> either. One possibility to solving that and also making this workaround
>>> more generic is to reuse the existing retry logic in __i2c_transfer() by
>>> making it retry in all relevant error cases (timeout/IO) and setting
>>> i2c_adapter::retries for the LSPCON adaptor. But that can be done as a
>>> follow-up.
>>>
>>>> +			break;
>>>> +	}
>>>> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    	if (ret != ARRAY_SIZE(msgs))
>>>> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>>>    	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>>>    				    &data, sizeof(data));
>>>>    	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 v3 1/2] 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 and respond.
- changes one error log's level from ERROR->DEBUG as we want
  to call it an error only after all the retries are exhausted.

V2: Addressed review comments from Jani (for loop for retry)
V3: Addressed review comments from Imre (break on partial read too)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 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..8263660 100644
--- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
@@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
 		},
 	};
 	int ret;
+	int retry;
+
+	for (retry = 0; retry < 6; retry++) {
+		if (retry)
+			usleep_range(500, 1000);
+		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+		if (ret >= 0)
+			break;
+	}
 
-	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
 	if (ret < 0)
 		return ret;
 	if (ret != ARRAY_SIZE(msgs))
@@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
 	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
 				    &data, sizeof(data));
 	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 v3 1/2] drm: Add retries for dp dual mode read

[Ville Syrjälä]

On Thu, Aug 24, 2017 at 06:53:43PM +0530, Shashank Sharma wrote:
> >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 and respond.
> - changes one error log's level from ERROR->DEBUG as we want
>   to call it an error only after all the retries are exhausted.
> 
> V2: Addressed review comments from Jani (for loop for retry)
> V3: Addressed review comments from Imre (break on partial read too)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 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..8263660 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>  		},
>  	};
>  	int ret;
> +	int retry;
> +
> +	for (retry = 0; retry < 6; retry++) {
> +		if (retry)
> +			usleep_range(500, 1000);
> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +		if (ret >= 0)
> +			break;
> +	}

I can't say I really like this approach. It's going to slow down
every failed DP++ dongle detection by several millieconds. I'd prefer
that we pay that cost only for LSPCON and not for every HDMI connector.

Alternatives I discussed with Imre would be:
1) Get the i2c core to do the retries for us, but just for LSPCON
   This would require checking which error we get exactly in this failure
   case and checking of the i2c core will perform retries for tha
   particular error. If the i2c core doesn't do what we need then we
   can't use this approach
2) Raise the abstraction level on the DP++ helper and have it do the
   retries only for LSPCON. This seems like a lot of work for little
   gain
3) Just handle the retries on a higher level in LSPCON specific code

I guess 1) and 3) would be the interesting options to try.

>  
> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>  	if (ret < 0)
>  		return ret;
>  	if (ret != ARRAY_SIZE(msgs))
> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>  				    &data, sizeof(data));
>  	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Add retries for dp dual mode reads (rev3)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev3)
URL   : https://patchwork.freedesktop.org/series/29155/
State : failure

== Summary ==

Series 29155v3 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/3/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> INCOMPLETE (fi-kbl-7560u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                fail       -> PASS       (fi-skl-6700k) fdo#100367

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

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:456s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:442s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:363s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:552s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:254s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:532s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:540s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:528s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:434s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:614s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:451s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:549s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:480s
fi-kbl-7560u     total:209  pass:205  dwarn:0   dfail:0   fail:0   skip:3  
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:527s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:477s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:492s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:454s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:562s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:408s

3adc9e3cacefe82619fc56ec78a0ca3a67a4b00c drm-tip: 2017y-08m-23d-21h-35m-35s UTC integration manifest
2b32ba499e0a drm/i915: Don't give up waiting on INVALID_MODE
f804ebb7766d drm: Add retries for dp dual mode read

== Logs ==

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

Re: [PATCH v3 1/2] drm: Add retries for dp dual mode read

[Manasi Navare]

On Thu, Aug 24, 2017 at 06:53:43PM +0530, Shashank Sharma wrote:
> 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 and respond.

How was this delay and number of retries determined? Is there a specific
delay or retry number mentioned in the spec for those LSPCON boards?
If not then how do we know that it is optimised for all LSPCON boards?

Manasi

> - changes one error log's level from ERROR->DEBUG as we want
>   to call it an error only after all the retries are exhausted.
> 
> V2: Addressed review comments from Jani (for loop for retry)
> V3: Addressed review comments from Imre (break on partial read too)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 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..8263660 100644
> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>  		},
>  	};
>  	int ret;
> +	int retry;
> +
> +	for (retry = 0; retry < 6; retry++) {
> +		if (retry)
> +			usleep_range(500, 1000);
> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +		if (ret >= 0)
> +			break;
> +	}
>  
> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>  	if (ret < 0)
>  		return ret;
>  	if (ret != ARRAY_SIZE(msgs))
> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>  				    &data, sizeof(data));
>  	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Add retries for dp dual mode reads (rev3)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev3)
URL   : https://patchwork.freedesktop.org/series/29155/
State : failure

== Summary ==

Series 29155v3 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/3/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> DMESG-WARN (fi-pnv-d510) fdo#101600
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-4770)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-4770)

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:459s
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:361s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:566s
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:534s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:521s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:448s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:622s
fi-hsw-4770      total:279  pass:261  dwarn:0   dfail:0   fail:2   skip:16  time:467s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:430s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:558s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:472s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:482s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:629s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:522s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:483s
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:490s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:456s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:559s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:411s

068cd5b2db68ecd9f52d4d7c66e6e9e71d33ab0d drm-tip: 2017y-08m-24d-22h-49m-38s UTC integration manifest
2741909feb38 drm/i915: Don't give up waiting on INVALID_MODE
1c6a878f5265 drm: Add retries for dp dual mode read

== Logs ==

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

✗ Fi.CI.BAT: failure for Add retries for dp dual mode reads (rev3)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev3)
URL   : https://patchwork.freedesktop.org/series/29155/
State : failure

== Summary ==

Series 29155v3 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/3/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> DMESG-WARN (fi-pnv-d510) fdo#101600
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215 +1
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-4770)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-4770)
Test prime_vgem:
        Subgroup basic-fence-flip:
                incomplete -> SKIP       (fi-kbl-7560u)

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:458s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:443s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:360s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:575s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:253s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:523s
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:516s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:443s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:613s
fi-hsw-4770      total:279  pass:261  dwarn:0   dfail:0   fail:2   skip:16  time:469s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:431s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:547s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:484s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:601s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:632s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:522s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:476s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:486s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:490s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:443s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:557s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:407s
fi-ilk-650 failed to connect after reboot

068cd5b2db68ecd9f52d4d7c66e6e9e71d33ab0d drm-tip: 2017y-08m-24d-22h-49m-38s UTC integration manifest
b914d659b8aa drm/i915: Don't give up waiting on INVALID_MODE
9891e9191746 drm: Add retries for dp dual mode read

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5490/
_______________________________________________
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 reads (rev3)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev3)
URL   : https://patchwork.freedesktop.org/series/29155/
State : success

== Summary ==

Series 29155v3 Add retries for dp dual mode reads
https://patchwork.freedesktop.org/api/1.0/series/29155/revisions/3/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> DMESG-WARN (fi-pnv-d510) fdo#101600
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781
Test prime_vgem:
        Subgroup basic-fence-flip:
                incomplete -> SKIP       (fi-kbl-7560u)

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:464s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:446s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:360s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:564s
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:516s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:532s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:522s
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:615s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:450s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:430s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:425s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:551s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:606s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:626s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:520s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:479s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:489s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:495s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:452s
fi-skl-x1585l    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:509s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:561s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:414s

068cd5b2db68ecd9f52d4d7c66e6e9e71d33ab0d drm-tip: 2017y-08m-24d-22h-49m-38s UTC integration manifest
eb69a22cba81 drm/i915: Don't give up waiting on INVALID_MODE
19d161de2e01 drm: Add retries for dp dual mode read

== Logs ==

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

✗ Fi.CI.IGT: failure for Add retries for dp dual mode reads (rev3)

[Patchwork]

== Series Details ==

Series: Add retries for dp dual mode reads (rev3)
URL   : https://patchwork.freedesktop.org/series/29155/
State : failure

== Summary ==

Test kms_flip:
        Subgroup flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw)
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-hsw) fdo#100047

fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-hsw        total:2230 pass:1229 dwarn:0   dfail:0   fail:19  skip:981 time:9744s

== Logs ==

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

Re: [PATCH v3 1/2] drm: Add retries for dp dual mode read

[Sharma, Shashank]

Regards

Shashank


On 8/24/2017 7:38 PM, Ville Syrjälä wrote:
> On Thu, Aug 24, 2017 at 06:53:43PM +0530, Shashank Sharma wrote:
>> >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 and respond.
>> - changes one error log's level from ERROR->DEBUG as we want
>>    to call it an error only after all the retries are exhausted.
>>
>> V2: Addressed review comments from Jani (for loop for retry)
>> V3: Addressed review comments from Imre (break on partial read too)
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 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..8263660 100644
>> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
>> @@ -75,8 +75,16 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>>   		},
>>   	};
>>   	int ret;
>> +	int retry;
>> +
>> +	for (retry = 0; retry < 6; retry++) {
>> +		if (retry)
>> +			usleep_range(500, 1000);
>> +		ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>> +		if (ret >= 0)
>> +			break;
>> +	}
> I can't say I really like this approach. It's going to slow down
> every failed DP++ dongle detection by several millieconds. I'd prefer
> that we pay that cost only for LSPCON and not for every HDMI connector.
I had seen one of the Parade HDMI type2 dongle also, on one of the 
Lenovo boards in
need of retries. I thought this might help for those devices too. 
Wouldn't it be better
to try again before calling a modeset failure ?

I guess you are worried about the detection delays, during the hot 
unplug time, is it ?

Would it make sense to pass the previous connector state here, and do 
retries only if
previous state was disconnected (and we know this might be for connect ? )
> Alternatives I discussed with Imre would be:
> 1) Get the i2c core to do the retries for us, but just for LSPCON
>     This would require checking which error we get exactly in this failure
>     case and checking of the i2c core will perform retries for tha
>     particular error. If the i2c core doesn't do what we need then we
>     can't use this approach
> 2) Raise the abstraction level on the DP++ helper and have it do the
>     retries only for LSPCON. This seems like a lot of work for little
>     gain
> 3) Just handle the retries on a higher level in LSPCON specific code
We can do this (3), in fact, we had done this only in the previous patch 
sets, but then
later we realized that we have to do this from every call to 
dual_mode_read() like
probe, resume, etc, but still its doable if you think we should not 
touch the dp_dual_mode
layer, and content this in LSPCON layer.
> I guess 1) and 3) would be the interesting options to try.
Agree.

- Shashank
>
>>   
>> -	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>>   	if (ret < 0)
>>   		return ret;
>>   	if (ret != ARRAY_SIZE(msgs))
>> @@ -420,7 +428,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
>>   	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>>   				    &data, sizeof(data));
>>   	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