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