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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@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.

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

I guess 1) and 3) would be the interesting options to try.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx