[PATCH 00/13] Atomic Properties

[PATCH 00/13] Atomic Properties

[Rob Clark]

Now that we have core atomic helpers, let's expose that to userspace!
The first step here is to, for drivers supporting atomic, expose all
the core plane/CRTC/connector state as properties.  Once this is in
place, all that remains is the atomic ioctl to allow userspace to
atomically set a bunch of properties on a bunch of objects (RFC patch
at the end).

All of the core properties have their set/get/check's handled  by the
drm_atomic core, so drivers needn't be bothered about that.

NOTE: that there is one notable omission here.  The 'MODE' property
on the CRTC.  Without this, you can do atomic pageflip but not yet
atomic modeset.  I am debating between three paths for handling MODE:

 1) Add back blob property support to atomic ioctl
 2) Add ioctls to create/destroy blob properties, and then simply
    pass blob property id to atomic ioctl.
 3) Stick our head in the sands and expose mode as a bunch of descrete
    properties.

Of these, at the moment I prefer #2, since it is more consistent with
how userspace currently reads blob properties, and it avoids making the
atomic ioctl function even more gnarly.

The whole patchset can also be found at:

 http://cgit.freedesktop.org/~robclark/linux/log/?h=atomic-properties
 git://people.freedesktop.org/~robclark/linux atomic-properties

Rob Clark (13):
  drm: allow property validation for refcnted props
  drm: store property instead of id in obj attachment
  drm: get rid of direct property value access
  drm: add atomic_set_property wrappers
  drm: add atomic_get_property
  drm: add atomic hook to read property plus helper
  drm: small property creation cleanup
  drm: tweak getconnector locking
  drm/atomic: atomic_check functions
  drm/atomic: atomic plane properties
  drm/atomic: atomic connector properties
  drm/msm: atomic property support
  RFC: drm: Atomic modeset ioctl

 Documentation/DocBook/drm.tmpl                     |  83 ++-
 drivers/gpu/drm/drm_atomic.c                       | 613 +++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c                | 105 +++-
 drivers/gpu/drm/drm_crtc.c                         | 230 ++++++--
 drivers/gpu/drm/drm_ioctl.c                        |   1 +
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |   1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c           |   1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |   1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c          |   1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c           |   1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c          |   2 +
 drivers/gpu/drm/msm/msm_drv.c                      |   1 +
 include/drm/drm_atomic.h                           |  22 +
 include/drm/drm_atomic_helper.h                    |   2 +
 include/drm/drm_crtc.h                             |  50 +-
 include/uapi/drm/drm.h                             |   1 +
 include/uapi/drm/drm_mode.h                        |  21 +
 17 files changed, 1063 insertions(+), 73 deletions(-)

-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/13] drm: allow property validation for refcnted props

[Rob Clark]

We can already have object properties, which technically can be fb's.
Switch the property validation logic over to a get/put style interface
so it can take a ref to refcnt'd objects, and then drop that ref after
the driver has a chance to take it's own ref.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 58 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5213da4..5ee4b88 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4193,12 +4193,22 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
 
-static bool drm_property_change_is_valid(struct drm_property *property,
-					 uint64_t value)
+/* Some properties could refer to dynamic refcnt'd objects, or things that
+ * need special locking to handle lifetime issues (ie. to ensure the prop
+ * value doesn't become invalid part way through the property update due to
+ * race).  The value returned by reference via 'obj' should be passed back
+ * to drm_property_change_valid_put() after the property is set (and the
+ * object to which the property is attached has a chance to take it's own
+ * reference).
+ */
+static bool drm_property_change_valid_get(struct drm_property *property,
+					 uint64_t value, struct drm_mode_object **ref)
 {
 	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
 		return false;
 
+	*ref = NULL;
+
 	if (drm_property_type_is(property, DRM_MODE_PROP_RANGE)) {
 		if (value < property->values[0] || value > property->values[1])
 			return false;
@@ -4219,19 +4229,23 @@ static bool drm_property_change_is_valid(struct drm_property *property,
 		/* Only the driver knows */
 		return true;
 	} else if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
-		struct drm_mode_object *obj;
 		/* a zero value for an object property translates to null: */
 		if (value == 0)
 			return true;
-		/*
-		 * NOTE: use _object_find() directly to bypass restriction on
-		 * looking up refcnt'd objects (ie. fb's).  For a refcnt'd
-		 * object this could race against object finalization, so it
-		 * simply tells us that the object *was* valid.  Which is good
-		 * enough.
-		 */
-		obj = _object_find(property->dev, value, property->values[0]);
-		return obj != NULL;
+
+		/* handle refcnt'd objects specially: */
+		if (property->values[0] == DRM_MODE_OBJECT_FB) {
+			struct drm_framebuffer *fb;
+			fb = drm_framebuffer_lookup(property->dev, value);
+			if (fb) {
+				*ref = &fb->base;
+				return true;
+			} else {
+				return false;
+			}
+		} else {
+			return _object_find(property->dev, value, property->values[0]) != NULL;
+		}
 	} else {
 		int i;
 		for (i = 0; i < property->num_values; i++)
@@ -4241,6 +4255,18 @@ static bool drm_property_change_is_valid(struct drm_property *property,
 	}
 }
 
+static void drm_property_change_valid_put(struct drm_property *property,
+		struct drm_mode_object *ref)
+{
+	if (!ref)
+		return;
+
+	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
+		if (property->values[0] == DRM_MODE_OBJECT_FB)
+			drm_framebuffer_unreference(obj_to_fb(ref));
+	}
+}
+
 /**
  * drm_mode_connector_property_set_ioctl - set the current value of a connector property
  * @dev: DRM device
@@ -4429,8 +4455,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	struct drm_mode_object *arg_obj;
 	struct drm_mode_object *prop_obj;
 	struct drm_property *property;
-	int ret = -EINVAL;
-	int i;
+	int i, ret = -EINVAL;
+	struct drm_mode_object *ref;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -4460,7 +4486,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	}
 	property = obj_to_property(prop_obj);
 
-	if (!drm_property_change_is_valid(property, arg->value))
+	if (!drm_property_change_valid_get(property, arg->value, &ref))
 		goto out;
 
 	switch (arg_obj->type) {
@@ -4477,6 +4503,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		break;
 	}
 
+	drm_property_change_valid_put(property, ref);
+
 out:
 	drm_modeset_unlock_all(dev);
 	return ret;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/13] drm: store property instead of id in obj attachment

[Rob Clark]

Keep property pointer, instead of id, in per mode-object attachments.
This will simplify things in later patches.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
 include/drm/drm_crtc.h     |  7 ++++++-
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5ee4b88..2780a08 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2105,12 +2105,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 		prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
 		prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
 		for (i = 0; i < connector->properties.count; i++) {
-			if (put_user(connector->properties.ids[i],
-				     prop_ptr + copied)) {
+			struct drm_property *prop = connector->properties.properties[i];
+			if (put_user(prop->base.id, prop_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
-
 			if (put_user(connector->properties.values[i],
 				     prop_values + copied)) {
 				ret = -EFAULT;
@@ -3822,7 +3821,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
 		return;
 	}
 
-	obj->properties->ids[count] = property->base.id;
+	obj->properties->properties[count] = property;
 	obj->properties->values[count] = init_val;
 	obj->properties->count++;
 }
@@ -3847,7 +3846,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
 	int i;
 
 	for (i = 0; i < obj->properties->count; i++) {
-		if (obj->properties->ids[i] == property->base.id) {
+		if (obj->properties->properties[i] == property) {
 			obj->properties->values[i] = val;
 			return 0;
 		}
@@ -3877,7 +3876,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
 	int i;
 
 	for (i = 0; i < obj->properties->count; i++) {
-		if (obj->properties->ids[i] == property->base.id) {
+		if (obj->properties->properties[i] == property) {
 			*val = obj->properties->values[i];
 			return 0;
 		}
@@ -4413,8 +4412,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 		prop_values_ptr = (uint64_t __user *)(unsigned long)
 				  (arg->prop_values_ptr);
 		for (i = 0; i < props_count; i++) {
-			if (put_user(obj->properties->ids[i],
-				     props_ptr + copied)) {
+			struct drm_property *prop = obj->properties->properties[i];
+			if (put_user(prop->base.id, props_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
@@ -4472,7 +4471,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 		goto out;
 
 	for (i = 0; i < arg_obj->properties->count; i++)
-		if (arg_obj->properties->ids[i] == arg->prop_id)
+		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
 			break;
 
 	if (i == arg_obj->properties->count)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b863298..02758e8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -64,7 +64,12 @@ struct drm_mode_object {
 #define DRM_OBJECT_MAX_PROPERTY 24
 struct drm_object_properties {
 	int count;
-	uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
+	/* NOTE: if we ever start dynamically destroying properties (ie.
+	 * not at drm_mode_config_cleanup() time), then we'd have to do
+	 * a better job of detaching property from mode objects to avoid
+	 * dangling property pointers:
+	 */
+	struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
 	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/13] drm: get rid of direct property value access

[Rob Clark]

For atomic drivers, we won't use the values array but instead shunt
things off to obj->atomic_get_property().  So to simplify things make
all read/write of properties values go through the accessors.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++----
 include/drm/drm_crtc.h     |  3 +++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2780a08..481bb25 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2106,12 +2106,17 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 		prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
 		for (i = 0; i < connector->properties.count; i++) {
 			struct drm_property *prop = connector->properties.properties[i];
+			uint64_t val;
+
+			ret = drm_object_property_get_value(&connector->base, prop, &val);
+			if (ret)
+				goto out;
+
 			if (put_user(prop->base.id, prop_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
-			if (put_user(connector->properties.values[i],
-				     prop_values + copied)) {
+			if (put_user(val, prop_values + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
@@ -4413,12 +4418,18 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				  (arg->prop_values_ptr);
 		for (i = 0; i < props_count; i++) {
 			struct drm_property *prop = obj->properties->properties[i];
+			uint64_t val;
+
+			ret = drm_object_property_get_value(obj, prop, &val);
+			if (ret)
+				goto out;
+
 			if (put_user(prop->base.id, props_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
-			if (put_user(obj->properties->values[i],
-				     prop_values_ptr + copied)) {
+
+			if (put_user(val, prop_values_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
 			}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 02758e8..61ab3e5 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -70,6 +70,9 @@ struct drm_object_properties {
 	 * dangling property pointers:
 	 */
 	struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
+	/* do not read/write values directly, but use drm_object_property_get_value()
+	 * and drm_object_property_set_value():
+	 */
 	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/13] drm: add atomic_set_property wrappers

[Rob Clark]

As we add properties for all the standard plane/crtc/connector
attributes (in preperation for the atomic ioctl), we are going to want
to handle core state in core (rather than per driver).  Intercepting the
core properties will be easier if the atomic_set_property vfuncs are not
called directly, but instead have a mandatory wrapper function (which
will later serve as the point to intercept core properties).

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c        | 78 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
 include/drm/drm_atomic.h            |  9 +++++
 include/drm/drm_crtc.h              |  3 ++
 4 files changed, 96 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index ff5f034..1261ade 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -217,6 +217,28 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 
 /**
+ * drm_atomic_crtc_set_property - set property on connector
+ * @crtc: the drm CRTC to set a property on
+ * @state: the state object to update with the new property value
+ * @property: the property to set
+ * @val: the new property value
+ *
+ * Use this instead of calling crtc->atomic_set_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
+		struct drm_crtc_state *state, struct drm_property *property,
+		uint64_t val)
+{
+	if (crtc->funcs->atomic_set_property)
+		return crtc->funcs->atomic_set_property(crtc, state, property, val);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(drm_atomic_crtc_set_property);
+
+/**
  * drm_atomic_get_plane_state - get plane state
  * @state: global atomic state object
  * @plane: plane to get state object for
@@ -272,6 +294,28 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_plane_state);
 
 /**
+ * drm_atomic_plane_set_property - set property on plane
+ * @plane: the drm plane to set a property on
+ * @state: the state object to update with the new property value
+ * @property: the property to set
+ * @val: the new property value
+ *
+ * Use this instead of calling plane->atomic_set_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_plane_set_property(struct drm_plane *plane,
+		struct drm_plane_state *state, struct drm_property *property,
+		uint64_t val)
+{
+	if (plane->funcs->atomic_set_property)
+		return plane->funcs->atomic_set_property(plane, state, property, val);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(drm_atomic_plane_set_property);
+
+/**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
  * @connector: connector to get state object for
@@ -343,6 +387,40 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_connector_state);
 
 /**
+ * drm_atomic_connector_set_property - set property on connector.
+ * @connector: the drm connector to set a property on
+ * @state: the state object to update with the new property value
+ * @property: the property to set
+ * @val: the new property value
+ *
+ * Use this instead of calling connector->atomic_set_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_connector_set_property(struct drm_connector *connector,
+		struct drm_connector_state *state, struct drm_property *property,
+		uint64_t val)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->dpms_property) {
+		/* setting DPMS property requires special handling, which
+		 * is done in legacy setprop path for us.  Disallow (for
+		 * now?) atomic writes to DPMS property:
+		 */
+		return -EINVAL;
+	} else if (connector->funcs->atomic_set_property) {
+		return connector->funcs->atomic_set_property(connector,
+				state, property, val);
+	} else {
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL(drm_atomic_connector_set_property);
+
+/**
  * drm_atomic_set_crtc_for_plane - set crtc for plane
  * @state: the incoming atomic state
  * @plane: the plane whose incoming state to update
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4a78a77..d42fdb1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1558,8 +1558,8 @@ retry:
 		goto fail;
 	}
 
-	ret = crtc->funcs->atomic_set_property(crtc, crtc_state,
-					       property, val);
+	ret = drm_atomic_crtc_set_property(crtc, crtc_state,
+			property, val);
 	if (ret)
 		goto fail;
 
@@ -1617,8 +1617,8 @@ retry:
 		goto fail;
 	}
 
-	ret = plane->funcs->atomic_set_property(plane, plane_state,
-					       property, val);
+	ret = drm_atomic_plane_set_property(plane, plane_state,
+			property, val);
 	if (ret)
 		goto fail;
 
@@ -1676,8 +1676,8 @@ retry:
 		goto fail;
 	}
 
-	ret = connector->funcs->atomic_set_property(connector, connector_state,
-					       property, val);
+	ret = drm_atomic_connector_set_property(connector, connector_state,
+			property, val);
 	if (ret)
 		goto fail;
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index ad22295..b0834dc 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -38,12 +38,21 @@ void drm_atomic_state_free(struct drm_atomic_state *state);
 struct drm_crtc_state * __must_check
 drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 			  struct drm_crtc *crtc);
+int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
+		struct drm_crtc_state *state, struct drm_property *property,
+		uint64_t val);
 struct drm_plane_state * __must_check
 drm_atomic_get_plane_state(struct drm_atomic_state *state,
 			   struct drm_plane *plane);
+int drm_atomic_plane_set_property(struct drm_plane *plane,
+		struct drm_plane_state *state, struct drm_property *property,
+		uint64_t val);
 struct drm_connector_state * __must_check
 drm_atomic_get_connector_state(struct drm_atomic_state *state,
 			       struct drm_connector *connector);
+int drm_atomic_connector_set_property(struct drm_connector *connector,
+		struct drm_connector_state *state, struct drm_property *property,
+		uint64_t val);
 
 int __must_check
 drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 61ab3e5..1b71f60 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -300,6 +300,7 @@ struct drm_crtc_state {
  * @atomic_duplicate_state: duplicate the atomic state for this CRTC
  * @atomic_destroy_state: destroy an atomic state for this CRTC
  * @atomic_set_property: set a property on an atomic state for this CRTC
+ *    (do not call directly, use drm_atomic_crtc_set_property())
  *
  * The drm_crtc_funcs structure is the central CRTC management structure
  * in the DRM.  Each CRTC controls one or more connectors (note that the name
@@ -483,6 +484,7 @@ struct drm_connector_state {
  * @atomic_duplicate_state: duplicate the atomic state for this connector
  * @atomic_destroy_state: destroy an atomic state for this connector
  * @atomic_set_property: set a property on an atomic state for this connector
+ *    (do not call directly, use drm_atomic_connector_set_property())
  *
  * Each CRTC may have one or more connectors attached to it.  The functions
  * below allow the core DRM code to control connectors, enumerate available modes,
@@ -743,6 +745,7 @@ struct drm_plane_state {
  * @atomic_duplicate_state: duplicate the atomic state for this plane
  * @atomic_destroy_state: destroy an atomic state for this plane
  * @atomic_set_property: set a property on an atomic state for this plane
+ *    (do not call directly, use drm_atomic_plane_set_property())
  */
 struct drm_plane_funcs {
 	int (*update_plane)(struct drm_plane *plane,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/13] drm: add atomic_get_property

[Rob Clark]

Since we won't be using the obj->properties->values[] array to shadow
property values for atomic drivers, we are going to need a vfunc for
getting prop values.  Add that along w/ mandatory wrapper fxns.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  9 ++++++
 include/drm/drm_crtc.h       | 18 +++++++++++
 3 files changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1261ade..4099b44 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -239,6 +239,28 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_atomic_crtc_set_property);
 
 /**
+ * drm_atomic_crtc_get_property - get property on connector
+ * @crtc: the drm CRTC to get a property on
+ * @state: the state object with the property value to read
+ * @property: the property to get
+ * @val: the property value (returned by reference)
+ *
+ * Use this instead of calling crtc->atomic_get_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
+		const struct drm_crtc_state *state,
+		struct drm_property *property, uint64_t *val)
+{
+	if (crtc->funcs->atomic_get_property)
+		return crtc->funcs->atomic_get_property(crtc, state, property, val);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(drm_atomic_crtc_get_property);
+
+/**
  * drm_atomic_get_plane_state - get plane state
  * @state: global atomic state object
  * @plane: plane to get state object for
@@ -316,6 +338,28 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 EXPORT_SYMBOL(drm_atomic_plane_set_property);
 
 /**
+ * drm_atomic_plane_get_property - get property on plane
+ * @plane: the drm plane to get a property on
+ * @state: the state object with the property value to read
+ * @property: the property to get
+ * @val: the property value (returned by reference)
+ *
+ * Use this instead of calling plane->atomic_get_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_plane_get_property(struct drm_plane *plane,
+		const struct drm_plane_state *state,
+		struct drm_property *property, uint64_t *val)
+{
+	if (plane->funcs->atomic_get_property)
+		return plane->funcs->atomic_get_property(plane, state, property, val);
+	return -EINVAL;
+}
+EXPORT_SYMBOL(drm_atomic_plane_get_property);
+
+/**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
  * @connector: connector to get state object for
@@ -421,6 +465,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_atomic_connector_set_property);
 
 /**
+ * drm_atomic_connector_get_property - get property on connector
+ * @connector: the drm connector to get a property on
+ * @state: the state object with the property value to read
+ * @property: the property to get
+ * @val: the property value (returned by reference)
+ *
+ * Use this instead of calling connector->atomic_get_property directly
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_connector_get_property(struct drm_connector *connector,
+		const struct drm_connector_state *state,
+		struct drm_property *property, uint64_t *val)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->dpms_property) {
+		*val = connector->dpms;
+	} else if (connector->funcs->atomic_get_property) {
+		return connector->funcs->atomic_get_property(connector,
+				state, property, val);
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_connector_get_property);
+
+/**
  * drm_atomic_set_crtc_for_plane - set crtc for plane
  * @state: the incoming atomic state
  * @plane: the plane whose incoming state to update
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index b0834dc..b34224a 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -41,18 +41,27 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_crtc_state *state, struct drm_property *property,
 		uint64_t val);
+int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
+		const struct drm_crtc_state *state,
+		struct drm_property *property, uint64_t *val);
 struct drm_plane_state * __must_check
 drm_atomic_get_plane_state(struct drm_atomic_state *state,
 			   struct drm_plane *plane);
 int drm_atomic_plane_set_property(struct drm_plane *plane,
 		struct drm_plane_state *state, struct drm_property *property,
 		uint64_t val);
+int drm_atomic_plane_get_property(struct drm_plane *plane,
+		const struct drm_plane_state *state,
+		struct drm_property *property, uint64_t *val);
 struct drm_connector_state * __must_check
 drm_atomic_get_connector_state(struct drm_atomic_state *state,
 			       struct drm_connector *connector);
 int drm_atomic_connector_set_property(struct drm_connector *connector,
 		struct drm_connector_state *state, struct drm_property *property,
 		uint64_t val);
+int drm_atomic_connector_get_property(struct drm_connector *connector,
+		const struct drm_connector_state *state,
+		struct drm_property *property, uint64_t *val);
 
 int __must_check
 drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1b71f60..e6cfdaf 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -301,6 +301,8 @@ struct drm_crtc_state {
  * @atomic_destroy_state: destroy an atomic state for this CRTC
  * @atomic_set_property: set a property on an atomic state for this CRTC
  *    (do not call directly, use drm_atomic_crtc_set_property())
+ * @atomic_get_property: get a property on an atomic state for this CRTC
+ *    (do not call directly, use drm_atomic_crtc_get_property())
  *
  * The drm_crtc_funcs structure is the central CRTC management structure
  * in the DRM.  Each CRTC controls one or more connectors (note that the name
@@ -360,6 +362,10 @@ struct drm_crtc_funcs {
 				   struct drm_crtc_state *state,
 				   struct drm_property *property,
 				   uint64_t val);
+	int (*atomic_get_property)(struct drm_crtc *crtc,
+				   const struct drm_crtc_state *state,
+				   struct drm_property *property,
+				   uint64_t *val);
 };
 
 /**
@@ -485,6 +491,8 @@ struct drm_connector_state {
  * @atomic_destroy_state: destroy an atomic state for this connector
  * @atomic_set_property: set a property on an atomic state for this connector
  *    (do not call directly, use drm_atomic_connector_set_property())
+ * @atomic_get_property: get a property on an atomic state for this connector
+ *    (do not call directly, use drm_atomic_connector_get_property())
  *
  * Each CRTC may have one or more connectors attached to it.  The functions
  * below allow the core DRM code to control connectors, enumerate available modes,
@@ -518,6 +526,10 @@ struct drm_connector_funcs {
 				   struct drm_connector_state *state,
 				   struct drm_property *property,
 				   uint64_t val);
+	int (*atomic_get_property)(struct drm_connector *connector,
+				   const struct drm_connector_state *state,
+				   struct drm_property *property,
+				   uint64_t *val);
 };
 
 /**
@@ -746,6 +758,8 @@ struct drm_plane_state {
  * @atomic_destroy_state: destroy an atomic state for this plane
  * @atomic_set_property: set a property on an atomic state for this plane
  *    (do not call directly, use drm_atomic_plane_set_property())
+ * @atomic_get_property: get a property on an atomic state for this plane
+ *    (do not call directly, use drm_atomic_plane_get_property())
  */
 struct drm_plane_funcs {
 	int (*update_plane)(struct drm_plane *plane,
@@ -769,6 +783,10 @@ struct drm_plane_funcs {
 				   struct drm_plane_state *state,
 				   struct drm_property *property,
 				   uint64_t val);
+	int (*atomic_get_property)(struct drm_plane *plane,
+				   const struct drm_plane_state *state,
+				   struct drm_property *property,
+				   uint64_t *val);
 };
 
 enum drm_plane_type {
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/13] drm: add atomic hook to read property plus helper

[Rob Clark]

Once a driver is using atomic helpers for modeset, the next step is to
switch over to atomic properties.  To do this, plug in the helper
function to your modeconfig funcs and be sure to implement the plane/
crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
objects which have driver custom properties.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
 include/drm/drm_atomic_helper.h     |  2 ++
 include/drm/drm_crtc.h              |  3 +++
 4 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d42fdb1..1a1ab34 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1703,6 +1703,52 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
 
 /**
+ * drm_atomic_helper_get_property - helper to read atomic property
+ * @obj: drm mode object whose property to read
+ * @property: the property to read
+ * @val: the read value, returned by reference
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_helper_get_property(struct drm_mode_object *obj,
+		struct drm_property *property, uint64_t *val)
+{
+	struct drm_device *dev = property->dev;
+	int ret;
+
+	switch (obj->type) {
+	case DRM_MODE_OBJECT_CONNECTOR: {
+		struct drm_connector *connector = obj_to_connector(obj);
+		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+		ret = connector->funcs->atomic_get_property(connector,
+				connector->state, property, val);
+		break;
+	}
+	case DRM_MODE_OBJECT_CRTC: {
+		struct drm_crtc *crtc = obj_to_crtc(obj);
+		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+		ret = crtc->funcs->atomic_get_property(crtc,
+				crtc->state, property, val);
+		break;
+	}
+	case DRM_MODE_OBJECT_PLANE: {
+		struct drm_plane *plane = obj_to_plane(obj);
+		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
+		ret = plane->funcs->atomic_get_property(plane,
+				plane->state, property, val);
+		break;
+	}
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_get_property);
+
+/**
  * drm_atomic_helper_page_flip - execute a legacy page flip
  * @crtc: DRM crtc
  * @fb: DRM framebuffer
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 481bb25..57cd950 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3878,8 +3878,17 @@ EXPORT_SYMBOL(drm_object_property_set_value);
 int drm_object_property_get_value(struct drm_mode_object *obj,
 				  struct drm_property *property, uint64_t *val)
 {
+	struct drm_mode_config *config = &property->dev->mode_config;
 	int i;
 
+	/* read-only properties bypass atomic mechanism and still store
+	 * their value in obj->properties->values[].. mostly to avoid
+	 * having to deal w/ EDID and similar props in atomic paths:
+	 */
+	if (config->funcs->atomic_get_property &&
+			!(property->flags & DRM_MODE_PROP_IMMUTABLE))
+		return config->funcs->atomic_get_property(obj, property, val);
+
 	for (i = 0; i < obj->properties->count; i++) {
 		if (obj->properties->properties[i] == property) {
 			*val = obj->properties->values[i];
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index f956b41..5e72b82 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -74,6 +74,8 @@ int drm_atomic_helper_plane_set_property(struct drm_plane *plane,
 int drm_atomic_helper_connector_set_property(struct drm_connector *connector,
 					struct drm_property *property,
 					uint64_t val);
+int drm_atomic_helper_get_property(struct drm_mode_object *obj,
+			   struct drm_property *property, uint64_t *val);
 int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e6cfdaf..ca8ba72 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -941,6 +941,7 @@ struct drm_mode_set {
  * struct drm_mode_config_funcs - basic driver provided mode setting functions
  * @fb_create: create a new framebuffer object
  * @output_poll_changed: function to handle output configuration changes
+ * @atomic_get_property: atomic way to read property value
  * @atomic_check: check whether a give atomic state update is possible
  * @atomic_commit: commit an atomic state update previously verified with
  * 	atomic_check()
@@ -954,6 +955,8 @@ struct drm_mode_config_funcs {
 					     struct drm_mode_fb_cmd2 *mode_cmd);
 	void (*output_poll_changed)(struct drm_device *dev);
 
+	int (*atomic_get_property)(struct drm_mode_object *obj,
+				struct drm_property *property, uint64_t *val);
 	int (*atomic_check)(struct drm_device *dev,
 			    struct drm_atomic_state *a);
 	int (*atomic_commit)(struct drm_device *dev,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/13] drm: small property creation cleanup

[Rob Clark]

Getting ready to add a lot more standard properties for atomic.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 57cd950..96965ec 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1330,33 +1330,40 @@ EXPORT_SYMBOL(drm_plane_force_disable);
 
 static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 {
-	struct drm_property *edid;
-	struct drm_property *dpms;
-	struct drm_property *dev_path;
+	struct drm_property *prop;
 
 	/*
 	 * Standard properties (apply to all connectors)
 	 */
-	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
 				   DRM_MODE_PROP_IMMUTABLE,
 				   "EDID", 0);
-	dev->mode_config.edid_property = edid;
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.edid_property = prop;
 
-	dpms = drm_property_create_enum(dev, 0,
+	prop = drm_property_create_enum(dev, 0,
 				   "DPMS", drm_dpms_enum_list,
 				   ARRAY_SIZE(drm_dpms_enum_list));
-	dev->mode_config.dpms_property = dpms;
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.dpms_property = prop;
 
-	dev_path = drm_property_create(dev,
+	prop = drm_property_create(dev,
 				       DRM_MODE_PROP_BLOB |
 				       DRM_MODE_PROP_IMMUTABLE,
 				       "PATH", 0);
-	dev->mode_config.path_property = dev_path;
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.path_property = prop;
 
-	dev->mode_config.tile_property = drm_property_create(dev,
+	prop = drm_property_create(dev,
 							     DRM_MODE_PROP_BLOB |
 							     DRM_MODE_PROP_IMMUTABLE,
 							     "TILE", 0);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.tile_property = prop;
 
 	return 0;
 }
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/13] drm: tweak getconnector locking

[Rob Clark]

We need to hold connection_mutex as we read the properties.  Easiest
thing is just widen the scope where connection_mutex is held.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 96965ec..62f5dc8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2043,6 +2043,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
 
 	mutex_lock(&dev->mode_config.mutex);
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 
 	connector = drm_connector_find(dev, out_resp->connector_id);
 	if (!connector) {
@@ -2076,14 +2077,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->mm_height = connector->display_info.height_mm;
 	out_resp->subpixel = connector->display_info.subpixel_order;
 	out_resp->connection = connector->status;
-	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-
 	encoder = drm_connector_get_encoder(connector);
 	if (encoder)
 		out_resp->encoder_id = encoder->base.id;
 	else
 		out_resp->encoder_id = 0;
-	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
@@ -2149,6 +2147,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 	out_resp->count_encoders = encoders_count;
 
 out:
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return ret;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/13] drm/atomic: atomic_check functions

[Rob Clark]

Add functions to check core plane/crtc state.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c        | 87 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++++++++++------
 include/drm/drm_atomic.h            |  4 ++
 3 files changed, 124 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 4099b44..afb830d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -261,6 +261,27 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_atomic_crtc_get_property);
 
 /**
+ * drm_atomic_crtc_check - check crtc state
+ * @crtc: crtc to check
+ * @state: crtc state to check
+ *
+ * Provides core sanity checks for crtc state.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_crtc_check(struct drm_crtc *crtc,
+		struct drm_crtc_state *state)
+{
+	/* TODO anything to check?  I think if drivers want to enforce that
+	 * primary layer covers entire screen, they should do that in their
+	 * own crtc->atomic_check() vfunc..
+	 */
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_crtc_check);
+
+/**
  * drm_atomic_get_plane_state - get plane state
  * @state: global atomic state object
  * @plane: plane to get state object for
@@ -360,6 +381,72 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
 EXPORT_SYMBOL(drm_atomic_plane_get_property);
 
 /**
+ * drm_atomic_plane_check - check plane state
+ * @plane: plane to check
+ * @state: plane state to check
+ *
+ * Provides core sanity checks for plane state.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_atomic_plane_check(struct drm_plane *plane,
+		struct drm_plane_state *state)
+{
+	unsigned int fb_width, fb_height;
+	unsigned int i;
+
+	/* either *both* CRTC and FB must be set, or neither */
+	if (WARN_ON(state->crtc && !state->fb)) {
+		DRM_DEBUG_KMS("CRTC set but no FB\n");
+		return -EINVAL;
+	} else if (WARN_ON(state->fb && !state->crtc)) {
+		DRM_DEBUG_KMS("FB set but no CRTC\n");
+		return -EINVAL;
+	}
+
+	/* if disabled, we don't care about the rest of the state: */
+	if (!state->crtc)
+		return 0;
+
+	/* Check whether this plane is usable on this CRTC */
+	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
+		DRM_DEBUG_KMS("Invalid crtc for plane\n");
+		return -EINVAL;
+	}
+
+	/* Check whether this plane supports the fb pixel format. */
+	for (i = 0; i < plane->format_count; i++)
+		if (state->fb->pixel_format == plane->format_types[i])
+			break;
+	if (i == plane->format_count) {
+		DRM_DEBUG_KMS("Invalid pixel format %s\n",
+			      drm_get_format_name(state->fb->pixel_format));
+		return -EINVAL;
+	}
+
+	fb_width = state->fb->width << 16;
+	fb_height = state->fb->height << 16;
+
+	/* Make sure source coordinates are inside the fb. */
+	if (state->src_w > fb_width ||
+	    state->src_x > fb_width - state->src_w ||
+	    state->src_h > fb_height ||
+	    state->src_y > fb_height - state->src_h) {
+		DRM_DEBUG_KMS("Invalid source coordinates "
+			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
+			      state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
+			      state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
+			      state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
+			      state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_plane_check);
+
+/**
  * drm_atomic_get_connector_state - get connector state
  * @state: global atomic state object
  * @connector: connector to get state object for
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1a1ab34..55b6981 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -407,6 +407,37 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	return mode_fixup(state);
 }
 
+static int plane_check(struct drm_plane *plane, struct drm_plane_state *state)
+{
+	struct drm_plane_helper_funcs *funcs = plane->helper_private;
+	int ret;
+
+	ret = drm_atomic_plane_check(plane, state);
+	if (ret)
+		return ret;
+
+
+	if (funcs && funcs->atomic_check)
+		ret = funcs->atomic_check(plane, state);
+
+	return ret;
+}
+
+static int crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
+{
+	struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
+	int ret;
+
+	ret = drm_atomic_crtc_check(crtc, state);
+	if (ret)
+		return ret;
+
+	if (funcs && funcs->atomic_check)
+		ret = funcs->atomic_check(crtc, state);
+
+	return ret;
+}
+
 /**
  * drm_atomic_helper_check - validate state object
  * @dev: DRM device
@@ -429,21 +460,15 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	int i, ret = 0;
 
 	for (i = 0; i < nplanes; i++) {
-		struct drm_plane_helper_funcs *funcs;
 		struct drm_plane *plane = state->planes[i];
 		struct drm_plane_state *plane_state = state->plane_states[i];
 
 		if (!plane)
 			continue;
 
-		funcs = plane->helper_private;
-
 		drm_atomic_helper_plane_changed(state, plane_state, plane);
 
-		if (!funcs || !funcs->atomic_check)
-			continue;
-
-		ret = funcs->atomic_check(plane, plane_state);
+		ret = plane_check(plane, plane_state);
 		if (ret) {
 			DRM_DEBUG_KMS("[PLANE:%d] atomic check failed\n",
 				      plane->base.id);
@@ -452,18 +477,12 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	}
 
 	for (i = 0; i < ncrtcs; i++) {
-		struct drm_crtc_helper_funcs *funcs;
 		struct drm_crtc *crtc = state->crtcs[i];
 
 		if (!crtc)
 			continue;
 
-		funcs = crtc->helper_private;
-
-		if (!funcs || !funcs->atomic_check)
-			continue;
-
-		ret = funcs->atomic_check(crtc, state->crtc_states[i]);
+		ret = crtc_check(crtc, state->crtc_states[i]);
 		if (ret) {
 			DRM_DEBUG_KMS("[CRTC:%d] atomic check failed\n",
 				      crtc->base.id);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index b34224a..742d027 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -44,6 +44,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		const struct drm_crtc_state *state,
 		struct drm_property *property, uint64_t *val);
+int drm_atomic_crtc_check(struct drm_crtc *crtc,
+		struct drm_crtc_state *state);
 struct drm_plane_state * __must_check
 drm_atomic_get_plane_state(struct drm_atomic_state *state,
 			   struct drm_plane *plane);
@@ -53,6 +55,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 int drm_atomic_plane_get_property(struct drm_plane *plane,
 		const struct drm_plane_state *state,
 		struct drm_property *property, uint64_t *val);
+int drm_atomic_plane_check(struct drm_plane *plane,
+		struct drm_plane_state *state);
 struct drm_connector_state * __must_check
 drm_atomic_get_connector_state(struct drm_atomic_state *state,
 			       struct drm_connector *connector);
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/13] drm/atomic: atomic plane properties

[Rob Clark]

Expose the core plane state as properties, so they can be updated via
atomic ioctl.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 Documentation/DocBook/drm.tmpl | 74 ++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic.c   | 69 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_crtc.c     | 82 +++++++++++++++++++++++++++++++++++++++---
 include/drm/drm_crtc.h         | 10 ++++++
 4 files changed, 224 insertions(+), 11 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 4b592ff..282fa6b 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2564,7 +2564,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Description/Restrictions</td>
 	</tr>
 	<tr>
-	<td rowspan="25" valign="top" >DRM</td>
+	<td rowspan="35" valign="top" >DRM</td>
 	<td rowspan="4" valign="top" >Generic</td>
 	<td valign="top" >“EDID”</td>
 	<td valign="top" >BLOB | IMMUTABLE</td>
@@ -2594,7 +2594,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Contains tiling information for a connector.</td>
 	</tr>
 	<tr>
-	<td rowspan="1" valign="top" >Plane</td>
+	<td rowspan="11" valign="top" >Plane</td>
 	<td valign="top" >“type”</td>
 	<td valign="top" >ENUM | IMMUTABLE</td>
 	<td valign="top" >{ "Overlay", "Primary", "Cursor" }</td>
@@ -2602,6 +2602,76 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Plane type</td>
 	</tr>
 	<tr>
+	<td valign="top" >“SRC_X”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout source x coordinate in 16.16 fixed point (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“SRC_Y”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout source y coordinate in 16.16 fixed point (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“SRC_W”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout source width in 16.16 fixed point (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“SRC_H”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout source height in 16.16 fixed point (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“CRTC_X”</td>
+	<td valign="top" >SIGNED_RANGE</td>
+	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout CRTC (destination) x coordinate (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“CRTC_Y”</td>
+	<td valign="top" >SIGNED_RANGE</td>
+	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout CRTC (destination) y coordinate (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“CRTC_W”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout CRTC (destination) width (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“CRTC_H”</td>
+	<td valign="top" >RANGE</td>
+	<td valign="top" >Min=0, Max=UINT_MAX</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout CRTC (destination) height (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“FB_ID”</td>
+	<td valign="top" >OBJECT</td>
+	<td valign="top" >DRM_MODE_OBJECT_FB</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >Scanout framebuffer (atomic)</td>
+	</tr>
+	<tr>
+	<td valign="top" >“CRTC_ID”</td>
+	<td valign="top" >OBJECT</td>
+	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
+	<td valign="top" >Plane</td>
+	<td valign="top" >CRTC that plane is attached to (atomic)</td>
+	</tr>
+	<tr>
 	<td rowspan="2" valign="top" >DVI-I</td>
 	<td valign="top" >“subconnector”</td>
 	<td valign="top" >ENUM</td>
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index afb830d..c09a05a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -352,9 +352,41 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 		struct drm_plane_state *state, struct drm_property *property,
 		uint64_t val)
 {
-	if (plane->funcs->atomic_set_property)
-		return plane->funcs->atomic_set_property(plane, state, property, val);
-	return -EINVAL;
+	struct drm_device *dev = plane->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->prop_fb_id) {
+		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
+		drm_atomic_set_fb_for_plane(state, fb);
+		if (fb)
+			drm_framebuffer_unreference(fb);
+	} else if (property == config->prop_crtc_id) {
+		struct drm_crtc *crtc = drm_crtc_find(dev, val);
+		return drm_atomic_set_crtc_for_plane(state->state, plane, crtc);
+	} else if (property == config->prop_crtc_x) {
+		state->crtc_x = U642I64(val);
+	} else if (property == config->prop_crtc_y) {
+		state->crtc_y = U642I64(val);
+	} else if (property == config->prop_crtc_w) {
+		state->crtc_w = val;
+	} else if (property == config->prop_crtc_h) {
+		state->crtc_h = val;
+	} else if (property == config->prop_src_x) {
+		state->src_x = val;
+	} else if (property == config->prop_src_y) {
+		state->src_y = val;
+	} else if (property == config->prop_src_w) {
+		state->src_w = val;
+	} else if (property == config->prop_src_h) {
+		state->src_h = val;
+	} else if (plane->funcs->atomic_set_property) {
+		return plane->funcs->atomic_set_property(plane, state,
+				property, val);
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_plane_set_property);
 
@@ -374,9 +406,36 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
 		const struct drm_plane_state *state,
 		struct drm_property *property, uint64_t *val)
 {
-	if (plane->funcs->atomic_get_property)
+	struct drm_device *dev = plane->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->prop_fb_id) {
+		*val = (state->fb) ? state->fb->base.id : 0;
+	} else if (property == config->prop_crtc_id) {
+		*val = (state->crtc) ? state->crtc->base.id : 0;
+	} else if (property == config->prop_crtc_x) {
+		*val = I642U64(state->crtc_x);
+	} else if (property == config->prop_crtc_y) {
+		*val = I642U64(state->crtc_y);
+	} else if (property == config->prop_crtc_w) {
+		*val = state->crtc_w;
+	} else if (property == config->prop_crtc_h) {
+		*val = state->crtc_h;
+	} else if (property == config->prop_src_x) {
+		*val = state->src_x;
+	} else if (property == config->prop_src_y) {
+		*val = state->src_y;
+	} else if (property == config->prop_src_w) {
+		*val = state->src_w;
+	} else if (property == config->prop_src_h) {
+		*val = state->src_h;
+	} else if (plane->funcs->atomic_get_property) {
 		return plane->funcs->atomic_get_property(plane, state, property, val);
-	return -EINVAL;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_plane_get_property);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 62f5dc8..fd6f91d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -819,6 +819,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
 		      mode->interlace ?  " interlaced" : "");
 }
 
+static bool
+has_atomic_properties(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	return !!config->funcs->atomic_get_property;
+}
+
 /**
  * drm_connector_init - Init a preallocated connector
  * @dev: DRM device
@@ -1174,6 +1181,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 			     const uint32_t *formats, uint32_t format_count,
 			     enum drm_plane_type type)
 {
+	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
 
 	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
@@ -1198,15 +1206,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	plane->possible_crtcs = possible_crtcs;
 	plane->type = type;
 
-	list_add_tail(&plane->head, &dev->mode_config.plane_list);
-	dev->mode_config.num_total_plane++;
+	list_add_tail(&plane->head, &config->plane_list);
+	config->num_total_plane++;
 	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
-		dev->mode_config.num_overlay_plane++;
+		config->num_overlay_plane++;
 
 	drm_object_attach_property(&plane->base,
-				   dev->mode_config.plane_type_property,
+				   config->plane_type_property,
 				   plane->type);
 
+	if (has_atomic_properties(dev)) {
+		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
+		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
+		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
+		drm_object_attach_property(&plane->base, config->prop_crtc_w, 0);
+		drm_object_attach_property(&plane->base, config->prop_crtc_h, 0);
+		drm_object_attach_property(&plane->base, config->prop_src_x, 0);
+		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
+		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
+		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_universal_plane_init);
@@ -1365,6 +1386,59 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.tile_property = prop;
 
+	prop = drm_property_create_range(dev, 0, "SRC_X", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_src_x = prop;
+
+	prop = drm_property_create_range(dev, 0, "SRC_Y", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_src_y = prop;
+
+	prop = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_src_w = prop;
+
+	prop = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_src_h = prop;
+
+	prop = drm_property_create_signed_range(dev, 0, "CRTC_X",
+			INT_MIN, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_x = prop;
+
+	prop = drm_property_create_signed_range(dev, 0, "CRTC_Y",
+			INT_MIN, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_y = prop;
+
+	prop = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_w = prop;
+
+	prop = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_h = prop;
+
+	prop = drm_property_create_object(dev, 0, "FB_ID", DRM_MODE_OBJECT_FB);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fb_id = prop;
+
+	prop = drm_property_create_object(dev, 0,
+			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_id = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ca8ba72..48fcd98 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1085,6 +1085,16 @@ struct drm_mode_config {
 	struct drm_property *tile_property;
 	struct drm_property *plane_type_property;
 	struct drm_property *rotation_property;
+	struct drm_property *prop_src_x;
+	struct drm_property *prop_src_y;
+	struct drm_property *prop_src_w;
+	struct drm_property *prop_src_h;
+	struct drm_property *prop_crtc_x;
+	struct drm_property *prop_crtc_y;
+	struct drm_property *prop_crtc_w;
+	struct drm_property *prop_crtc_h;
+	struct drm_property *prop_fb_id;
+	struct drm_property *prop_crtc_id;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/13] drm/atomic: atomic connector properties

[Rob Clark]

Expose the core connector state as properties so it can be updated via
atomic ioctl.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 Documentation/DocBook/drm.tmpl | 11 +++++++++--
 drivers/gpu/drm/drm_atomic.c   |  9 +++++++--
 drivers/gpu/drm/drm_crtc.c     | 13 +++++++++----
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 282fa6b..15cb9b9 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2564,8 +2564,8 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Description/Restrictions</td>
 	</tr>
 	<tr>
-	<td rowspan="35" valign="top" >DRM</td>
-	<td rowspan="4" valign="top" >Generic</td>
+	<td rowspan="36" valign="top" >DRM</td>
+	<td rowspan="5" valign="top" >Connector</td>
 	<td valign="top" >“EDID”</td>
 	<td valign="top" >BLOB | IMMUTABLE</td>
 	<td valign="top" >0</td>
@@ -2594,6 +2594,13 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Contains tiling information for a connector.</td>
 	</tr>
 	<tr>
+	<td valign="top" >“CRTC_ID”</td>
+	<td valign="top" >OBJECT</td>
+	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
+	<td valign="top" >Connector</td>
+	<td valign="top" >CRTC that connector is attached to (atomic)</td>
+	</tr>
+	<tr>
 	<td rowspan="11" valign="top" >Plane</td>
 	<td valign="top" >“type”</td>
 	<td valign="top" >ENUM | IMMUTABLE</td>
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c09a05a..71b48a0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -595,7 +595,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
 	struct drm_device *dev = connector->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 
-	if (property == config->dpms_property) {
+	if (property == config->prop_crtc_id) {
+		struct drm_crtc *crtc = drm_crtc_find(dev, val);
+		return drm_atomic_set_crtc_for_connector(state, crtc);
+	} else if (property == config->dpms_property) {
 		/* setting DPMS property requires special handling, which
 		 * is done in legacy setprop path for us.  Disallow (for
 		 * now?) atomic writes to DPMS property:
@@ -629,7 +632,9 @@ int drm_atomic_connector_get_property(struct drm_connector *connector,
 	struct drm_device *dev = connector->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 
-	if (property == config->dpms_property) {
+	if (property == config->prop_crtc_id) {
+		*val = (state->crtc) ? state->crtc->base.id : 0;
+	} else if (property == config->dpms_property) {
 		*val = connector->dpms;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index fd6f91d..39c3e06 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -844,6 +844,7 @@ int drm_connector_init(struct drm_device *dev,
 		       const struct drm_connector_funcs *funcs,
 		       int connector_type)
 {
+	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
 	struct ida *connector_ida =
 		&drm_connector_enum_list[connector_type].ida;
@@ -882,16 +883,20 @@ int drm_connector_init(struct drm_device *dev,
 
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much. */
-	list_add_tail(&connector->head, &dev->mode_config.connector_list);
-	dev->mode_config.num_connector++;
+	list_add_tail(&connector->head, &config->connector_list);
+	config->num_connector++;
 
 	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
 		drm_object_attach_property(&connector->base,
-					      dev->mode_config.edid_property,
+					      config->edid_property,
 					      0);
 
 	drm_object_attach_property(&connector->base,
-				      dev->mode_config.dpms_property, 0);
+				      config->dpms_property, 0);
+
+	if (has_atomic_properties(dev)) {
+		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
+	}
 
 	connector->debugfs_entry = NULL;
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/13] drm/msm: atomic property support

[Rob Clark]

Not too much to do, as we haven't exposed any driver custom properties
yet, so all we need to do is plug in the helpers.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c          | 1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c           | 1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c          | 1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c           | 1 +
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c          | 2 ++
 drivers/gpu/drm/msm/msm_drv.c                      | 1 +
 7 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index fbebb04..3db9e71 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -399,6 +399,7 @@ static const struct drm_connector_funcs hdmi_connector_funcs = {
 	.detect = hdmi_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = hdmi_connector_destroy,
+	.set_property = drm_atomic_helper_connector_set_property,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
index 786efb3..2e6e431 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c
@@ -501,6 +501,7 @@ static const struct drm_crtc_funcs mdp4_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = mdp4_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
+	.set_property = drm_atomic_helper_crtc_set_property,
 	.cursor_set = mdp4_crtc_cursor_set,
 	.cursor_move = mdp4_crtc_cursor_move,
 	.reset = drm_atomic_helper_crtc_reset,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
index 4ddc28e1..583baac 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c
@@ -98,6 +98,7 @@ static const struct drm_connector_funcs mdp4_lvds_connector_funcs = {
 	.detect = mdp4_lvds_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = mdp4_lvds_connector_destroy,
+	.set_property = drm_atomic_helper_connector_set_property,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
index e6d31db..d102d98 100644
--- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
@@ -60,6 +60,7 @@ static const struct drm_plane_funcs mdp4_plane_funcs = {
 		.update_plane = drm_atomic_helper_update_plane,
 		.disable_plane = drm_atomic_helper_disable_plane,
 		.destroy = mdp4_plane_destroy,
+		.set_property = drm_atomic_helper_plane_set_property,
 		.reset = drm_atomic_helper_plane_reset,
 		.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 		.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
index 538967d..17fc4ae 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
@@ -386,6 +386,7 @@ static const struct drm_crtc_funcs mdp5_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = mdp5_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
+	.set_property = drm_atomic_helper_crtc_set_property,
 	.reset = drm_atomic_helper_crtc_reset,
 	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
index 67a4e80..134b47b 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
@@ -136,6 +136,8 @@ static const struct drm_plane_funcs mdp5_plane_funcs = {
 		.update_plane = drm_atomic_helper_update_plane,
 		.disable_plane = drm_atomic_helper_disable_plane,
 		.destroy = mdp5_plane_destroy,
+		.set_property = drm_atomic_helper_plane_set_property,
+		/* TODO expose zpos property and override atomic_get/set: */
 		.reset = mdp5_plane_reset,
 		.atomic_duplicate_state = mdp5_plane_duplicate_state,
 		.atomic_destroy_state = mdp5_plane_destroy_state,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index d3b791b..6cc8513 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -29,6 +29,7 @@ static void msm_fb_output_poll_changed(struct drm_device *dev)
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = msm_framebuffer_create,
 	.output_poll_changed = msm_fb_output_poll_changed,
+	.atomic_get_property = drm_atomic_helper_get_property,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = msm_atomic_commit,
 };
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Rob Clark]

The atomic modeset ioctl can be used to push any number of new values
for object properties. The driver can then check the full device
configuration as single unit, and try to apply the changes atomically.

The ioctl simply takes a list of object IDs and property IDs and their
values.

Originally based on a patch from Ville Syrjälä, although it has mutated
(mutilated?) enough since then that you probably shouldn't blame it on
him ;-)

TODO:
 * hide behind moduleparam initially
 * either bring back blob property support, or add ioctls for create/
   destroy blob properties so they can be passed in by id in this
   ioctl.  I'm leaning towards the latter approach as (a) it is more
   in line with how blob properties are read, and (b) the atomic
   ioctl function is big enough already.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c | 308 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c   |   4 +-
 drivers/gpu/drm/drm_ioctl.c  |   1 +
 include/drm/drm_crtc.h       |   6 +
 include/uapi/drm/drm.h       |   1 +
 include/uapi/drm/drm_mode.h  |  21 +++
 6 files changed, 339 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 71b48a0..e7a45ec 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -960,3 +960,311 @@ int drm_atomic_async_commit(struct drm_atomic_state *state)
 	return config->funcs->atomic_commit(state->dev, state, true);
 }
 EXPORT_SYMBOL(drm_atomic_async_commit);
+
+/*
+ * The big monstor ioctl
+ */
+
+static struct drm_pending_vblank_event *create_vblank_event(
+		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
+{
+	struct drm_pending_vblank_event *e = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	if (file_priv->event_space < sizeof e->event) {
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+		goto out;
+	}
+	file_priv->event_space -= sizeof e->event;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	e = kzalloc(sizeof *e, GFP_KERNEL);
+	if (e == NULL) {
+		spin_lock_irqsave(&dev->event_lock, flags);
+		file_priv->event_space += sizeof e->event;
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+		goto out;
+	}
+
+	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
+	e->event.base.length = sizeof e->event;
+	e->event.user_data = user_data;
+	e->base.event = &e->event.base;
+	e->base.file_priv = file_priv;
+	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
+
+out:
+	return e;
+}
+
+static void destroy_vblank_event(struct drm_device *dev,
+		struct drm_file *file_priv, struct drm_pending_vblank_event *e)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	file_priv->event_space += sizeof e->event;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+	kfree(e);
+}
+
+int drm_mode_atomic_ioctl(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_atomic *arg = data;
+	uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
+	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
+	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
+	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
+	unsigned int copied_objs, copied_props;
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_plane *plane;
+	unsigned plane_mask = 0;
+	int ret = 0;
+	unsigned int i, j;
+
+	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS)
+		return -EINVAL;
+
+	/* can't test and expect an event at the same time. */
+	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
+			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+		return -EINVAL;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = &ctx;
+
+retry:
+	copied_objs = 0;
+	copied_props = 0;
+
+	for (i = 0; i < arg->count_objs; i++) {
+		uint32_t obj_id, count_props;
+		struct drm_mode_object *obj;
+
+		if (get_user(obj_id, objs_ptr + copied_objs)) {
+			ret = -EFAULT;
+			goto fail;
+		}
+
+		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
+		if (!obj || !obj->properties) {
+			ret = -ENOENT;
+			goto fail;
+		}
+
+		if (obj->type == DRM_MODE_OBJECT_PLANE) {
+			plane = obj_to_plane(obj);
+			plane_mask |= (1 << drm_plane_index(plane));
+			plane->old_fb = plane->fb;
+		}
+
+		if (get_user(count_props, count_props_ptr + copied_objs)) {
+			ret = -EFAULT;
+			goto fail;
+		}
+
+		copied_objs++;
+
+		for (j = 0; j < count_props; j++) {
+			uint32_t prop_id;
+			uint64_t prop_value;
+			struct drm_property *prop;
+			struct drm_mode_object *ref;
+
+			if (get_user(prop_id, props_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto fail;
+			}
+
+			prop = drm_property_find(dev, prop_id);
+			if (!prop) {
+				ret = -ENOENT;
+				goto fail;
+			}
+
+			if (get_user(prop_value, prop_values_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto fail;
+			}
+
+			if (!drm_property_change_valid_get(prop, prop_value, &ref)) {
+				ret = -EINVAL;
+				goto fail;
+			}
+
+			switch (obj->type) {
+			case DRM_MODE_OBJECT_CONNECTOR: {
+				struct drm_connector *connector = obj_to_connector(obj);
+				struct drm_connector_state *connector_state;
+
+				connector_state = drm_atomic_get_connector_state(state, connector);
+				if (IS_ERR(connector_state)) {
+					ret = PTR_ERR(connector_state);
+					break;
+				}
+
+				ret = drm_atomic_connector_set_property(connector,
+						connector_state, prop, prop_value);
+				break;
+			}
+			case DRM_MODE_OBJECT_CRTC: {
+				struct drm_crtc *crtc = obj_to_crtc(obj);
+				struct drm_crtc_state *crtc_state;
+
+				crtc_state = drm_atomic_get_crtc_state(state, crtc);
+				if (IS_ERR(crtc_state)) {
+					ret = PTR_ERR(crtc_state);
+					break;
+				}
+
+				ret = drm_atomic_crtc_set_property(crtc,
+						crtc_state, prop, prop_value);
+				break;
+			}
+			case DRM_MODE_OBJECT_PLANE: {
+				struct drm_plane *plane = obj_to_plane(obj);
+				struct drm_plane_state *plane_state;
+
+				plane_state = drm_atomic_get_plane_state(state, plane);
+				if (IS_ERR(plane_state)) {
+					ret = PTR_ERR(plane_state);
+					break;
+				}
+
+				ret = drm_atomic_plane_set_property(plane,
+						plane_state, prop, prop_value);
+				break;
+			}
+			default:
+				ret = -EINVAL;
+				break;
+			}
+
+			copied_props++;
+
+			drm_property_change_valid_put(prop, ref);
+
+			if (ret)
+				goto fail;
+		}
+	}
+
+	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		int ncrtcs = dev->mode_config.num_crtc;
+
+		for (i = 0; i < ncrtcs; i++) {
+			struct drm_crtc_state *crtc_state = state->crtc_states[i];
+			struct drm_pending_vblank_event *e;
+
+			if (!crtc_state)
+				continue;
+
+			e = create_vblank_event(dev, file_priv, arg->user_data);
+			if (!e) {
+				ret = -ENOMEM;
+				goto fail;
+			}
+
+			crtc_state->event = e;
+		}
+	}
+
+	/* sanity check incoming state, because danvet wanted this
+	 * to be an even bigger monster ioctl and wanted to be able
+	 * to WARN_ON() in these cases in the helpers:
+	 */
+	drm_for_each_plane_mask(plane, dev, plane_mask) {
+		struct drm_plane_state *plane_state =
+			drm_atomic_get_plane_state(state, plane);
+
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto fail;
+		}
+
+		if (WARN_ON(plane_state->crtc && !plane_state->fb)) {
+			DRM_DEBUG_KMS("CRTC set but no FB\n");
+			ret = -EINVAL;
+			goto fail;
+		} else if (WARN_ON(plane_state->fb && !plane_state->crtc)) {
+			DRM_DEBUG_KMS("FB set but no CRTC\n");
+			ret = -EINVAL;
+			goto fail;
+		}
+	}
+
+	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
+		ret = drm_atomic_check_only(state);
+		/* _check_only() does not free state, unlike _commit() */
+		drm_atomic_state_free(state);
+	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
+		ret = drm_atomic_async_commit(state);
+	} else {
+		ret = drm_atomic_commit(state);
+	}
+
+	/* if succeeded, fixup legacy plane crtc/fb ptrs before dropping
+	 * locks (ie. while it is still safe to deref plane->state).  We
+	 * need to do this here because the driver entry points cannot
+	 * distinguish between legacy and atomic ioctls.
+	 */
+	drm_for_each_plane_mask(plane, dev, plane_mask) {
+		if (ret == 0) {
+			struct drm_framebuffer *new_fb = plane->state->fb;
+			if (new_fb)
+				drm_framebuffer_reference(new_fb);
+			plane->fb = new_fb;
+			plane->crtc = plane->state->crtc;
+		} else {
+			plane->old_fb = NULL;
+		}
+		if (plane->old_fb) {
+			drm_framebuffer_unreference(plane->old_fb);
+			plane->old_fb = NULL;
+		}
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+
+fail:
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		int ncrtcs = dev->mode_config.num_crtc;
+
+		for (i = 0; i < ncrtcs; i++) {
+			struct drm_crtc_state *crtc_state = state->crtc_states[i];
+
+			if (!crtc_state)
+				continue;
+
+			destroy_vblank_event(dev, file_priv, crtc_state->event);
+			crtc_state->event = NULL;
+		}
+	}
+
+	drm_atomic_state_free(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+
+backoff:
+	drm_atomic_state_clear(state);
+	drm_modeset_backoff(&ctx);
+
+	goto retry;
+}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 39c3e06..948d04d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4299,7 +4299,7 @@ EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
  * object to which the property is attached has a chance to take it's own
  * reference).
  */
-static bool drm_property_change_valid_get(struct drm_property *property,
+bool drm_property_change_valid_get(struct drm_property *property,
 					 uint64_t value, struct drm_mode_object **ref)
 {
 	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
@@ -4353,7 +4353,7 @@ static bool drm_property_change_valid_get(struct drm_property *property,
 	}
 }
 
-static void drm_property_change_valid_put(struct drm_property *property,
+void drm_property_change_valid_put(struct drm_property *property,
 		struct drm_mode_object *ref)
 {
 	if (!ref)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 00587a1..fe76815 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -620,6 +620,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 48fcd98..8330676 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1332,6 +1332,10 @@ extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
 extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
+extern bool drm_property_change_valid_get(struct drm_property *property,
+					 uint64_t value, struct drm_mode_object **ref);
+extern void drm_property_change_valid_put(struct drm_property *property,
+		struct drm_mode_object *ref);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
@@ -1423,6 +1427,8 @@ extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 				       struct drm_property *property,
 				       uint64_t value);
+extern int drm_mode_atomic_ioctl(struct drm_device *dev,
+				 void *data, struct drm_file *file_priv);
 
 extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b0b8556..9cea966 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -777,6 +777,7 @@ struct drm_prime_handle {
 #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES	DRM_IOWR(0xB9, struct drm_mode_obj_get_properties)
 #define DRM_IOCTL_MODE_OBJ_SETPROPERTY	DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
 #define DRM_IOCTL_MODE_CURSOR2		DRM_IOWR(0xBB, struct drm_mode_cursor2)
+#define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 86574b0..3459778 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+/* page-flip flags are valid, plus: */
+#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
+#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
+
+#define DRM_MODE_ATOMIC_FLAGS (\
+		DRM_MODE_PAGE_FLIP_EVENT |\
+		DRM_MODE_PAGE_FLIP_ASYNC |\
+		DRM_MODE_ATOMIC_TEST_ONLY |\
+		DRM_MODE_ATOMIC_NONBLOCK)
+
+struct drm_mode_atomic {
+	__u32 flags;
+	__u32 count_objs;
+	__u64 objs_ptr;
+	__u64 count_props_ptr;
+	__u64 props_ptr;
+	__u64 prop_values_ptr;
+	__u64 blob_values_ptr;  /* remove? */
+	__u64 user_data;
+};
+
 #endif
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Michel Dänzer]

On 17.12.2014 08:05, Rob Clark wrote:
> The atomic modeset ioctl can be used to push any number of new values
> for object properties. The driver can then check the full device
> configuration as single unit, and try to apply the changes atomically.
> 
> The ioctl simply takes a list of object IDs and property IDs and their
> values.

[...]

> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 86574b0..3459778 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
>  	uint32_t handle;
>  };
>  
> +/* page-flip flags are valid, plus: */
> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> +
> +#define DRM_MODE_ATOMIC_FLAGS (\
> +		DRM_MODE_PAGE_FLIP_EVENT |\
> +		DRM_MODE_PAGE_FLIP_ASYNC |\
> +		DRM_MODE_ATOMIC_TEST_ONLY |\
> +		DRM_MODE_ATOMIC_NONBLOCK)
> +
> +struct drm_mode_atomic {
> +	__u32 flags;
> +	__u32 count_objs;
> +	__u64 objs_ptr;
> +	__u64 count_props_ptr;
> +	__u64 props_ptr;
> +	__u64 prop_values_ptr;
> +	__u64 blob_values_ptr;  /* remove? */
> +	__u64 user_data;
> +};
> +
>  #endif
> 

The new ioctl(s) should take an explicit parameter specifying when the
changes should take effect. And since variable refresh rate displays are
becoming mainstream, that parameter should probably be a timestamp
rather than a frame counter.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Pekka Paalanen]

On Wed, 17 Dec 2014 11:48:51 +0900
Michel Dänzer <michel@daenzer.net> wrote:

> On 17.12.2014 08:05, Rob Clark wrote:
> > The atomic modeset ioctl can be used to push any number of new values
> > for object properties. The driver can then check the full device
> > configuration as single unit, and try to apply the changes atomically.
> > 
> > The ioctl simply takes a list of object IDs and property IDs and their
> > values.
> 
> [...]
> 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 86574b0..3459778 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
> >  	uint32_t handle;
> >  };
> >  
> > +/* page-flip flags are valid, plus: */
> > +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> > +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> > +
> > +#define DRM_MODE_ATOMIC_FLAGS (\
> > +		DRM_MODE_PAGE_FLIP_EVENT |\
> > +		DRM_MODE_PAGE_FLIP_ASYNC |\
> > +		DRM_MODE_ATOMIC_TEST_ONLY |\
> > +		DRM_MODE_ATOMIC_NONBLOCK)
> > +
> > +struct drm_mode_atomic {
> > +	__u32 flags;
> > +	__u32 count_objs;
> > +	__u64 objs_ptr;
> > +	__u64 count_props_ptr;
> > +	__u64 props_ptr;
> > +	__u64 prop_values_ptr;
> > +	__u64 blob_values_ptr;  /* remove? */
> > +	__u64 user_data;
> > +};
> > +
> >  #endif
> > 
> 
> The new ioctl(s) should take an explicit parameter specifying when the
> changes should take effect. And since variable refresh rate displays are
> becoming mainstream, that parameter should probably be a timestamp
> rather than a frame counter.

That sounds cool to me, but also a rabbit hole. While having worked on
the Wayland Presentation queueing extension, I'd like to ask the
following questions:

- If you set the atomic kick to happen in the future, is there any way
  to cancel it? I'd be ok with not being able to cancel initially, but
  if one wants to add that later, we should already know how to
  reference this atomic submission in the cancel request. What if user
  space has a bug and schedules an update at one hour or three days from
  now, how would we abort that?

- Can you VT-switch or drop DRM master if you have a pending atomic
  update?

- Should one be able to set multiple pending atomic updates?

- If I schedule an atomic update for one CTRC, can I schedule another
  update for another CRTC before the first one completes? Or am I
  forced to gather all updates over all outputs in the same atomic
  submission even if I don't care about or want inter-output sync and
  the outputs might even be running at different refresh rates?
  (Actually, this seems to be a valid question even without any target
  time parameter.)

- If there can be multiple pending atomic updates on the same DRM
  device, is there any way to guarantee that the
  DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
  atomic update actually kicks in? Another update may have changed the
  configuration before this update kicks in, which means the overall
  state isn't the same that was tested.

- Does a pending atomic update prevent immediate (old style) KMS
  changes?

- Assuming hardware cannot do arbitrary time updates, how do you round
  the given timestamp? Strictly "not before" given time? Round to
  nearest possible time? The effect of required vs. unwanted sync to
  vblank?

- How would user space match the page flip event to the atomic
  submission it did?

I wonder if there is a way to postpone these hard(?) questions, so that
we could have atomic sooner and add scheduling later? I would imagine
solving everything above is quite some work.


Thanks,
pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Michel Dänzer]

On 17.12.2014 16:20, Pekka Paalanen wrote:
> On Wed, 17 Dec 2014 11:48:51 +0900
> Michel Dänzer <michel@daenzer.net> wrote:
> 
>> On 17.12.2014 08:05, Rob Clark wrote:
>>> The atomic modeset ioctl can be used to push any number of new values
>>> for object properties. The driver can then check the full device
>>> configuration as single unit, and try to apply the changes atomically.
>>>
>>> The ioctl simply takes a list of object IDs and property IDs and their
>>> values.
>>
>> [...]
>>
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index 86574b0..3459778 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
>>>  	uint32_t handle;
>>>  };
>>>  
>>> +/* page-flip flags are valid, plus: */
>>> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>>> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>>> +
>>> +#define DRM_MODE_ATOMIC_FLAGS (\
>>> +		DRM_MODE_PAGE_FLIP_EVENT |\
>>> +		DRM_MODE_PAGE_FLIP_ASYNC |\
>>> +		DRM_MODE_ATOMIC_TEST_ONLY |\
>>> +		DRM_MODE_ATOMIC_NONBLOCK)
>>> +
>>> +struct drm_mode_atomic {
>>> +	__u32 flags;
>>> +	__u32 count_objs;
>>> +	__u64 objs_ptr;
>>> +	__u64 count_props_ptr;
>>> +	__u64 props_ptr;
>>> +	__u64 prop_values_ptr;
>>> +	__u64 blob_values_ptr;  /* remove? */
>>> +	__u64 user_data;
>>> +};
>>> +
>>>  #endif
>>>
>>
>> The new ioctl(s) should take an explicit parameter specifying when the
>> changes should take effect. And since variable refresh rate displays are
>> becoming mainstream, that parameter should probably be a timestamp
>> rather than a frame counter.
> 
> That sounds cool to me, but also a rabbit hole. While having worked on
> the Wayland Presentation queueing extension, I'd like to ask the
> following questions:
> 
> - If you set the atomic kick to happen in the future, is there any way
>   to cancel it? I'd be ok with not being able to cancel initially, but
>   if one wants to add that later, we should already know how to
>   reference this atomic submission in the cancel request. What if user
>   space has a bug and schedules an update at one hour or three days from
>   now, how would we abort that?
> 
> - Can you VT-switch or drop DRM master if you have a pending atomic
>   update?
> 
> - Should one be able to set multiple pending atomic updates?
> 
> - If I schedule an atomic update for one CTRC, can I schedule another
>   update for another CRTC before the first one completes? Or am I
>   forced to gather all updates over all outputs in the same atomic
>   submission even if I don't care about or want inter-output sync and
>   the outputs might even be running at different refresh rates?
>   (Actually, this seems to be a valid question even without any target
>   time parameter.)
> 
> - If there can be multiple pending atomic updates on the same DRM
>   device, is there any way to guarantee that the
>   DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
>   atomic update actually kicks in? Another update may have changed the
>   configuration before this update kicks in, which means the overall
>   state isn't the same that was tested.
> 
> - Does a pending atomic update prevent immediate (old style) KMS
>   changes?
> 
> - Assuming hardware cannot do arbitrary time updates, how do you round
>   the given timestamp? Strictly "not before" given time? Round to
>   nearest possible time? The effect of required vs. unwanted sync to
>   vblank?
> 
> - How would user space match the page flip event to the atomic
>   submission it did?
> 
> I wonder if there is a way to postpone these hard(?) questions, so that
> we could have atomic sooner and add scheduling later? I would imagine
> solving everything above is quite some work.

I agree. The main reason I brought it up is because I'd like to avoid
getting into the same situation as with the current
DRM_IOCTL_MODE_PAGE_FLIP ioctl, which doesn't explicitly communicate
between userspace and kernel when the flip is supposed/expected to
occur. We recently had to jump through some hoops in the radeon kernel
driver to prevent flips from occurring sooner than expected by userspace.


But I also think it would be a shame at this point to add new ioctls
which aren't designed with variable refresh rate displays in mind.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Daniel Vetter]

On Wed, Dec 17, 2014 at 06:31:13PM +0900, Michel Dänzer wrote:
> On 17.12.2014 16:20, Pekka Paalanen wrote:
> > On Wed, 17 Dec 2014 11:48:51 +0900
> > Michel Dänzer <michel@daenzer.net> wrote:
> > 
> >> On 17.12.2014 08:05, Rob Clark wrote:
> >>> The atomic modeset ioctl can be used to push any number of new values
> >>> for object properties. The driver can then check the full device
> >>> configuration as single unit, and try to apply the changes atomically.
> >>>
> >>> The ioctl simply takes a list of object IDs and property IDs and their
> >>> values.
> >>
> >> [...]
> >>
> >>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >>> index 86574b0..3459778 100644
> >>> --- a/include/uapi/drm/drm_mode.h
> >>> +++ b/include/uapi/drm/drm_mode.h
> >>> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
> >>>  	uint32_t handle;
> >>>  };
> >>>  
> >>> +/* page-flip flags are valid, plus: */
> >>> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> >>> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> >>> +
> >>> +#define DRM_MODE_ATOMIC_FLAGS (\
> >>> +		DRM_MODE_PAGE_FLIP_EVENT |\
> >>> +		DRM_MODE_PAGE_FLIP_ASYNC |\
> >>> +		DRM_MODE_ATOMIC_TEST_ONLY |\
> >>> +		DRM_MODE_ATOMIC_NONBLOCK)
> >>> +
> >>> +struct drm_mode_atomic {
> >>> +	__u32 flags;
> >>> +	__u32 count_objs;
> >>> +	__u64 objs_ptr;
> >>> +	__u64 count_props_ptr;
> >>> +	__u64 props_ptr;
> >>> +	__u64 prop_values_ptr;
> >>> +	__u64 blob_values_ptr;  /* remove? */
> >>> +	__u64 user_data;
> >>> +};
> >>> +
> >>>  #endif
> >>>
> >>
> >> The new ioctl(s) should take an explicit parameter specifying when the
> >> changes should take effect. And since variable refresh rate displays are
> >> becoming mainstream, that parameter should probably be a timestamp
> >> rather than a frame counter.
> > 
> > That sounds cool to me, but also a rabbit hole. While having worked on
> > the Wayland Presentation queueing extension, I'd like to ask the
> > following questions:
> > 
> > - If you set the atomic kick to happen in the future, is there any way
> >   to cancel it? I'd be ok with not being able to cancel initially, but
> >   if one wants to add that later, we should already know how to
> >   reference this atomic submission in the cancel request. What if user
> >   space has a bug and schedules an update at one hour or three days from
> >   now, how would we abort that?
> > 
> > - Can you VT-switch or drop DRM master if you have a pending atomic
> >   update?
> > 
> > - Should one be able to set multiple pending atomic updates?
> > 
> > - If I schedule an atomic update for one CTRC, can I schedule another
> >   update for another CRTC before the first one completes? Or am I
> >   forced to gather all updates over all outputs in the same atomic
> >   submission even if I don't care about or want inter-output sync and
> >   the outputs might even be running at different refresh rates?
> >   (Actually, this seems to be a valid question even without any target
> >   time parameter.)
> > 
> > - If there can be multiple pending atomic updates on the same DRM
> >   device, is there any way to guarantee that the
> >   DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
> >   atomic update actually kicks in? Another update may have changed the
> >   configuration before this update kicks in, which means the overall
> >   state isn't the same that was tested.
> > 
> > - Does a pending atomic update prevent immediate (old style) KMS
> >   changes?
> > 
> > - Assuming hardware cannot do arbitrary time updates, how do you round
> >   the given timestamp? Strictly "not before" given time? Round to
> >   nearest possible time? The effect of required vs. unwanted sync to
> >   vblank?
> > 
> > - How would user space match the page flip event to the atomic
> >   submission it did?
> > 
> > I wonder if there is a way to postpone these hard(?) questions, so that
> > we could have atomic sooner and add scheduling later? I would imagine
> > solving everything above is quite some work.
> 
> I agree. The main reason I brought it up is because I'd like to avoid
> getting into the same situation as with the current
> DRM_IOCTL_MODE_PAGE_FLIP ioctl, which doesn't explicitly communicate
> between userspace and kernel when the flip is supposed/expected to
> occur. We recently had to jump through some hoops in the radeon kernel
> driver to prevent flips from occurring sooner than expected by userspace.

The current approach is to ask for a vblank event 1 frame earlier in the
ddx and schedule the flip when you receive that. And hope for the best.
There shouldn't be anything really required in the kernel.

> But I also think it would be a shame at this point to add new ioctls
> which aren't designed with variable refresh rate displays in mind.

Let's please not aim for the kitchen sink, we won't get this in anytime
soon otherwise. And there is _lots_ more little features all over that we
want but don't yet have:
- update rectangle for manual/selective update panels
- async flips/flip merging and precise semantics
- integration with android syncpts
- flip queues and cancelling flips
- ...

It doesn't make sense to add everything in v1. And the atomic ioctl
extends fairly well (maybe we need write-only properties for some of this
stuff). Worst case we get atomic_2/3/4 ioctls. After all we're fairly good
at rev'ing interface in drm.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm: allow property validation for refcnted props

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:29PM -0500, Rob Clark wrote:
> We can already have object properties, which technically can be fb's.
> Switch the property validation logic over to a get/put style interface
> so it can take a ref to refcnt'd objects, and then drop that ref after
> the driver has a chance to take it's own ref.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Yeah I think of all the options we've looked at this is the best wrt to
interface safety.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c | 58 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5213da4..5ee4b88 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4193,12 +4193,22 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>  
> -static bool drm_property_change_is_valid(struct drm_property *property,
> -					 uint64_t value)
> +/* Some properties could refer to dynamic refcnt'd objects, or things that
> + * need special locking to handle lifetime issues (ie. to ensure the prop
> + * value doesn't become invalid part way through the property update due to
> + * race).  The value returned by reference via 'obj' should be passed back
> + * to drm_property_change_valid_put() after the property is set (and the
> + * object to which the property is attached has a chance to take it's own
> + * reference).
> + */
> +static bool drm_property_change_valid_get(struct drm_property *property,
> +					 uint64_t value, struct drm_mode_object **ref)
>  {
>  	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
>  		return false;
>  
> +	*ref = NULL;
> +
>  	if (drm_property_type_is(property, DRM_MODE_PROP_RANGE)) {
>  		if (value < property->values[0] || value > property->values[1])
>  			return false;
> @@ -4219,19 +4229,23 @@ static bool drm_property_change_is_valid(struct drm_property *property,
>  		/* Only the driver knows */
>  		return true;
>  	} else if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
> -		struct drm_mode_object *obj;
>  		/* a zero value for an object property translates to null: */
>  		if (value == 0)
>  			return true;
> -		/*
> -		 * NOTE: use _object_find() directly to bypass restriction on
> -		 * looking up refcnt'd objects (ie. fb's).  For a refcnt'd
> -		 * object this could race against object finalization, so it
> -		 * simply tells us that the object *was* valid.  Which is good
> -		 * enough.
> -		 */
> -		obj = _object_find(property->dev, value, property->values[0]);
> -		return obj != NULL;
> +
> +		/* handle refcnt'd objects specially: */
> +		if (property->values[0] == DRM_MODE_OBJECT_FB) {
> +			struct drm_framebuffer *fb;
> +			fb = drm_framebuffer_lookup(property->dev, value);
> +			if (fb) {
> +				*ref = &fb->base;
> +				return true;
> +			} else {
> +				return false;
> +			}
> +		} else {
> +			return _object_find(property->dev, value, property->values[0]) != NULL;
> +		}
>  	} else {
>  		int i;
>  		for (i = 0; i < property->num_values; i++)
> @@ -4241,6 +4255,18 @@ static bool drm_property_change_is_valid(struct drm_property *property,
>  	}
>  }
>  
> +static void drm_property_change_valid_put(struct drm_property *property,
> +		struct drm_mode_object *ref)
> +{
> +	if (!ref)
> +		return;
> +
> +	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
> +		if (property->values[0] == DRM_MODE_OBJECT_FB)
> +			drm_framebuffer_unreference(obj_to_fb(ref));
> +	}
> +}
> +
>  /**
>   * drm_mode_connector_property_set_ioctl - set the current value of a connector property
>   * @dev: DRM device
> @@ -4429,8 +4455,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	struct drm_mode_object *arg_obj;
>  	struct drm_mode_object *prop_obj;
>  	struct drm_property *property;
> -	int ret = -EINVAL;
> -	int i;
> +	int i, ret = -EINVAL;
> +	struct drm_mode_object *ref;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -4460,7 +4486,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  	}
>  	property = obj_to_property(prop_obj);
>  
> -	if (!drm_property_change_is_valid(property, arg->value))
> +	if (!drm_property_change_valid_get(property, arg->value, &ref))
>  		goto out;
>  
>  	switch (arg_obj->type) {
> @@ -4477,6 +4503,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  		break;
>  	}
>  
> +	drm_property_change_valid_put(property, ref);
> +
>  out:
>  	drm_modeset_unlock_all(dev);
>  	return ret;
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm: add atomic_set_property wrappers

[Sean Paul]

On Tue, Dec 16, 2014 at 06:05:32PM -0500, Rob Clark wrote:
> As we add properties for all the standard plane/crtc/connector
> attributes (in preperation for the atomic ioctl), we are going to want
> to handle core state in core (rather than per driver).  Intercepting the
> core properties will be easier if the atomic_set_property vfuncs are not
> called directly, but instead have a mandatory wrapper function (which
> will later serve as the point to intercept core properties).
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Tiny copypasta nit in one comment.

> ---
>  drivers/gpu/drm/drm_atomic.c        | 78 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
>  include/drm/drm_atomic.h            |  9 +++++
>  include/drm/drm_crtc.h              |  3 ++
>  4 files changed, 96 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff5f034..1261ade 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -217,6 +217,28 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
>  /**
> + * drm_atomic_crtc_set_property - set property on connector

s/connector/crtc/

> + * @crtc: the drm CRTC to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling crtc->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	if (crtc->funcs->atomic_set_property)
> +		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_set_property);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -272,6 +294,28 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_plane_state);
>  
>  /**
> + * drm_atomic_plane_set_property - set property on plane
> + * @plane: the drm plane to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling plane->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> +		struct drm_plane_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	if (plane->funcs->atomic_set_property)
> +		return plane->funcs->atomic_set_property(plane, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_set_property);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> @@ -343,6 +387,40 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_connector_state);
>  
>  /**
> + * drm_atomic_connector_set_property - set property on connector.
> + * @connector: the drm connector to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling connector->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> +		struct drm_connector_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->dpms_property) {
> +		/* setting DPMS property requires special handling, which
> +		 * is done in legacy setprop path for us.  Disallow (for
> +		 * now?) atomic writes to DPMS property:
> +		 */
> +		return -EINVAL;
> +	} else if (connector->funcs->atomic_set_property) {
> +		return connector->funcs->atomic_set_property(connector,
> +				state, property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_connector_set_property);
> +
> +/**
>   * drm_atomic_set_crtc_for_plane - set crtc for plane
>   * @state: the incoming atomic state
>   * @plane: the plane whose incoming state to update
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4a78a77..d42fdb1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1558,8 +1558,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = crtc->funcs->atomic_set_property(crtc, crtc_state,
> -					       property, val);
> +	ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> @@ -1617,8 +1617,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = plane->funcs->atomic_set_property(plane, plane_state,
> -					       property, val);
> +	ret = drm_atomic_plane_set_property(plane, plane_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> @@ -1676,8 +1676,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = connector->funcs->atomic_set_property(connector, connector_state,
> -					       property, val);
> +	ret = drm_atomic_connector_set_property(connector, connector_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index ad22295..b0834dc 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -38,12 +38,21 @@ void drm_atomic_state_free(struct drm_atomic_state *state);
>  struct drm_crtc_state * __must_check
>  drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  			  struct drm_crtc *crtc);
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state, struct drm_property *property,
> +		uint64_t val);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> +		struct drm_plane_state *state, struct drm_property *property,
> +		uint64_t val);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> +		struct drm_connector_state *state, struct drm_property *property,
> +		uint64_t val);
>  
>  int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61ab3e5..1b71f60 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -300,6 +300,7 @@ struct drm_crtc_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this CRTC
>   * @atomic_destroy_state: destroy an atomic state for this CRTC
>   * @atomic_set_property: set a property on an atomic state for this CRTC
> + *    (do not call directly, use drm_atomic_crtc_set_property())
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -483,6 +484,7 @@ struct drm_connector_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this connector
>   * @atomic_destroy_state: destroy an atomic state for this connector
>   * @atomic_set_property: set a property on an atomic state for this connector
> + *    (do not call directly, use drm_atomic_connector_set_property())
>   *
>   * Each CRTC may have one or more connectors attached to it.  The functions
>   * below allow the core DRM code to control connectors, enumerate available modes,
> @@ -743,6 +745,7 @@ struct drm_plane_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this plane
>   * @atomic_destroy_state: destroy an atomic state for this plane
>   * @atomic_set_property: set a property on an atomic state for this plane
> + *    (do not call directly, use drm_atomic_plane_set_property())
>   */
>  struct drm_plane_funcs {
>  	int (*update_plane)(struct drm_plane *plane,
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/13] drm: add atomic_get_property

[Sean Paul]

On Tue, Dec 16, 2014 at 06:05:33PM -0500, Rob Clark wrote:
> Since we won't be using the obj->properties->values[] array to shadow
> property values for atomic drivers, we are going to need a vfunc for
> getting prop values.  Add that along w/ mandatory wrapper fxns.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Same nit here

> ---
>  drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  9 ++++++
>  include/drm/drm_crtc.h       | 18 +++++++++++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1261ade..4099b44 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -239,6 +239,28 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_atomic_crtc_set_property);
>  
>  /**
> + * drm_atomic_crtc_get_property - get property on connector

s/connector/crtc/

> + * @crtc: the drm CRTC to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling crtc->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> +		const struct drm_crtc_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	if (crtc->funcs->atomic_get_property)
> +		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_get_property);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -316,6 +338,28 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_plane_set_property);
>  
>  /**
> + * drm_atomic_plane_get_property - get property on plane
> + * @plane: the drm plane to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling plane->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_get_property(struct drm_plane *plane,
> +		const struct drm_plane_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	if (plane->funcs->atomic_get_property)
> +		return plane->funcs->atomic_get_property(plane, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_get_property);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> @@ -421,6 +465,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_atomic_connector_set_property);
>  
>  /**
> + * drm_atomic_connector_get_property - get property on connector
> + * @connector: the drm connector to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling connector->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_connector_get_property(struct drm_connector *connector,
> +		const struct drm_connector_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->dpms_property) {
> +		*val = connector->dpms;
> +	} else if (connector->funcs->atomic_get_property) {
> +		return connector->funcs->atomic_get_property(connector,
> +				state, property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_connector_get_property);
> +
> +/**
>   * drm_atomic_set_crtc_for_plane - set crtc for plane
>   * @state: the incoming atomic state
>   * @plane: the plane whose incoming state to update
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b0834dc..b34224a 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -41,18 +41,27 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> +		const struct drm_crtc_state *state,
> +		struct drm_property *property, uint64_t *val);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
>  int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		struct drm_plane_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_plane_get_property(struct drm_plane *plane,
> +		const struct drm_plane_state *state,
> +		struct drm_property *property, uint64_t *val);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
>  int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		struct drm_connector_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_connector_get_property(struct drm_connector *connector,
> +		const struct drm_connector_state *state,
> +		struct drm_property *property, uint64_t *val);
>  
>  int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1b71f60..e6cfdaf 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -301,6 +301,8 @@ struct drm_crtc_state {
>   * @atomic_destroy_state: destroy an atomic state for this CRTC
>   * @atomic_set_property: set a property on an atomic state for this CRTC
>   *    (do not call directly, use drm_atomic_crtc_set_property())
> + * @atomic_get_property: get a property on an atomic state for this CRTC
> + *    (do not call directly, use drm_atomic_crtc_get_property())
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -360,6 +362,10 @@ struct drm_crtc_funcs {
>  				   struct drm_crtc_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_crtc *crtc,
> +				   const struct drm_crtc_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  /**
> @@ -485,6 +491,8 @@ struct drm_connector_state {
>   * @atomic_destroy_state: destroy an atomic state for this connector
>   * @atomic_set_property: set a property on an atomic state for this connector
>   *    (do not call directly, use drm_atomic_connector_set_property())
> + * @atomic_get_property: get a property on an atomic state for this connector
> + *    (do not call directly, use drm_atomic_connector_get_property())
>   *
>   * Each CRTC may have one or more connectors attached to it.  The functions
>   * below allow the core DRM code to control connectors, enumerate available modes,
> @@ -518,6 +526,10 @@ struct drm_connector_funcs {
>  				   struct drm_connector_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_connector *connector,
> +				   const struct drm_connector_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  /**
> @@ -746,6 +758,8 @@ struct drm_plane_state {
>   * @atomic_destroy_state: destroy an atomic state for this plane
>   * @atomic_set_property: set a property on an atomic state for this plane
>   *    (do not call directly, use drm_atomic_plane_set_property())
> + * @atomic_get_property: get a property on an atomic state for this plane
> + *    (do not call directly, use drm_atomic_plane_get_property())
>   */
>  struct drm_plane_funcs {
>  	int (*update_plane)(struct drm_plane *plane,
> @@ -769,6 +783,10 @@ struct drm_plane_funcs {
>  				   struct drm_plane_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_plane *plane,
> +				   const struct drm_plane_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  enum drm_plane_type {
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/13] Atomic Properties

[Sean Paul]

On Tue, Dec 16, 2014 at 6:05 PM, Rob Clark <robdclark@gmail.com> wrote:
> Now that we have core atomic helpers, let's expose that to userspace!
> The first step here is to, for drivers supporting atomic, expose all
> the core plane/CRTC/connector state as properties.  Once this is in
> place, all that remains is the atomic ioctl to allow userspace to
> atomically set a bunch of properties on a bunch of objects (RFC patch
> at the end).
>
> All of the core properties have their set/get/check's handled  by the
> drm_atomic core, so drivers needn't be bothered about that.
>
> NOTE: that there is one notable omission here.  The 'MODE' property
> on the CRTC.  Without this, you can do atomic pageflip but not yet
> atomic modeset.  I am debating between three paths for handling MODE:
>
>  1) Add back blob property support to atomic ioctl
>  2) Add ioctls to create/destroy blob properties, and then simply
>     pass blob property id to atomic ioctl.
>  3) Stick our head in the sands and expose mode as a bunch of descrete
>     properties.
>
> Of these, at the moment I prefer #2, since it is more consistent with
> how userspace currently reads blob properties, and it avoids making the
> atomic ioctl function even more gnarly.
>

Sounds reasonable.

> The whole patchset can also be found at:
>
>  http://cgit.freedesktop.org/~robclark/linux/log/?h=atomic-properties
>  git://people.freedesktop.org/~robclark/linux atomic-properties
>

Unsurprisingly, since I took a pass last week, the set looks good to
me (aside from 2 comment nits, which I trust you can fix up). Feel
free to add my Rb to all 13 patches.

Reviewed-by: Sean Paul <seanpaul@chromium.org>



> Rob Clark (13):
>   drm: allow property validation for refcnted props
>   drm: store property instead of id in obj attachment
>   drm: get rid of direct property value access
>   drm: add atomic_set_property wrappers
>   drm: add atomic_get_property
>   drm: add atomic hook to read property plus helper
>   drm: small property creation cleanup
>   drm: tweak getconnector locking
>   drm/atomic: atomic_check functions
>   drm/atomic: atomic plane properties
>   drm/atomic: atomic connector properties
>   drm/msm: atomic property support
>   RFC: drm: Atomic modeset ioctl
>
>  Documentation/DocBook/drm.tmpl                     |  83 ++-
>  drivers/gpu/drm/drm_atomic.c                       | 613 +++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c                | 105 +++-
>  drivers/gpu/drm/drm_crtc.c                         | 230 ++++++--
>  drivers/gpu/drm/drm_ioctl.c                        |   1 +
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c          |   1 +
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_crtc.c           |   1 +
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c |   1 +
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c          |   1 +
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c           |   1 +
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c          |   2 +
>  drivers/gpu/drm/msm/msm_drv.c                      |   1 +
>  include/drm/drm_atomic.h                           |  22 +
>  include/drm/drm_atomic_helper.h                    |   2 +
>  include/drm/drm_crtc.h                             |  50 +-
>  include/uapi/drm/drm.h                             |   1 +
>  include/uapi/drm/drm_mode.h                        |  21 +
>  17 files changed, 1063 insertions(+), 73 deletions(-)
>
> --
> 2.1.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/13] drm: store property instead of id in obj attachment

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:30PM -0500, Rob Clark wrote:
> Keep property pointer, instead of id, in per mode-object attachments.
> This will simplify things in later patches.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c | 17 ++++++++---------
>  include/drm/drm_crtc.h     |  7 ++++++-
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5ee4b88..2780a08 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2105,12 +2105,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  		prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr);
>  		prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
>  		for (i = 0; i < connector->properties.count; i++) {
> -			if (put_user(connector->properties.ids[i],
> -				     prop_ptr + copied)) {
> +			struct drm_property *prop = connector->properties.properties[i];
> +			if (put_user(prop->base.id, prop_ptr + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> -
>  			if (put_user(connector->properties.values[i],
>  				     prop_values + copied)) {
>  				ret = -EFAULT;
> @@ -3822,7 +3821,7 @@ void drm_object_attach_property(struct drm_mode_object *obj,
>  		return;
>  	}
>  
> -	obj->properties->ids[count] = property->base.id;
> +	obj->properties->properties[count] = property;
>  	obj->properties->values[count] = init_val;
>  	obj->properties->count++;
>  }
> @@ -3847,7 +3846,7 @@ int drm_object_property_set_value(struct drm_mode_object *obj,
>  	int i;
>  
>  	for (i = 0; i < obj->properties->count; i++) {
> -		if (obj->properties->ids[i] == property->base.id) {
> +		if (obj->properties->properties[i] == property) {
>  			obj->properties->values[i] = val;
>  			return 0;
>  		}
> @@ -3877,7 +3876,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
>  	int i;
>  
>  	for (i = 0; i < obj->properties->count; i++) {
> -		if (obj->properties->ids[i] == property->base.id) {
> +		if (obj->properties->properties[i] == property) {
>  			*val = obj->properties->values[i];
>  			return 0;
>  		}
> @@ -4413,8 +4412,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  		prop_values_ptr = (uint64_t __user *)(unsigned long)
>  				  (arg->prop_values_ptr);
>  		for (i = 0; i < props_count; i++) {
> -			if (put_user(obj->properties->ids[i],
> -				     props_ptr + copied)) {
> +			struct drm_property *prop = obj->properties->properties[i];
> +			if (put_user(prop->base.id, props_ptr + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> @@ -4472,7 +4471,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  
>  	for (i = 0; i < arg_obj->properties->count; i++)
> -		if (arg_obj->properties->ids[i] == arg->prop_id)
> +		if (arg_obj->properties->properties[i]->base.id == arg->prop_id)
>  			break;
>  
>  	if (i == arg_obj->properties->count)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b863298..02758e8 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -64,7 +64,12 @@ struct drm_mode_object {
>  #define DRM_OBJECT_MAX_PROPERTY 24
>  struct drm_object_properties {
>  	int count;
> -	uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
> +	/* NOTE: if we ever start dynamically destroying properties (ie.
> +	 * not at drm_mode_config_cleanup() time), then we'd have to do
> +	 * a better job of detaching property from mode objects to avoid
> +	 * dangling property pointers:
> +	 */
> +	struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
>  	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
>  };
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/13] drm: get rid of direct property value access

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:31PM -0500, Rob Clark wrote:
> For atomic drivers, we won't use the values array but instead shunt
> things off to obj->atomic_get_property().  So to simplify things make
> all read/write of properties values go through the accessors.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 19 +++++++++++++++----
>  include/drm/drm_crtc.h     |  3 +++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 2780a08..481bb25 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2106,12 +2106,17 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  		prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr);
>  		for (i = 0; i < connector->properties.count; i++) {
>  			struct drm_property *prop = connector->properties.properties[i];
> +			uint64_t val;
> +
> +			ret = drm_object_property_get_value(&connector->base, prop, &val);
> +			if (ret)
> +				goto out;
> +
>  			if (put_user(prop->base.id, prop_ptr + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> -			if (put_user(connector->properties.values[i],
> -				     prop_values + copied)) {
> +			if (put_user(val, prop_values + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> @@ -4413,12 +4418,18 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  				  (arg->prop_values_ptr);
>  		for (i = 0; i < props_count; i++) {
>  			struct drm_property *prop = obj->properties->properties[i];
> +			uint64_t val;
> +
> +			ret = drm_object_property_get_value(obj, prop, &val);
> +			if (ret)
> +				goto out;
> +
>  			if (put_user(prop->base.id, props_ptr + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> -			if (put_user(obj->properties->values[i],
> -				     prop_values_ptr + copied)) {
> +
> +			if (put_user(val, prop_values_ptr + copied)) {
>  				ret = -EFAULT;
>  				goto out;
>  			}
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 02758e8..61ab3e5 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -70,6 +70,9 @@ struct drm_object_properties {
>  	 * dangling property pointers:
>  	 */
>  	struct drm_property *properties[DRM_OBJECT_MAX_PROPERTY];
> +	/* do not read/write values directly, but use drm_object_property_get_value()
> +	 * and drm_object_property_set_value():
> +	 */
>  	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
>  };
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm: add atomic_set_property wrappers

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:32PM -0500, Rob Clark wrote:
> As we add properties for all the standard plane/crtc/connector
> attributes (in preperation for the atomic ioctl), we are going to want
> to handle core state in core (rather than per driver).  Intercepting the
> core properties will be easier if the atomic_set_property vfuncs are not
> called directly, but instead have a mandatory wrapper function (which
> will later serve as the point to intercept core properties).
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Two suggestions for comment improvements below. With that addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c        | 78 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
>  include/drm/drm_atomic.h            |  9 +++++
>  include/drm/drm_crtc.h              |  3 ++
>  4 files changed, 96 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff5f034..1261ade 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -217,6 +217,28 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
>  /**
> + * drm_atomic_crtc_set_property - set property on connector
> + * @crtc: the drm CRTC to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling crtc->atomic_set_property directly

Maybe clarify a bit more here. What about

"This function directly processes generic properties (both mandatory and
optional ones) and calls into the driver's ->atomic_set_property hook for
anything else. To esnure consistent behavior this wrapper must be used
instead of calling the driver hook directly, even for driver private
properties."

A bit much perhaps, but kerneldoc is cheap ;-)

> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	if (crtc->funcs->atomic_set_property)
> +		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_set_property);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -272,6 +294,28 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_plane_state);
>  
>  /**
> + * drm_atomic_plane_set_property - set property on plane
> + * @plane: the drm plane to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling plane->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> +		struct drm_plane_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	if (plane->funcs->atomic_set_property)
> +		return plane->funcs->atomic_set_property(plane, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_set_property);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> @@ -343,6 +387,40 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_connector_state);
>  
>  /**
> + * drm_atomic_connector_set_property - set property on connector.
> + * @connector: the drm connector to set a property on
> + * @state: the state object to update with the new property value
> + * @property: the property to set
> + * @val: the new property value
> + *
> + * Use this instead of calling connector->atomic_set_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> +		struct drm_connector_state *state, struct drm_property *property,
> +		uint64_t val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->dpms_property) {
> +		/* setting DPMS property requires special handling, which
> +		 * is done in legacy setprop path for us.  Disallow (for
> +		 * now?) atomic writes to DPMS property:
> +		 */

Argh, I still need to write those patches for dpms in atomic. But imo a
better comment would be:

	/*
	 * Per-connector is a legacy-only feature, disallow for atomic
	 * updates.
	 */

I'll extend the comment once the atomic dpms is there.

> +		return -EINVAL;
> +	} else if (connector->funcs->atomic_set_property) {
> +		return connector->funcs->atomic_set_property(connector,
> +				state, property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_connector_set_property);
> +
> +/**
>   * drm_atomic_set_crtc_for_plane - set crtc for plane
>   * @state: the incoming atomic state
>   * @plane: the plane whose incoming state to update
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4a78a77..d42fdb1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1558,8 +1558,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = crtc->funcs->atomic_set_property(crtc, crtc_state,
> -					       property, val);
> +	ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> @@ -1617,8 +1617,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = plane->funcs->atomic_set_property(plane, plane_state,
> -					       property, val);
> +	ret = drm_atomic_plane_set_property(plane, plane_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> @@ -1676,8 +1676,8 @@ retry:
>  		goto fail;
>  	}
>  
> -	ret = connector->funcs->atomic_set_property(connector, connector_state,
> -					       property, val);
> +	ret = drm_atomic_connector_set_property(connector, connector_state,
> +			property, val);
>  	if (ret)
>  		goto fail;
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index ad22295..b0834dc 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -38,12 +38,21 @@ void drm_atomic_state_free(struct drm_atomic_state *state);
>  struct drm_crtc_state * __must_check
>  drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  			  struct drm_crtc *crtc);
> +int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state, struct drm_property *property,
> +		uint64_t val);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> +		struct drm_plane_state *state, struct drm_property *property,
> +		uint64_t val);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
> +int drm_atomic_connector_set_property(struct drm_connector *connector,
> +		struct drm_connector_state *state, struct drm_property *property,
> +		uint64_t val);
>  
>  int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61ab3e5..1b71f60 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -300,6 +300,7 @@ struct drm_crtc_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this CRTC
>   * @atomic_destroy_state: destroy an atomic state for this CRTC
>   * @atomic_set_property: set a property on an atomic state for this CRTC
> + *    (do not call directly, use drm_atomic_crtc_set_property())
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -483,6 +484,7 @@ struct drm_connector_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this connector
>   * @atomic_destroy_state: destroy an atomic state for this connector
>   * @atomic_set_property: set a property on an atomic state for this connector
> + *    (do not call directly, use drm_atomic_connector_set_property())
>   *
>   * Each CRTC may have one or more connectors attached to it.  The functions
>   * below allow the core DRM code to control connectors, enumerate available modes,
> @@ -743,6 +745,7 @@ struct drm_plane_state {
>   * @atomic_duplicate_state: duplicate the atomic state for this plane
>   * @atomic_destroy_state: destroy an atomic state for this plane
>   * @atomic_set_property: set a property on an atomic state for this plane
> + *    (do not call directly, use drm_atomic_plane_set_property())
>   */
>  struct drm_plane_funcs {
>  	int (*update_plane)(struct drm_plane *plane,
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/13] drm: add atomic_get_property

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:33PM -0500, Rob Clark wrote:
> Since we won't be using the obj->properties->values[] array to shadow
> property values for atomic drivers, we are going to need a vfunc for
> getting prop values.  Add that along w/ mandatory wrapper fxns.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

With Sean's nitpick: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  9 ++++++
>  include/drm/drm_crtc.h       | 18 +++++++++++
>  3 files changed, 103 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1261ade..4099b44 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -239,6 +239,28 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_atomic_crtc_set_property);
>  
>  /**
> + * drm_atomic_crtc_get_property - get property on connector
> + * @crtc: the drm CRTC to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling crtc->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> +		const struct drm_crtc_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	if (crtc->funcs->atomic_get_property)
> +		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_get_property);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -316,6 +338,28 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_plane_set_property);
>  
>  /**
> + * drm_atomic_plane_get_property - get property on plane
> + * @plane: the drm plane to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling plane->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_get_property(struct drm_plane *plane,
> +		const struct drm_plane_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	if (plane->funcs->atomic_get_property)
> +		return plane->funcs->atomic_get_property(plane, state, property, val);
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_get_property);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> @@ -421,6 +465,38 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_atomic_connector_set_property);
>  
>  /**
> + * drm_atomic_connector_get_property - get property on connector
> + * @connector: the drm connector to get a property on
> + * @state: the state object with the property value to read
> + * @property: the property to get
> + * @val: the property value (returned by reference)
> + *
> + * Use this instead of calling connector->atomic_get_property directly
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_connector_get_property(struct drm_connector *connector,
> +		const struct drm_connector_state *state,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->dpms_property) {
> +		*val = connector->dpms;
> +	} else if (connector->funcs->atomic_get_property) {
> +		return connector->funcs->atomic_get_property(connector,
> +				state, property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_connector_get_property);
> +
> +/**
>   * drm_atomic_set_crtc_for_plane - set crtc for plane
>   * @state: the incoming atomic state
>   * @plane: the plane whose incoming state to update
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b0834dc..b34224a 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -41,18 +41,27 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> +		const struct drm_crtc_state *state,
> +		struct drm_property *property, uint64_t *val);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
>  int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		struct drm_plane_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_plane_get_property(struct drm_plane *plane,
> +		const struct drm_plane_state *state,
> +		struct drm_property *property, uint64_t *val);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
>  int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		struct drm_connector_state *state, struct drm_property *property,
>  		uint64_t val);
> +int drm_atomic_connector_get_property(struct drm_connector *connector,
> +		const struct drm_connector_state *state,
> +		struct drm_property *property, uint64_t *val);
>  
>  int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1b71f60..e6cfdaf 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -301,6 +301,8 @@ struct drm_crtc_state {
>   * @atomic_destroy_state: destroy an atomic state for this CRTC
>   * @atomic_set_property: set a property on an atomic state for this CRTC
>   *    (do not call directly, use drm_atomic_crtc_set_property())
> + * @atomic_get_property: get a property on an atomic state for this CRTC
> + *    (do not call directly, use drm_atomic_crtc_get_property())
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -360,6 +362,10 @@ struct drm_crtc_funcs {
>  				   struct drm_crtc_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_crtc *crtc,
> +				   const struct drm_crtc_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  /**
> @@ -485,6 +491,8 @@ struct drm_connector_state {
>   * @atomic_destroy_state: destroy an atomic state for this connector
>   * @atomic_set_property: set a property on an atomic state for this connector
>   *    (do not call directly, use drm_atomic_connector_set_property())
> + * @atomic_get_property: get a property on an atomic state for this connector
> + *    (do not call directly, use drm_atomic_connector_get_property())
>   *
>   * Each CRTC may have one or more connectors attached to it.  The functions
>   * below allow the core DRM code to control connectors, enumerate available modes,
> @@ -518,6 +526,10 @@ struct drm_connector_funcs {
>  				   struct drm_connector_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_connector *connector,
> +				   const struct drm_connector_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  /**
> @@ -746,6 +758,8 @@ struct drm_plane_state {
>   * @atomic_destroy_state: destroy an atomic state for this plane
>   * @atomic_set_property: set a property on an atomic state for this plane
>   *    (do not call directly, use drm_atomic_plane_set_property())
> + * @atomic_get_property: get a property on an atomic state for this plane
> + *    (do not call directly, use drm_atomic_plane_get_property())
>   */
>  struct drm_plane_funcs {
>  	int (*update_plane)(struct drm_plane *plane,
> @@ -769,6 +783,10 @@ struct drm_plane_funcs {
>  				   struct drm_plane_state *state,
>  				   struct drm_property *property,
>  				   uint64_t val);
> +	int (*atomic_get_property)(struct drm_plane *plane,
> +				   const struct drm_plane_state *state,
> +				   struct drm_property *property,
> +				   uint64_t *val);
>  };
>  
>  enum drm_plane_type {
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
> Once a driver is using atomic helpers for modeset, the next step is to
> switch over to atomic properties.  To do this, plug in the helper
> function to your modeconfig funcs and be sure to implement the plane/
> crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
> objects which have driver custom properties.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  include/drm/drm_crtc.h              |  3 +++
>  4 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d42fdb1..1a1ab34 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1703,6 +1703,52 @@ backoff:
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
>  /**
> + * drm_atomic_helper_get_property - helper to read atomic property
> + * @obj: drm mode object whose property to read
> + * @property: the property to read
> + * @val: the read value, returned by reference
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> +		struct drm_property *property, uint64_t *val)
> +{
> +	struct drm_device *dev = property->dev;
> +	int ret;
> +
> +	switch (obj->type) {
> +	case DRM_MODE_OBJECT_CONNECTOR: {
> +		struct drm_connector *connector = obj_to_connector(obj);
> +		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +		ret = connector->funcs->atomic_get_property(connector,
> +				connector->state, property, val);

Shouldn't we use the get helpers introduced in previous patches here? For
legacy support we definitely want old core stuff like dpms to keep
working. Of course that means all the new atomic properties won't get
magically filtered out. But I think we need to hide them explicitly
anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
legacy paths.
-Daniel

> +		break;
> +	}
> +	case DRM_MODE_OBJECT_CRTC: {
> +		struct drm_crtc *crtc = obj_to_crtc(obj);
> +		WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
> +		ret = crtc->funcs->atomic_get_property(crtc,
> +				crtc->state, property, val);
> +		break;
> +	}
> +	case DRM_MODE_OBJECT_PLANE: {
> +		struct drm_plane *plane = obj_to_plane(obj);
> +		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> +		ret = plane->funcs->atomic_get_property(plane,
> +				plane->state, property, val);
> +		break;
> +	}
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_get_property);
> +
> +/**
>   * drm_atomic_helper_page_flip - execute a legacy page flip
>   * @crtc: DRM crtc
>   * @fb: DRM framebuffer
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 481bb25..57cd950 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3878,8 +3878,17 @@ EXPORT_SYMBOL(drm_object_property_set_value);
>  int drm_object_property_get_value(struct drm_mode_object *obj,
>  				  struct drm_property *property, uint64_t *val)
>  {
> +	struct drm_mode_config *config = &property->dev->mode_config;
>  	int i;
>  
> +	/* read-only properties bypass atomic mechanism and still store
> +	 * their value in obj->properties->values[].. mostly to avoid
> +	 * having to deal w/ EDID and similar props in atomic paths:
> +	 */
> +	if (config->funcs->atomic_get_property &&
> +			!(property->flags & DRM_MODE_PROP_IMMUTABLE))
> +		return config->funcs->atomic_get_property(obj, property, val);
> +
>  	for (i = 0; i < obj->properties->count; i++) {
>  		if (obj->properties->properties[i] == property) {
>  			*val = obj->properties->values[i];
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index f956b41..5e72b82 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -74,6 +74,8 @@ int drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>  int drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>  					struct drm_property *property,
>  					uint64_t val);
> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> +			   struct drm_property *property, uint64_t *val);
>  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e6cfdaf..ca8ba72 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -941,6 +941,7 @@ struct drm_mode_set {
>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
>   * @fb_create: create a new framebuffer object
>   * @output_poll_changed: function to handle output configuration changes
> + * @atomic_get_property: atomic way to read property value
>   * @atomic_check: check whether a give atomic state update is possible
>   * @atomic_commit: commit an atomic state update previously verified with
>   * 	atomic_check()
> @@ -954,6 +955,8 @@ struct drm_mode_config_funcs {
>  					     struct drm_mode_fb_cmd2 *mode_cmd);
>  	void (*output_poll_changed)(struct drm_device *dev);
>  
> +	int (*atomic_get_property)(struct drm_mode_object *obj,
> +				struct drm_property *property, uint64_t *val);
>  	int (*atomic_check)(struct drm_device *dev,
>  			    struct drm_atomic_state *a);
>  	int (*atomic_commit)(struct drm_device *dev,
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Rob Clark]

On Wed, Dec 17, 2014 at 4:31 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 17.12.2014 16:20, Pekka Paalanen wrote:
>> On Wed, 17 Dec 2014 11:48:51 +0900
>> Michel Dänzer <michel@daenzer.net> wrote:
>>
>>> On 17.12.2014 08:05, Rob Clark wrote:
>>>> The atomic modeset ioctl can be used to push any number of new values
>>>> for object properties. The driver can then check the full device
>>>> configuration as single unit, and try to apply the changes atomically.
>>>>
>>>> The ioctl simply takes a list of object IDs and property IDs and their
>>>> values.
>>>
>>> [...]
>>>
>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>> index 86574b0..3459778 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
>>>>     uint32_t handle;
>>>>  };
>>>>
>>>> +/* page-flip flags are valid, plus: */
>>>> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>>>> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>>>> +
>>>> +#define DRM_MODE_ATOMIC_FLAGS (\
>>>> +           DRM_MODE_PAGE_FLIP_EVENT |\
>>>> +           DRM_MODE_PAGE_FLIP_ASYNC |\
>>>> +           DRM_MODE_ATOMIC_TEST_ONLY |\
>>>> +           DRM_MODE_ATOMIC_NONBLOCK)
>>>> +
>>>> +struct drm_mode_atomic {
>>>> +   __u32 flags;
>>>> +   __u32 count_objs;
>>>> +   __u64 objs_ptr;
>>>> +   __u64 count_props_ptr;
>>>> +   __u64 props_ptr;
>>>> +   __u64 prop_values_ptr;
>>>> +   __u64 blob_values_ptr;  /* remove? */
>>>> +   __u64 user_data;
>>>> +};
>>>> +
>>>>  #endif
>>>>
>>>
>>> The new ioctl(s) should take an explicit parameter specifying when the
>>> changes should take effect. And since variable refresh rate displays are
>>> becoming mainstream, that parameter should probably be a timestamp
>>> rather than a frame counter.
>>
>> That sounds cool to me, but also a rabbit hole. While having worked on
>> the Wayland Presentation queueing extension, I'd like to ask the
>> following questions:
>>
>> - If you set the atomic kick to happen in the future, is there any way
>>   to cancel it? I'd be ok with not being able to cancel initially, but
>>   if one wants to add that later, we should already know how to
>>   reference this atomic submission in the cancel request. What if user
>>   space has a bug and schedules an update at one hour or three days from
>>   now, how would we abort that?
>>
>> - Can you VT-switch or drop DRM master if you have a pending atomic
>>   update?
>>
>> - Should one be able to set multiple pending atomic updates?
>>
>> - If I schedule an atomic update for one CTRC, can I schedule another
>>   update for another CRTC before the first one completes? Or am I
>>   forced to gather all updates over all outputs in the same atomic
>>   submission even if I don't care about or want inter-output sync and
>>   the outputs might even be running at different refresh rates?
>>   (Actually, this seems to be a valid question even without any target
>>   time parameter.)
>>
>> - If there can be multiple pending atomic updates on the same DRM
>>   device, is there any way to guarantee that the
>>   DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
>>   atomic update actually kicks in? Another update may have changed the
>>   configuration before this update kicks in, which means the overall
>>   state isn't the same that was tested.
>>
>> - Does a pending atomic update prevent immediate (old style) KMS
>>   changes?
>>
>> - Assuming hardware cannot do arbitrary time updates, how do you round
>>   the given timestamp? Strictly "not before" given time? Round to
>>   nearest possible time? The effect of required vs. unwanted sync to
>>   vblank?
>>
>> - How would user space match the page flip event to the atomic
>>   submission it did?
>>
>> I wonder if there is a way to postpone these hard(?) questions, so that
>> we could have atomic sooner and add scheduling later? I would imagine
>> solving everything above is quite some work.
>
> I agree. The main reason I brought it up is because I'd like to avoid
> getting into the same situation as with the current
> DRM_IOCTL_MODE_PAGE_FLIP ioctl, which doesn't explicitly communicate
> between userspace and kernel when the flip is supposed/expected to
> occur. We recently had to jump through some hoops in the radeon kernel
> driver to prevent flips from occurring sooner than expected by userspace.
>
>
> But I also think it would be a shame at this point to add new ioctls
> which aren't designed with variable refresh rate displays in mind.
>

So, I think this is something we can safely add later (and possibly
could be added to pageflip ioctl too in the same way.. but would need
to check the git history about whether pageflip ioctl handler was
always good enough at checking for invalid flags).  My suggestion
would be to add a new field at end of ioctl along w/ a new arg->flags
bit to indicate that a timestamp is provided.  New userspace on old
kernel would fail it due to invalid flag.  Old userspace on new kernel
wouldn't set the bit.

Either way, we should perhaps put a limit on how far in the future an
update can be requested, so vt-switch and those sort of cases could
just block until pending updates complete.  If we allow userspace to
request a flip 5 minutes from now, that wouldn't be so cool.

BR,
-R

>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Daniel Vetter]

On Wed, Dec 17, 2014 at 03:02:41PM +0100, Daniel Vetter wrote:
> On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
> > Once a driver is using atomic helpers for modeset, the next step is to
> > switch over to atomic properties.  To do this, plug in the helper
> > function to your modeconfig funcs and be sure to implement the plane/
> > crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
> > objects which have driver custom properties.
> > 
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  include/drm/drm_crtc.h              |  3 +++
> >  4 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index d42fdb1..1a1ab34 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1703,6 +1703,52 @@ backoff:
> >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >  
> >  /**
> > + * drm_atomic_helper_get_property - helper to read atomic property
> > + * @obj: drm mode object whose property to read
> > + * @property: the property to read
> > + * @val: the read value, returned by reference
> > + *
> > + * RETURNS:
> > + * Zero on success, error code on failure
> > + */
> > +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> > +		struct drm_property *property, uint64_t *val)
> > +{
> > +	struct drm_device *dev = property->dev;
> > +	int ret;
> > +
> > +	switch (obj->type) {
> > +	case DRM_MODE_OBJECT_CONNECTOR: {
> > +		struct drm_connector *connector = obj_to_connector(obj);
> > +		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > +		ret = connector->funcs->atomic_get_property(connector,
> > +				connector->state, property, val);
> 
> Shouldn't we use the get helpers introduced in previous patches here? For
> legacy support we definitely want old core stuff like dpms to keep
> working. Of course that means all the new atomic properties won't get
> magically filtered out. But I think we need to hide them explicitly
> anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
> legacy paths.

Also I'm not sure if there's a benefit to make this a vfunc+helper instead
of just core code called unconditionally. Meaning I can't think of any
case where drivers want to overwrite this, ever. They can do all the fancy
they want for their private properties in the per-object hooks. And for
core/shared stuff we better not allow them to play with fire.

Or do I miss an important case here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Rob Clark]

On Wed, Dec 17, 2014 at 9:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
>> Once a driver is using atomic helpers for modeset, the next step is to
>> switch over to atomic properties.  To do this, plug in the helper
>> function to your modeconfig funcs and be sure to implement the plane/
>> crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
>> objects which have driver custom properties.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
>>  include/drm/drm_atomic_helper.h     |  2 ++
>>  include/drm/drm_crtc.h              |  3 +++
>>  4 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index d42fdb1..1a1ab34 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1703,6 +1703,52 @@ backoff:
>>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>>
>>  /**
>> + * drm_atomic_helper_get_property - helper to read atomic property
>> + * @obj: drm mode object whose property to read
>> + * @property: the property to read
>> + * @val: the read value, returned by reference
>> + *
>> + * RETURNS:
>> + * Zero on success, error code on failure
>> + */
>> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
>> +             struct drm_property *property, uint64_t *val)
>> +{
>> +     struct drm_device *dev = property->dev;
>> +     int ret;
>> +
>> +     switch (obj->type) {
>> +     case DRM_MODE_OBJECT_CONNECTOR: {
>> +             struct drm_connector *connector = obj_to_connector(obj);
>> +             WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> +             ret = connector->funcs->atomic_get_property(connector,
>> +                             connector->state, property, val);
>
> Shouldn't we use the get helpers introduced in previous patches here? For
> legacy support we definitely want old core stuff like dpms to keep
> working. Of course that means all the new atomic properties won't get
> magically filtered out. But I think we need to hide them explicitly
> anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
> legacy paths.

bleh, you are right.. and they did at one point use the helpers.. but
somehow in my squash/rebase/juggle act I lost that :-(

But I am still not a huge fan of hiding props for atomic drivers via
legacy getprop/setprop interfaces.. if nothing else it will prevent
them from showing up in modetest.  And it seems kind of inconsistent
and unnecessary.

BR,
-R

> -Daniel
>
>> +             break;
>> +     }
>> +     case DRM_MODE_OBJECT_CRTC: {
>> +             struct drm_crtc *crtc = obj_to_crtc(obj);
>> +             WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>> +             ret = crtc->funcs->atomic_get_property(crtc,
>> +                             crtc->state, property, val);
>> +             break;
>> +     }
>> +     case DRM_MODE_OBJECT_PLANE: {
>> +             struct drm_plane *plane = obj_to_plane(obj);
>> +             WARN_ON(!drm_modeset_is_locked(&plane->mutex));
>> +             ret = plane->funcs->atomic_get_property(plane,
>> +                             plane->state, property, val);
>> +             break;
>> +     }
>> +     default:
>> +             ret = -EINVAL;
>> +             break;
>> +     }
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_get_property);
>> +
>> +/**
>>   * drm_atomic_helper_page_flip - execute a legacy page flip
>>   * @crtc: DRM crtc
>>   * @fb: DRM framebuffer
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 481bb25..57cd950 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -3878,8 +3878,17 @@ EXPORT_SYMBOL(drm_object_property_set_value);
>>  int drm_object_property_get_value(struct drm_mode_object *obj,
>>                                 struct drm_property *property, uint64_t *val)
>>  {
>> +     struct drm_mode_config *config = &property->dev->mode_config;
>>       int i;
>>
>> +     /* read-only properties bypass atomic mechanism and still store
>> +      * their value in obj->properties->values[].. mostly to avoid
>> +      * having to deal w/ EDID and similar props in atomic paths:
>> +      */
>> +     if (config->funcs->atomic_get_property &&
>> +                     !(property->flags & DRM_MODE_PROP_IMMUTABLE))
>> +             return config->funcs->atomic_get_property(obj, property, val);
>> +
>>       for (i = 0; i < obj->properties->count; i++) {
>>               if (obj->properties->properties[i] == property) {
>>                       *val = obj->properties->values[i];
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index f956b41..5e72b82 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -74,6 +74,8 @@ int drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>>  int drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>>                                       struct drm_property *property,
>>                                       uint64_t val);
>> +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
>> +                        struct drm_property *property, uint64_t *val);
>>  int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>>                               struct drm_framebuffer *fb,
>>                               struct drm_pending_vblank_event *event,
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index e6cfdaf..ca8ba72 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -941,6 +941,7 @@ struct drm_mode_set {
>>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
>>   * @fb_create: create a new framebuffer object
>>   * @output_poll_changed: function to handle output configuration changes
>> + * @atomic_get_property: atomic way to read property value
>>   * @atomic_check: check whether a give atomic state update is possible
>>   * @atomic_commit: commit an atomic state update previously verified with
>>   *   atomic_check()
>> @@ -954,6 +955,8 @@ struct drm_mode_config_funcs {
>>                                            struct drm_mode_fb_cmd2 *mode_cmd);
>>       void (*output_poll_changed)(struct drm_device *dev);
>>
>> +     int (*atomic_get_property)(struct drm_mode_object *obj,
>> +                             struct drm_property *property, uint64_t *val);
>>       int (*atomic_check)(struct drm_device *dev,
>>                           struct drm_atomic_state *a);
>>       int (*atomic_commit)(struct drm_device *dev,
>> --
>> 2.1.0
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/13] drm: small property creation cleanup

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:35PM -0500, Rob Clark wrote:
> Getting ready to add a lot more standard properties for atomic.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 57cd950..96965ec 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1330,33 +1330,40 @@ EXPORT_SYMBOL(drm_plane_force_disable);
>  
>  static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>  {
> -	struct drm_property *edid;
> -	struct drm_property *dpms;
> -	struct drm_property *dev_path;
> +	struct drm_property *prop;
>  
>  	/*
>  	 * Standard properties (apply to all connectors)
>  	 */
> -	edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
>  				   DRM_MODE_PROP_IMMUTABLE,
>  				   "EDID", 0);
> -	dev->mode_config.edid_property = edid;
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.edid_property = prop;
>  
> -	dpms = drm_property_create_enum(dev, 0,
> +	prop = drm_property_create_enum(dev, 0,
>  				   "DPMS", drm_dpms_enum_list,
>  				   ARRAY_SIZE(drm_dpms_enum_list));
> -	dev->mode_config.dpms_property = dpms;
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.dpms_property = prop;
>  
> -	dev_path = drm_property_create(dev,
> +	prop = drm_property_create(dev,
>  				       DRM_MODE_PROP_BLOB |
>  				       DRM_MODE_PROP_IMMUTABLE,
>  				       "PATH", 0);
> -	dev->mode_config.path_property = dev_path;
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.path_property = prop;
>  
> -	dev->mode_config.tile_property = drm_property_create(dev,
> +	prop = drm_property_create(dev,
>  							     DRM_MODE_PROP_BLOB |
>  							     DRM_MODE_PROP_IMMUTABLE,
>  							     "TILE", 0);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.tile_property = prop;
>  
>  	return 0;
>  }
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/13] drm: tweak getconnector locking

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:36PM -0500, Rob Clark wrote:
> We need to hold connection_mutex as we read the properties.  Easiest
> thing is just widen the scope where connection_mutex is held.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

I'm not yet sure on the story we want to have for getprop locking (and
whether it's even relevant). But this gets us forward at least.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/drm_crtc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 96965ec..62f5dc8 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2043,6 +2043,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>  
>  	mutex_lock(&dev->mode_config.mutex);
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  
>  	connector = drm_connector_find(dev, out_resp->connector_id);
>  	if (!connector) {
> @@ -2076,14 +2077,11 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	out_resp->mm_height = connector->display_info.height_mm;
>  	out_resp->subpixel = connector->display_info.subpixel_order;
>  	out_resp->connection = connector->status;
> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -
>  	encoder = drm_connector_get_encoder(connector);
>  	if (encoder)
>  		out_resp->encoder_id = encoder->base.id;
>  	else
>  		out_resp->encoder_id = 0;
> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
> @@ -2149,6 +2147,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>  	out_resp->count_encoders = encoders_count;
>  
>  out:
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  	mutex_unlock(&dev->mode_config.mutex);
>  
>  	return ret;
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Rob Clark]

On Wed, Dec 17, 2014 at 9:06 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 17, 2014 at 03:02:41PM +0100, Daniel Vetter wrote:
>> On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
>> > Once a driver is using atomic helpers for modeset, the next step is to
>> > switch over to atomic properties.  To do this, plug in the helper
>> > function to your modeconfig funcs and be sure to implement the plane/
>> > crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
>> > objects which have driver custom properties.
>> >
>> > Signed-off-by: Rob Clark <robdclark@gmail.com>
>> > ---
>> >  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
>> >  include/drm/drm_atomic_helper.h     |  2 ++
>> >  include/drm/drm_crtc.h              |  3 +++
>> >  4 files changed, 60 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> > index d42fdb1..1a1ab34 100644
>> > --- a/drivers/gpu/drm/drm_atomic_helper.c
>> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> > @@ -1703,6 +1703,52 @@ backoff:
>> >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>> >
>> >  /**
>> > + * drm_atomic_helper_get_property - helper to read atomic property
>> > + * @obj: drm mode object whose property to read
>> > + * @property: the property to read
>> > + * @val: the read value, returned by reference
>> > + *
>> > + * RETURNS:
>> > + * Zero on success, error code on failure
>> > + */
>> > +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
>> > +           struct drm_property *property, uint64_t *val)
>> > +{
>> > +   struct drm_device *dev = property->dev;
>> > +   int ret;
>> > +
>> > +   switch (obj->type) {
>> > +   case DRM_MODE_OBJECT_CONNECTOR: {
>> > +           struct drm_connector *connector = obj_to_connector(obj);
>> > +           WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> > +           ret = connector->funcs->atomic_get_property(connector,
>> > +                           connector->state, property, val);
>>
>> Shouldn't we use the get helpers introduced in previous patches here? For
>> legacy support we definitely want old core stuff like dpms to keep
>> working. Of course that means all the new atomic properties won't get
>> magically filtered out. But I think we need to hide them explicitly
>> anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
>> legacy paths.
>
> Also I'm not sure if there's a benefit to make this a vfunc+helper instead
> of just core code called unconditionally. Meaning I can't think of any
> case where drivers want to overwrite this, ever. They can do all the fancy
> they want for their private properties in the per-object hooks. And for
> core/shared stuff we better not allow them to play with fire.
>
> Or do I miss an important case here?

drivers can have their own custom properties too :-)

umm, or are you talking about the vfunc in config->funcs?  (In which
case, it was at least a convenient way to for core to know whether to
take legacy path or atomic path)

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/13] drm/atomic: atomic_check functions

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:37PM -0500, Rob Clark wrote:
> Add functions to check core plane/crtc state.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 87 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 47 ++++++++++++++------
>  include/drm/drm_atomic.h            |  4 ++
>  3 files changed, 124 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4099b44..afb830d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -261,6 +261,27 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_atomic_crtc_get_property);
>  
>  /**
> + * drm_atomic_crtc_check - check crtc state
> + * @crtc: crtc to check
> + * @state: crtc state to check
> + *
> + * Provides core sanity checks for crtc state.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_crtc_check(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state)
> +{
> +	/* TODO anything to check?  I think if drivers want to enforce that
> +	 * primary layer covers entire screen, they should do that in their
> +	 * own crtc->atomic_check() vfunc..
> +	 */

We could do things like mode sanity checking, like Ville's latest series
does. But for now there's nothing, so imo drop the comment. Maybe instead

	/* TODO: Add generic modeset state checks once we support those. */

> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_crtc_check);
> +
> +/**
>   * drm_atomic_get_plane_state - get plane state
>   * @state: global atomic state object
>   * @plane: plane to get state object for
> @@ -360,6 +381,72 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_atomic_plane_get_property);
>  
>  /**
> + * drm_atomic_plane_check - check plane state
> + * @plane: plane to check
> + * @state: plane state to check
> + *
> + * Provides core sanity checks for plane state.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_atomic_plane_check(struct drm_plane *plane,
> +		struct drm_plane_state *state)
> +{
> +	unsigned int fb_width, fb_height;
> +	unsigned int i;

The overflow checks on crtc_w/x/h/y from drm_mode_setplane seem to be
missing here. I think we should add them to the atomic paths, too.

> +
> +	/* either *both* CRTC and FB must be set, or neither */
> +	if (WARN_ON(state->crtc && !state->fb)) {
> +		DRM_DEBUG_KMS("CRTC set but no FB\n");
> +		return -EINVAL;
> +	} else if (WARN_ON(state->fb && !state->crtc)) {
> +		DRM_DEBUG_KMS("FB set but no CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	/* if disabled, we don't care about the rest of the state: */
> +	if (!state->crtc)
> +		return 0;
> +
> +	/* Check whether this plane is usable on this CRTC */
> +	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
> +		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Check whether this plane supports the fb pixel format. */
> +	for (i = 0; i < plane->format_count; i++)
> +		if (state->fb->pixel_format == plane->format_types[i])
> +			break;
> +	if (i == plane->format_count) {
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(state->fb->pixel_format));
> +		return -EINVAL;
> +	}
> +
> +	fb_width = state->fb->width << 16;
> +	fb_height = state->fb->height << 16;
> +
> +	/* Make sure source coordinates are inside the fb. */
> +	if (state->src_w > fb_width ||
> +	    state->src_x > fb_width - state->src_w ||
> +	    state->src_h > fb_height ||
> +	    state->src_y > fb_height - state->src_h) {
> +		DRM_DEBUG_KMS("Invalid source coordinates "
> +			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> +			      state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> +			      state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> +			      state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> +			      state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_plane_check);
> +
> +/**
>   * drm_atomic_get_connector_state - get connector state
>   * @state: global atomic state object
>   * @connector: connector to get state object for
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1a1ab34..55b6981 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -407,6 +407,37 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	return mode_fixup(state);
>  }
>  
> +static int plane_check(struct drm_plane *plane, struct drm_plane_state *state)
> +{
> +	struct drm_plane_helper_funcs *funcs = plane->helper_private;
> +	int ret;
> +
> +	ret = drm_atomic_plane_check(plane, state);
> +	if (ret)
> +		return ret;
> +
> +
> +	if (funcs && funcs->atomic_check)
> +		ret = funcs->atomic_check(plane, state);
> +
> +	return ret;
> +}
> +
> +static int crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state)
> +{
> +	struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
> +	int ret;
> +
> +	ret = drm_atomic_crtc_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	if (funcs && funcs->atomic_check)
> +		ret = funcs->atomic_check(crtc, state);
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_atomic_helper_check - validate state object
>   * @dev: DRM device
> @@ -429,21 +460,15 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	int i, ret = 0;
>  
>  	for (i = 0; i < nplanes; i++) {
> -		struct drm_plane_helper_funcs *funcs;
>  		struct drm_plane *plane = state->planes[i];
>  		struct drm_plane_state *plane_state = state->plane_states[i];
>  
>  		if (!plane)
>  			continue;
>  
> -		funcs = plane->helper_private;
> -
>  		drm_atomic_helper_plane_changed(state, plane_state, plane);
>  
> -		if (!funcs || !funcs->atomic_check)
> -			continue;
> -
> -		ret = funcs->atomic_check(plane, plane_state);
> +		ret = plane_check(plane, plane_state);
>  		if (ret) {
>  			DRM_DEBUG_KMS("[PLANE:%d] atomic check failed\n",
>  				      plane->base.id);
> @@ -452,18 +477,12 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  	}
>  
>  	for (i = 0; i < ncrtcs; i++) {
> -		struct drm_crtc_helper_funcs *funcs;
>  		struct drm_crtc *crtc = state->crtcs[i];
>  
>  		if (!crtc)
>  			continue;
>  
> -		funcs = crtc->helper_private;
> -
> -		if (!funcs || !funcs->atomic_check)
> -			continue;
> -
> -		ret = funcs->atomic_check(crtc, state->crtc_states[i]);
> +		ret = crtc_check(crtc, state->crtc_states[i]);

Imo this should be moved into drm_atomic_check_only since core checks
shouldn't be allowed to be overwritten. We could then also drop the
EXPORT_SYMBOL for the two functions added to drm_atomic.c

That means a bit of copypasta, but that just seems unavoidable with our
multiple paths all around. And it's just copying of the loops here.

I think I also need to move some of the connector sanity checks
eventually from helpers to core. But that can be reviewed once i915 comes
around.

>  		if (ret) {
>  			DRM_DEBUG_KMS("[CRTC:%d] atomic check failed\n",
>  				      crtc->base.id);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b34224a..742d027 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -44,6 +44,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		const struct drm_crtc_state *state,
>  		struct drm_property *property, uint64_t *val);
> +int drm_atomic_crtc_check(struct drm_crtc *crtc,
> +		struct drm_crtc_state *state);
>  struct drm_plane_state * __must_check
>  drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  			   struct drm_plane *plane);
> @@ -53,6 +55,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  int drm_atomic_plane_get_property(struct drm_plane *plane,
>  		const struct drm_plane_state *state,
>  		struct drm_property *property, uint64_t *val);
> +int drm_atomic_plane_check(struct drm_plane *plane,
> +		struct drm_plane_state *state);
>  struct drm_connector_state * __must_check
>  drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  			       struct drm_connector *connector);
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Daniel Vetter]

On Wed, Dec 17, 2014 at 09:15:12AM -0500, Rob Clark wrote:
> On Wed, Dec 17, 2014 at 9:06 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 17, 2014 at 03:02:41PM +0100, Daniel Vetter wrote:
> >> On Tue, Dec 16, 2014 at 06:05:34PM -0500, Rob Clark wrote:
> >> > Once a driver is using atomic helpers for modeset, the next step is to
> >> > switch over to atomic properties.  To do this, plug in the helper
> >> > function to your modeconfig funcs and be sure to implement the plane/
> >> > crtc/connector atomic_{get,set}_property vfuncs for any of your mode-
> >> > objects which have driver custom properties.
> >> >
> >> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> > ---
> >> >  drivers/gpu/drm/drm_atomic_helper.c | 46 +++++++++++++++++++++++++++++++++++++
> >> >  drivers/gpu/drm/drm_crtc.c          |  9 ++++++++
> >> >  include/drm/drm_atomic_helper.h     |  2 ++
> >> >  include/drm/drm_crtc.h              |  3 +++
> >> >  4 files changed, 60 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> > index d42fdb1..1a1ab34 100644
> >> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> > @@ -1703,6 +1703,52 @@ backoff:
> >> >  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> >> >
> >> >  /**
> >> > + * drm_atomic_helper_get_property - helper to read atomic property
> >> > + * @obj: drm mode object whose property to read
> >> > + * @property: the property to read
> >> > + * @val: the read value, returned by reference
> >> > + *
> >> > + * RETURNS:
> >> > + * Zero on success, error code on failure
> >> > + */
> >> > +int drm_atomic_helper_get_property(struct drm_mode_object *obj,
> >> > +           struct drm_property *property, uint64_t *val)
> >> > +{
> >> > +   struct drm_device *dev = property->dev;
> >> > +   int ret;
> >> > +
> >> > +   switch (obj->type) {
> >> > +   case DRM_MODE_OBJECT_CONNECTOR: {
> >> > +           struct drm_connector *connector = obj_to_connector(obj);
> >> > +           WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >> > +           ret = connector->funcs->atomic_get_property(connector,
> >> > +                           connector->state, property, val);
> >>
> >> Shouldn't we use the get helpers introduced in previous patches here? For
> >> legacy support we definitely want old core stuff like dpms to keep
> >> working. Of course that means all the new atomic properties won't get
> >> magically filtered out. But I think we need to hide them explicitly
> >> anyway, e.g. with a new DRM_MODE_PROP_ATOMIC and checking for that in
> >> legacy paths.
> >
> > Also I'm not sure if there's a benefit to make this a vfunc+helper instead
> > of just core code called unconditionally. Meaning I can't think of any
> > case where drivers want to overwrite this, ever. They can do all the fancy
> > they want for their private properties in the per-object hooks. And for
> > core/shared stuff we better not allow them to play with fire.
> >
> > Or do I miss an important case here?
> 
> drivers can have their own custom properties too :-)
> 
> umm, or are you talking about the vfunc in config->funcs?  (In which
> case, it was at least a convenient way to for core to know whether to
> take legacy path or atomic path)

Yeah I only meant the config->func hook. Is there no other way to handle
this in the core? It looks funny if we use a func pointer essentially as a
boolean and don't allow drivers to set anything else than the defaults.
Essentially makes the helpers mandatory. Maybe just have
bool config->use_atomic_properties instead?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/13] drm: add atomic hook to read property plus helper

[Daniel Vetter]

On Wed, Dec 17, 2014 at 09:12:31AM -0500, Rob Clark wrote:
> But I am still not a huge fan of hiding props for atomic drivers via
> legacy getprop/setprop interfaces.. if nothing else it will prevent
> them from showing up in modetest.  And it seems kind of inconsistent
> and unnecessary.

By default at least the intel X driver reads/writes _all_ props it finds.
Maybe just on connectors, but even there we have the crtc prop. Which
means with the current legacy set_prop helpers we'll end up doing tons of
modesets, and that will piss of users. So I think we don't have a choice
but hide them.

In i915 all the legacy props have careful checks to short-circuit such
noop changes out, exactly for this reasons. Adding all these short-circuit
checks would pretty badly complicate helpers and hence is something I've
explicitly left out (for now at least).

And the PROP_ATOMIC flag is a useful hint to userspace to maybe not do
stupid things with that property just for fun.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/13] drm/atomic: atomic plane properties

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:38PM -0500, Rob Clark wrote:
> Expose the core plane state as properties, so they can be updated via
> atomic ioctl.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Just comments about the lack of PROP_ATOMIC and one suggestion for a
comment. With that addressed this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  Documentation/DocBook/drm.tmpl | 74 ++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_atomic.c   | 69 ++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_crtc.c     | 82 +++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_crtc.h         | 10 ++++++
>  4 files changed, 224 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 4b592ff..282fa6b 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2564,7 +2564,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="25" valign="top" >DRM</td>
> +	<td rowspan="35" valign="top" >DRM</td>
>  	<td rowspan="4" valign="top" >Generic</td>
>  	<td valign="top" >“EDID”</td>
>  	<td valign="top" >BLOB | IMMUTABLE</td>
> @@ -2594,7 +2594,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Contains tiling information for a connector.</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="1" valign="top" >Plane</td>
> +	<td rowspan="11" valign="top" >Plane</td>
>  	<td valign="top" >“type”</td>
>  	<td valign="top" >ENUM | IMMUTABLE</td>
>  	<td valign="top" >{ "Overlay", "Primary", "Cursor" }</td>
> @@ -2602,6 +2602,76 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Plane type</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" >“SRC_X”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source x coordinate in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_Y”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source y coordinate in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_W”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source width in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_H”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source height in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_X”</td>
> +	<td valign="top" >SIGNED_RANGE</td>
> +	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) x coordinate (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_Y”</td>
> +	<td valign="top" >SIGNED_RANGE</td>
> +	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) y coordinate (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_W”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) width (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_H”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) height (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“FB_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_FB</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout framebuffer (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >CRTC that plane is attached to (atomic)</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="2" valign="top" >DVI-I</td>
>  	<td valign="top" >“subconnector”</td>
>  	<td valign="top" >ENUM</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index afb830d..c09a05a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -352,9 +352,41 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		struct drm_plane_state *state, struct drm_property *property,
>  		uint64_t val)
>  {
> -	if (plane->funcs->atomic_set_property)
> -		return plane->funcs->atomic_set_property(plane, state, property, val);
> -	return -EINVAL;
> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->prop_fb_id) {
> +		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> +		drm_atomic_set_fb_for_plane(state, fb);
> +		if (fb)
> +			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_crtc_id) {
> +		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> +		return drm_atomic_set_crtc_for_plane(state->state, plane, crtc);
> +	} else if (property == config->prop_crtc_x) {
> +		state->crtc_x = U642I64(val);
> +	} else if (property == config->prop_crtc_y) {
> +		state->crtc_y = U642I64(val);
> +	} else if (property == config->prop_crtc_w) {
> +		state->crtc_w = val;
> +	} else if (property == config->prop_crtc_h) {
> +		state->crtc_h = val;
> +	} else if (property == config->prop_src_x) {
> +		state->src_x = val;
> +	} else if (property == config->prop_src_y) {
> +		state->src_y = val;
> +	} else if (property == config->prop_src_w) {
> +		state->src_w = val;
> +	} else if (property == config->prop_src_h) {
> +		state->src_h = val;

We need to check for PROP_ATOMIC somewhere. Well more precisely

	if ((prop->flags & PROP_ATOMIC) && !file_priv->atomic_kms)
		return -EINVAL;

And with all the scaffolding for file_priv->atomic_kms like we have for
file_priv->universal_planes. I guess that should be directly in the legacy
get_props paths, since otherwise we can't get at the file_priv. Other
callers probably don't care about this filtering.


> +	} else if (plane->funcs->atomic_set_property) {
> +		return plane->funcs->atomic_set_property(plane, state,
> +				property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_plane_set_property);
>  
> @@ -374,9 +406,36 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
>  		const struct drm_plane_state *state,
>  		struct drm_property *property, uint64_t *val)
>  {
> -	if (plane->funcs->atomic_get_property)
> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->prop_fb_id) {
> +		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_crtc_id) {
> +		*val = (state->crtc) ? state->crtc->base.id : 0;
> +	} else if (property == config->prop_crtc_x) {
> +		*val = I642U64(state->crtc_x);
> +	} else if (property == config->prop_crtc_y) {
> +		*val = I642U64(state->crtc_y);
> +	} else if (property == config->prop_crtc_w) {
> +		*val = state->crtc_w;
> +	} else if (property == config->prop_crtc_h) {
> +		*val = state->crtc_h;
> +	} else if (property == config->prop_src_x) {
> +		*val = state->src_x;
> +	} else if (property == config->prop_src_y) {
> +		*val = state->src_y;
> +	} else if (property == config->prop_src_w) {
> +		*val = state->src_w;
> +	} else if (property == config->prop_src_h) {
> +		*val = state->src_h;
> +	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
> -	return -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_plane_get_property);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62f5dc8..fd6f91d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -819,6 +819,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		      mode->interlace ?  " interlaced" : "");
>  }
>  
> +static bool
> +has_atomic_properties(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	return !!config->funcs->atomic_get_property;

Yeah, I think we really want to call this config->uses_atomic_props, much
clearer what it does. And gets rid of this wrapper. Aside: Casting to bool
automatically applies the !! trick, so not needed here.

> +}
> +
>  /**
>   * drm_connector_init - Init a preallocated connector
>   * @dev: DRM device
> @@ -1174,6 +1181,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     const uint32_t *formats, uint32_t format_count,
>  			     enum drm_plane_type type)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
>  
>  	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> @@ -1198,15 +1206,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> -	list_add_tail(&plane->head, &dev->mode_config.plane_list);
> -	dev->mode_config.num_total_plane++;
> +	list_add_tail(&plane->head, &config->plane_list);
> +	config->num_total_plane++;
>  	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> -		dev->mode_config.num_overlay_plane++;
> +		config->num_overlay_plane++;
>  
>  	drm_object_attach_property(&plane->base,
> -				   dev->mode_config.plane_type_property,
> +				   config->plane_type_property,
>  				   plane->type);
>  
> +	if (has_atomic_properties(dev)) {
> +		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_w, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_h, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_x, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_h, 0);

s/0/DRM_MODE_PROP_ATOMIC/ in all the above.

> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_universal_plane_init);
> @@ -1365,6 +1386,59 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.tile_property = prop;
>  
> +	prop = drm_property_create_range(dev, 0, "SRC_X", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_x = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_Y", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_y = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_w = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_h = prop;
> +
> +	prop = drm_property_create_signed_range(dev, 0, "CRTC_X",
> +			INT_MIN, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_x = prop;
> +
> +	prop = drm_property_create_signed_range(dev, 0, "CRTC_Y",
> +			INT_MIN, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_y = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_w = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_h = prop;
> +
> +	prop = drm_property_create_object(dev, 0, "FB_ID", DRM_MODE_OBJECT_FB);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fb_id = prop;
> +
> +	prop = drm_property_create_object(dev, 0,
> +			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_id = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ca8ba72..48fcd98 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1085,6 +1085,16 @@ struct drm_mode_config {
>  	struct drm_property *tile_property;
>  	struct drm_property *plane_type_property;
>  	struct drm_property *rotation_property;

Imo a comment here like /* plane atomic props */ to keep our sanity would
be good.

> +	struct drm_property *prop_src_x;
> +	struct drm_property *prop_src_y;
> +	struct drm_property *prop_src_w;
> +	struct drm_property *prop_src_h;
> +	struct drm_property *prop_crtc_x;
> +	struct drm_property *prop_crtc_y;
> +	struct drm_property *prop_crtc_w;
> +	struct drm_property *prop_crtc_h;
> +	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_crtc_id;
>  
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/13] drm/atomic: atomic connector properties

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:39PM -0500, Rob Clark wrote:
> Expose the core connector state as properties so it can be updated via
> atomic ioctl.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Oh, didn't realize that you're sharing config->prop_crtc_id with plane and
connector. Please add a comment about that to drm_crtc.h.

Plus same comment about setting DRM_MODE_PROB_ATOMIC. With these two this
is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I wasn't to clear in my reply to the previous patch probably, but I think
the atomic filtering should be in an earlier patch. Best in the missing
one to add the modparam and file_priv->atomic_kms.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl | 11 +++++++++--
>  drivers/gpu/drm/drm_atomic.c   |  9 +++++++--
>  drivers/gpu/drm/drm_crtc.c     | 13 +++++++++----
>  3 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 282fa6b..15cb9b9 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2564,8 +2564,8 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="35" valign="top" >DRM</td>
> -	<td rowspan="4" valign="top" >Generic</td>
> +	<td rowspan="36" valign="top" >DRM</td>
> +	<td rowspan="5" valign="top" >Connector</td>
>  	<td valign="top" >“EDID”</td>
>  	<td valign="top" >BLOB | IMMUTABLE</td>
>  	<td valign="top" >0</td>
> @@ -2594,6 +2594,13 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Contains tiling information for a connector.</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" >“CRTC_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
> +	<td valign="top" >Connector</td>
> +	<td valign="top" >CRTC that connector is attached to (atomic)</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="11" valign="top" >Plane</td>
>  	<td valign="top" >“type”</td>
>  	<td valign="top" >ENUM | IMMUTABLE</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c09a05a..71b48a0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -595,7 +595,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  
> -	if (property == config->dpms_property) {
> +	if (property == config->prop_crtc_id) {
> +		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> +		return drm_atomic_set_crtc_for_connector(state, crtc);
> +	} else if (property == config->dpms_property) {
>  		/* setting DPMS property requires special handling, which
>  		 * is done in legacy setprop path for us.  Disallow (for
>  		 * now?) atomic writes to DPMS property:
> @@ -629,7 +632,9 @@ int drm_atomic_connector_get_property(struct drm_connector *connector,
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  
> -	if (property == config->dpms_property) {
> +	if (property == config->prop_crtc_id) {
> +		*val = (state->crtc) ? state->crtc->base.id : 0;
> +	} else if (property == config->dpms_property) {
>  		*val = connector->dpms;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fd6f91d..39c3e06 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -844,6 +844,7 @@ int drm_connector_init(struct drm_device *dev,
>  		       const struct drm_connector_funcs *funcs,
>  		       int connector_type)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
>  	struct ida *connector_ida =
>  		&drm_connector_enum_list[connector_type].ida;
> @@ -882,16 +883,20 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	/* We should add connectors at the end to avoid upsetting the connector
>  	 * index too much. */
> -	list_add_tail(&connector->head, &dev->mode_config.connector_list);
> -	dev->mode_config.num_connector++;
> +	list_add_tail(&connector->head, &config->connector_list);
> +	config->num_connector++;
>  
>  	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>  		drm_object_attach_property(&connector->base,
> -					      dev->mode_config.edid_property,
> +					      config->edid_property,
>  					      0);
>  
>  	drm_object_attach_property(&connector->base,
> -				      dev->mode_config.dpms_property, 0);
> +				      config->dpms_property, 0);
> +
> +	if (has_atomic_properties(dev)) {
> +		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
> +	}
>  
>  	connector->debugfs_entry = NULL;
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Daniel Vetter]

On Tue, Dec 16, 2014 at 06:05:41PM -0500, Rob Clark wrote:
> The atomic modeset ioctl can be used to push any number of new values
> for object properties. The driver can then check the full device
> configuration as single unit, and try to apply the changes atomically.
> 
> The ioctl simply takes a list of object IDs and property IDs and their
> values.
> 
> Originally based on a patch from Ville Syrjälä, although it has mutated
> (mutilated?) enough since then that you probably shouldn't blame it on
> him ;-)
> 
> TODO:
>  * hide behind moduleparam initially

Yeah we need this, and I think we really need this (plus
file_priv->atomic_kms to allow new userspace to enable atomic, similar to
file_priv->universal_planes) even for patches 6, 10&11.

>  * either bring back blob property support, or add ioctls for create/
>    destroy blob properties so they can be passed in by id in this
>    ioctl.  I'm leaning towards the latter approach as (a) it is more
>    in line with how blob properties are read, and (b) the atomic
>    ioctl function is big enough already.

The blob approach is a bit more work, since we need to copypaste all the
logic we have for per-filpriv framebuffers: cleanup lists & refcounting.
Not sure whether we should just go with lots more properties, they're
cheap ;-) But I'm ok with this blob approach, too. Adding blobs to the
atomic ioctl itself feels a bit wrong.

One thing I have to consider with my intel hat on is the bridge we need to
adf. Everyting plain 64bit props would be a bit easier to marshal through
adf into atomic interfaces. But only slightly, we can just add an in-line
set of blobs at the end and write them directly, too. So either approach
is fine really.

Below a few comments all over, but looks good overall.

Cheers, Daniel

> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 308 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c   |   4 +-
>  drivers/gpu/drm/drm_ioctl.c  |   1 +
>  include/drm/drm_crtc.h       |   6 +
>  include/uapi/drm/drm.h       |   1 +
>  include/uapi/drm/drm_mode.h  |  21 +++
>  6 files changed, 339 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 71b48a0..e7a45ec 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -960,3 +960,311 @@ int drm_atomic_async_commit(struct drm_atomic_state *state)
>  	return config->funcs->atomic_commit(state->dev, state, true);
>  }
>  EXPORT_SYMBOL(drm_atomic_async_commit);
> +
> +/*
> + * The big monstor ioctl
> + */
> +
> +static struct drm_pending_vblank_event *create_vblank_event(
> +		struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> +{
> +	struct drm_pending_vblank_event *e = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	if (file_priv->event_space < sizeof e->event) {
> +		spin_unlock_irqrestore(&dev->event_lock, flags);
> +		goto out;
> +	}
> +	file_priv->event_space -= sizeof e->event;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	e = kzalloc(sizeof *e, GFP_KERNEL);
> +	if (e == NULL) {
> +		spin_lock_irqsave(&dev->event_lock, flags);
> +		file_priv->event_space += sizeof e->event;
> +		spin_unlock_irqrestore(&dev->event_lock, flags);
> +		goto out;
> +	}
> +
> +	e->event.base.type = DRM_EVENT_FLIP_COMPLETE;
> +	e->event.base.length = sizeof e->event;
> +	e->event.user_data = user_data;
> +	e->base.event = &e->event.base;
> +	e->base.file_priv = file_priv;
> +	e->base.destroy = (void (*) (struct drm_pending_event *)) kfree;
> +
> +out:
> +	return e;
> +}
> +
> +static void destroy_vblank_event(struct drm_device *dev,
> +		struct drm_file *file_priv, struct drm_pending_vblank_event *e)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	file_priv->event_space += sizeof e->event;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +	kfree(e);
> +}

I guess these two are the skeleton functions for the refactoring you've
started? Just an aside really.

> +int drm_mode_atomic_ioctl(struct drm_device *dev,
> +			  void *data, struct drm_file *file_priv)
> +{
> +	struct drm_mode_atomic *arg = data;
> +	uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
> +	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> +	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> +	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> +	unsigned int copied_objs, copied_props;
> +	struct drm_atomic_state *state;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_plane *plane;
> +	unsigned plane_mask = 0;
> +	int ret = 0;
> +	unsigned int i, j;
> +
> +	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS)
> +		return -EINVAL;
> +
> +	/* can't test and expect an event at the same time. */
> +	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
> +			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> +		return -EINVAL;

Maybe check for async implies nonblock, too?

And I think we need to write the flags from userspace into
drm_atomic_state. At least for most hw async flips have fairly strict
requirements, a lot stricter than normal flips at least.

Or we just disallow async updates for now through atomic.

> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	copied_objs = 0;
> +	copied_props = 0;
> +
> +	for (i = 0; i < arg->count_objs; i++) {
> +		uint32_t obj_id, count_props;
> +		struct drm_mode_object *obj;
> +
> +		if (get_user(obj_id, objs_ptr + copied_objs)) {
> +			ret = -EFAULT;
> +			goto fail;
> +		}
> +
> +		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> +		if (!obj || !obj->properties) {
> +			ret = -ENOENT;
> +			goto fail;
> +		}
> +
> +		if (obj->type == DRM_MODE_OBJECT_PLANE) {
> +			plane = obj_to_plane(obj);
> +			plane_mask |= (1 << drm_plane_index(plane));
> +			plane->old_fb = plane->fb;
> +		}
> +
> +		if (get_user(count_props, count_props_ptr + copied_objs)) {
> +			ret = -EFAULT;
> +			goto fail;
> +		}
> +
> +		copied_objs++;
> +
> +		for (j = 0; j < count_props; j++) {

If it's not too horrible I think extracting the inner properties loop
would be really nice to tame the monster.

> +			uint32_t prop_id;
> +			uint64_t prop_value;
> +			struct drm_property *prop;
> +			struct drm_mode_object *ref;
> +
> +			if (get_user(prop_id, props_ptr + copied_props)) {
> +				ret = -EFAULT;
> +				goto fail;
> +			}
> +
> +			prop = drm_property_find(dev, prop_id);
> +			if (!prop) {
> +				ret = -ENOENT;
> +				goto fail;
> +			}
> +
> +			if (get_user(prop_value, prop_values_ptr + copied_props)) {
> +				ret = -EFAULT;
> +				goto fail;
> +			}
> +
> +			if (!drm_property_change_valid_get(prop, prop_value, &ref)) {
> +				ret = -EINVAL;
> +				goto fail;
> +			}
> +
> +			switch (obj->type) {
> +			case DRM_MODE_OBJECT_CONNECTOR: {
> +				struct drm_connector *connector = obj_to_connector(obj);
> +				struct drm_connector_state *connector_state;
> +
> +				connector_state = drm_atomic_get_connector_state(state, connector);
> +				if (IS_ERR(connector_state)) {
> +					ret = PTR_ERR(connector_state);
> +					break;
> +				}
> +
> +				ret = drm_atomic_connector_set_property(connector,
> +						connector_state, prop, prop_value);
> +				break;
> +			}
> +			case DRM_MODE_OBJECT_CRTC: {
> +				struct drm_crtc *crtc = obj_to_crtc(obj);
> +				struct drm_crtc_state *crtc_state;
> +
> +				crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +				if (IS_ERR(crtc_state)) {
> +					ret = PTR_ERR(crtc_state);
> +					break;
> +				}
> +
> +				ret = drm_atomic_crtc_set_property(crtc,
> +						crtc_state, prop, prop_value);
> +				break;
> +			}
> +			case DRM_MODE_OBJECT_PLANE: {
> +				struct drm_plane *plane = obj_to_plane(obj);
> +				struct drm_plane_state *plane_state;
> +
> +				plane_state = drm_atomic_get_plane_state(state, plane);
> +				if (IS_ERR(plane_state)) {
> +					ret = PTR_ERR(plane_state);
> +					break;
> +				}
> +
> +				ret = drm_atomic_plane_set_property(plane,
> +						plane_state, prop, prop_value);
> +				break;
> +			}
> +			default:
> +				ret = -EINVAL;
> +				break;
> +			}
> +
> +			copied_props++;
> +
> +			drm_property_change_valid_put(prop, ref);
> +
> +			if (ret)
> +				goto fail;
> +		}
> +	}
> +
> +	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +		int ncrtcs = dev->mode_config.num_crtc;
> +
> +		for (i = 0; i < ncrtcs; i++) {
> +			struct drm_crtc_state *crtc_state = state->crtc_states[i];
> +			struct drm_pending_vblank_event *e;
> +
> +			if (!crtc_state)
> +				continue;
> +
> +			e = create_vblank_event(dev, file_priv, arg->user_data);
> +			if (!e) {
> +				ret = -ENOMEM;
> +				goto fail;
> +			}
> +
> +			crtc_state->event = e;
> +		}
> +	}
> +
> +	/* sanity check incoming state, because danvet wanted this
> +	 * to be an even bigger monster ioctl and wanted to be able
> +	 * to WARN_ON() in these cases in the helpers:
> +	 */
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		struct drm_plane_state *plane_state =
> +			drm_atomic_get_plane_state(state, plane);
> +
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto fail;
> +		}
> +
> +		if (WARN_ON(plane_state->crtc && !plane_state->fb)) {
> +			DRM_DEBUG_KMS("CRTC set but no FB\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		} else if (WARN_ON(plane_state->fb && !plane_state->crtc)) {
> +			DRM_DEBUG_KMS("FB set but no CRTC\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +	}

With my suggestion to move the checks from heleprs to check_only we could
remove the above hunk as redundant.

> +
> +	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> +		ret = drm_atomic_check_only(state);
> +		/* _check_only() does not free state, unlike _commit() */
> +		drm_atomic_state_free(state);
> +	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> +		ret = drm_atomic_async_commit(state);
> +	} else {
> +		ret = drm_atomic_commit(state);
> +	}
> +
> +	/* if succeeded, fixup legacy plane crtc/fb ptrs before dropping
> +	 * locks (ie. while it is still safe to deref plane->state).  We
> +	 * need to do this here because the driver entry points cannot
> +	 * distinguish between legacy and atomic ioctls.
> +	 */
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		if (ret == 0) {
> +			struct drm_framebuffer *new_fb = plane->state->fb;
> +			if (new_fb)
> +				drm_framebuffer_reference(new_fb);
> +			plane->fb = new_fb;
> +			plane->crtc = plane->state->crtc;
> +		} else {
> +			plane->old_fb = NULL;
> +		}
> +		if (plane->old_fb) {
> +			drm_framebuffer_unreference(plane->old_fb);
> +			plane->old_fb = NULL;
> +		}
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +
> +fail:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +		int ncrtcs = dev->mode_config.num_crtc;
> +
> +		for (i = 0; i < ncrtcs; i++) {
> +			struct drm_crtc_state *crtc_state = state->crtc_states[i];
> +
> +			if (!crtc_state)
> +				continue;
> +
> +			destroy_vblank_event(dev, file_priv, crtc_state->event);
> +			crtc_state->event = NULL;
> +		}
> +	}
> +
> +	drm_atomic_state_free(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_modeset_backoff(&ctx);
> +
> +	goto retry;
> +}
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 39c3e06..948d04d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4299,7 +4299,7 @@ EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>   * object to which the property is attached has a chance to take it's own
>   * reference).
>   */
> -static bool drm_property_change_valid_get(struct drm_property *property,
> +bool drm_property_change_valid_get(struct drm_property *property,
>  					 uint64_t value, struct drm_mode_object **ref)
>  {
>  	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
> @@ -4353,7 +4353,7 @@ static bool drm_property_change_valid_get(struct drm_property *property,
>  	}
>  }
>  
> -static void drm_property_change_valid_put(struct drm_property *property,
> +void drm_property_change_valid_put(struct drm_property *property,
>  		struct drm_mode_object *ref)
>  {
>  	if (!ref)
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 00587a1..fe76815 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -620,6 +620,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>  };
>  
>  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 48fcd98..8330676 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1332,6 +1332,10 @@ extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
>  extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
> +extern bool drm_property_change_valid_get(struct drm_property *property,
> +					 uint64_t value, struct drm_mode_object **ref);
> +extern void drm_property_change_valid_put(struct drm_property *property,
> +		struct drm_mode_object *ref);
>  
>  extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>  					     struct drm_encoder *encoder);
> @@ -1423,6 +1427,8 @@ extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>  extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>  				       struct drm_property *property,
>  				       uint64_t value);
> +extern int drm_mode_atomic_ioctl(struct drm_device *dev,
> +				 void *data, struct drm_file *file_priv);
>  
>  extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>  				 int *bpp);
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b0b8556..9cea966 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -777,6 +777,7 @@ struct drm_prime_handle {
>  #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES	DRM_IOWR(0xB9, struct drm_mode_obj_get_properties)
>  #define DRM_IOCTL_MODE_OBJ_SETPROPERTY	DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
>  #define DRM_IOCTL_MODE_CURSOR2		DRM_IOWR(0xBB, struct drm_mode_cursor2)
> +#define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
>  
>  /**
>   * Device specific ioctls should only be in their respective headers
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 86574b0..3459778 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
>  	uint32_t handle;
>  };
>  
> +/* page-flip flags are valid, plus: */
> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> +
> +#define DRM_MODE_ATOMIC_FLAGS (\
> +		DRM_MODE_PAGE_FLIP_EVENT |\
> +		DRM_MODE_PAGE_FLIP_ASYNC |\
> +		DRM_MODE_ATOMIC_TEST_ONLY |\
> +		DRM_MODE_ATOMIC_NONBLOCK)

Just for future-proofing: Should we add an ATOMIC_ALLOW_MODESET right away
(and add checks in the helpers for that case, should just be a check for
crtc_state->modeset_changed at the very end of
atomic_helper_check_modeset?

I really think we need to clarify this before we make the abi stable,
otherwise there will be surprise compositors doing a full modeset once per
frame. Which is totally uncool ofc. But doesn't need to be done right
away, but a small patch on top would be great (since it's so simple with
helpers at least).

> +
> +struct drm_mode_atomic {
> +	__u32 flags;
> +	__u32 count_objs;
> +	__u64 objs_ptr;
> +	__u64 count_props_ptr;
> +	__u64 props_ptr;
> +	__u64 prop_values_ptr;
> +	__u64 blob_values_ptr;  /* remove? */
> +	__u64 user_data;
> +};
> +
>  #endif
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Michel Dänzer]

On 17.12.2014 20:18, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 06:31:13PM +0900, Michel Dänzer wrote:
>> On 17.12.2014 16:20, Pekka Paalanen wrote:
>>> On Wed, 17 Dec 2014 11:48:51 +0900
>>> Michel Dänzer <michel@daenzer.net> wrote:
>>>
>>>> On 17.12.2014 08:05, Rob Clark wrote:
>>>>> The atomic modeset ioctl can be used to push any number of new values
>>>>> for object properties. The driver can then check the full device
>>>>> configuration as single unit, and try to apply the changes atomically.
>>>>>
>>>>> The ioctl simply takes a list of object IDs and property IDs and their
>>>>> values.
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>>> index 86574b0..3459778 100644
>>>>> --- a/include/uapi/drm/drm_mode.h
>>>>> +++ b/include/uapi/drm/drm_mode.h
>>>>> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
>>>>>  	uint32_t handle;
>>>>>  };
>>>>>  
>>>>> +/* page-flip flags are valid, plus: */
>>>>> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
>>>>> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
>>>>> +
>>>>> +#define DRM_MODE_ATOMIC_FLAGS (\
>>>>> +		DRM_MODE_PAGE_FLIP_EVENT |\
>>>>> +		DRM_MODE_PAGE_FLIP_ASYNC |\
>>>>> +		DRM_MODE_ATOMIC_TEST_ONLY |\
>>>>> +		DRM_MODE_ATOMIC_NONBLOCK)
>>>>> +
>>>>> +struct drm_mode_atomic {
>>>>> +	__u32 flags;
>>>>> +	__u32 count_objs;
>>>>> +	__u64 objs_ptr;
>>>>> +	__u64 count_props_ptr;
>>>>> +	__u64 props_ptr;
>>>>> +	__u64 prop_values_ptr;
>>>>> +	__u64 blob_values_ptr;  /* remove? */
>>>>> +	__u64 user_data;
>>>>> +};
>>>>> +
>>>>>  #endif
>>>>>
>>>>
>>>> The new ioctl(s) should take an explicit parameter specifying when the
>>>> changes should take effect. And since variable refresh rate displays are
>>>> becoming mainstream, that parameter should probably be a timestamp
>>>> rather than a frame counter.
>>>
>>> That sounds cool to me, but also a rabbit hole. While having worked on
>>> the Wayland Presentation queueing extension, I'd like to ask the
>>> following questions:
>>>
>>> - If you set the atomic kick to happen in the future, is there any way
>>>   to cancel it? I'd be ok with not being able to cancel initially, but
>>>   if one wants to add that later, we should already know how to
>>>   reference this atomic submission in the cancel request. What if user
>>>   space has a bug and schedules an update at one hour or three days from
>>>   now, how would we abort that?
>>>
>>> - Can you VT-switch or drop DRM master if you have a pending atomic
>>>   update?
>>>
>>> - Should one be able to set multiple pending atomic updates?
>>>
>>> - If I schedule an atomic update for one CTRC, can I schedule another
>>>   update for another CRTC before the first one completes? Or am I
>>>   forced to gather all updates over all outputs in the same atomic
>>>   submission even if I don't care about or want inter-output sync and
>>>   the outputs might even be running at different refresh rates?
>>>   (Actually, this seems to be a valid question even without any target
>>>   time parameter.)
>>>
>>> - If there can be multiple pending atomic updates on the same DRM
>>>   device, is there any way to guarantee that the
>>>   DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
>>>   atomic update actually kicks in? Another update may have changed the
>>>   configuration before this update kicks in, which means the overall
>>>   state isn't the same that was tested.
>>>
>>> - Does a pending atomic update prevent immediate (old style) KMS
>>>   changes?
>>>
>>> - Assuming hardware cannot do arbitrary time updates, how do you round
>>>   the given timestamp? Strictly "not before" given time? Round to
>>>   nearest possible time? The effect of required vs. unwanted sync to
>>>   vblank?
>>>
>>> - How would user space match the page flip event to the atomic
>>>   submission it did?
>>>
>>> I wonder if there is a way to postpone these hard(?) questions, so that
>>> we could have atomic sooner and add scheduling later? I would imagine
>>> solving everything above is quite some work.
>>
>> I agree. The main reason I brought it up is because I'd like to avoid
>> getting into the same situation as with the current
>> DRM_IOCTL_MODE_PAGE_FLIP ioctl, which doesn't explicitly communicate
>> between userspace and kernel when the flip is supposed/expected to
>> occur. We recently had to jump through some hoops in the radeon kernel
>> driver to prevent flips from occurring sooner than expected by userspace.
> 
> The current approach is to ask for a vblank event 1 frame earlier in the
> ddx and schedule the flip when you receive that. And hope for the best.

I'm well aware of that; it's exactly what broke down with radeon.

The fundamental problem is that if the timing semantics are not
explicitly specified in the interface, different drivers may end up
implementing slightly different semantics, and different userspace may
end up expecting the semantics implemented by different drivers.

If you guys are aware of this risk and confident it can be handled
without explicitly specifying the timing semantics in the interface
upfront, that's great. I wanted to make sure you're aware.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/13] RFC: drm: Atomic modeset ioctl

[Daniel Vetter]

On Fri, Dec 19, 2014 at 12:29:22PM +0900, Michel Dänzer wrote:
> On 17.12.2014 20:18, Daniel Vetter wrote:
> > On Wed, Dec 17, 2014 at 06:31:13PM +0900, Michel Dänzer wrote:
> >> On 17.12.2014 16:20, Pekka Paalanen wrote:
> >>> On Wed, 17 Dec 2014 11:48:51 +0900
> >>> Michel Dänzer <michel@daenzer.net> wrote:
> >>>
> >>>> On 17.12.2014 08:05, Rob Clark wrote:
> >>>>> The atomic modeset ioctl can be used to push any number of new values
> >>>>> for object properties. The driver can then check the full device
> >>>>> configuration as single unit, and try to apply the changes atomically.
> >>>>>
> >>>>> The ioctl simply takes a list of object IDs and property IDs and their
> >>>>> values.
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >>>>> index 86574b0..3459778 100644
> >>>>> --- a/include/uapi/drm/drm_mode.h
> >>>>> +++ b/include/uapi/drm/drm_mode.h
> >>>>> @@ -519,4 +519,25 @@ struct drm_mode_destroy_dumb {
> >>>>>  	uint32_t handle;
> >>>>>  };
> >>>>>  
> >>>>> +/* page-flip flags are valid, plus: */
> >>>>> +#define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> >>>>> +#define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> >>>>> +
> >>>>> +#define DRM_MODE_ATOMIC_FLAGS (\
> >>>>> +		DRM_MODE_PAGE_FLIP_EVENT |\
> >>>>> +		DRM_MODE_PAGE_FLIP_ASYNC |\
> >>>>> +		DRM_MODE_ATOMIC_TEST_ONLY |\
> >>>>> +		DRM_MODE_ATOMIC_NONBLOCK)
> >>>>> +
> >>>>> +struct drm_mode_atomic {
> >>>>> +	__u32 flags;
> >>>>> +	__u32 count_objs;
> >>>>> +	__u64 objs_ptr;
> >>>>> +	__u64 count_props_ptr;
> >>>>> +	__u64 props_ptr;
> >>>>> +	__u64 prop_values_ptr;
> >>>>> +	__u64 blob_values_ptr;  /* remove? */
> >>>>> +	__u64 user_data;
> >>>>> +};
> >>>>> +
> >>>>>  #endif
> >>>>>
> >>>>
> >>>> The new ioctl(s) should take an explicit parameter specifying when the
> >>>> changes should take effect. And since variable refresh rate displays are
> >>>> becoming mainstream, that parameter should probably be a timestamp
> >>>> rather than a frame counter.
> >>>
> >>> That sounds cool to me, but also a rabbit hole. While having worked on
> >>> the Wayland Presentation queueing extension, I'd like to ask the
> >>> following questions:
> >>>
> >>> - If you set the atomic kick to happen in the future, is there any way
> >>>   to cancel it? I'd be ok with not being able to cancel initially, but
> >>>   if one wants to add that later, we should already know how to
> >>>   reference this atomic submission in the cancel request. What if user
> >>>   space has a bug and schedules an update at one hour or three days from
> >>>   now, how would we abort that?
> >>>
> >>> - Can you VT-switch or drop DRM master if you have a pending atomic
> >>>   update?
> >>>
> >>> - Should one be able to set multiple pending atomic updates?
> >>>
> >>> - If I schedule an atomic update for one CTRC, can I schedule another
> >>>   update for another CRTC before the first one completes? Or am I
> >>>   forced to gather all updates over all outputs in the same atomic
> >>>   submission even if I don't care about or want inter-output sync and
> >>>   the outputs might even be running at different refresh rates?
> >>>   (Actually, this seems to be a valid question even without any target
> >>>   time parameter.)
> >>>
> >>> - If there can be multiple pending atomic updates on the same DRM
> >>>   device, is there any way to guarantee that the
> >>>   DRM_MODE_ATOMIC_TEST_ONLY results will be accurate also when the
> >>>   atomic update actually kicks in? Another update may have changed the
> >>>   configuration before this update kicks in, which means the overall
> >>>   state isn't the same that was tested.
> >>>
> >>> - Does a pending atomic update prevent immediate (old style) KMS
> >>>   changes?
> >>>
> >>> - Assuming hardware cannot do arbitrary time updates, how do you round
> >>>   the given timestamp? Strictly "not before" given time? Round to
> >>>   nearest possible time? The effect of required vs. unwanted sync to
> >>>   vblank?
> >>>
> >>> - How would user space match the page flip event to the atomic
> >>>   submission it did?
> >>>
> >>> I wonder if there is a way to postpone these hard(?) questions, so that
> >>> we could have atomic sooner and add scheduling later? I would imagine
> >>> solving everything above is quite some work.
> >>
> >> I agree. The main reason I brought it up is because I'd like to avoid
> >> getting into the same situation as with the current
> >> DRM_IOCTL_MODE_PAGE_FLIP ioctl, which doesn't explicitly communicate
> >> between userspace and kernel when the flip is supposed/expected to
> >> occur. We recently had to jump through some hoops in the radeon kernel
> >> driver to prevent flips from occurring sooner than expected by userspace.
> > 
> > The current approach is to ask for a vblank event 1 frame earlier in the
> > ddx and schedule the flip when you receive that. And hope for the best.
> 
> I'm well aware of that; it's exactly what broke down with radeon.
> 
> The fundamental problem is that if the timing semantics are not
> explicitly specified in the interface, different drivers may end up
> implementing slightly different semantics, and different userspace may
> end up expecting the semantics implemented by different drivers.
> 
> If you guys are aware of this risk and confident it can be handled
> without explicitly specifying the timing semantics in the interface
> upfront, that's great. I wanted to make sure you're aware.

There's two answers to that:
- With my intel hat on: We check all these things plus piles of other
  races (flip-vs-anything tests really) in igt. I think i915 is pretty
  much just the standard here so I'll be fine with that for no.

- With my community hat on I'd really like for someone (can't really
  justify this to my boss) to port igt to libkms or something similar for
  the generic tests and make it run everywhere. It might need some detail
  work but overall we have the infrastructure to skip specific tests as
  needed easily.

  Until that happens my approach is to try to push as much as possible of
  the i915 semantics into core/shared helpers, like what I've proposed for
  the dpms atomic stuff.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel