[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 13:03 -0500, Lyude Paul wrote:
> 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

i915 or others may benefit from controlling AUX retries to optimize and
minimize timing spent on the aux transactions.
 
drm_dpcd_access handles multiple aux retries cases the same way (retry
32 times at worst case). The 4 cases are:
1- *AUX receive or IO error*. I believe it is up to specific
implementation/HW once it detects a receive error to retry based on
their internal understanding using the timeout appropriate to the HW
capabilities.
 
2- *AUX Timeout* As the driver follows the specs for the recommended
timeout timer as defined in section  (2.11.2 AUX Transaction
Response/Reply Timeouts), the driver has more intelligence to know how
many retries it needs. This is more useful in case of DP-ALT if some
issues happen in Type-C stack that cause AUX timeout (e.g. USB3 dock
detected as high speed (USB2) causing SBU/AUX lines to be disabled).
Also the disconnect of MST hub/docks is dependent how fast a userspace
disconnect all MST connectors.  Not all user spaces do it as fast like
Chrome (ubuntu is an example): 
https://chromium-review.googlesource.com/c/chromium/src/+/2512550/  

3- *AUX_NACK* DP spec mentions retry 3 times for NACK(2.16.1.3 GTC Lock
Acquisition). However, sometimes we really don’t need any retry for
NACK, if DPRX replied AUX_NACK for DPCD that it doesn’t support (e.g.
reading LTTPR Capability and ID Field at DPCD F0000h ~ F0007).

4- *AUX_DEFER* The specs stated we may retry 7 times on AUX_DEFER
(3.5.1.2.3 LANEx_CHANNEL_EQ_DONE Sequence) and may terminate LT. Also
with the increased usage of USB4, TBT/Type-C Docks/Displays, and active
cables, the use of LTTPR becomes common which adds more challenge
especially if we have multiple LTTPRs and all operate in non-LTTPR
mode. In this case all LTTPRs will reply AUX_DEFER in 300us if it did
not receive any aux response from other LTTPRs and DPRX. The SCR states
we need to retry 7 times and not more than 50ms (DP v2.0 SCR on 8b/10b
DPTX and LTTPR Requirements Update to Section 3.6)

In addition I believe this function is not correct in treating
AUX_DEFER and AUX_NACK as -EIO. Especially for AUX_DEFER, it is a valid
1 byte response that can provide a valuable debugging information
especially in the case of on-board LTTPR where there is no way to
capture this AUX response between the SoC and LTTPR unless you solder
two wires on AUX_P and AUX_N lines and use i2c/aux analyzer to decode.
At least we should provide the same debug information as we do in
drm_dp_i2c_do_msg by printing AUX_DEFER and AUX_NACK.

Thank you for your feedback and review.

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