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

[Lyude Paul]

(JFYI I have seen this email, but haven't gotten a chance to actually read
through it yet. I should have the time to do so tomorrow)

On Thu, 2021-02-11 at 06:56 +0000, Almahallawy, Khaled wrote:
> 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);
> > >         /**

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