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

2015-05-26 Thread Daniel Vetter 
On Mon, May 25, 2015 at 02:06:13PM +0100, Daniel Stone wrote:
> On 22 May 2015 at 15:34, Daniel Vetter  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

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

2015-05-25 Thread Daniel Stone 
Hi,

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

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

2015-05-22 Thread Daniel Vetter 
On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
> Atomic modesetting: now with modesetting support.
> 
> Signed-off-by: Daniel Stone 
> Tested-by: Sean Paul 
> ---
>  drivers/gpu/drm/drm_atomic.c | 12 +++-
>  drivers/gpu/drm/drm_crtc.c   |  7 +++
>  include/drm/drm_crtc.h   |  1 +
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 63fc58a..a4ab03a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  {
>   struct drm_device *dev = crtc->dev;
>   struct drm_mode_config *config = >mode_config;
> + int ret;
>  
> - /* 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).

> + return ret;
> + }
>   else if (crtc->funcs->atomic_set_property)
>   return crtc->funcs->atomic_set_property(crtc, state, property, 
> val);
>   else
> @@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  
>   if (property == config->prop_active)
>   *val = state->active;
> + else if (property == config->prop_mode_id)
> + *val = (state->mode_blob) ? state->mode_blob->base.id : 0;
>   else if (crtc->funcs->atomic_get_property)
>   return crtc->funcs->atomic_get_property(crtc, state, property, 
> val);
>   else
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ee87045..8f18412 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
> struct drm_crtc *crtc,
>  
>   if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>   drm_object_attach_property(>base, config->prop_active, 0);
> + drm_object_attach_property(>base, config->prop_mode_id, 
> 0);
>   }
>  
>   return 0;
> @@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.prop_active = prop;
>  
> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> + "MODE_ID", DRM_MODE_OBJECT_BLOB);

Ah, here we go. Why not DRM_MODE_PROP_BLOB? Imo confusing to userspace
that some prop blobs are objects and some are old blob props.
-Daniel

> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_mode_id = prop;
> +
>   return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c54fa4a..3b4d8a4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1146,6 +1146,7 @@ struct drm_mode_config {
>   struct drm_property *prop_fb_id;
>   struct drm_property *prop_crtc_id;
>   struct drm_property *prop_active;
> + struct drm_property *prop_mode_id;
>  
>   /* DVI-I properties */
>   struct drm_property *dvi_i_subconnector_property;
> -- 
> 2.4.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

2015-05-22 Thread Daniel Stone 
Atomic modesetting: now with modesetting support.

Signed-off-by: Daniel Stone 
Tested-by: Sean Paul 
---
 drivers/gpu/drm/drm_atomic.c | 12 +++-
 drivers/gpu/drm/drm_crtc.c   |  7 +++
 include/drm/drm_crtc.h   |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 63fc58a..a4ab03a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 {
struct drm_device *dev = crtc->dev;
struct drm_mode_config *config = >mode_config;
+   int ret;

-   /* 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);
+   return ret;
+   }
else if (crtc->funcs->atomic_set_property)
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
else
@@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,

if (property == config->prop_active)
*val = state->active;
+   else if (property == config->prop_mode_id)
+   *val = (state->mode_blob) ? state->mode_blob->base.id : 0;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
else
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ee87045..8f18412 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, 
struct drm_crtc *crtc,

if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
drm_object_attach_property(>base, config->prop_active, 0);
+   drm_object_attach_property(>base, config->prop_mode_id, 
0);
}

return 0;
@@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct 
drm_device *dev)
return -ENOMEM;
dev->mode_config.prop_active = prop;

+   prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
+   "MODE_ID", DRM_MODE_OBJECT_BLOB);
+   if (!prop)
+   return -ENOMEM;
+   dev->mode_config.prop_mode_id = prop;
+
return 0;
 }

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c54fa4a..3b4d8a4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1146,6 +1146,7 @@ struct drm_mode_config {
struct drm_property *prop_fb_id;
struct drm_property *prop_crtc_id;
struct drm_property *prop_active;
+   struct drm_property *prop_mode_id;

/* DVI-I properties */
struct drm_property *dvi_i_subconnector_property;
-- 
2.4.1