Hi,
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.
Sound good?
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
