[Intel-gfx] [RFC PATCH 1/2] drm/dp: Make number of AUX retries configurable by display drivers.

[Lyude Paul]

On Wed, 2021-02-10 at 00:33 -0800, Khaled Almahallawy wrote:
> The number of AUX retries specified in the DP specs is 7. Currently, to make
> Dell 4k monitors happier, the number of retries are 32.
> i915 also retries 5 times (intel_dp_aux_xfer) which means in the case of AUX
> timeout we actually retries 32 * 5 = 160 times.

Is there any good reason for i915 to actually be doing retries itself? It seems
like an easier solution here might just to be to fix i915 so it doesn't retry,
and just rely on DRM to do the retries as appropriate.

That being said though, I'm open to this if there is a legitimate reason for
i915 to be handling retries on its own

> 
> So making the number of aux retires a variable to allow for fine tuning and
> optimization of aux timing.
> 
> Signed-off-by: Khaled Almahallawy <khaled.almahallawy at intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 10 +++-------
>  include/drm/drm_dp_helper.h     |  1 +
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index eedbb48815b7..8fdf57b4a06c 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -249,13 +249,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8
> request,
>  
>         mutex_lock(&aux->hw_mutex);
>  
> -       /*
> -        * The specification doesn't give any recommendation on how often to
> -        * retry native transactions. We used to retry 7 times like for
> -        * aux i2c transactions but real world devices this wasn't
> -        * sufficient, bump to 32 which makes Dell 4k monitors happier.
> -        */
> -       for (retry = 0; retry < 32; retry++) {
> +       for (retry = 0; retry < aux->num_retries; retry++) {
>                 if (ret != 0 && ret != -ETIMEDOUT) {
>                         usleep_range(AUX_RETRY_INTERVAL,
>                                      AUX_RETRY_INTERVAL + 100);
> @@ -1744,6 +1738,8 @@ void drm_dp_aux_init(struct drm_dp_aux *aux)
>         aux->ddc.retries = 3;
>  
>         aux->ddc.lock_ops = &drm_dp_i2c_lock_ops;
> +       /*Still making the Dell 4k monitors happier*/
> +       aux->num_retries = 32;
>  }
>  EXPORT_SYMBOL(drm_dp_aux_init);
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index edffd1dcca3e..16cbfc8f5e66 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1876,6 +1876,7 @@ struct drm_dp_aux {
>         struct mutex hw_mutex;
>         struct work_struct crc_work;
>         u8 crc_count;
> +       int num_retries;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
>         /**

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!