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