Re: [PATCH v3 5/7] drm/atomic: Handle encoder assignment conflicts in a separate check, v3.

On Thu, Mar 03, 2016 at 10:17:40AM +0100, Maarten Lankhorst wrote:

The current check doesn't handle the case where we don't steal an encoder, but keep it on the current connector. If we repurpose disable_conflicting_encoders to do the checking, we just have to reject the ones that conflict.

Changes since v1:

• Return early with empty encoder_mask, drm_for_each_connector requires connection_mutex held.

Changes since v2:

• Add comments for the loops.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Testcase: kms_setmode.invalid-clone-single-crtc-stealing

I was a bit lazy on looking at the individual patches 3-5, but the result made sense to me, so Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com for all three.

---

drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index db11c2f9b098..bb60148c5c8d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -86,7 +86,8 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, } }

-static int disable_conflicting_connectors(struct drm_atomic_state *state) +static int handle_conflicting_encoders(struct drm_atomic_state *state,

• 		       bool disable_conflicting_encoders)
  

{ struct drm_connector_state *conn_state; struct drm_connector *connector; @@ -94,6 +95,11 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state) unsigned encoder_mask = 0; int i, ret;

• /*

• * First loop, find all newly assigned encoders from the connectors
  

• * part of the state. If the same encoder is assigned to multiple
  

• * connectors bail out.
  

• */
  

  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;

@@ -106,10 +112,33 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state) else new_encoder = funcs->best_encoder(connector);

• if (new_encoder)
  

• if (new_encoder) {
  

• 	if (encoder_mask & (1 << drm_encoder_index(new_encoder))) {
  

• 		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
  

• 			new_encoder->base.id, new_encoder->name,
  

• 			connector->base.id, connector->name);
  

• 		return -EINVAL;
  

• 	}
  

• encoder_mask |= 1 << drm_encoder_index(new_encoder);
  

• }
  

  }

• if (!encoder_mask)

• return 0;
  

• /*

• * Second loop, iterate over all connectors not part of the state.
  

• *
  

• * If a conflicting encoder is found and disable_conflicting_encoders
  

• * is not set, an error is returned. Userspace can provide a solution
  

• * through the atomic ioctl.
  

• *
  

• * If the flag is set conflicting connectors are removed from the crtc
  

• * and the crtc is disabled if no encoder is left. This preserves
  

• * compatibility with the legacy set_config behavior.
  

• */
  

  drm_for_each_connector(connector, state->dev) { struct drm_crtc_state *crtc_state;

@@ -120,6 +149,15 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state) if (!encoder || !(encoder_mask & (1 << drm_encoder_index(encoder)))) continue;

• if (!disable_conflicting_encoders) {
  

• 	DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
  

• 			 encoder->base.id, encoder->name,
  

• 			 connector->state->crtc->base.id,
  

• 			 connector->state->crtc->name,
  

• 			 connector->base.id, connector->name);
  

• 	return -EINVAL;
  

• }
  

• conn_state = drm_atomic_get_connector_state(state, connector); if (IS_ERR(conn_state)) return PTR_ERR(conn_state);

@@ -148,26 +186,6 @@ static int disable_conflicting_connectors(struct drm_atomic_state *state) return 0; }

-static bool -check_pending_encoder_assignment(struct drm_atomic_state *state,

• 		 struct drm_encoder *new_encoder)
  

-{

• struct drm_connector *connector;
• struct drm_connector_state *conn_state;
• int i;
• for_each_connector_in_state(state, connector, conn_state, i) {

• if (conn_state->best_encoder != new_encoder)
  

• 	continue;
  

• /* encoder already assigned and we're trying to re-steal it! */
  

• if (connector->state->best_encoder != conn_state->best_encoder)
  

• 	return false;
  

• }
• return true;

-}

static void set_best_encoder(struct drm_atomic_state *state, struct drm_connector_state *conn_state, @@ -325,13 +343,6 @@ update_connector_routing(struct drm_atomic_state *state, return 0; }

• if (!check_pending_encoder_assignment(state, new_encoder)) {

• DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
  

• 		 connector->base.id,
  

• 		 connector->name);
  

• return -EINVAL;
  

• }
• ret = steal_encoder(state, new_encoder); if (ret) { DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",

@@ -510,11 +521,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } }

• if (state->legacy_set_config) {

• ret = disable_conflicting_connectors(state);
  

• if (ret)
  

• 	return ret;
  

• }

• ret = handle_conflicting_encoders(state, state->legacy_set_config);

• if (ret)

• return ret;
  
  

  for_each_connector_in_state(state, connector, connector_state, i) { /*

-- 2.1.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel