Adding Harry and Tony from our display team to review. > -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] > Sent: Thursday, November 10, 2016 1:20 PM > To: Manasi Navare; dri-devel@xxxxxxxxxxxxxxxxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Dave Airlie; Peres, Martin; Deucher, Alexander > Subject: Re: [PATCH 0/5] Handle Link Training Failure during > modeset > > 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
