On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare@xxxxxxxxx> wrote:
> Link training failure is handled by lowering the link rate first
> until it reaches the minimum and keeping the lane count maximum
> and then lowering the lane count until it reaches minimim. These
> fallback values are saved and hotplug uevent is sent to the userspace
> after setting the connector link status property to BAD. Userspace
> should triiger another modeset on a uevent and if link status property
> is BAD. This will retrain the link at fallback values.
> This is repeated until the link is successfully trained.
>
> This has been validated to pass DP compliance.
This cover letter and the commit messages do a good job of explaining
what the patches do. However, you're lacking the crucial information of
*why* we need userspace cooperation to handle link training failures on
DP mode setting, and *why* a new connector property is a good solution
for this.
Here goes, including some alternative approaches we've considered (and
even tried):
At the time userspace does setcrtc, we've already promised the mode
would work. The promise is based on the theoretical capabilities of the
link, but it's possible we can't reach this in practice. The DP spec
describes how the link should be reduced, but we can't reduce the link
below the requirements of the mode. Black screen follows.
One idea would be to have setcrtc return a failure. However, it is my
understanding that it already should not fail as the atomic checks have
passed [citation needed]. It would also conflict with the idea of making
setcrtc asynchronous in the future, returning before the actual mode
setting and link training.
Another idea is to train the link "upfront" at hotplug time, before
pruning the mode list, so that we can do the pruning based on practical
not theoretical capabilities. However, the changes for link training are
pretty drastic, all for the sake of error handling and DP compliance,
when the most common happy day scenario is the current approach of link
training at mode setting time, using the optimal parameters for the
mode. It is also not certain all hardware could do this without the pipe
on; not even all our hardware can do this. Some of this can be solved,
but not trivially.
Both of the above ideas also fail to address link degradation *during*
operation.
So the idea presented in these patches is to do this in a way that a)
changes the current happy day scenario as little as possible, to avoid
regressions, b) can be implemented the same way by all drm drivers, c)
is still opt-in for the drivers and userspace, and opting out doesn't
regress the user experience, d) doesn't prevent drivers from
implementing better or alternate approaches, possibly without userspace
involvement. And, of course, handles all the issues presented.
The solution is to add a "link status" connector property. In the usual
happy day scenario, this is always "good". If something fails during or
after a mode set, the kernel driver can set the link status to "bad",
prune the mode list based on new information as necessary, and send a
hotplug uevent for userspace to have it re-check the valid modes through
getconnector, and try again. If the theoretical capabilities of the link
can't be reached, the mode list is trimmed based on that.
If the userspace is not aware of the property, the user experience is
the same as it currently is. If the userspace is aware of the property,
it has a chance to improve user experience. If a drm driver does not
modify the property (it stays "good"), the user experience is the same
as it currently is. A drm driver can also choose to try to handle more
of the failures in kernel, hardware not limiting, or it can choose to
involve userspace more. Up to the drivers.
The reason for adding the property is to handle link training failures,
but it is not limited to DP or link training. For example, if we
implement asynchronous setcrtc, we can use this to report any failures
in that.
Finally, while DP CTS compliance is advertized (which is great, and
could be made to work similarly for all drm drivers), this can be used
for the more important goal of improving user experience on link
training failures, by avoiding black screens.
BR,
Jani.
>
> Manasi Navare (5):
> drm: Add a new connector property for link status
> drm/i915: Set link status property for DP connector
> drm/i915: Update CRTC state if connector link status property changed
^^^^^^^^
This is really drm, not i915.
> drm/i915: Find fallback link rate/lane count
> drm/i915: Implement Link Rate fallback on Link training failure
>
> drivers/gpu/drm/drm_atomic_helper.c | 7 ++
> drivers/gpu/drm/drm_connector.c | 17 ++++
> drivers/gpu/drm/i915/intel_ddi.c | 21 +++-
> drivers/gpu/drm/i915/intel_dp.c | 138 +++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_dp_link_training.c | 12 ++-
> drivers/gpu/drm/i915/intel_drv.h | 12 ++-
> include/drm/drm_connector.h | 7 +-
> include/drm/drm_crtc.h | 5 +
> include/uapi/drm/drm_mode.h | 4 +
> 9 files changed, 214 insertions(+), 9 deletions(-)
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
