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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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?

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx