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@xxxxxxxxxxxxxxx>
Cc: Imre Deak <imre.deak@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
---
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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
