Re: [PATCH 11/11] drm: atomic: Add MODE_ID property

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel