Regards
Shashank
On 8/22/2017 8:57 PM, Jani Nikula wrote:
> On Tue, 22 Aug 2017, Shashank Sharma <shashank.sharma at intel.com> 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.
>>
>> V2: addressed review comments from Imre, and added r-b
>> - use int instead of u8 for counter
>> - use for loop instead of do...while();
>> V3: Rebase
>>
>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: Imre Deak <imre.deak at intel.com>
>>
>> Reviewed-by: 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..fc8c7ac 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;
>> + int retry;
>> 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));
>> + for (retry = 5; ; retry--) {
>> + ret = drm_dp_dual_mode_read(adapter,
>> + DP_DUAL_MODE_LSPCON_CURRENT_MODE,
>> + &data, sizeof(data));
>> + if (!ret || !retry)
>> + break;
>> + usleep_range(500, 1000);
>> + }
> Sorry, but that looks like a programming quiz to me. At most how many
> time does drm_dp_dual_mode_read() get called?
Yes, you are correct, the loop gets called 6 times, in the initial
version of this code, I had the condition
to be if (!ret || !--retry) break; which was serving the purpose of 6
calls to drm_dp_dual_mode_read()
but only 5 delays. But later I moved some code, and messed with the logic.
I will modify this.
>
> With this, for example, the C programmer's spine tells you "six times"
> for the paradigm for loop before you even really stop to think about it:
>
> for (try = 0; try < 6; try++) {
> if (try)
> usleep_range(500, 1000);
> ret = drm_dp_dual_mode_read();
> if (!ret)
> break;
> }
>
> BR,
> Jani.
>
>
> PS. I'm left wondering if "retry = 5" was there to emphasize that
> there's one try and five *retries*.
Actually, the aim was to do 6 call to drm_dp_dual_mode_read() but add
only 5 delays, as I wanted
the first read also in the loop. But as you rightly mention, the latest
loop was not good enough.
- Shashank
>
>
>> +
>> if (ret < 0) {
>> - DRM_ERROR("LSPCON read(0x80, 0x41) failed\n");
>> + DRM_DEBUG_KMS("LSPCON read(0x80, 0x41) failed\n");
>> return -EFAULT;
>> }