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

[Sharma, Shashank]

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