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

[Souza, Jose]

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_check
> > 		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.

> 
> > 
> > Which will cause the drm_atomic_commit/IOCTL for disabling one of MST stream pipes (Main MST) to fail.
> > 
> > The problem happens when a user space (as Chrome) doesn’t retry a falling commit, leaving a disconnected MST pipe still ON,
> > which will result in failing reconnect of MST hub or even worse leaving TC PHY in a connected state while the MST Hub is disconnected.
> > 
> > Tested on Ubuntu(drm-tip) and Chrome(kernel-next 5.9 rc7)
> > 
> > Signed-off-by: Khaled Almahallawy <khaled.almahallawy at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index e948aacbd4ab..1ede980876ed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -265,6 +265,9 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> >  			return ret;
> >  		}
> >  		crtc_state->uapi.mode_changed = true;
> > +
> > +		if (connector_iter->base.status == connector_status_disconnected)
> > +			crtc_state->uapi.active = false;
> 
> That will make the state userspace last set inconsistent with what's
> really going on. Which means suddenly page flips/vblank waits and
> whatnot will start to fail.
> 
> Also that wil directly mutate the prop visible to user space, which
> is not how these things are supposed to work. I think if we did do
> something like this we should maybe have some kind of internal
> flag for it.
> 
> >  	}
> >  	drm_connector_list_iter_end(&connector_list_iter);
> >  
> > 
> > 
> > 
> > -- 
> > 2.25.1
>