On Wed, Aug 30, 2023 at 05:41:37PM -0400, Lyude Paul wrote: > Other then the name typo (s/Pual/Paul): > > Signed-off-by: Lyude Paul <ly...@redhat.com> (just since I co-authored > things~)
I believe having the Co-developed-by: in the patches that you helped out would be nice. > Reviewed-by: Lyude Paul <ly...@redhat.com> > > I think we definitely want to make sure we get intel's opinions on this > though, especially regarding the usage of link-status. I think we're close > enough to link-status's intended purpose, but I definitely would like to know > what others think about that since userspace will definitely have to handle > situations like this a bit differently than with SST. > > Also - definitely make sure you take a look at Imre's patch series that's > currently on the list (I just finished reviewing it), since it adds some > things to the helpers that might end up being useful here :) > > https://patchwork.freedesktop.org/series/122589/ > > On Thu, 2023-08-24 at 16:50 -0400, Gil Dekel wrote: > > Next version of https://patchwork.freedesktop.org/series/122850/ > > > > v4: > > Another blunder. I uploaded the patches from my ChromeiumOS kernel dev > > repo > > instead of drm-tip/drm-tip. Apologies for the noise :( > > > > v3: > > Still learning the ropes of upstream workflow. Apologies for mucking up > > v2. > > This is just a re-upload. > > > > v2: > > Reorganize into: > > 1) Add for final failure state for SST and MST link training fallback. > > 2) Add a DRM helper for setting downstream MST ports' link-status state. > > 3) Make handling SST and MST connectors simpler via intel_dp. > > 4) Update link-status for downstream MST ports. > > 5) Emit a uevent with the "link-status" trigger property. > > > > v1: > > Currently, when link training fails after all fallback values have been > > exhausted, the i915 driver seizes to send uevents to userspace. This leave > > userspace thinking that the last passing atomic commit was successful, and > > that > > all connectors (displays) are connected and operational, when in fact, the > > last > > link failed to train and the displays remain dark. This manifests as > > "zombie" > > displays in userspace, in which users observe the displays appear in their > > display settings page, but they are dark and unresponsive. > > > > Since, at the time of writing, MST link training fallback is not > > implemented, > > failing MST link training is a significantly more common case then a > > complete > > SST link training failure. And with users using MST hubs more than ever to > > connect multiple displays via their USB-C ports we observe this case often. > > > > This patchset series suggest a solution, in which a final failure state is > > defined. In this final state, the connector's bit rate capabilities, namely > > max_link_rate and max_link_lane_count, are set to 0. This effectively set > > the > > connector's bandwidth to 0Gbps, thus causing all its modes to be pruned in > > the > > following connector probing. > > > > Next, with this state defined, we emit a link-status=Bad uevent. The next > > time > > userspace probes the connector, it should recognize that the connector has > > no > > modes and ignore it since it is in a bad state. > > > > I am aware that always sending a uevent and never stopping may result in > > some > > userspaces having their expectations broken and enter an infinite loop of > > modesets and link-training attempts. However, per DRM link-status spec: > > ``` > > * link-status: > > * Connector link-status property to indicate the status of link. The > > * default value of link-status is "GOOD". If something fails during or > > * after modeset, the kernel driver may set this to "BAD" and issue a > > * hotplug uevent. Drivers should update this value using > > * drm_connector_set_link_status_property(). > > * > > * When user-space receives the hotplug uevent and detects a "BAD" > > * link-status, the sink doesn't receive pixels anymore (e.g. the > > screen > > * becomes completely black). The list of available modes may have > > * changed. User-space is expected to pick a new mode if the current > > one > > * has disappeared and perform a new modeset with link-status set to > > * "GOOD" to re-enable the connector. > > ``` > > (form drivers/gpu/drm/drm_connector.c - DOC: standard connector properties) > > > > it seems reasonable to assume that the suggested state is an extension of > > the > > spec's guidelines, in which the next new mode userspace picks for a > > connector > > with no modes is - none, thus breaking the cycle of failed link-training > > attempts. > > > > I suspect that, maybe, zeroing out the bit rate capabilities is not the > > right > > way to go, and perhaps marking the connector as disconnected instead may be > > a > > better solution. However, if marking a connector disconnected is the way to > > go, > > We will have to iterate over all MST ports in the MST case and mark the > > spawned > > connectors as disconnected as well. > > I -think- this is probably fine, that's likely how I'd > > > > > As a final note I should add that this approach was tested with ChromeOS as > > userspace, and we observed that the zombie displays stop showing up once the > > connectors are pruned of all their modes and are ignored by userspace. > > > > For your consideration and guidance. > > Thanks, > > > > Gil Dekel (6): > > drm/i915/dp_link_training: Add a final failing state to link training > > fallback > > drm/i915/dp_link_training: Add a final failing state to link training > > fallback for MST > > drm/dp_mst: Add drm_dp_set_mst_topology_link_status() > > drm/i915: Move DP modeset_retry_work into intel_dp > > drm/i915/dp_link_training: Set all downstream MST ports to BAD before > > retrying > > drm/i915/dp_link_training: Emit a link-status=Bad uevent with trigger > > property > > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 39 ++++++++++ > > drivers/gpu/drm/i915/display/intel_display.c | 14 +++- > > .../drm/i915/display/intel_display_types.h | 6 +- > > drivers/gpu/drm/i915/display/intel_dp.c | 75 ++++++++++++------- > > drivers/gpu/drm/i915/display/intel_dp.h | 2 +- > > .../drm/i915/display/intel_dp_link_training.c | 11 ++- > > include/drm/display/drm_dp_mst_helper.h | 3 + > > 7 files changed, 110 insertions(+), 40 deletions(-) > > > > -- > > Gil Dekel, Software Engineer, Google / ChromeOS Display and Graphics > > > > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
