| Message ID | 1432298094-22239-12-git-send-email-daniels@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote: > Atomic modesetting: now with modesetting support. > > Signed-off-by: Daniel Stone <daniels@collabora.com> > Tested-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_atomic.c | 12 +++++++++++- > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > include/drm/drm_crtc.h | 1 + > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 63fc58a..a4ab03a 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > { > struct drm_device *dev = crtc->dev; > struct drm_mode_config *config = &dev->mode_config; > + int ret; > > - /* FIXME: Mode prop is missing, which also controls ->enable. */ > if (property == config->prop_active) > state->active = val; > + else if (property == config->prop_mode_id) { > + struct drm_property_blob *mode = > + drm_property_lookup_blob(dev, val); > + ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > + if (mode) > + drm_property_unreference_blob(mode); Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. We need it for legacy helpers, but for atomic userspace this code means depending upon whether active or mode_id is first in the prop list it will get clamped or not, which isn't awesome. Imo that's a good reason to remove the ->active clamping from set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since that's only used internally and in legacy paths. Perhaps with a comment as to why (and why not in set_mode_prop). > + return ret; > + } > else if (crtc->funcs->atomic_set_property) > return crtc->funcs->atomic_set_property(crtc, state, property, val); > else > @@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc, > > if (property == config->prop_active) > *val = state->active; > + else if (property == config->prop_mode_id) > + *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > else if (crtc->funcs->atomic_get_property) > return crtc->funcs->atomic_get_property(crtc, state, property, val); > else > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index ee87045..8f18412 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > > if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { > drm_object_attach_property(&crtc->base, config->prop_active, 0); > + drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); > } > > return 0; > @@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_active = prop; > > + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, > + "MODE_ID", DRM_MODE_OBJECT_BLOB); Ah, here we go. Why not DRM_MODE_PROP_BLOB? Imo confusing to userspace that some prop blobs are objects and some are old blob props. -Daniel > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_mode_id = prop; > + > return 0; > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index c54fa4a..3b4d8a4 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1146,6 +1146,7 @@ struct drm_mode_config { > struct drm_property *prop_fb_id; > struct drm_property *prop_crtc_id; > struct drm_property *prop_active; > + struct drm_property *prop_mode_id; > > /* DVI-I properties */ > struct drm_property *dvi_i_subconnector_property; > -- > 2.4.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 22 May 2015 at 15:34, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote: >> - /* FIXME: Mode prop is missing, which also controls ->enable. */ >> if (property == config->prop_active) >> state->active = val; >> + else if (property == config->prop_mode_id) { >> + struct drm_property_blob *mode = >> + drm_property_lookup_blob(dev, val); >> + ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >> + if (mode) >> + drm_property_unreference_blob(mode); > > Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. > We need it for legacy helpers, but for atomic userspace this code means > depending upon whether active or mode_id is first in the prop list it will > get clamped or not, which isn't awesome. > > Imo that's a good reason to remove the ->active clamping from > set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since > that's only used internally and in legacy paths. Perhaps with a comment as > to why (and why not in set_mode_prop). No argument to not touching mode_changed, but I'm a bit confused about this one. We don't touch ->active when setting a mode (i.e. if active is true and you change MODE_ID without changing the ACTIVE prop, active remains true; if active is false and you change MODE_ID, active remains false, but it gains a mode). I've been working on the following assumption: - enable is a proxy for having a valid mode (enable == !!MODE_ID) - active cannot be true without enable also being true Setting MODE_ID to 0 removes the current mode, and when it becomes 0, we can no longer report back a mode that we're scanning out. So how would we have active == true (a particular mode is enabled and being displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in the same request is a broken configuration which should be rejected. Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not only pretty pathological but impossible with current libdrm, but you're right that it should be respected. So, I guess the way forward would be to not clamp either active or enable, check that the dependencies (active -> enable -> MODE_ID) are satisfied in drm_atomic_helper_check, and hope that everyone implementing their own check gets it right too. Sound good? Cheers, Daniel
On Mon, May 25, 2015 at 02:06:13PM +0100, Daniel Stone wrote: > On 22 May 2015 at 15:34, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote: > >> - /* FIXME: Mode prop is missing, which also controls ->enable. */ > >> if (property == config->prop_active) > >> state->active = val; > >> + else if (property == config->prop_mode_id) { > >> + struct drm_property_blob *mode = > >> + drm_property_lookup_blob(dev, val); > >> + ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >> + if (mode) > >> + drm_property_unreference_blob(mode); > > > > Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. > > We need it for legacy helpers, but for atomic userspace this code means > > depending upon whether active or mode_id is first in the prop list it will > > get clamped or not, which isn't awesome. > > > > Imo that's a good reason to remove the ->active clamping from > > set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since > > that's only used internally and in legacy paths. Perhaps with a comment as > > to why (and why not in set_mode_prop). > > No argument to not touching mode_changed, but I'm a bit confused about this one. > > We don't touch ->active when setting a mode (i.e. if active is true > and you change MODE_ID without changing the ACTIVE prop, active > remains true; if active is false and you change MODE_ID, active > remains false, but it gains a mode). > > I've been working on the following assumption: > - enable is a proxy for having a valid mode (enable == !!MODE_ID) > - active cannot be true without enable also being true > > Setting MODE_ID to 0 removes the current mode, and when it becomes 0, > we can no longer report back a mode that we're scanning out. So how > would we have active == true (a particular mode is enabled and being > displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in > the same request is a broken configuration which should be rejected. > Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not > only pretty pathological but impossible with current libdrm, but > you're right that it should be respected. > > So, I guess the way forward would be to not clamp either active or > enable, check that the dependencies (active -> enable -> MODE_ID) are > satisfied in drm_atomic_helper_check, and hope that everyone > implementing their own check gets it right too. Just to summarize the irc discussion: - ->enable is a derived state and should == !!MODE_ID - ->active is it's own independent atomic property, and as a rule we shouldn't try to "intelligently" patch up what userspace passes in, but instead just reject invalid configurations. Hence no auto-clamping for active, but proper computation of enable - There's no risk that some drivers will get the active vs. enable checks wrong, they're core drm code: Everything in drm_atomic.c is non-optional (and that's the hunk where you've removed the check). Maybe that was part of the confusion, since you're description above sounds like you put this in the helper library? -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 63fc58a..a4ab03a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct drm_mode_config *config = &dev->mode_config; + int ret; - /* FIXME: Mode prop is missing, which also controls ->enable. */ if (property == config->prop_active) state->active = val; + else if (property == config->prop_mode_id) { + struct drm_property_blob *mode = + drm_property_lookup_blob(dev, val); + ret = drm_atomic_set_mode_prop_for_crtc(state, mode); + if (mode) + drm_property_unreference_blob(mode); + return ret; + } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else @@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc, if (property == config->prop_active) *val = state->active; + else if (property == config->prop_mode_id) + *val = (state->mode_blob) ? state->mode_blob->base.id : 0; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ee87045..8f18412 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); + drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); } return 0; @@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_active = prop; + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, + "MODE_ID", DRM_MODE_OBJECT_BLOB); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_mode_id = prop; + return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c54fa4a..3b4d8a4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1146,6 +1146,7 @@ struct drm_mode_config { struct drm_property *prop_fb_id; struct drm_property *prop_crtc_id; struct drm_property *prop_active; + struct drm_property *prop_mode_id; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property;