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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Khaled Almahallawy <khaled.almahallawy@intel.com>,
	dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
Cc: seanpaul@chromium.org
Subject: Re: [Intel-gfx] [RFC PATCH 1/2] drm/dp: Make number of AUX retries configurable by display drivers.
Date: Wed, 10 Feb 2021 09:55:40 +0100	[thread overview]
Message-ID: <9c290abd-7770-e386-76a4-c821ab33752b@suse.de> (raw)
In-Reply-To: <20210210083338.100068-1-khaled.almahallawy@intel.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 2695 bytes --]

Hi

Am 10.02.21 um 09:33 schrieb Khaled Almahallawy:
> 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.
> 
> 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@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*/

The original comment was helpful; this one isn't.

Besides that, what problem does this patchset address? Too much probing? 
If I connect a Dell monitor to an Intel card, how often does it have to 
probe?

Best regards
Thomas

> +	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);
>   	/**
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx