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

[Ville Syrjälä]

On Wed, Oct 21, 2020 at 05:25:40PM -0400, Lyude Paul wrote:
> 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.

It's all racy anyway is it not? Because of that I'm pretty firmly in
the "just plow ahead blindly" camp.

-- 
Ville Syrjälä
Intel