This is a resend of "drm/atomic: Fix encoder stealing" with updated patches.
Maarten Lankhorst (7): drm/atomic: Clean up update_output_state. drm/atomic: Pass connector and state to update_connector_routing. drm/atomic: Always call steal_encoder, v2. drm/atomic: Handle encoder stealing from set_config better. drm/atomic: Handle encoder assignment conflicts in a separate check, v3. drm/atomic: Clean up steal_encoder, v2. drm/atomic: Clean up update_connector_routing.
drivers/gpu/drm/drm_atomic_helper.c | 252 +++++++++++++++++++----------------- include/drm/drm_crtc.h | 2 + 2 files changed, 132 insertions(+), 122 deletions(-)
With the addition of crtc_state->connector_mask other connectors from different crtc's aren't needed any more, so only call add_affected_connectors on the target crtc. This allows a cleanup to first remove all current connectors, then add all set->connectors to the target crtc.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++---------------------- 1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4da4f2a49078..a0226d4b949a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1761,28 +1761,18 @@ static int update_output_state(struct drm_atomic_state *state, struct drm_crtc_state *crtc_state; struct drm_connector *connector; struct drm_connector_state *conn_state; - int ret, i, j; + int ret, i;
ret = drm_modeset_lock(&dev->mode_config.connection_mutex, state->acquire_ctx); if (ret) return ret;
- /* First grab all affected connector/crtc states. */ - for (i = 0; i < set->num_connectors; i++) { - conn_state = drm_atomic_get_connector_state(state, - set->connectors[i]); - if (IS_ERR(conn_state)) - return PTR_ERR(conn_state); - } - - for_each_crtc_in_state(state, crtc, crtc_state, i) { - ret = drm_atomic_add_affected_connectors(state, crtc); - if (ret) - return ret; - } + /* First disable all connectors on the target crtc. */ + ret = drm_atomic_add_affected_connectors(state, set->crtc); + if (ret) + return ret;
- /* Then recompute connector->crtc links and crtc enabling state. */ for_each_connector_in_state(state, connector, conn_state, i) { if (conn_state->crtc == set->crtc) { ret = drm_atomic_set_crtc_for_connector(conn_state, @@ -1790,16 +1780,19 @@ static int update_output_state(struct drm_atomic_state *state, if (ret) return ret; } + }
- for (j = 0; j < set->num_connectors; j++) { - if (set->connectors[j] == connector) { - ret = drm_atomic_set_crtc_for_connector(conn_state, - set->crtc); - if (ret) - return ret; - break; - } - } + /* Then set all connectors from set->connectors on the target crtc */ + for (i = 0; i < set->num_connectors; i++) { + conn_state = drm_atomic_get_connector_state(state, + set->connectors[i]); + if (IS_ERR(conn_state)) + return PTR_ERR(conn_state); + + ret = drm_atomic_set_crtc_for_connector(conn_state, + set->crtc); + if (ret) + return ret; }
for_each_crtc_in_state(state, crtc, crtc_state, i) {
Minor cleanup, connector and connector_state are always non-NULL here.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a0226d4b949a..3d1f97a832fc 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -215,22 +215,16 @@ steal_encoder(struct drm_atomic_state *state, }
static int -update_connector_routing(struct drm_atomic_state *state, int conn_idx) +update_connector_routing(struct drm_atomic_state *state, + struct drm_connector *connector, + struct drm_connector_state *connector_state) { const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; struct drm_crtc *encoder_crtc; - struct drm_connector *connector; - struct drm_connector_state *connector_state; struct drm_crtc_state *crtc_state; int idx, ret;
- connector = state->connectors[conn_idx]; - connector_state = state->connector_states[conn_idx]; - - if (!connector) - return 0; - DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n", connector->base.id, connector->name); @@ -494,7 +488,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * drivers must set crtc->mode_changed themselves when connector * properties need to be updated. */ - ret = update_connector_routing(state, i); + ret = update_connector_routing(state, connector, + connector_state); if (ret) return ret; }
There's no need to have a separate function to get the crtc which is stolen, this can already be found when actually stealing the encoder.
drm_for_each_connector already checks for connection_mutex, so use that macro now.
Changes since v1: - Do not check for NULL crtc in connector_state, this may happen when a crtc is disabled and its encoder stolen. - Because of this, use connector->state->crtc instead of conn_state->crtc.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 74 ++++++++++--------------------------- 1 file changed, 20 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3d1f97a832fc..72ca85b32260 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -106,25 +106,6 @@ check_pending_encoder_assignment(struct drm_atomic_state *state, return true; }
-static struct drm_crtc * -get_current_crtc_for_encoder(struct drm_device *dev, - struct drm_encoder *encoder) -{ - struct drm_mode_config *config = &dev->mode_config; - struct drm_connector *connector; - - WARN_ON(!drm_modeset_is_locked(&config->connection_mutex)); - - drm_for_each_connector(connector, dev) { - if (connector->state->best_encoder != encoder) - continue; - - return connector->state->crtc; - } - - return NULL; -} - static void set_best_encoder(struct drm_atomic_state *state, struct drm_connector_state *conn_state, @@ -168,38 +149,18 @@ set_best_encoder(struct drm_atomic_state *state,
static int steal_encoder(struct drm_atomic_state *state, - struct drm_encoder *encoder, - struct drm_crtc *encoder_crtc) + struct drm_encoder *encoder) { - struct drm_mode_config *config = &state->dev->mode_config; struct drm_crtc_state *crtc_state; struct drm_connector *connector; struct drm_connector_state *connector_state;
- /* - * We can only steal an encoder coming from a connector, which means we - * must already hold the connection_mutex. - */ - WARN_ON(!drm_modeset_is_locked(&config->connection_mutex)); - - DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n", - encoder->base.id, encoder->name, - encoder_crtc->base.id, encoder_crtc->name); + drm_for_each_connector(connector, state->dev) { + struct drm_crtc *encoder_crtc;
- crtc_state = drm_atomic_get_crtc_state(state, encoder_crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - - crtc_state->connectors_changed = true; - - list_for_each_entry(connector, &config->connector_list, head) { if (connector->state->best_encoder != encoder) continue;
- DRM_DEBUG_ATOMIC("Stealing encoder from [CONNECTOR:%d:%s]\n", - connector->base.id, - connector->name); - connector_state = drm_atomic_get_connector_state(state, connector); if (IS_ERR(connector_state)) @@ -208,7 +169,18 @@ steal_encoder(struct drm_atomic_state *state, if (connector_state->best_encoder != encoder) continue;
+ encoder_crtc = connector->state->crtc; + + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n", + encoder->base.id, encoder->name, + encoder_crtc->base.id, encoder_crtc->name); + set_best_encoder(state, connector_state, NULL); + + crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); + crtc_state->connectors_changed = true; + + return 0; }
return 0; @@ -221,7 +193,6 @@ update_connector_routing(struct drm_atomic_state *state, { const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; - struct drm_crtc *encoder_crtc; struct drm_crtc_state *crtc_state; int idx, ret;
@@ -299,17 +270,12 @@ update_connector_routing(struct drm_atomic_state *state, return -EINVAL; }
- encoder_crtc = get_current_crtc_for_encoder(state->dev, - new_encoder); - - if (encoder_crtc) { - ret = steal_encoder(state, new_encoder, encoder_crtc); - if (ret) { - DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n", - connector->base.id, - connector->name); - return ret; - } + ret = steal_encoder(state, new_encoder); + if (ret) { + DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n", + connector->base.id, + connector->name); + return ret; }
if (WARN_ON(!connector_state->crtc))
Instead of failing with -EINVAL when conflicting encoders are found, the legacy set_config will disable other connectors when encoders conflict.
With the previous commit this becomes 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 required.
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(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 72ca85b32260..db11c2f9b098 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) @@ -448,6 +510,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, @@ -1796,6 +1864,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;
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 --- 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) { /*
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.*/@@ -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
Now that only encoders can be stolen that are part of the state steal_encoder no longer needs to inspect all connectors, just those that are part of the atomic state.
Changes since v1: - Change return value to void, can no longer fail.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bb60148c5c8d..2395201eb7ab 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -227,25 +227,18 @@ set_best_encoder(struct drm_atomic_state *state, conn_state->best_encoder = encoder; }
-static int +static void steal_encoder(struct drm_atomic_state *state, struct drm_encoder *encoder) { struct drm_crtc_state *crtc_state; struct drm_connector *connector; struct drm_connector_state *connector_state; + int i;
- drm_for_each_connector(connector, state->dev) { + for_each_connector_in_state(state, connector, connector_state, i) { struct drm_crtc *encoder_crtc;
- if (connector->state->best_encoder != encoder) - continue; - - connector_state = drm_atomic_get_connector_state(state, - connector); - if (IS_ERR(connector_state)) - return PTR_ERR(connector_state); - if (connector_state->best_encoder != encoder) continue;
@@ -260,10 +253,8 @@ steal_encoder(struct drm_atomic_state *state, crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); crtc_state->connectors_changed = true;
- return 0; + return; } - - return 0; }
static int @@ -343,13 +334,7 @@ update_connector_routing(struct drm_atomic_state *state, return 0; }
- ret = steal_encoder(state, new_encoder); - if (ret) { - DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n", - connector->base.id, - connector->name); - return ret; - } + steal_encoder(state, new_encoder);
if (WARN_ON(!connector_state->crtc)) return -EINVAL;
Hi Maarten,
[auto build test WARNING on drm/drm-next] [also build test WARNING on next-20160303] [cannot apply to v4.5-rc6] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Fix-en... base: git://people.freedesktop.org/~airlied/linux.git drm-next config: x86_64-randconfig-x014-201609 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/gpu/drm/drm_atomic_helper.c: In function 'update_connector_routing':
drivers/gpu/drm/drm_atomic_helper.c:268:11: warning: unused variable 'ret' [-Wunused-variable]
int idx, ret; ^
vim +/ret +268 drivers/gpu/drm/drm_atomic_helper.c
c6c869e2 Maarten Lankhorst 2016-03-03 252 c6c869e2 Maarten Lankhorst 2016-03-03 253 crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); c6c869e2 Maarten Lankhorst 2016-03-03 254 crtc_state->connectors_changed = true; c6c869e2 Maarten Lankhorst 2016-03-03 255 49bf38f8 Maarten Lankhorst 2016-03-03 256 return; 623369e5 Daniel Vetter 2014-09-16 257 } 623369e5 Daniel Vetter 2014-09-16 258 } 623369e5 Daniel Vetter 2014-09-16 259 623369e5 Daniel Vetter 2014-09-16 260 static int 184a4cc1 Maarten Lankhorst 2016-03-03 261 update_connector_routing(struct drm_atomic_state *state, 184a4cc1 Maarten Lankhorst 2016-03-03 262 struct drm_connector *connector, 184a4cc1 Maarten Lankhorst 2016-03-03 263 struct drm_connector_state *connector_state) 623369e5 Daniel Vetter 2014-09-16 264 { b5ceff20 Ville Syrjälä 2015-03-10 265 const struct drm_connector_helper_funcs *funcs; 623369e5 Daniel Vetter 2014-09-16 266 struct drm_encoder *new_encoder; 623369e5 Daniel Vetter 2014-09-16 267 struct drm_crtc_state *crtc_state; 623369e5 Daniel Vetter 2014-09-16 @268 int idx, ret; 623369e5 Daniel Vetter 2014-09-16 269 17a38d9c Daniel Vetter 2015-02-22 270 DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n", 623369e5 Daniel Vetter 2014-09-16 271 connector->base.id, 623369e5 Daniel Vetter 2014-09-16 272 connector->name); 623369e5 Daniel Vetter 2014-09-16 273 623369e5 Daniel Vetter 2014-09-16 274 if (connector->state->crtc != connector_state->crtc) { 623369e5 Daniel Vetter 2014-09-16 275 if (connector->state->crtc) { 623369e5 Daniel Vetter 2014-09-16 276 idx = drm_crtc_index(connector->state->crtc);
:::::: The code at line 268 was first introduced by commit :::::: 623369e533e8a5f85999d605723efa4523554bae drm: Atomic crtc/connector updates using crtc/plane helper interfaces
:::::: TO: Daniel Vetter daniel.vetter@ffwll.ch :::::: CC: Daniel Vetter daniel.vetter@ffwll.ch
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Mar 03, 2016 at 10:17:41AM +0100, Maarten Lankhorst wrote:
Now that only encoders can be stolen that are part of the state steal_encoder no longer needs to inspect all connectors, just those that are part of the atomic state.
Changes since v1:
- Change return value to void, can no longer fail.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bb60148c5c8d..2395201eb7ab 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -227,25 +227,18 @@ set_best_encoder(struct drm_atomic_state *state, conn_state->best_encoder = encoder; }
-static int +static void steal_encoder(struct drm_atomic_state *state, struct drm_encoder *encoder) { struct drm_crtc_state *crtc_state; struct drm_connector *connector; struct drm_connector_state *connector_state;
- int i;
- drm_for_each_connector(connector, state->dev) {
- for_each_connector_in_state(state, connector, connector_state, i) { struct drm_crtc *encoder_crtc;
if (connector->state->best_encoder != encoder)continue;connector_state = drm_atomic_get_connector_state(state,connector);if (IS_ERR(connector_state))return PTR_ERR(connector_state);- if (connector_state->best_encoder != encoder) continue;
@@ -260,10 +253,8 @@ steal_encoder(struct drm_atomic_state *state, crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); crtc_state->connectors_changed = true;
return 0;
return;
I was wondering if we should add some kind of sanity check here (or maybe better do it afterwards across the entire device state?) to make sure the same encoder didn't end up used multiple times?
Anyway, patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
}
- return 0;
}
static int @@ -343,13 +334,7 @@ update_connector_routing(struct drm_atomic_state *state, return 0; }
- ret = steal_encoder(state, new_encoder);
- if (ret) {
DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n",connector->base.id,connector->name);return ret;- }
steal_encoder(state, new_encoder);
if (WARN_ON(!connector_state->crtc)) return -EINVAL;
-- 2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
connector_state->crtc can no longer be unset by accident, so that check can be removed. The other code open-codes drm_atomic_get_existing_crtc_state, so use that.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2395201eb7ab..9f0d16eb04b2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state, const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; struct drm_crtc_state *crtc_state; - int idx, ret; + int ret;
DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n", connector->base.id, @@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
if (connector->state->crtc != connector_state->crtc) { if (connector->state->crtc) { - idx = drm_crtc_index(connector->state->crtc); - - crtc_state = state->crtc_states[idx]; + crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc); crtc_state->connectors_changed = true; }
if (connector_state->crtc) { - idx = drm_crtc_index(connector_state->crtc); - - crtc_state = state->crtc_states[idx]; + crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); crtc_state->connectors_changed = true; } } @@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
steal_encoder(state, new_encoder);
- if (WARN_ON(!connector_state->crtc)) - return -EINVAL; - set_best_encoder(state, connector_state, new_encoder);
- idx = drm_crtc_index(connector_state->crtc); - - crtc_state = state->crtc_states[idx]; + crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); crtc_state->connectors_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
On Thu, Mar 03, 2016 at 10:17:42AM +0100, Maarten Lankhorst wrote:
connector_state->crtc can no longer be unset by accident, so that check can be removed. The other code open-codes drm_atomic_get_existing_crtc_state, so use that.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2395201eb7ab..9f0d16eb04b2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state, const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; struct drm_crtc_state *crtc_state;
- int idx, ret;
int ret;
DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n", connector->base.id,
@@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
if (connector->state->crtc != connector_state->crtc) { if (connector->state->crtc) {
idx = drm_crtc_index(connector->state->crtc);crtc_state = state->crtc_states[idx];
crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc); crtc_state->connectors_changed = true;}
if (connector_state->crtc) {
idx = drm_crtc_index(connector_state->crtc);crtc_state = state->crtc_states[idx];
crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); crtc_state->connectors_changed = true;@@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
steal_encoder(state, new_encoder);
- if (WARN_ON(!connector_state->crtc))
return -EINVAL;
I was going to suggest just this when I read the code previously.
set_best_encoder(state, connector_state, new_encoder);
- idx = drm_crtc_index(connector_state->crtc);
- crtc_state = state->crtc_states[idx];
- crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
These could have been a separate patch, but no biggie
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
crtc_state->connectors_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 04, 2016 at 03:29:03PM +0200, Ville Syrjälä wrote:
On Thu, Mar 03, 2016 at 10:17:42AM +0100, Maarten Lankhorst wrote:
connector_state->crtc can no longer be unset by accident, so that check can be removed. The other code open-codes drm_atomic_get_existing_crtc_state, so use that.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2395201eb7ab..9f0d16eb04b2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -265,7 +265,7 @@ update_connector_routing(struct drm_atomic_state *state, const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; struct drm_crtc_state *crtc_state;
- int idx, ret;
int ret;
DRM_DEBUG_ATOMIC("Updating routing for [CONNECTOR:%d:%s]\n", connector->base.id,
@@ -273,16 +273,12 @@ update_connector_routing(struct drm_atomic_state *state,
if (connector->state->crtc != connector_state->crtc) { if (connector->state->crtc) {
idx = drm_crtc_index(connector->state->crtc);crtc_state = state->crtc_states[idx];
crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc); crtc_state->connectors_changed = true;}
if (connector_state->crtc) {
idx = drm_crtc_index(connector_state->crtc);crtc_state = state->crtc_states[idx];
crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); crtc_state->connectors_changed = true;@@ -336,14 +332,9 @@ update_connector_routing(struct drm_atomic_state *state,
steal_encoder(state, new_encoder);
- if (WARN_ON(!connector_state->crtc))
return -EINVAL;I was going to suggest just this when I read the code previously.
set_best_encoder(state, connector_state, new_encoder);
- idx = drm_crtc_index(connector_state->crtc);
- crtc_state = state->crtc_states[idx];
- crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc);
These could have been a separate patch, but no biggie
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Remaining patches applied to drm-misc, thanks. -Daniel
crtc_state->connectors_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
kbuild test robot -
Maarten Lankhorst -
Ville Syrjälä