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@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
