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@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..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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
