[Intel-gfx] [PATCH 0/5] Handle Link Training Failure during modeset

[Deucher, Alexander]

Adding Harry and Tony from our display team to review.

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Thursday, November 10, 2016 1:20 PM
> To: Manasi Navare; dri-devel at lists.freedesktop.org; intel-
> gfx at lists.freedesktop.org
> Cc: Dave Airlie; Peres, Martin; Deucher, Alexander
> Subject: Re: [Intel-gfx] [PATCH 0/5] Handle Link Training Failure during
> modeset
> 
> On Thu, 10 Nov 2016, Manasi Navare <manasi.d.navare at intel.com> 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