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.
> 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
