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

[Daniel Stone]

Hi,

On 22 May 2015 at 15:34, Daniel Vetter <daniel at 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