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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
