Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp_mst: don't pull unregistered connectors into state

[Imre Deak <imre.deak@intel.com>]

On Tue, Dec 20, 2022 at 12:39:17PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 15, 2022 at 03:51:50PM +0000, Simon Ser wrote:
> > In intel_dp_mst_atomic_master_trans_check(), we pull connectors
> > sharing the same DP-MST stream into the atomic state. However,
> > if the connector is unregistered, this later fails with:
> > 
> >     [  559.425658] i915 0000:00:02.0: [drm:drm_atomic_helper_check_modeset] [CONNECTOR:378:DP-7] is not registered
> > 
> > Skip these unregistered connectors to allow user-space to turn them
> > off.
> > 
> > Fixes part of this wlroots issue:
> > https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3407
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index f773e117ebc4..70859a927a9d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -280,7 +280,8 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector,
> >  		struct intel_crtc *crtc;
> >  
> >  		if (connector_iter->mst_port != connector->mst_port ||
> > -		    connector_iter == connector)
> > +		    connector_iter == connector ||
> > +		    connector_iter->base.registration_state == DRM_CONNECTOR_UNREGISTERED)
> >  			continue;
> 
> We can't really do that. It would risk leaving slave transcoders
> enabled while the master is undergoing a full modeset.
> 
> I think a couple of ways we could go about this:
> - kill the registration check entirely/partially
>   I think Imre already has some plans for the partial killing
>   due to some type-c vs. pm firmware issues that also need force
>   a full modeset

Looks like in this case the problem is that the core's check for routing
changes should be applied only to connectors passed in via the commit state,
however it's also done for some of the connectors added by drivers as a
dependency. Making that consistent would need the following change, probably
fixing the above issue as well:

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d579fd8f7cb83..9c4c67f8059b8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -673,9 +673,15 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	for_each_new_connector_in_state(state, connector, new_connector_state, i)
+		connectors_mask |= BIT(i);
+
 	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
 		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 
+		if (!(BIT(i) & connectors_mask))
+			continue;
+
 		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 		/*
@@ -708,8 +714,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				       connector->base.id, connector->name);
 			return ret;
 		}
-
-		connectors_mask |= BIT(i);
 	}
 
 	/*

> - relocate this stuff to happen after drm_atomic_helper_check_modeset()
>   like we already do for eg. bigjoiner. IIRC this was discussed as an
>   option when we added intel_dp_mst_atomic_master_trans_check() but
>   I don't recall anymore why we specifically chose to do this from
>   connector.atomic_check().
> 
> >  
> >  		conn_iter_state = intel_atomic_get_digital_connector_state(state,
> > -- 
> > 2.39.0
> > 
> 
> -- 
> Ville Syrjälä
> Intel