[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 16:25 -0700, José Roberto de Souza 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_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.

Oh it do not have information about other connectors so not conflicting but anyways userspace should retry atomic commits anyways.

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