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