[Intel-gfx] [PATCH v2 1/2] drm: Add retries for dp dual mode read

[Imre Deak]

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 at linux.intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > Signed-off-by: Shashank Sharma <shashank.sharma at 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 at 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
> > > 
>