On Mon, May 25, 2015 at 02:06:13PM +0100, Daniel Stone wrote:
> On 22 May 2015 at 15:34, Daniel Vetter <daniel@xxxxxxxx> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
