(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@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); > > > /** -- 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! _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
