[22/28] drm/atomic-helper: Reject attempts at re-stealing encoders
diff mbox

Message ID 1449218769-16577-23-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Dec. 4, 2015, 8:46 a.m. UTC
This can happen when we run out of encoders for a multi-crtc modeset,
or also when userspace is silly and tries to clone multiple connectors
that need the same encoder on the same crtc.

Reported-and-Tested-and-Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Thierry Reding Dec. 7, 2015, 1:26 p.m. UTC | #1
On Fri, Dec 04, 2015 at 09:46:03AM +0100, Daniel Vetter wrote:
> This can happen when we run out of encoders for a multi-crtc modeset,
> or also when userspace is silly and tries to clone multiple connectors
> that need the same encoder on the same crtc.
> 
> Reported-and-Tested-and-Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2cf8ab7dbc8c..ab275499d2a3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -86,6 +86,27 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  	}
>  }
>  
> +static bool
> +check_pending_encoder_assignment(struct drm_atomic_state *state,
> +				 struct drm_encoder *new_encoder,
> +				 struct drm_connector *new_connector)

new_connector seems to be unused.

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

Was this supposed to be new_connector->state->best_encoder?

Thierry
Daniel Vetter Dec. 7, 2015, 1:40 p.m. UTC | #2
On Mon, Dec 07, 2015 at 02:26:04PM +0100, Thierry Reding wrote:
> On Fri, Dec 04, 2015 at 09:46:03AM +0100, Daniel Vetter wrote:
> > This can happen when we run out of encoders for a multi-crtc modeset,
> > or also when userspace is silly and tries to clone multiple connectors
> > that need the same encoder on the same crtc.
> > 
> > Reported-and-Tested-and-Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Oops that patch shouldn't have been in here. And it's already merged -
somehow I botched the rebase before sending out this series and it didn't
drop out. I'll address your feedback with a follow-up patch.

> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 2cf8ab7dbc8c..ab275499d2a3 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -86,6 +86,27 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> >  	}
> >  }
> >  
> > +static bool
> > +check_pending_encoder_assignment(struct drm_atomic_state *state,
> > +				 struct drm_encoder *new_encoder,
> > +				 struct drm_connector *new_connector)
> 
> new_connector seems to be unused.
> 
> > +{
> > +	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)
> 
> Was this supposed to be new_connector->state->best_encoder?

Nah, new_connector was just leftovers from an earlier, broken version of
this. The function really only checks whether the given encoder has
alreaddy been reassigned in this update. That's enough to decide that
there's a conflict and reject the modeset. I'll do a patch to drop the
superflous argument.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2cf8ab7dbc8c..ab275499d2a3 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -86,6 +86,27 @@  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 	}
 }
 
+static bool
+check_pending_encoder_assignment(struct drm_atomic_state *state,
+				 struct drm_encoder *new_encoder,
+				 struct drm_connector *new_connector)
+{
+	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 struct drm_crtc *
 get_current_crtc_for_encoder(struct drm_device *dev,
 			     struct drm_encoder *encoder)
@@ -235,6 +256,13 @@  update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 		return 0;
 	}
 
+	if (!check_pending_encoder_assignment(state, new_encoder, connector)) {
+		DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n",
+				 connector->base.id,
+				 connector->name);
+		return -EINVAL;
+	}
+
 	encoder_crtc = get_current_crtc_for_encoder(state->dev,
 						    new_encoder);