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

[Sharma, Shashank]

Regards

Shashank


On 8/24/2017 7:38 PM, Ville Syrjälä wrote:
> On Thu, Aug 24, 2017 at 06:53:43PM +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)
>> V3: Addressed review comments from Imre (break on partial read too)
>>
>> 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..8263660 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)
>> +			break;
>> +	}
> I can't say I really like this approach. It's going to slow down
> every failed DP++ dongle detection by several millieconds. I'd prefer
> that we pay that cost only for LSPCON and not for every HDMI connector.
I had seen one of the Parade HDMI type2 dongle also, on one of the 
Lenovo boards in
need of retries. I thought this might help for those devices too. 
Wouldn't it be better
to try again before calling a modeset failure ?

I guess you are worried about the detection delays, during the hot 
unplug time, is it ?

Would it make sense to pass the previous connector state here, and do 
retries only if
previous state was disconnected (and we know this might be for connect ? )
> Alternatives I discussed with Imre would be:
> 1) Get the i2c core to do the retries for us, but just for LSPCON
>     This would require checking which error we get exactly in this failure
>     case and checking of the i2c core will perform retries for tha
>     particular error. If the i2c core doesn't do what we need then we
>     can't use this approach
> 2) Raise the abstraction level on the DP++ helper and have it do the
>     retries only for LSPCON. This seems like a lot of work for little
>     gain
> 3) Just handle the retries on a higher level in LSPCON specific code
We can do this (3), in fact, we had done this only in the previous patch 
sets, but then
later we realized that we have to do this from every call to 
dual_mode_read() like
probe, resume, etc, but still its doable if you think we should not 
touch the dp_dual_mode
layer, and content this in LSPCON layer.
> I guess 1) and 3) would be the interesting options to try.
Agree.

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