| Message ID | 1503581023-19748-1-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }
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(-)