Re: [PATCH v2 4/6] drm/atomic: Handle encoder stealing from set_config better.

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 24, 2016 at 09:37:31AM +0100, Maarten Lankhorst wrote:
> Instead of failing with -EINVAL when conflicting encoders are found,
> the legacy set_config will disable other connectors when encoders
> conflict.
> 
> With the cleanup to update_output_state this is a lot easier to
> implement. set_config only adds connectors to the state that are
> modified, and because of the previous commit that calls
> add_affected_connectors only on set->crtc it means any connector not
> part of the modeset can be stolen from. We disable the connector in
> that case, and possibly the crtc if no connectors are left.
> 
> Atomic modeset itself still doesn't allow encoder stealing, the
> results would be too unpredictable.

Shouldn't this be reworked to say something like "With this we can rework
the atomic core to not allow stealing"?
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 69 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h              |  2 ++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e89a5da27463..3543c7fcd072 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -86,6 +86,68 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  	}
>  }
>  
> +static int disable_conflicting_connectors(struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *connector;
> +	struct drm_encoder *encoder;
> +	unsigned encoder_mask = 0;
> +	int i, ret;
> +
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> +		struct drm_encoder *new_encoder;
> +
> +		if (!conn_state->crtc)
> +			continue;
> +
> +		if (funcs->atomic_best_encoder)
> +			new_encoder = funcs->atomic_best_encoder(connector, conn_state);
> +		else
> +			new_encoder = funcs->best_encoder(connector);
> +
> +		if (new_encoder)
> +			encoder_mask |= 1 << drm_encoder_index(new_encoder);
> +	}
> +
> +	drm_for_each_connector(connector, state->dev) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (drm_atomic_get_existing_connector_state(state, connector))
> +			continue;
> +
> +		encoder = connector->state->best_encoder;
> +		if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder))))
> +			continue;
> +
> +		conn_state = drm_atomic_get_connector_state(state, connector);
> +		if (IS_ERR(conn_state))
> +			return PTR_ERR(conn_state);
> +
> +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], disabling [CONNECTOR:%d:%s]\n",
> +				 encoder->base.id, encoder->name,
> +				 conn_state->crtc->base.id, conn_state->crtc->name,
> +				 connector->base.id, connector->name);
> +
> +		crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc);
> +
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +		if (ret)
> +			return ret;
> +
> +		if (!crtc_state->connector_mask) {
> +			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
> +								NULL);
> +			if (ret < 0)
> +				return ret;
> +
> +			crtc_state->active = false;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static bool
>  check_pending_encoder_assignment(struct drm_atomic_state *state,
>  				 struct drm_encoder *new_encoder)
> @@ -449,6 +511,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		}
>  	}
>  
> +	if (state->legacy_set_config) {
> +		ret = disable_conflicting_connectors(state);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for_each_connector_in_state(state, connector, connector_state, i) {
>  		/*
>  		 * This only sets crtc->mode_changed for routing changes,
> @@ -1797,6 +1865,7 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
>  	if (!state)
>  		return -ENOMEM;
>  
> +	state->legacy_set_config = true;
>  	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
>  retry:
>  	ret = __drm_atomic_helper_set_config(set, state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7fad193dc645..9a946df27f07 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1677,6 +1677,7 @@ struct drm_bridge {
>   * @dev: parent DRM device
>   * @allow_modeset: allow full modeset
>   * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> + * @legacy_set_config: Disable conflicting encoders instead of failing with -EINVAL.
>   * @planes: pointer to array of plane pointers
>   * @plane_states: pointer to array of plane states pointers
>   * @crtcs: pointer to array of CRTC pointers
> @@ -1690,6 +1691,7 @@ struct drm_atomic_state {
>  	struct drm_device *dev;
>  	bool allow_modeset : 1;
>  	bool legacy_cursor_update : 1;
> +	bool legacy_set_config : 1;
>  	struct drm_plane **planes;
>  	struct drm_plane_state **plane_states;
>  	struct drm_crtc **crtcs;
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel