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

[Almahallawy, Khaled]

On Wed, 2021-02-10 at 09:55 +0100, Thomas Zimmermann wrote:
> 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 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*/
> 
> The original comment was helpful; this one isn't.

Noted and I apologize for the comment. Was just copy/past original
comment. 
> 

> Besides that, what problem does this patchset address? Too much
> probing? 

The problem mainly with disconnect. When disconnecting, AUX read will
fail because of timing out. The 32 retries cause the disconnect flow
especially for Type-C/TBT Docks with MST hubs taking a long time.
Just trying to reduce this disconnect time. In addition as I noted,
i915 does retry in addition to retries by this function. 

Currently this function retry for 4 AUX situations:
AUX_NAK, AUX_DEFER, I/O error reported by driver and AUX timeout.

> If I connect a Dell monitor to an Intel card, how often does it have
> to 
> probe?

I guess it depends on how you connect (direct, behind MST hub, behind DOCK with MST). But I believe the most time consuming is the disconnect flow. 

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