diff mbox

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

Message ID 1456303053-28806-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst Feb. 24, 2016, 8:37 a.m. UTC
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.

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(+)

Comments

Daniel Vetter March 4, 2016, 4:20 p.m. UTC | #1
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
diff mbox

Patch

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;