Re: [PATCH 1/2] drm: add retries for lspcon status check

["Sharma, Shashank" <shashank.sharma@intel.com>]

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@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@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 ?
>>   	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 :) ?
- Shashank
>
> With the above optionally changed:
> Reviewed-by: Imre Deak <imre.deak@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
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx