[Intel-gfx] [PATCH] drm/i915/tgl: Set drm_crtc_state.active=false for all added disconnected CRTCs sharing MST stream.

[Lyude Paul]

On Wed, 2020-10-21 at 16:26 +0300, Ville Syrjälä wrote:
> On Tue, Oct 20, 2020 at 11:25:53PM +0000, Souza, Jose wrote:
> > On Tue, 2020-10-20 at 15:41 +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 20, 2020 at 12:45:55AM -0700, Khaled Almahallawy wrote:
> > > > This patch avoids failing atomic commits sent by user space by making
> > > > sure CRTC/Connector added to drm_atomic_state by the driver are in valid
> > > > state.
> > > > 
> > > > When disconnecting MST hub with two or more connected displays. The user
> > > > space sends IOCTL for each MST pipe to disable.
> > > > drm_atomic_state object sent from user space contains only the state of
> > > > the crtc/pipe intended to disable.
> > > > In TGL, intel_dp_mst_atomic_master_trans_check will add all other CRTC
> > > > and connectors that share the MST stream to drm_atomic_state:
> > > > 
> > > > drm_atomic_commit
> > > >    drm_atomic_helper_check_modeset
> > > >        update_connector_routing
> > > >        intel_dp_mst_atomic_check = funcs-
> > > > >atomic_check(connector, state);
> > > >        	   intel_dp_mst_atomic_master_trans_chec
> > > > k
> > > > 		intel_atomic_get_digital_connector_state
> > > > 			drm_atomic_get_connector_state   <-- Add all
> > > > Connectors
> > > > 			    drm_atomic_get_crtc_state <-- Add all CRTCs
> > > >        update_connector_routing <-- Check added
> > > > Connector/CRTCs - Will fail
> > > > 
> > > > However the added crtc/connector pair will be in invalid state (enabled
> > > > state for a removed connector)
> > > > triggering this condition in
> > > > drm_atomic_helper.c/update_connector_routing:
> > > > 
> > > > 	if (!state->duplicated &&
> > > > drm_connector_is_unregistered(connector) &&
> > > > 	    crtc_state->active) {
> > > > 		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not
> > > > registered\n",
> > > > 				 connector->base.id, connector->name);
> > > > 		return -EINVAL;
> > > > 	}
> > > 
> > > Yeah, I think that "reject modeset on unregistered connectors" idea is
> > > a bit broken given how the uapi has worked in the past. Cc:ing danvet
> > > and lyude who IIRC were involved with that.
> > > 
> > > Hmm. Maybe we could add the other stuff to the state only after the
> > > connector .atomic_check() stuff has been done? I don't quite remember
> > > why we decided to do it here. José do you recall the details?
> > 
> > Because the connector check function runs twice in
> > drm_atomic_helper_check_modeset(), in the first iteration it will add all
> > connectors that share the
> > same MST stream to state, the second one will make sure all other checks
> > passed in all connectors of the MST stream.
> > 
> > To me looks like the Chrome userspace is not doing the right thing, it is
> > sending asynchronous atomic commits with conflicting state between each
> > commit.
> > If it had a pool that dispatch one atomic state at time waiting for
> > completion before dispatch the next one it would not be a issue.
> 
> Yeah, with atomic userspace could avoid this potentially. Though it
> may be racy depending on whether it has noticed all the MST connectors
> disappearing yet or not. Either way it's still an issue for legacy
> uapi.

Sigh-I had hoped that we would have hooked this up such that we'd avoid this (as
I've already had to fix some issues this caused with legacy modesetting) but I
guess not. Have you guys considered trying to use the connector epochs whenever
you receive a hotplug event to differentiate between removed ('stale')
connectors and other connectors? tbh, if you can't find a connector with the
same mst path and epoch you last had as your stale connector then it's safe to
just assume it's gone.

Also - I'm totally open to better ideas for handling this or making it more
obvious when a connector has been removed, most of the reason for adding these
checks was to try our best (as this is impossible to fully guarantee) to avoid
situations where a host tried to enable an MST display that no longer existed
and put the hardware into a weird state. At least if I remember correctly, it's
been a while.

> 
-- 
Sincerely,
      Lyude Paul (she/her)
      Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!