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@xxxxxxxxx> > > --- > > 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); > > /** _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
