* [PATCH] drm/atomic-helper: Check encoder/crtc constraints @ 2015-11-18 17:46 Daniel Vetter 2015-11-19 8:50 ` [Intel-gfx] " Daniel Stone ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Daniel Vetter @ 2015-11-18 17:46 UTC (permalink / raw) To: Intel Graphics Development Cc: Daniel Vetter, Daniel Vetter, DRI Development, Daniel Stone This was totally lost when I originally created the atomic helpers. We probably should also check possible_clones in the helpers, but since the legacy ones didn't do that this is for a separate patch. Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Stone <daniels@collabora.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0c6f62168776..cfdc9931b08a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) return -EINVAL; } + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", + new_encoder->base.id, + new_encoder->name, + connector_state->crtc->base.id); + return -EINVAL; + } + if (new_encoder == connector_state->best_encoder) { DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n", connector->base.id, -- 2.5.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-18 17:46 [PATCH] drm/atomic-helper: Check encoder/crtc constraints Daniel Vetter @ 2015-11-19 8:50 ` Daniel Stone 2015-11-19 10:12 ` Ville Syrjälä 2015-11-19 15:38 ` Jani Nikula 2 siblings, 0 replies; 7+ messages in thread From: Daniel Stone @ 2015-11-19 8:50 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development Hi, On 18 November 2015 at 17:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This was totally lost when I originally created the atomic helpers. > > We probably should also check possible_clones in the helpers, but > since the legacy ones didn't do that this is for a separate patch. Heh, before reading this chunk of the commit message, I also went looking for possible_clones and came to the same conclusion. On the grounds that great minds think alike (or fools never differ, not sure): Reviewed-by: Daniel Stone <daniels@collabora.com> Cheers, Daniel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-18 17:46 [PATCH] drm/atomic-helper: Check encoder/crtc constraints Daniel Vetter 2015-11-19 8:50 ` [Intel-gfx] " Daniel Stone @ 2015-11-19 10:12 ` Ville Syrjälä 2015-11-19 14:02 ` Daniel Vetter 2015-11-19 15:38 ` Jani Nikula 2 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-11-19 10:12 UTC (permalink / raw) To: Daniel Vetter Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone, DRI Development On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote: > This was totally lost when I originally created the atomic helpers. > > We probably should also check possible_clones in the helpers, but > since the legacy ones didn't do that this is for a separate patch. > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Stone <daniels@collabora.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> But the rest of update_connector_routing() still looks somewhat bonkers to me. For one, it assumes that both the old and new crtc for the connector are part of the atomic state, but drm_atomic_set_crtc_for_connector() only adds the new crtc, not the old one. > --- > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0c6f62168776..cfdc9931b08a 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) > return -EINVAL; > } > > + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", > + new_encoder->base.id, > + new_encoder->name, > + connector_state->crtc->base.id); > + return -EINVAL; > + } > + > if (new_encoder == connector_state->best_encoder) { > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n", > connector->base.id, > -- > 2.5.1 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-19 10:12 ` Ville Syrjälä @ 2015-11-19 14:02 ` Daniel Vetter 2015-11-19 14:24 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-11-19 14:02 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone, DRI Development, Daniel Vetter On Thu, Nov 19, 2015 at 12:12:28PM +0200, Ville Syrjälä wrote: > On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote: > > This was totally lost when I originally created the atomic helpers. > > > > We probably should also check possible_clones in the helpers, but > > since the legacy ones didn't do that this is for a separate patch. > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Daniel Stone <daniels@collabora.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > But the rest of update_connector_routing() still looks somewhat bonkers > to me. For one, it assumes that both the old and new crtc for the > connector are part of the atomic state, but drm_atomic_set_crtc_for_connector() > only adds the new crtc, not the old one. Nothing bonkers about it since drm_atomic_get_connector_state ensures that the corresponding crtc state is part of the update. Same logic applies for planes. -Daniel > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 0c6f62168776..cfdc9931b08a 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) > > return -EINVAL; > > } > > > > + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { > > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", > > + new_encoder->base.id, > > + new_encoder->name, > > + connector_state->crtc->base.id); > > + return -EINVAL; > > + } > > + > > if (new_encoder == connector_state->best_encoder) { > > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n", > > connector->base.id, > > -- > > 2.5.1 > > -- > Ville Syrjälä > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-19 14:02 ` Daniel Vetter @ 2015-11-19 14:24 ` Ville Syrjälä 2015-11-19 14:44 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-11-19 14:24 UTC (permalink / raw) To: Daniel Vetter Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone, DRI Development, Daniel Vetter On Thu, Nov 19, 2015 at 03:02:09PM +0100, Daniel Vetter wrote: > On Thu, Nov 19, 2015 at 12:12:28PM +0200, Ville Syrjälä wrote: > > On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote: > > > This was totally lost when I originally created the atomic helpers. > > > > > > We probably should also check possible_clones in the helpers, but > > > since the legacy ones didn't do that this is for a separate patch. > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Daniel Stone <daniels@collabora.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > But the rest of update_connector_routing() still looks somewhat bonkers > > to me. For one, it assumes that both the old and new crtc for the > > connector are part of the atomic state, but drm_atomic_set_crtc_for_connector() > > only adds the new crtc, not the old one. > > Nothing bonkers about it since drm_atomic_get_connector_state ensures that > the corresponding crtc state is part of the update. Same logic applies for > planes. Ah, that's where it was. OK, so it's just seems bonkers in other ways ;) Let's say we have as a starting point: crtc0 -> enc0 -> conn0 crtc1 -> enc1 -> conn1 Then we do an atomic_ioctl() -> set conn0 -> NULL -> set conn1 -> crtc0 -> set conn2 -> crtc1 And if enc1 is the best_encoder for both conn1 and conn2, it looks to me like we would silently end up with just: crtc1 -> enc1 -> conn2 Even though the user asked for: crtc0 -> conn1 crtc1 -> conn2 Also the code seems to assume that a single encoder can't drive multiple connectors at once. Is that by design or an accident? So I'm thinking maybe we should first check for routing conflicts in the new state and fail if any were found. After that we could go through the old state to steal stuff, and we should disallow stealing for the atomic ioctl totally and only allow if from setcrtc. > -Daniel > > > > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index 0c6f62168776..cfdc9931b08a 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) > > > return -EINVAL; > > > } > > > > > > + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { > > > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", > > > + new_encoder->base.id, > > > + new_encoder->name, > > > + connector_state->crtc->base.id); > > > + return -EINVAL; > > > + } > > > + > > > if (new_encoder == connector_state->best_encoder) { > > > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n", > > > connector->base.id, > > > -- > > > 2.5.1 > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-19 14:24 ` Ville Syrjälä @ 2015-11-19 14:44 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2015-11-19 14:44 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone, DRI Development On Thu, Nov 19, 2015 at 3:24 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Nov 19, 2015 at 03:02:09PM +0100, Daniel Vetter wrote: >> On Thu, Nov 19, 2015 at 12:12:28PM +0200, Ville Syrjälä wrote: >> > On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote: >> > > This was totally lost when I originally created the atomic helpers. >> > > >> > > We probably should also check possible_clones in the helpers, but >> > > since the legacy ones didn't do that this is for a separate patch. >> > > >> > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > Cc: Daniel Stone <daniels@collabora.com> >> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> > >> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > But the rest of update_connector_routing() still looks somewhat bonkers >> > to me. For one, it assumes that both the old and new crtc for the >> > connector are part of the atomic state, but drm_atomic_set_crtc_for_connector() >> > only adds the new crtc, not the old one. >> >> Nothing bonkers about it since drm_atomic_get_connector_state ensures that >> the corresponding crtc state is part of the update. Same logic applies for >> planes. > > Ah, that's where it was. OK, so it's just seems bonkers in other ways ;) > > Let's say we have as a starting point: > crtc0 -> enc0 -> conn0 > crtc1 -> enc1 -> conn1 > > Then we do an atomic_ioctl() > -> set conn0 -> NULL > -> set conn1 -> crtc0 > -> set conn2 -> crtc1 > > And if enc1 is the best_encoder for both conn1 and conn2, > it looks to me like we would silently end up with just: > crtc1 -> enc1 -> conn2 > > Even though the user asked for: > crtc0 -> conn1 > crtc1 -> conn2 Yeah this is what will happen, since perfect backwards compat to old Vjuserspace. We could fix this (probably by outright disabling encoder stealing), but I'm not too concerned really: Practically it's impossible to ask for this config (since you can't plug in both HDMI and DP). Same holds for other drivers, where encoder routing is at most used to select between dvi and crt encoders for DVI-I/D. > Also the code seems to assume that a single encoder can't drive multiple > connectors at once. Is that by design or an accident? That's by design - the cloning mask it as the encoder level, so hw cloning happens between crtc/encders. > So I'm thinking maybe we should first check for routing conflicts in the > new state and fail if any were found. After that we could go through the > old state to steal stuff, and we should disallow stealing for the atomic > ioctl totally and only allow if from setcrtc. Otoh I guess we could do that too, just for consistency. Simples approach would be a bool in connector_state which we leave at false in duplicate_state. Then on the first set_crtc_for_connector we set it to true and refuse to steal from such connectors. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/atomic-helper: Check encoder/crtc constraints 2015-11-18 17:46 [PATCH] drm/atomic-helper: Check encoder/crtc constraints Daniel Vetter 2015-11-19 8:50 ` [Intel-gfx] " Daniel Stone 2015-11-19 10:12 ` Ville Syrjälä @ 2015-11-19 15:38 ` Jani Nikula 2 siblings, 0 replies; 7+ messages in thread From: Jani Nikula @ 2015-11-19 15:38 UTC (permalink / raw) To: Intel Graphics Development Cc: Daniel Vetter, Daniel Stone, DRI Development, Daniel Vetter On Wed, 18 Nov 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This was totally lost when I originally created the atomic helpers. > > We probably should also check possible_clones in the helpers, but > since the legacy ones didn't do that this is for a separate patch. > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Stone <daniels@collabora.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Pushed to our topic/drm-fixes branch, thanks for the patch and review. BR, Jani. > --- > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0c6f62168776..cfdc9931b08a 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) > return -EINVAL; > } > > + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", > + new_encoder->base.id, > + new_encoder->name, > + connector_state->crtc->base.id); > + return -EINVAL; > + } > + > if (new_encoder == connector_state->best_encoder) { > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n", > connector->base.id, -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-19 15:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-18 17:46 [PATCH] drm/atomic-helper: Check encoder/crtc constraints Daniel Vetter 2015-11-19 8:50 ` [Intel-gfx] " Daniel Stone 2015-11-19 10:12 ` Ville Syrjälä 2015-11-19 14:02 ` Daniel Vetter 2015-11-19 14:24 ` Ville Syrjälä 2015-11-19 14:44 ` Daniel Vetter 2015-11-19 15:38 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.