On Mon, Oct 15, 2018 at 10:33:06PM -0400, Lyude Paul wrote:
> Unfortunately, it appears our fix in:
> commit b5d29843d8ef ("drm/atomic_helper: Allow DPMS On<->Off changes
> for unregistered connectors")
>
> Which attempted to work around the problems introduced by:
> commit 4d80273976bf ("drm/atomic_helper: Disallow new modesets on
> unregistered connectors")
>
> Is still not the right solution, as modesets can still be triggered
> outside of drm_atomic_set_crtc_for_connector().
>
> So in order to fix this, while still being careful that we don't break
> modesets that a driver may perform before being registered with
> userspace, we replace connector->registered with a tristate member,
> connector->registration_state. This allows us to keep track of whether
> or not a connector is still initializing and hasn't been exposed to
> userspace, is currently registered and exposed to userspace, or has been
> legitimately removed from the system after having once been present.
>
> Using this info, we can prevent userspace from performing new modesets
> on unregistered connectors while still allowing the driver to perform
> modesets on unregistered connectors before the driver has finished being
> registered.
>
> Fixes: b5d29843d8ef ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors")
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: David Airlie <airlied@xxxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 60 +++++++++++++++++++++----
> drivers/gpu/drm/drm_atomic_uapi.c | 21 ---------
> drivers/gpu/drm/drm_connector.c | 10 ++---
> drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++--
> include/drm/drm_connector.h | 68 ++++++++++++++++++++++++++++-
> 5 files changed, 127 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 6f66777dca4b..6cadeaf28ae4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -529,6 +529,35 @@ mode_valid(struct drm_atomic_state *state)
> return 0;
> }
>
> +static int
> +unregistered_connector_check(struct drm_atomic_state *state,
> + struct drm_connector *connector,
> + struct drm_connector_state *old_conn_state,
> + struct drm_connector_state *new_conn_state)
> +{
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc *crtc;
> +
> + if (!drm_connector_unregistered(connector))
> + return 0;
> +
> + crtc = new_conn_state->crtc;
> + if (!crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + if (!crtc_state || !drm_atomic_crtc_needs_modeset(crtc_state))
> + return 0;
> +
> + if (crtc_state->mode_changed) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] can't change mode on unregistered connector\n",
> + connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /**
> * drm_atomic_helper_check_modeset - validate state object for modeset changes
> * @dev: DRM device
> @@ -684,18 +713,33 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> return ret;
> }
>
> - /*
> - * Iterate over all connectors again, to make sure atomic_check()
> - * has been called on them when a modeset is forced.
> - */
> 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 (connectors_mask & BIT(i))
> - continue;
> + /* Make sure atomic_check() is called on any unchecked
> + * connectors when a modeset has been forced
> + */
> + if (connectors_mask & BIT(i) && funcs->atomic_check) {
> + ret = funcs->atomic_check(connector,
> + new_connector_state);
> + if (ret)
> + return ret;
> + }
>
> - if (funcs->atomic_check)
> - ret = funcs->atomic_check(connector, new_connector_state);
> + /*
> + * Prevent userspace from turning on new displays or setting
> + * new modes using connectors which have been removed from
> + * userspace. This is racy since an unplug could happen at any
> + * time including after this check, but that's OK: we only
> + * care about preventing userspace from trying to set invalid
> + * state using destroyed connectors that it's been notified
> + * about. No one can save us after the atomic check completes
> + * but ourselves.
> + */
> + ret = unregistered_connector_check(state,
> + connector,
> + old_connector_state,
> + new_connector_state);
I expect a functional revert of b5d29843d8ef ("drm/atomic_helper: Allow
DPMS On<->Off changes for unregistered connectors"), but with
drm_connector_unregistered() instead of the READ_ONCE. This here is
entirely different, and I'm not clear on why.
Aside from this looks good to me.
[snip]
> +/**
> + * drm_connector_unregistered - has the connector been unregistered from
> + * userspace?
> + * @connector: DRM connector
> + *
> + * Checks whether or not @connector has been unregistered from userspace.
> + *
> + * Returns:
> + * True if the connector was unregistered, false if the connector is
> + * registered or has not yet been registered with userspace.
> + */
> +static inline bool drm_connector_unregistered(struct drm_connector *connector)
Bikeshed: I'd call this drm_connector_is_unregistered(). C is imperative,
functions without verbs always look a bit funny to me :-)
Feel free to ignore the bikeshed.
Cheers, Daniel
> +{
> + return READ_ONCE(connector->registration_state) ==
> + DRM_CONNECTOR_UNREGISTERED;
> +}
> +
> const char *drm_get_connector_status_name(enum drm_connector_status status);
> const char *drm_get_subpixel_order_name(enum subpixel_order order);
> const char *drm_get_dpms_name(int val);
> --
> 2.17.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
