| Message ID | 1455785683-5477-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: > Because encoder <-> connector mapping is fixed when not moving to > another crtc we can just reject connectors trying to steal an encoder > from a connector not part of the state. This won't break MST on i915 > because in that case connectors will be part of the state if you switch > them between crtc's. If they're not they stay on the same crtc, and > encoder stealing would have failed anyway. We must do this for backwards compat. setCrtc on a connector that needs an encoder already used on some other crtc is supposed to disable that encoder (and the entire pipe if it's all unused) if we need it. -Daniel > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fa708559542c..b2028d592483 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -92,14 +92,21 @@ check_pending_encoder_assignment(struct drm_atomic_state *state, > int conn_idx) > { > struct drm_connector *connector; > - struct drm_connector_state *conn_state; > - int i; > > - for_each_connector_in_state(state, connector, conn_state, i) { > - if (i >= conn_idx) > - break; > + drm_for_each_connector(connector, state->dev) { > + struct drm_connector_state *conn_state = > + drm_atomic_get_existing_connector_state(state, > + connector); > > - if (conn_state->best_encoder == new_encoder) > + if (!conn_state) { > + if (connector->state->best_encoder == new_encoder) > + return false; > + > + continue; > + } > + > + if (drm_connector_index(connector) < conn_idx && > + conn_state->best_encoder == new_encoder) > return false; > } > > @@ -149,29 +156,19 @@ set_best_encoder(struct drm_atomic_state *state, > > static int > steal_encoder(struct drm_atomic_state *state, > - struct drm_encoder *encoder) > + struct drm_encoder *encoder, > + int conn_idx) > { > - 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; > + int i; > > - /* > - * 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)); > - > - list_for_each_entry(connector, &config->connector_list, head) { > + 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 (i >= conn_idx) > + break; > > if (connector_state->best_encoder != encoder || > WARN_ON(!connector_state->crtc)) > @@ -279,7 +276,7 @@ update_connector_routing(struct drm_atomic_state *state, > return -EINVAL; > } > > - ret = steal_encoder(state, new_encoder); > + ret = steal_encoder(state, new_encoder, conn_idx); > if (ret) { > DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n", > connector->base.id, > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 18-02-16 om 12:07 schreef Daniel Vetter: > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: >> Because encoder <-> connector mapping is fixed when not moving to >> another crtc we can just reject connectors trying to steal an encoder >> from a connector not part of the state. This won't break MST on i915 >> because in that case connectors will be part of the state if you switch >> them between crtc's. If they're not they stay on the same crtc, and >> encoder stealing would have failed anyway. > We must do this for backwards compat. setCrtc on a connector that needs an > encoder already used on some other crtc is supposed to disable that > encoder (and the entire pipe if it's all unused) if we need it. > -Daniel > Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior. ~Maarten
Op 18-02-16 om 12:07 schreef Daniel Vetter: > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: >> Because encoder <-> connector mapping is fixed when not moving to >> another crtc we can just reject connectors trying to steal an encoder >> from a connector not part of the state. This won't break MST on i915 >> because in that case connectors will be part of the state if you switch >> them between crtc's. If they're not they stay on the same crtc, and >> encoder stealing would have failed anyway. > We must do this for backwards compat. setCrtc on a connector that needs an > encoder already used on some other crtc is supposed to disable that > encoder (and the entire pipe if it's all unused) if we need it. Well that won't do what you expect it to do in certain cases.. crtc1 has MST_DP1 setcrtc(crtc1, { MST_DP1, MST_DP2 }); MST_DP1 gets disabled. or setcrtc fails with -EINVAL Seems to me that if you steal an encoder, failing with an error would be better. But for backward compat we could add some code to allow encoder stealing in the legacy setcrtc helper?
On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote: > Op 18-02-16 om 12:07 schreef Daniel Vetter: > > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: > >> Because encoder <-> connector mapping is fixed when not moving to > >> another crtc we can just reject connectors trying to steal an encoder > >> from a connector not part of the state. This won't break MST on i915 > >> because in that case connectors will be part of the state if you switch > >> them between crtc's. If they're not they stay on the same crtc, and > >> encoder stealing would have failed anyway. > > We must do this for backwards compat. setCrtc on a connector that needs an > > encoder already used on some other crtc is supposed to disable that > > encoder (and the entire pipe if it's all unused) if we need it. > > -Daniel > > > Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior. If you want to avoid stealing with atomic, supply _all_ the connectors/crtcs when doing an atomic modeset. After all the point of atomic is to do global updates. I don't think it makes sense to have a special case just for setcrtc, since it makes compat/transition unecesserily complicated. And we do this kind of stealing in other places too with public api objects, e.g. if you move a plane. -Daniel
On Thu, Feb 18, 2016 at 01:43:11PM +0100, Daniel Vetter wrote: > On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote: > > Op 18-02-16 om 12:07 schreef Daniel Vetter: > > > On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: > > >> Because encoder <-> connector mapping is fixed when not moving to > > >> another crtc we can just reject connectors trying to steal an encoder > > >> from a connector not part of the state. This won't break MST on i915 > > >> because in that case connectors will be part of the state if you switch > > >> them between crtc's. If they're not they stay on the same crtc, and > > >> encoder stealing would have failed anyway. > > > We must do this for backwards compat. setCrtc on a connector that needs an > > > encoder already used on some other crtc is supposed to disable that > > > encoder (and the entire pipe if it's all unused) if we need it. > > > -Daniel > > > > > Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior. > > If you want to avoid stealing with atomic, supply _all_ the > connectors/crtcs when doing an atomic modeset. After all the point of > atomic is to do global updates. I don't think it makes sense to have a > special case just for setcrtc, since it makes compat/transition > unecesserily complicated. I disagree. Having properties change magically is just a bad idea IMO. As far as checking for conflicts, IIRC I did that with a few bitmasks in my original atomic code, and it was pretty trivial. The current stealing code we have is way too complicated for what it does IMO. > And we do this kind of stealing in other places > too with public api objects, e.g. if you move a plane. Mm. What exactly do we steal with planes?
Hey, Op 18-02-16 om 13:59 schreef Ville Syrjälä: > On Thu, Feb 18, 2016 at 01:43:11PM +0100, Daniel Vetter wrote: >> On Thu, Feb 18, 2016 at 12:18:53PM +0100, Maarten Lankhorst wrote: >>> Op 18-02-16 om 12:07 schreef Daniel Vetter: >>>> On Thu, Feb 18, 2016 at 09:54:43AM +0100, Maarten Lankhorst wrote: >>>>> Because encoder <-> connector mapping is fixed when not moving to >>>>> another crtc we can just reject connectors trying to steal an encoder >>>>> from a connector not part of the state. This won't break MST on i915 >>>>> because in that case connectors will be part of the state if you switch >>>>> them between crtc's. If they're not they stay on the same crtc, and >>>>> encoder stealing would have failed anyway. >>>> We must do this for backwards compat. setCrtc on a connector that needs an >>>> encoder already used on some other crtc is supposed to disable that >>>> encoder (and the entire pipe if it's all unused) if we need it. >>>> -Daniel >>>> >>> Could this be done from the setcrtc helper? Seems with atomic that wouldn't be desired behavior. >> If you want to avoid stealing with atomic, supply _all_ the >> connectors/crtcs when doing an atomic modeset. After all the point of >> atomic is to do global updates. I don't think it makes sense to have a >> special case just for setcrtc, since it makes compat/transition >> unecesserily complicated. > I disagree. Having properties change magically is just a bad idea IMO. > As far as checking for conflicts, IIRC I did that with a few bitmasks > in my original atomic code, and it was pretty trivial. The current > stealing code we have is way too complicated for what it does IMO. > >> And we do this kind of stealing in other places >> too with public api objects, e.g. if you move a plane. > Mm. What exactly do we steal with planes? I've sent a v2 that works nicely with "[IGT PATCH] tests/kms_setmode: Add tests when not stealing encoders on same crtc." For all other calls disabling connectors to steal its encoder is rejected, but the behavior is preserved for set_config only. ~Maarten
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fa708559542c..b2028d592483 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -92,14 +92,21 @@ check_pending_encoder_assignment(struct drm_atomic_state *state, int conn_idx) { struct drm_connector *connector; - struct drm_connector_state *conn_state; - int i; - for_each_connector_in_state(state, connector, conn_state, i) { - if (i >= conn_idx) - break; + drm_for_each_connector(connector, state->dev) { + struct drm_connector_state *conn_state = + drm_atomic_get_existing_connector_state(state, + connector); - if (conn_state->best_encoder == new_encoder) + if (!conn_state) { + if (connector->state->best_encoder == new_encoder) + return false; + + continue; + } + + if (drm_connector_index(connector) < conn_idx && + conn_state->best_encoder == new_encoder) return false; } @@ -149,29 +156,19 @@ set_best_encoder(struct drm_atomic_state *state, static int steal_encoder(struct drm_atomic_state *state, - struct drm_encoder *encoder) + struct drm_encoder *encoder, + int conn_idx) { - 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; + int i; - /* - * 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)); - - list_for_each_entry(connector, &config->connector_list, head) { + 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 (i >= conn_idx) + break; if (connector_state->best_encoder != encoder || WARN_ON(!connector_state->crtc)) @@ -279,7 +276,7 @@ update_connector_routing(struct drm_atomic_state *state, return -EINVAL; } - ret = steal_encoder(state, new_encoder); + ret = steal_encoder(state, new_encoder, conn_idx); if (ret) { DRM_DEBUG_ATOMIC("Encoder stealing failed for [CONNECTOR:%d:%s]\n", connector->base.id,
Because encoder <-> connector mapping is fixed when not moving to another crtc we can just reject connectors trying to steal an encoder from a connector not part of the state. This won't break MST on i915 because in that case connectors will be part of the state if you switch them between crtc's. If they're not they stay on the same crtc, and encoder stealing would have failed anyway. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 23 deletions(-)