[Intel-gfx] [PATCH 1/2] drm: add retries for lspcon status check

[Imre Deak]

On Wed, Aug 16, 2017 at 09:18:58PM +0530, Sharma, Shashank wrote:
> Thanks for the review, Imre.
> 
> My comments, inline.
> 
> Regards
> Shashank
> On 8/16/2017 7:35 PM, Imre Deak wrote:
> > On Fri, Aug 11, 2017 at 06:58:26PM +0530, Shashank Sharma wrote:
> > > It's an observation during some CI tests that few LSPCON chips
> > > respond slow while system is under load, and need some delay
> > > while reading current mode status using i2c-over-aux channel.
> > > 
> > > This patch:
> > > - Adds few retries and delays before declaring a read
> > >    failure from LSPCON hardware.
> > > - Changes the debug level of the print from ERROR->DEBUG
> > >    whereas another patch in I915 will add an ERROR message
> > >    from the caller when we have timed out all our limits.
> > Hm yea, I was wondering if this is the same issue we saw earlier due to
> > HPD not being asserted. But looking at the logs it's not the case. After
> > HPD is asserted it really takes this much time (~36ms) for the adaptor
> > to respond. This is against the DP 1.4 spec which specifies a 20ms
> > maximum wake up time, but what can you do.
> > 
> > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> > > ---
> > >   drivers/gpu/drm/drm_dp_dual_mode_helper.c | 14 +++++++++++---
> > >   1 file changed, 11 insertions(+), 3 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..c63eac8 100644
> > > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c
> > > @@ -409,6 +409,7 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   			enum drm_lspcon_mode *mode)
> > >   {
> > >   	u8 data;
> > > +	u8 retry = 5;
> > Nitpick: no reason for a specific type so just int?
> Actually, was trying to save 3 bytes, as I knew max value for retry would be
> 5 < 255, but might be too much optimization ?

Yes, imo it's odd to use u8 for simple loop counter. The compiler may
even generate some extra code to zero extend or truncate the result.

> > >   	int ret = 0;
> > >   	if (!mode) {
> > > @@ -417,10 +418,17 @@ int drm_lspcon_get_mode(struct i2c_adapter *adapter,
> > >   	}
> > >   	/* Read Status: i2c over aux */
> > > -	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > -				    &data, sizeof(data));
> > > +	do {
> > Nitpick: a for loop whenever possible is generally clearer.
> Sorry, I dint understand this comment, little more help required :) ?

for (retry = 0; retry < 5; retry++) ...

would be clearer imo.

--Imre

> - Shashank
> > 
> > With the above optionally changed:
> > Reviewed-by: Imre Deak <imre.deak at intel.com>
> > 
> > > +		ret = drm_dp_dual_mode_read(adapter,
> > > +					    DP_DUAL_MODE_LSPCON_CURRENT_MODE,
> > > +					    &data, sizeof(data));
> > > +		if (!ret || !retry--)
> > > +			break;
> > > +		usleep_range(500, 1000);
> > > +	} while (1);
> > > +
> > >   	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
> > > 
>