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

[Sharma, Shashank]

Regards

Shashank


On 8/16/2017 9:42 PM, Imre Deak wrote:
> 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.
cool, so int it is.
>>>>    	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.
Sure, will do it.
> --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
>>>>