Re: [PATCH] drm/atomic_helper: Stop modesets on unregistered connectors harder

[Daniel Vetter]

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