[RFC 00/11] atomic pageflip (v2)

[RFC 00/11] atomic pageflip (v2)

[Rob Clark]

From: Rob Clark <rob@ti.com>

This is following a bit the approach that Ville is taking for atomic-
modeset, in that it is switching over to using properties for everything.
The advantage of this approach is that it makes it easier to add new
attributes to set as part of a page-flip (and even opens the option to
add new object types).

The basic principles are:
 a) split out object state (in this case, plane and crtc, although I
    expect more to be added as atomic-modeset is added) into seperate
    structures that can be atomically commited or rolled back.  The
    property values array is also split out into the state structs,
    so that the property values visible to userspace automatically
    reflect the current state (ie. property changes are either
    committed or discarded depending on whether the state changes
    are committed or discarded).
 b) expand the object property API (set_property()) to take a state
    object.  The obj->set_property() simply updates the state object
    without actually applying the changes to the hw.
 c) after all the property updates are done, the updated state can
    be checked for correctness and against the hw capabilities, and
    then either discarded or committed atomically.

Since we need to include properties in the atomic-pageflip scheme,
doing everything via properties avoids updating a bunch of additional
driver provided callbacks.  Instead we just drop crtc->page_flip()
and plane->update_plane().  By splitting out the object's mutable
state into drm_{plane,crtc,etc}_state structs (which are wrapped by
the individual drivers to add their own hw specific state), we can
use some helpers (drm_{plane,crtc,etc}_set_property() and
drm_{plane,crtc,etc}_check_state()) to keep core error checking in
drm core and avoid pushing the burden of dealing with common
properties to the individual drivers.

The intention is for this to also simplify atomic-modeset, by
providing some of the necessary infrastructure (object property types,
signed property values, and property's whose usespace visible values
automatically tracks the object state which is either committed or
discarded depending on whether the state change was committed to hw
or not) which will also be needed for atomic-modeset.

So far, I've only updated omapdrm to the new APIs, as a proof of
concept.  Only a few drivers support drm plane, so I expect the
updates to convert drm-plane to properties should not be so hard.
Possibly for crtc/pageflip we might need to have a transition period
where we still support crtc->page_flip() code path until all drivers
are updated.

My complete branch is here:

  https://github.com/robclark/kernel-omap4/commits/drm_nuclear
  git://github.com/robclark/kernel-omap4.git drm_nuclear

Open points (I think) are:
 + should atomic-pageflip support flips on multiple CRTCs.  I think
   it at least simplifies things if the driver knows up front which
   CRTC(s) are involved, and whether the operation is modeset (sync)
   or pageflip (async).
 + How to deal w/ hardware restrictions which span multiple CRTCs.
   One idea is to get rid of the test flag in the atomic-pageflip
   ioctl, and always do test operations via atomic-modeset ioctl
   w/ the test flag.

Rob Clark (11):
  drm: add atomic fxns
  drm: add object property type
  drm: add DRM_MODE_PROP_DYNAMIC property flag
  drm: add DRM_MODE_PROP_SIGNED property flag
  drm: split property values out
  drm: convert plane to properties
  drm: add drm_plane_state
  drm: convert page_flip to properties
  drm: add drm_crtc_state
  drm: atomic pageflip
  drm/omap: update for atomic age

 drivers/gpu/drm/drm_crtc.c            |  838 ++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_crtc_helper.c     |   51 +-
 drivers/gpu/drm/drm_drv.c             |    1 +
 drivers/gpu/drm/drm_fb_helper.c       |   11 +-
 drivers/staging/omapdrm/Makefile      |    1 +
 drivers/staging/omapdrm/omap_atomic.c |  339 +++++++++++++
 drivers/staging/omapdrm/omap_atomic.h |   52 ++
 drivers/staging/omapdrm/omap_crtc.c   |  246 +++++-----
 drivers/staging/omapdrm/omap_drv.c    |   21 +-
 drivers/staging/omapdrm/omap_drv.h    |   45 +-
 drivers/staging/omapdrm/omap_fb.c     |   44 +-
 drivers/staging/omapdrm/omap_plane.c  |  296 ++++++------
 include/drm/drm.h                     |    2 +
 include/drm/drmP.h                    |   52 ++
 include/drm/drm_crtc.h                |  179 +++++--
 include/drm/drm_mode.h                |   50 ++
 16 files changed, 1607 insertions(+), 621 deletions(-)
 create mode 100644 drivers/staging/omapdrm/omap_atomic.c
 create mode 100644 drivers/staging/omapdrm/omap_atomic.h

-- 
1.7.9.5

[RFC 01/11] drm: add atomic fxns

[Rob Clark]

From: Rob Clark <rob@ti.com>

The 'atomic' mechanism allows for multiple properties to be updated,
checked, and commited atomically.  This will be the basis of atomic-
modeset and nuclear-pageflip.

The basic flow is:

   state = dev->atomic_begin();
   for (... one or more ...)
      obj->set_property(obj, state, prop, value);
   if (dev->atomic_check(state))
      dev->atomic_commit(state, event);
   dev->atomic_end(state);

The split of check and commit steps is to allow for ioctls with a
test-only flag (which would skip the commit step).

The atomic functions are mandatory, as they will end up getting
called from enough places that it is easier not to have to bother
with if-null checks everywhere.

TODO:
  + add some stub atomic functions that can be used by drivers
    until they are updated to support atomic operations natively
  + roll-back previous property values if check/commit fails, so
    userspace doesn't see incorrect values.
---
 drivers/gpu/drm/drm_crtc.c |  126 +++++++++++++++++++++++++++-----------------
 include/drm/drmP.h         |   52 ++++++++++++++++++
 include/drm/drm_crtc.h     |    8 +--
 3 files changed, 135 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7552675..3a087cf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3227,12 +3227,11 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 	return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv);
 }
 
-static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
-					   struct drm_property *property,
+static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
+					   void *state, struct drm_property *property,
 					   uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_connector *connector = obj_to_connector(obj);
 
 	/* Do DPMS ourselves */
 	if (property == connector->dev->mode_config.dpms_property) {
@@ -3240,7 +3239,7 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 			(*connector->funcs->dpms)(connector, (int)value);
 		ret = 0;
 	} else if (connector->funcs->set_property)
-		ret = connector->funcs->set_property(connector, property, value);
+		ret = connector->funcs->set_property(connector, state, property, value);
 
 	/* store the property value if successful */
 	if (!ret)
@@ -3248,36 +3247,87 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 	return ret;
 }
 
-static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
-				      struct drm_property *property,
+static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
+				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_crtc *crtc = obj_to_crtc(obj);
 
 	if (crtc->funcs->set_property)
-		ret = crtc->funcs->set_property(crtc, property, value);
+		ret = crtc->funcs->set_property(crtc, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(obj, property, value);
+		drm_object_property_set_value(&crtc->base, property, value);
 
 	return ret;
 }
 
-static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
-				      struct drm_property *property,
+static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
+				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_plane *plane = obj_to_plane(obj);
 
 	if (plane->funcs->set_property)
-		ret = plane->funcs->set_property(plane, property, value);
+		ret = plane->funcs->set_property(plane, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(obj, property, value);
+		drm_object_property_set_value(&plane->base, property, value);
 
 	return ret;
 }
 
+static int drm_mode_set_obj_prop(struct drm_device *dev,
+		struct drm_mode_object *obj, void *state,
+		struct drm_property *property, uint64_t value)
+{
+	if (drm_property_change_is_valid(property, value)) {
+		switch (obj->type) {
+		case DRM_MODE_OBJECT_CONNECTOR:
+			return drm_mode_connector_set_obj_prop(obj_to_connector(obj),
+					state, property, value);
+		case DRM_MODE_OBJECT_CRTC:
+			return drm_mode_crtc_set_obj_prop(obj_to_crtc(obj),
+					state, property, value);
+		case DRM_MODE_OBJECT_PLANE:
+			return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
+					state, property, value);
+		}
+	}
+
+	return -EINVAL;
+}
+
+/* call with mode_config mutex held */
+static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
+		uint32_t obj_id, uint32_t obj_type,
+		uint32_t prop_id, uint64_t value)
+{
+	struct drm_mode_object *arg_obj;
+	struct drm_mode_object *prop_obj;
+	struct drm_property *property;
+	int i;
+
+	arg_obj = drm_mode_object_find(dev, obj_id, obj_type);
+	if (!arg_obj)
+		return -EINVAL;
+	if (!arg_obj->properties)
+		return -EINVAL;
+
+	for (i = 0; i < arg_obj->properties->count; i++)
+		if (arg_obj->properties->ids[i] == prop_id)
+			break;
+
+	if (i == arg_obj->properties->count)
+		return -EINVAL;
+
+	prop_obj = drm_mode_object_find(dev, prop_id,
+					DRM_MODE_OBJECT_PROPERTY);
+	if (!prop_obj)
+		return -EINVAL;
+	property = obj_to_property(prop_obj);
+
+	return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);
+}
+
 int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv)
 {
@@ -3338,53 +3388,35 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
 {
 	struct drm_mode_obj_set_property *arg = data;
-	struct drm_mode_object *arg_obj;
-	struct drm_mode_object *prop_obj;
-	struct drm_property *property;
+	void *state = NULL;
 	int ret = -EINVAL;
-	int i;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
-	if (!arg_obj)
-		goto out;
-	if (!arg_obj->properties)
-		goto out;
-
-	for (i = 0; i < arg_obj->properties->count; i++)
-		if (arg_obj->properties->ids[i] == arg->prop_id)
-			break;
+	state = dev->driver->atomic_begin(dev, NULL);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto out_unlock;
+	}
 
-	if (i == arg_obj->properties->count)
+	ret = drm_mode_set_obj_prop_id(dev, state,
+			arg->obj_id, arg->obj_type,
+			arg->prop_id, arg->value);
+	if (ret)
 		goto out;
 
-	prop_obj = drm_mode_object_find(dev, arg->prop_id,
-					DRM_MODE_OBJECT_PROPERTY);
-	if (!prop_obj)
-		goto out;
-	property = obj_to_property(prop_obj);
-
-	if (!drm_property_change_is_valid(property, arg->value))
+	ret = dev->driver->atomic_check(dev, state);
+	if (ret)
 		goto out;
 
-	switch (arg_obj->type) {
-	case DRM_MODE_OBJECT_CONNECTOR:
-		ret = drm_mode_connector_set_obj_prop(arg_obj, property,
-						      arg->value);
-		break;
-	case DRM_MODE_OBJECT_CRTC:
-		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
-		break;
-	case DRM_MODE_OBJECT_PLANE:
-		ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);
-		break;
-	}
+	ret = dev->driver->atomic_commit(dev, state, NULL);
 
 out:
+	dev->driver->atomic_end(dev, state);
+out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d6b67bb..f719c4d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -933,6 +933,58 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/*
+	 * Atomic functions:
+	 */
+
+	/**
+	 * Begin a sequence of atomic property sets.  Returns a driver
+	 * private state object that is passed back into the various
+	 * object's set_property() fxns, and into the remainder of the
+	 * atomic funcs.  The state object should accumulate the changes
+	 * from one o more set_property()'s.  At the end, the state can
+	 * be checked, and optionally committed.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param crtc for asynchronous page-flip operations, the crtc
+	 *   that is being updated.  (The driver should return -EBUSY if
+	 *   a page-flip is still pending.)  Otherwise, NULL.
+	 * \returns a driver private state object, which is passed
+	 *   back in to the various other atomic fxns
+	 */
+	void *(*atomic_begin)(struct drm_device *dev, struct drm_crtc *crtc);
+
+	/**
+	 * Check the state object to see if the requested state is
+	 * physically possible.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 */
+	int (*atomic_check)(struct drm_device *dev, void *state);
+
+	/**
+	 * Commit the state.  This will only be called if atomic_check()
+	 * succeeds.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 * \param event for asynchronous page-flip operations, the
+	 *   userspace has requested an event to be sent when the
+	 *   page-flip completes, or NULL.  Will always be NULL for
+	 *   non-page-flip operations
+	 */
+	int (*atomic_commit)(struct drm_device *dev, void *state,
+			struct drm_pending_vblank_event *event);
+
+	/**
+	 * Release resources associated with the state object.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 */
+	void (*atomic_end)(struct drm_device *dev, void *state);
+
 	/* Driver private ops for this object */
 	const struct vm_operations_struct *gem_vm_ops;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1422b36..a609190 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -357,7 +357,7 @@ struct drm_crtc_funcs {
 			 struct drm_framebuffer *fb,
 			 struct drm_pending_vblank_event *event);
 
-	int (*set_property)(struct drm_crtc *crtc,
+	int (*set_property)(struct drm_crtc *crtc, void *state,
 			    struct drm_property *property, uint64_t val);
 };
 
@@ -455,8 +455,8 @@ struct drm_connector_funcs {
 	enum drm_connector_status (*detect)(struct drm_connector *connector,
 					    bool force);
 	int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height);
-	int (*set_property)(struct drm_connector *connector, struct drm_property *property,
-			     uint64_t val);
+	int (*set_property)(struct drm_connector *connector, void *state,
+			struct drm_property *property, uint64_t val);
 	void (*destroy)(struct drm_connector *connector);
 	void (*force)(struct drm_connector *connector);
 };
@@ -627,7 +627,7 @@ struct drm_plane_funcs {
 	int (*disable_plane)(struct drm_plane *plane);
 	void (*destroy)(struct drm_plane *plane);
 
-	int (*set_property)(struct drm_plane *plane,
+	int (*set_property)(struct drm_plane *plane, void *state,
 			    struct drm_property *property, uint64_t val);
 };
 
-- 
1.7.9.5

[RFC 02/11] drm: add object property type

[Rob Clark]

From: Rob Clark <rob@ti.com>

An object property is an id (idr) for a drm mode object.  This
will allow a property to be used set/get a framebuffer, CRTC, etc.
---
 drivers/gpu/drm/drm_crtc.c |   35 ++++++++++++++++++++++++++++++-----
 include/drm/drm_crtc.h     |   10 ++++++++++
 include/drm/drm_mode.h     |    1 +
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3a087cf..2b4526e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2756,6 +2756,8 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 	if (!property)
 		return NULL;
 
+	property->dev = dev;
+
 	if (num_values) {
 		property->values = kzalloc(sizeof(uint64_t)*num_values, GFP_KERNEL);
 		if (!property->values)
@@ -2859,6 +2861,23 @@ struct drm_property *drm_property_create_range(struct drm_device *dev, int flags
 }
 EXPORT_SYMBOL(drm_property_create_range);
 
+struct drm_property *drm_property_create_object(struct drm_device *dev,
+					 int flags, const char *name, uint32_t type)
+{
+	struct drm_property *property;
+
+	flags |= DRM_MODE_PROP_OBJECT;
+
+	property = drm_property_create(dev, flags, name, 1);
+	if (!property)
+		return NULL;
+
+	property->values[0] = type;
+
+	return property;
+}
+EXPORT_SYMBOL(drm_property_create_object);
+
 int drm_property_add_enum(struct drm_property *property, int index,
 			  uint64_t value, const char *name)
 {
@@ -3203,6 +3222,11 @@ static bool drm_property_change_is_valid(struct drm_property *property,
 		for (i = 0; i < property->num_values; i++)
 			valid_mask |= (1ULL << property->values[i]);
 		return !(value & ~valid_mask);
+	} else if (property->flags & DRM_MODE_PROP_OBJECT) {
+		/* a zero value for an object property translates to null: */
+		if (value)
+			return true;
+		return drm_property_get_obj(property, value) != NULL;
 	} else {
 		int i;
 		for (i = 0; i < property->num_values; i++)
@@ -3243,7 +3267,7 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
 
 	/* store the property value if successful */
 	if (!ret)
-		drm_connector_property_set_value(connector, property, value);
+		drm_object_property_set_value(&connector->base, property, value);
 	return ret;
 }
 
@@ -3275,9 +3299,8 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 	return ret;
 }
 
-static int drm_mode_set_obj_prop(struct drm_device *dev,
-		struct drm_mode_object *obj, void *state,
-		struct drm_property *property, uint64_t value)
+static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
+		void *state, struct drm_property *property, uint64_t value)
 {
 	if (drm_property_change_is_valid(property, value)) {
 		switch (obj->type) {
@@ -3291,6 +3314,8 @@ static int drm_mode_set_obj_prop(struct drm_device *dev,
 			return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
 					state, property, value);
 		}
+	} else {
+		DRM_DEBUG("invalid value: %s = %llx\n", property->name, value);
 	}
 
 	return -EINVAL;
@@ -3325,7 +3350,7 @@ static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
 		return -EINVAL;
 	property = obj_to_property(prop_obj);
 
-	return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);
+	return drm_mode_set_obj_prop(arg_obj, state, property, value);
 }
 
 int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a609190..0dbf2be 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -291,6 +291,7 @@ struct drm_property {
 	char name[DRM_PROP_NAME_LEN];
 	uint32_t num_values;
 	uint64_t *values;
+	struct drm_device *dev;
 
 	struct list_head enum_blob_list;
 };
@@ -955,6 +956,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
 struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
 					 const char *name,
 					 uint64_t min, uint64_t max);
+struct drm_property *drm_property_create_object(struct drm_device *dev,
+					 int flags, const char *name, uint32_t type);
 extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
 extern int drm_property_add_enum(struct drm_property *property, int index,
 				 uint64_t value, const char *name);
@@ -974,6 +977,13 @@ extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
 extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 		uint32_t id, uint32_t type);
+
+static inline struct drm_mode_object *
+drm_property_get_obj(struct drm_property *property, uint64_t value)
+{
+	return drm_mode_object_find(property->dev, value, property->values[0]);
+}
+
 /* IOCTLs */
 extern int drm_mode_getresources(struct drm_device *dev,
 				 void *data, struct drm_file *file_priv);
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 5581980..cee4c53 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -231,6 +231,7 @@ struct drm_mode_get_connector {
 #define DRM_MODE_PROP_ENUM	(1<<3) /* enumerated type with text strings */
 #define DRM_MODE_PROP_BLOB	(1<<4)
 #define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
+#define DRM_MODE_PROP_OBJECT	(1<<6) /* drm mode object */
 
 struct drm_mode_property_enum {
 	__u64 value;
-- 
1.7.9.5

[RFC 03/11] drm: add DRM_MODE_PROP_DYNAMIC property flag

[Rob Clark]

From: Rob Clark <rob@ti.com>

This indicates to userspace that the property is something that can
be set dynamically without requiring a "test" step to check if the
hw is capable.  This allows a userspace compositor, such as weston,
to avoid an extra ioctl to check whether it needs to fall-back to
GPU to composite some surface prior to submission of GPU render
commands.
---
 include/drm/drm_mode.h |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index cee4c53..15689d4 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -232,6 +232,15 @@ struct drm_mode_get_connector {
 #define DRM_MODE_PROP_BLOB	(1<<4)
 #define DRM_MODE_PROP_BITMASK	(1<<5) /* bitmask of enumerated types */
 #define DRM_MODE_PROP_OBJECT	(1<<6) /* drm mode object */
+/* Properties that are not dynamic cannot safely be changed without a
+ * atomic-modeset / atomic-pageflip test step.  But if userspace is
+ * only changing dynamic properties, it is guaranteed that the change
+ * will not exceed hw limits, so no test step is required.
+ *
+ * Note that fb_id properties are a bit ambiguous.. they of course can
+ * be changed dynamically, assuming the pixel format does not change.
+ */
+#define DRM_MODE_PROP_DYNAMIC	(1<<24)
 
 struct drm_mode_property_enum {
 	__u64 value;
-- 
1.7.9.5

[RFC 04/11] drm: add DRM_MODE_PROP_SIGNED property flag

[Rob Clark]

From: Rob Clark <rob@ti.com>

Flag for range property types indicating that the value is a signed
integer rather than unsigned.  For range properties, the signed flag
will trigger use of signed integer comparisions, to handle negative
values properly.
---
 drivers/gpu/drm/drm_crtc.c |   10 ++++++++--
 include/drm/drm_crtc.h     |    9 +++++++++
 include/drm/drm_mode.h     |    2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2b4526e..e62ae6a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3210,9 +3210,15 @@ EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
 static bool drm_property_change_is_valid(struct drm_property *property,
 					 uint64_t value)
 {
-	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
+	if (property->flags & DRM_MODE_PROP_IMMUTABLE) {
 		return false;
-	if (property->flags & DRM_MODE_PROP_RANGE) {
+	} else if (property->flags & (DRM_MODE_PROP_RANGE|DRM_MODE_PROP_SIGNED)) {
+		int64_t svalue = U642I64(value);
+		if (svalue < U642I64(property->values[0]) ||
+				svalue > U642I64(property->values[1]))
+			return false;
+		return true;
+	} else if (property->flags & DRM_MODE_PROP_RANGE) {
 		if (value < property->values[0] || value > property->values[1])
 			return false;
 		return true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0dbf2be..dc67f5f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -61,6 +61,15 @@ struct drm_object_properties {
 	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
+static inline int64_t U642I64(uint64_t val)
+{
+	return (int64_t)*((int64_t *)&val);
+}
+static inline uint64_t I642U64(int64_t val)
+{
+	return (uint64_t)*((uint64_t *)&val);
+}
+
 /*
  * Note on terminology:  here, for brevity and convenience, we refer to connector
  * control chips as 'CRTCs'.  They can control any type of connector, VGA, LVDS,
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 15689d4..9cc0336 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -241,6 +241,8 @@ struct drm_mode_get_connector {
  * be changed dynamically, assuming the pixel format does not change.
  */
 #define DRM_MODE_PROP_DYNAMIC	(1<<24)
+/* Indicates that numeric property values are signed rather than unsigned: */
+#define DRM_MODE_PROP_SIGNED   (1<<25)
 
 struct drm_mode_property_enum {
 	__u64 value;
-- 
1.7.9.5

[RFC 05/11] drm: split property values out

[Rob Clark]

From: Rob Clark <rob@ti.com>

Split property values out into a different struct, so we can later
move property values into state structs.  This will allow the
property values to stay in sync w/ the state updates which are
either discarded or atomically committed.
---
 drivers/gpu/drm/drm_crtc.c |   27 ++++++++++++++++++---------
 include/drm/drm_crtc.h     |   10 +++++++++-
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e62ae6a..245c462 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -446,6 +446,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		goto out;
 
 	crtc->base.properties = &crtc->properties;
+	crtc->base.propvals = &crtc->propvals;
 
 	list_add_tail(&crtc->head, &dev->mode_config.crtc_list);
 	dev->mode_config.num_crtc++;
@@ -547,6 +548,7 @@ int drm_connector_init(struct drm_device *dev,
 		goto out;
 
 	connector->base.properties = &connector->properties;
+	connector->base.propvals = &connector->propvals;
 	connector->dev = dev;
 	connector->funcs = funcs;
 	connector->connector_type = connector_type;
@@ -670,6 +672,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		goto out;
 
 	plane->base.properties = &plane->properties;
+	plane->base.propvals = &plane->propvals;
 	plane->dev = dev;
 	plane->funcs = funcs;
 	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
@@ -1549,7 +1552,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 				goto out;
 			}
 
-			if (put_user(connector->properties.values[i],
+			if (put_user(connector->propvals.values[i],
 				     prop_values + copied)) {
 				ret = -EFAULT;
 				goto out;
@@ -2944,7 +2947,8 @@ EXPORT_SYMBOL(drm_connector_attach_property);
 int drm_connector_property_set_value(struct drm_connector *connector,
 				  struct drm_property *property, uint64_t value)
 {
-	return drm_object_property_set_value(&connector->base, property, value);
+	return drm_object_property_set_value(&connector->base,
+			&connector->propvals, property, value);
 }
 EXPORT_SYMBOL(drm_connector_property_set_value);
 
@@ -2970,19 +2974,20 @@ void drm_object_attach_property(struct drm_mode_object *obj,
 	}
 
 	obj->properties->ids[count] = property->base.id;
-	obj->properties->values[count] = init_val;
+	obj->propvals->values[count] = init_val;
 	obj->properties->count++;
 }
 EXPORT_SYMBOL(drm_object_attach_property);
 
 int drm_object_property_set_value(struct drm_mode_object *obj,
+				  struct drm_object_property_values *propvals,
 				  struct drm_property *property, uint64_t val)
 {
 	int i;
 
 	for (i = 0; i < obj->properties->count; i++) {
 		if (obj->properties->ids[i] == property->base.id) {
-			obj->properties->values[i] = val;
+			propvals->values[i] = val;
 			return 0;
 		}
 	}
@@ -2998,7 +3003,7 @@ int drm_object_property_get_value(struct drm_mode_object *obj,
 
 	for (i = 0; i < obj->properties->count; i++) {
 		if (obj->properties->ids[i] == property->base.id) {
-			*val = obj->properties->values[i];
+			*val = obj->propvals->values[i];
 			return 0;
 		}
 	}
@@ -3273,7 +3278,9 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
 
 	/* store the property value if successful */
 	if (!ret)
-		drm_object_property_set_value(&connector->base, property, value);
+		drm_object_property_set_value(&connector->base,
+				&connector->propvals, property, value);
+
 	return ret;
 }
 
@@ -3286,7 +3293,8 @@ static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
 	if (crtc->funcs->set_property)
 		ret = crtc->funcs->set_property(crtc, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(&crtc->base, property, value);
+		drm_object_property_set_value(&crtc->base, &crtc->propvals,
+				property, value);
 
 	return ret;
 }
@@ -3300,7 +3308,8 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 	if (plane->funcs->set_property)
 		ret = plane->funcs->set_property(plane, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(&plane->base, property, value);
+		drm_object_property_set_value(&plane->base, &plane->propvals,
+				property, value);
 
 	return ret;
 }
@@ -3401,7 +3410,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				ret = -EFAULT;
 				goto out;
 			}
-			if (put_user(obj->properties->values[i],
+			if (put_user(obj->propvals->values[i],
 				     prop_values_ptr + copied)) {
 				ret = -EFAULT;
 				goto out;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index dc67f5f..b5f71f8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -37,7 +37,7 @@ struct drm_device;
 struct drm_mode_set;
 struct drm_framebuffer;
 struct drm_object_properties;
-
+struct drm_object_property_values;
 
 #define DRM_MODE_OBJECT_CRTC 0xcccccccc
 #define DRM_MODE_OBJECT_CONNECTOR 0xc0c0c0c0
@@ -52,12 +52,16 @@ struct drm_mode_object {
 	uint32_t id;
 	uint32_t type;
 	struct drm_object_properties *properties;
+	struct drm_object_property_values *propvals;
 };
 
 #define DRM_OBJECT_MAX_PROPERTY 24
 struct drm_object_properties {
 	int count;
 	uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
+};
+
+struct drm_object_property_values {
 	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
@@ -431,6 +435,7 @@ struct drm_crtc {
 	void *helper_private;
 
 	struct drm_object_properties properties;
+	struct drm_object_property_values propvals;
 };
 
 
@@ -597,6 +602,7 @@ struct drm_connector {
 	struct list_head user_modes;
 	struct drm_property_blob *edid_blob_ptr;
 	struct drm_object_properties properties;
+	struct drm_object_property_values propvals;
 
 	uint8_t polled; /* DRM_CONNECTOR_POLL_* */
 
@@ -681,6 +687,7 @@ struct drm_plane {
 	void *helper_private;
 
 	struct drm_object_properties properties;
+	struct drm_object_property_values propvals;
 };
 
 /**
@@ -934,6 +941,7 @@ extern int drm_connector_property_get_value(struct drm_connector *connector,
 					 struct drm_property *property,
 					 uint64_t *value);
 extern int drm_object_property_set_value(struct drm_mode_object *obj,
+					 struct drm_object_property_values *propvals,
 					 struct drm_property *property,
 					 uint64_t val);
 extern int drm_object_property_get_value(struct drm_mode_object *obj,
-- 
1.7.9.5

[RFC 06/11] drm: convert plane to properties

[Rob Clark]

From: Rob Clark <rob@ti.com>

Use atomic properties mechanism to set plane attributes.  This by
itself doesn't accomplish anything, but it avoids having multiple
code paths to do the same thing when nuclear-pageflip and atomic-
modeset are introduced.
---
 drivers/gpu/drm/drm_crtc.c |  257 ++++++++++++++++++++++++++------------------
 include/drm/drm_crtc.h     |   32 ++++--
 2 files changed, 179 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 245c462..81a63fe 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -38,6 +38,9 @@
 #include "drm_edid.h"
 #include "drm_fourcc.h"
 
+static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
+		void *state, struct drm_property *property, uint64_t value);
+
 /* Avoid boilerplate.  I'm tired of typing. */
 #define DRM_ENUM_NAME_FN(fnname, list)				\
 	char *fnname(int val)					\
@@ -380,11 +383,19 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
 void drm_framebuffer_remove(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
+	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	struct drm_mode_set set;
+	void *state;
 	int ret;
 
+	state = dev->driver->atomic_begin(dev, NULL);
+	if (IS_ERR(state)) {
+		DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
+		return;
+	}
+
 	/* remove from any CRTC */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (crtc->fb == fb) {
@@ -401,15 +412,19 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
 		if (plane->fb == fb) {
 			/* should turn off the crtc */
-			ret = plane->funcs->disable_plane(plane);
-			if (ret)
-				DRM_ERROR("failed to disable plane with busy fb\n");
-			/* disconnect the plane from the fb and crtc: */
-			plane->fb = NULL;
-			plane->crtc = NULL;
+			drm_mode_plane_set_obj_prop(plane, state, config->prop_crtc_id, 0);
+			drm_mode_plane_set_obj_prop(plane, state, config->prop_fb_id, 0);
 		}
 	}
 
+	/* just disabling stuff shouldn't fail, hopefully: */
+	if(dev->driver->atomic_check(dev, state))
+		DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
+	else
+		dev->driver->atomic_commit(dev, state, NULL);
+
+	dev->driver->atomic_end(dev, state);
+
 	list_del(&fb->filp_head);
 
 	drm_framebuffer_unreference(fb);
@@ -663,6 +678,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   const uint32_t *formats, uint32_t format_count,
 		   bool priv)
 {
+	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
 
 	mutex_lock(&dev->mode_config.mutex);
@@ -699,6 +715,17 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		INIT_LIST_HEAD(&plane->head);
 	}
 
+	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);
+
  out:
 	mutex_unlock(&dev->mode_config.mutex);
 
@@ -772,23 +799,91 @@ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode)
 }
 EXPORT_SYMBOL(drm_mode_destroy);
 
-static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
+static int drm_mode_create_standard_properties(struct drm_device *dev)
 {
-	struct drm_property *edid;
-	struct drm_property *dpms;
+	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;
+
+
+	/* TODO we need the driver to control which of these are dynamic
+	 * and which are not..  or maybe we should just set all to zero
+	 * and let the individual drivers frob the prop->flags for the
+	 * properties they can support dynamic changes on..
+	 */
+
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC,
+			"src_x", 0, UINT_MAX);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_src_x = prop;
+
+	prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC,
+			"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_range(dev,
+			DRM_MODE_PROP_DYNAMIC | DRM_MODE_PROP_SIGNED,
+			"crtc_x", I642U64(INT_MIN), I642U64(INT_MAX));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_x = prop;
+
+	prop = drm_property_create_range(dev,
+			DRM_MODE_PROP_DYNAMIC | DRM_MODE_PROP_SIGNED,
+			"crtc_y", I642U64(INT_MIN), I642U64(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, DRM_MODE_PROP_DYNAMIC,
+			"fb", DRM_MODE_OBJECT_FB);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_fb_id = prop;
+
+	prop = drm_property_create_object(dev, 0,
+			"crtc", DRM_MODE_OBJECT_CRTC);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_crtc_id = prop;
 
 	return 0;
 }
@@ -1003,7 +1098,7 @@ void drm_mode_config_init(struct drm_device *dev)
 	idr_init(&dev->mode_config.crtc_idr);
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_mode_create_standard_connector_properties(dev);
+	drm_mode_create_standard_properties(dev);
 	mutex_unlock(&dev->mode_config.mutex);
 
 	/* Just to be sure */
@@ -1750,116 +1845,71 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 			struct drm_file *file_priv)
 {
 	struct drm_mode_set_plane *plane_req = data;
+	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_mode_object *obj;
-	struct drm_plane *plane;
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
+	void *state;
 	int ret = 0;
-	unsigned int fb_width, fb_height;
-	int i;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	/*
-	 * First, find the plane, crtc, and fb objects.  If not available,
-	 * we don't bother to call the driver.
-	 */
-	obj = drm_mode_object_find(dev, plane_req->plane_id,
-				   DRM_MODE_OBJECT_PLANE);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown plane ID %d\n",
-			      plane_req->plane_id);
-		ret = -ENOENT;
-		goto out;
-	}
-	plane = obj_to_plane(obj);
-
-	/* No fb means shut it down */
-	if (!plane_req->fb_id) {
-		plane->funcs->disable_plane(plane);
-		plane->crtc = NULL;
-		plane->fb = NULL;
-		goto out;
-	}
-
 	obj = drm_mode_object_find(dev, plane_req->crtc_id,
 				   DRM_MODE_OBJECT_CRTC);
 	if (!obj) {
-		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
+		DRM_DEBUG_KMS("Unknown CRTC ID %d\n",
 			      plane_req->crtc_id);
 		ret = -ENOENT;
-		goto out;
+		goto out_unlock;
 	}
-	crtc = obj_to_crtc(obj);
 
-	obj = drm_mode_object_find(dev, plane_req->fb_id,
-				   DRM_MODE_OBJECT_FB);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
-			      plane_req->fb_id);
-		ret = -ENOENT;
-		goto out;
+	state = dev->driver->atomic_begin(dev, obj_to_crtc(obj));
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto out_unlock;
 	}
-	fb = obj_to_fb(obj);
 
-	/* Check whether this plane supports the fb pixel format. */
-	for (i = 0; i < plane->format_count; i++)
-		if (fb->pixel_format == plane->format_types[i])
-			break;
-	if (i == plane->format_count) {
-		DRM_DEBUG_KMS("Invalid pixel format 0x%08x\n", fb->pixel_format);
-		ret = -EINVAL;
+	obj = drm_mode_object_find(dev, plane_req->plane_id,
+				   DRM_MODE_OBJECT_PLANE);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown plane ID %d\n",
+			      plane_req->plane_id);
+		ret = -ENOENT;
 		goto out;
 	}
 
-	fb_width = fb->width << 16;
-	fb_height = fb->height << 16;
-
-	/* Make sure source coordinates are inside the fb. */
-	if (plane_req->src_w > fb_width ||
-	    plane_req->src_x > fb_width - plane_req->src_w ||
-	    plane_req->src_h > fb_height ||
-	    plane_req->src_y > fb_height - plane_req->src_h) {
-		DRM_DEBUG_KMS("Invalid source coordinates "
-			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
-			      plane_req->src_w >> 16,
-			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
-			      plane_req->src_h >> 16,
-			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
-			      plane_req->src_x >> 16,
-			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
-			      plane_req->src_y >> 16,
-			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
-		ret = -ENOSPC;
-		goto out;
-	}
+	ret =
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_crtc_id, plane_req->crtc_id) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_fb_id, plane_req->fb_id) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_crtc_x, I642U64(plane_req->crtc_x)) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_crtc_y, I642U64(plane_req->crtc_y)) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_crtc_w, plane_req->crtc_w) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_crtc_h, plane_req->crtc_h) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_src_w, plane_req->src_w) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_src_h, plane_req->src_h) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_src_x, plane_req->src_x) ||
+		drm_mode_set_obj_prop(obj, state,
+				config->prop_src_y, plane_req->src_y) ||
+		dev->driver->atomic_check(dev, state);
 
-	/* Give drivers some help against integer overflows */
-	if (plane_req->crtc_w > INT_MAX ||
-	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
-	    plane_req->crtc_h > INT_MAX ||
-	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
-		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      plane_req->crtc_w, plane_req->crtc_h,
-			      plane_req->crtc_x, plane_req->crtc_y);
-		ret = -ERANGE;
+	if (ret)
 		goto out;
-	}
 
-	ret = plane->funcs->update_plane(plane, crtc, fb,
-					 plane_req->crtc_x, plane_req->crtc_y,
-					 plane_req->crtc_w, plane_req->crtc_h,
-					 plane_req->src_x, plane_req->src_y,
-					 plane_req->src_w, plane_req->src_h);
-	if (!ret) {
-		plane->crtc = crtc;
-		plane->fb = fb;
-	}
+	ret = dev->driver->atomic_commit(dev, state, NULL);
 
 out:
+	dev->driver->atomic_end(dev, state);
+out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 
 	return ret;
@@ -3262,7 +3312,7 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 	return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv);
 }
 
-static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
+int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
 					   void *state, struct drm_property *property,
 					   uint64_t value)
 {
@@ -3283,8 +3333,9 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
 
 	return ret;
 }
+EXPORT_SYMBOL(drm_mode_connector_set_obj_prop);
 
-static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
+int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
 				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
@@ -3298,8 +3349,9 @@ static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
 
 	return ret;
 }
+EXPORT_SYMBOL(drm_mode_crtc_set_obj_prop);
 
-static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
+int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
@@ -3313,6 +3365,7 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 
 	return ret;
 }
+EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
 
 static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
 		void *state, struct drm_property *property, uint64_t value)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b5f71f8..c61b6a1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -634,13 +634,6 @@ struct drm_connector {
  * @set_property: called when a property is changed
  */
 struct drm_plane_funcs {
-	int (*update_plane)(struct drm_plane *plane,
-			    struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			    int crtc_x, int crtc_y,
-			    unsigned int crtc_w, unsigned int crtc_h,
-			    uint32_t src_x, uint32_t src_y,
-			    uint32_t src_w, uint32_t src_h);
-	int (*disable_plane)(struct drm_plane *plane);
 	void (*destroy)(struct drm_plane *plane);
 
 	int (*set_property)(struct drm_plane *plane, void *state,
@@ -810,8 +803,20 @@ struct drm_mode_config {
 	bool poll_enabled;
 	struct delayed_work output_poll_work;
 
-	/* pointers to standard properties */
+	/* just so blob properties can always be in a list: */
 	struct list_head property_blob_list;
+
+	/* pointers to standard properties */
+	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;
 	struct drm_property *edid_property;
 	struct drm_property *dpms_property;
 
@@ -947,6 +952,17 @@ extern int drm_object_property_set_value(struct drm_mode_object *obj,
 extern int drm_object_property_get_value(struct drm_mode_object *obj,
 					 struct drm_property *property,
 					 uint64_t *value);
+
+int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
+					   void *state, struct drm_property *property,
+					   uint64_t value);
+int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
+				      void *state, struct drm_property *property,
+				      uint64_t value);
+int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
+				      void *state, struct drm_property *property,
+				      uint64_t value);
+
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
-- 
1.7.9.5

[RFC 07/11] drm: add drm_plane_state

[Rob Clark]

From: Rob Clark <rob@ti.com>

Break the mutable state of a plane out into a separate structure.
This makes it easier to have some helpers for plane->set_property()
and for checking for invalid params.  The idea is that individual
drivers can wrap the state struct in their own struct which adds
driver specific parameters, for easy build-up of state across
multiple set_property() calls and for easy atomic commit or roll-
back.

The same should be done for CRTC, encoder, and connector, but this
patch only includes the first part (plane).
---
 drivers/gpu/drm/drm_crtc.c |  119 ++++++++++++++++++++++++++++++++++++++++----
 include/drm/drm_crtc.h     |   52 ++++++++++++++++---
 2 files changed, 154 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 81a63fe..5f310d3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -410,7 +410,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-		if (plane->fb == fb) {
+		if (plane->state->fb == fb) {
 			/* should turn off the crtc */
 			drm_mode_plane_set_obj_prop(plane, state, config->prop_crtc_id, 0);
 			drm_mode_plane_set_obj_prop(plane, state, config->prop_fb_id, 0);
@@ -688,7 +688,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		goto out;
 
 	plane->base.properties = &plane->properties;
-	plane->base.propvals = &plane->propvals;
+	plane->base.propvals = &plane->state->propvals;
 	plane->dev = dev;
 	plane->funcs = funcs;
 	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
@@ -749,6 +749,110 @@ void drm_plane_cleanup(struct drm_plane *plane)
 }
 EXPORT_SYMBOL(drm_plane_cleanup);
 
+int drm_plane_check_state(struct drm_plane *plane,
+		struct drm_plane_state *state)
+{
+	unsigned int fb_width, fb_height;
+	struct drm_framebuffer *fb = state->fb;
+	int i;
+
+	/* disabling the plane is allowed: */
+	if (!fb)
+		return 0;
+
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
+
+	/* Check whether this plane supports the fb pixel format. */
+	for (i = 0; i < plane->format_count; i++)
+		if (fb->pixel_format == plane->format_types[i])
+			break;
+	if (i == plane->format_count) {
+		DRM_DEBUG_KMS("Invalid pixel format 0x%08x\n", fb->pixel_format);
+		return -EINVAL;
+	}
+
+	/* 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;
+	}
+
+	/* Give drivers some help against integer overflows */
+	if (state->crtc_w > INT_MAX ||
+			state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
+			state->crtc_h > INT_MAX ||
+			state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
+		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
+			      state->crtc_w, state->crtc_h,
+			      state->crtc_x, state->crtc_y);
+		return -ERANGE;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_check_state);
+
+void drm_plane_commit_state(struct drm_plane *plane,
+		struct drm_plane_state *state)
+{
+	plane->state = state;
+	plane->base.propvals = &state->propvals;
+}
+EXPORT_SYMBOL(drm_plane_commit_state);
+
+int drm_plane_set_property(struct drm_plane *plane,
+		struct drm_plane_state *state,
+		struct drm_property *property, uint64_t value)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	drm_object_property_set_value(&plane->base,
+			&state->propvals, property, value);
+
+	if (property == config->prop_fb_id) {
+		struct drm_mode_object *obj = drm_property_get_obj(property, value);
+		state->fb = obj ? obj_to_fb(obj) : NULL;
+	} else if (property == config->prop_crtc_id) {
+		struct drm_mode_object *obj = drm_property_get_obj(property, value);
+		state->crtc = obj ? obj_to_crtc(obj) : NULL;
+	} else if (property == config->prop_crtc_x) {
+		state->crtc_x = U642I64(value);
+	} else if (property == config->prop_crtc_y) {
+		state->crtc_y = U642I64(value);
+	} else if (property == config->prop_crtc_w) {
+		state->crtc_w = value;
+	} else if (property == config->prop_crtc_h) {
+		state->crtc_h = value;
+	} else if (property == config->prop_src_x) {
+		state->src_x = value;
+	} else if (property == config->prop_src_y) {
+		state->src_y = value;
+	} else if (property == config->prop_src_w) {
+		state->src_w = value;
+	} else if (property == config->prop_src_h) {
+		state->src_h = value;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_set_property);
+
 /**
  * drm_mode_create - create a new display mode
  * @dev: DRM device
@@ -1794,13 +1898,13 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	}
 	plane = obj_to_plane(obj);
 
-	if (plane->crtc)
-		plane_resp->crtc_id = plane->crtc->base.id;
+	if (plane->state->crtc)
+		plane_resp->crtc_id = plane->state->crtc->base.id;
 	else
 		plane_resp->crtc_id = 0;
 
-	if (plane->fb)
-		plane_resp->fb_id = plane->fb->base.id;
+	if (plane->state->fb)
+		plane_resp->fb_id = plane->state->fb->base.id;
 	else
 		plane_resp->fb_id = 0;
 
@@ -3359,9 +3463,6 @@ int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 
 	if (plane->funcs->set_property)
 		ret = plane->funcs->set_property(plane, state, property, value);
-	if (!ret)
-		drm_object_property_set_value(&plane->base, &plane->propvals,
-				property, value);
 
 	return ret;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c61b6a1..3a622e4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -641,6 +641,37 @@ struct drm_plane_funcs {
 };
 
 /**
+ * drm_plane_state - mutable plane state
+ * @crtc: currently bound CRTC
+ * @fb: currently bound fb
+ * @crtc_x: left position of visible portion of plane on crtc
+ * @crtc_y: upper position of visible portion of plane on crtc
+ * @crtc_w: width of visible portion of plane on crtc
+ * @crtc_h: height of visible portion of plane on crtc
+ * @src_x: left position of visible portion of plane within
+ *   plane (in 16.16)
+ * @src_y: upper position of visible portion of plane within
+ *   plane (in 16.16)
+ * @src_w: width of visible portion of plane (in 16.16)
+ * @src_h: height of visible portion of plane (in 16.16)
+ * @propvals: property values
+ */
+struct drm_plane_state {
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *fb;
+
+	/* Signed dest location allows it to be partially off screen */
+	int32_t crtc_x, crtc_y;
+	uint32_t crtc_w, crtc_h;
+
+	/* Source values are 16.16 fixed point */
+	uint32_t src_x, src_y;
+	uint32_t src_h, src_w;
+
+	struct drm_object_property_values propvals;
+};
+
+/**
  * drm_plane - central DRM plane control structure
  * @dev: DRM device this plane belongs to
  * @head: for list management
@@ -648,11 +679,9 @@ struct drm_plane_funcs {
  * @possible_crtcs: pipes this plane can be bound to
  * @format_types: array of formats supported by this plane
  * @format_count: number of formats supported
- * @crtc: currently bound CRTC
- * @fb: currently bound fb
+ * @state: the mutable state
  * @gamma_size: size of gamma table
  * @gamma_store: gamma correction table
- * @enabled: enabled flag
  * @funcs: helper functions
  * @helper_private: storage for drver layer
  * @properties: property tracking for this plane
@@ -667,20 +696,20 @@ struct drm_plane {
 	uint32_t *format_types;
 	uint32_t format_count;
 
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
+	/*
+	 * State that can be updated from userspace, and atomically
+	 * commited or rolled back:
+	 */
+	struct drm_plane_state *state;
 
 	/* CRTC gamma size for reporting to userspace */
 	uint32_t gamma_size;
 	uint16_t *gamma_store;
 
-	bool enabled;
-
 	const struct drm_plane_funcs *funcs;
 	void *helper_private;
 
 	struct drm_object_properties properties;
-	struct drm_object_property_values propvals;
 };
 
 /**
@@ -888,6 +917,13 @@ extern int drm_plane_init(struct drm_device *dev,
 			  const uint32_t *formats, uint32_t format_count,
 			  bool priv);
 extern void drm_plane_cleanup(struct drm_plane *plane);
+extern int drm_plane_check_state(struct drm_plane *plane,
+		struct drm_plane_state *state);
+extern void drm_plane_commit_state(struct drm_plane *plane,
+		struct drm_plane_state *state);
+extern int drm_plane_set_property(struct drm_plane *plane,
+		struct drm_plane_state *state,
+		struct drm_property *property, uint64_t value);
 
 extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
-- 
1.7.9.5

[RFC 08/11] drm: convert page_flip to properties

[Rob Clark]

From: Rob Clark <rob@ti.com>

Use atomic properties mechanism for CRTC page_flip.  This by itself
doesn't accomplish anything, but it avoids having multiple code
paths to do the same thing when nuclear-pageflip and atomic-modeset
are introduced.
---
 drivers/gpu/drm/drm_crtc.c |   69 +++++++++++++++++++-------------------------
 include/drm/drm_crtc.h     |   13 ---------
 2 files changed, 30 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 5f310d3..f421fa6a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -386,9 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
-	struct drm_mode_set set;
 	void *state;
-	int ret;
 
 	state = dev->driver->atomic_begin(dev, NULL);
 	if (IS_ERR(state)) {
@@ -400,12 +398,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (crtc->fb == fb) {
 			/* should turn off the crtc */
-			memset(&set, 0, sizeof(struct drm_mode_set));
-			set.crtc = crtc;
-			set.fb = NULL;
-			ret = crtc->funcs->set_config(&set);
-			if (ret)
-				DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
+			drm_mode_crtc_set_obj_prop(crtc, state, config->prop_fb_id, 0);
 		}
 	}
 
@@ -448,6 +441,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove);
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   const struct drm_crtc_funcs *funcs)
 {
+	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
 
 	crtc->dev = dev;
@@ -466,6 +460,10 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	list_add_tail(&crtc->head, &dev->mode_config.crtc_list);
 	dev->mode_config.num_crtc++;
 
+	drm_object_attach_property(&crtc->base, config->prop_fb_id, 0);
+	drm_object_attach_property(&crtc->base, config->prop_crtc_x, 0);
+	drm_object_attach_property(&crtc->base, config->prop_crtc_y, 0);
+
  out:
 	mutex_unlock(&dev->mode_config.mutex);
 
@@ -3773,12 +3771,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_crtc_page_flip *page_flip = data;
+	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_mode_object *obj;
 	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
 	struct drm_pending_vblank_event *e = NULL;
 	unsigned long flags;
-	int hdisplay, vdisplay;
+	void *state;
 	int ret = -EINVAL;
 
 	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
@@ -3786,11 +3784,22 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -EINVAL;
 
 	mutex_lock(&dev->mode_config.mutex);
-	obj = drm_mode_object_find(dev, page_flip->crtc_id, DRM_MODE_OBJECT_CRTC);
-	if (!obj)
-		goto out;
+
+	obj = drm_mode_object_find(dev, page_flip->crtc_id,
+			DRM_MODE_OBJECT_CRTC);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown CRTC ID %d\n", page_flip->crtc_id);
+		ret = -ENOENT;
+		goto out_unlock;
+	}
 	crtc = obj_to_crtc(obj);
 
+	state = dev->driver->atomic_begin(dev, crtc);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto out_unlock;
+	}
+
 	if (crtc->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
@@ -3800,31 +3809,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	if (crtc->funcs->page_flip == NULL)
-		goto out;
-
-	obj = drm_mode_object_find(dev, page_flip->fb_id, DRM_MODE_OBJECT_FB);
-	if (!obj)
-		goto out;
-	fb = obj_to_fb(obj);
-
-	hdisplay = crtc->mode.hdisplay;
-	vdisplay = crtc->mode.vdisplay;
-
-	if (crtc->invert_dimensions)
-		swap(hdisplay, vdisplay);
-
-	if (hdisplay > fb->width ||
-	    vdisplay > fb->height ||
-	    crtc->x > fb->width - hdisplay ||
-	    crtc->y > fb->height - vdisplay) {
-		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
-			      fb->width, fb->height, hdisplay, vdisplay, crtc->x, crtc->y,
-			      crtc->invert_dimensions ? " (inverted)" : "");
-		ret = -ENOSPC;
-		goto out;
-	}
-
 	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 		ret = -ENOMEM;
 		spin_lock_irqsave(&dev->event_lock, flags);
@@ -3852,7 +3836,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			(void (*) (struct drm_pending_event *)) kfree;
 	}
 
-	ret = crtc->funcs->page_flip(crtc, fb, e);
+	ret = drm_mode_set_obj_prop(obj, state,
+			config->prop_fb_id, page_flip->fb_id);
+	if (ret)
+		goto out;
+
+	ret = dev->driver->atomic_commit(dev, state, e);
 	if (ret) {
 		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 			spin_lock_irqsave(&dev->event_lock, flags);
@@ -3863,6 +3852,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 out:
+	dev->driver->atomic_end(dev, state);
+out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3a622e4..73a8a14 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -358,19 +358,6 @@ struct drm_crtc_funcs {
 
 	int (*set_config)(struct drm_mode_set *set);
 
-	/*
-	 * Flip to the given framebuffer.  This implements the page
-	 * flip ioctl described in drm_mode.h, specifically, the
-	 * implementation must return immediately and block all
-	 * rendering to the current fb until the flip has completed.
-	 * If userspace set the event flag in the ioctl, the event
-	 * argument will point to an event to send back when the flip
-	 * completes, otherwise it will be NULL.
-	 */
-	int (*page_flip)(struct drm_crtc *crtc,
-			 struct drm_framebuffer *fb,
-			 struct drm_pending_vblank_event *event);
-
 	int (*set_property)(struct drm_crtc *crtc, void *state,
 			    struct drm_property *property, uint64_t val);
 };
-- 
1.7.9.5

[RFC 09/11] drm: add drm_crtc_state

[Rob Clark]

From: Rob Clark <rob@ti.com>

Start breaking out the mutable state of the CRTC into it's own
structure.  Plus add _check_state() and _set_property() helpers.
This only moves the state that is related to scanout fb, which
is needed for nuclear-pageflip.  The rest of the mutable state
should be moved from drm_crtc to drm_crtc_state as part of the
atomic-modeset implementation.
---
 drivers/gpu/drm/drm_crtc.c        |   88 ++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_crtc_helper.c |   51 ++++++++++-----------
 drivers/gpu/drm/drm_fb_helper.c   |   11 ++---
 include/drm/drm_crtc.h            |   49 ++++++++++++++++-----
 4 files changed, 143 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f421fa6a..6d22c8c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -396,7 +396,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 
 	/* remove from any CRTC */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		if (crtc->fb == fb) {
+		if (crtc->state->fb == fb) {
 			/* should turn off the crtc */
 			drm_mode_crtc_set_obj_prop(crtc, state, config->prop_fb_id, 0);
 		}
@@ -446,7 +446,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	crtc->dev = dev;
 	crtc->funcs = funcs;
-	crtc->invert_dimensions = false;
+	crtc->state->invert_dimensions = false;
 
 	mutex_lock(&dev->mode_config.mutex);
 
@@ -455,7 +455,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		goto out;
 
 	crtc->base.properties = &crtc->properties;
-	crtc->base.propvals = &crtc->propvals;
+	crtc->base.propvals = &crtc->state->propvals;
 
 	list_add_tail(&crtc->head, &dev->mode_config.crtc_list);
 	dev->mode_config.num_crtc++;
@@ -496,6 +496,67 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 }
 EXPORT_SYMBOL(drm_crtc_cleanup);
 
+int drm_crtc_check_state(struct drm_crtc *crtc,
+		struct drm_crtc_state *state)
+{
+	struct drm_framebuffer *fb = state->fb;
+	int hdisplay, vdisplay;
+
+	/* disabling the crtc is allowed: */
+	if (!fb)
+		return 0;
+
+	hdisplay = crtc->mode.hdisplay;
+	vdisplay = crtc->mode.vdisplay;
+
+	if (state->invert_dimensions)
+		swap(hdisplay, vdisplay);
+
+	if (hdisplay > fb->width ||
+	    vdisplay > fb->height ||
+	    state->x > fb->width - hdisplay ||
+	    state->y > fb->height - vdisplay) {
+		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
+			      fb->width, fb->height, hdisplay, vdisplay,
+			      state->x, state->y,
+			      state->invert_dimensions ? " (inverted)" : "");
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_check_state);
+
+void drm_crtc_commit_state(struct drm_crtc *crtc,
+		struct drm_crtc_state *state)
+{
+	crtc->state = state;
+	crtc->base.propvals = &state->propvals;
+}
+EXPORT_SYMBOL(drm_crtc_commit_state);
+
+int drm_crtc_set_property(struct drm_crtc *crtc,
+		struct drm_crtc_state *state,
+		struct drm_property *property, uint64_t value)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (property == config->prop_fb_id) {
+		struct drm_mode_object *obj = drm_property_get_obj(property, value);
+		state->fb = obj ? obj_to_fb(obj) : NULL;
+	} else if (property == config->prop_crtc_x) {
+		state->x = *(int *)&value;
+	} else if (property == config->prop_crtc_y) {
+		state->y = *(int32_t *)&value;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_set_property);
+
 /**
  * drm_mode_probed_add - add a mode to a connector's probed mode list
  * @connector: connector the new mode
@@ -1614,11 +1675,11 @@ int drm_mode_getcrtc(struct drm_device *dev,
 	}
 	crtc = obj_to_crtc(obj);
 
-	crtc_resp->x = crtc->x;
-	crtc_resp->y = crtc->y;
+	crtc_resp->x = crtc->state->x;
+	crtc_resp->y = crtc->state->y;
 	crtc_resp->gamma_size = crtc->gamma_size;
-	if (crtc->fb)
-		crtc_resp->fb_id = crtc->fb->base.id;
+	if (crtc->state->fb)
+		crtc_resp->fb_id = crtc->state->fb->base.id;
 	else
 		crtc_resp->fb_id = 0;
 
@@ -2072,12 +2133,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		/* If we have a mode we need a framebuffer. */
 		/* If we pass -1, set the mode with the currently bound fb */
 		if (crtc_req->fb_id == -1) {
-			if (!crtc->fb) {
+			if (!crtc->state->fb) {
 				DRM_DEBUG_KMS("CRTC doesn't have current FB\n");
 				ret = -EINVAL;
 				goto out;
 			}
-			fb = crtc->fb;
+			fb = crtc->state->fb;
 		} else {
 			obj = drm_mode_object_find(dev, crtc_req->fb_id,
 						   DRM_MODE_OBJECT_FB);
@@ -2107,7 +2168,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		hdisplay = mode->hdisplay;
 		vdisplay = mode->vdisplay;
 
-		if (crtc->invert_dimensions)
+		if (crtc->state->invert_dimensions)
 			swap(hdisplay, vdisplay);
 
 		if (hdisplay > fb->width ||
@@ -2117,7 +2178,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
 				      fb->width, fb->height,
 				      hdisplay, vdisplay, crtc_req->x, crtc_req->y,
-				      crtc->invert_dimensions ? " (inverted)" : "");
+				      crtc->state->invert_dimensions ? " (inverted)" : "");
 			ret = -ENOSPC;
 			goto out;
 		}
@@ -3445,9 +3506,6 @@ int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
 
 	if (crtc->funcs->set_property)
 		ret = crtc->funcs->set_property(crtc, state, property, value);
-	if (!ret)
-		drm_object_property_set_value(&crtc->base, &crtc->propvals,
-				property, value);
 
 	return ret;
 }
@@ -3800,7 +3858,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out_unlock;
 	}
 
-	if (crtc->fb == NULL) {
+	if (crtc->state->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
 		 * yet discovered.
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 3252e70..65ed229 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -266,7 +266,7 @@ void drm_helper_disable_unused_functions(struct drm_device *dev)
 				(*crtc_funcs->disable)(crtc);
 			else
 				(*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF);
-			crtc->fb = NULL;
+			crtc->state->fb = NULL;
 		}
 	}
 }
@@ -363,15 +363,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 
 	saved_hwmode = crtc->hwmode;
 	saved_mode = crtc->mode;
-	saved_x = crtc->x;
-	saved_y = crtc->y;
+	saved_x = crtc->state->x;
+	saved_y = crtc->state->y;
 
 	/* Update crtc values up front so the driver can rely on them for mode
 	 * setting.
 	 */
 	crtc->mode = *mode;
-	crtc->x = x;
-	crtc->y = y;
+	crtc->state->x = x;
+	crtc->state->y = y;
 
 	/* Pass our mode to the connectors and the CRTC to give them a chance to
 	 * adjust it according to limitations or connector properties, and also
@@ -456,8 +456,8 @@ done:
 	if (!ret) {
 		crtc->hwmode = saved_hwmode;
 		crtc->mode = saved_mode;
-		crtc->x = saved_x;
-		crtc->y = saved_y;
+		crtc->state->x = saved_x;
+		crtc->state->y = saved_y;
 	}
 
 	return ret;
@@ -591,29 +591,29 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 
 	save_set.crtc = set->crtc;
 	save_set.mode = &set->crtc->mode;
-	save_set.x = set->crtc->x;
-	save_set.y = set->crtc->y;
-	save_set.fb = set->crtc->fb;
+	save_set.x = set->crtc->state->x;
+	save_set.y = set->crtc->state->y;
+	save_set.fb = set->crtc->state->fb;
 
 	/* We should be able to check here if the fb has the same properties
 	 * and then just flip_or_move it */
-	if (set->crtc->fb != set->fb) {
+	if (set->crtc->state->fb != set->fb) {
 		/* If we have no fb then treat it as a full mode set */
-		if (set->crtc->fb == NULL) {
+		if (set->crtc->state->fb == NULL) {
 			DRM_DEBUG_KMS("crtc has no fb, full mode set\n");
 			mode_changed = true;
 		} else if (set->fb == NULL) {
 			mode_changed = true;
-		} else if (set->fb->depth != set->crtc->fb->depth) {
+		} else if (set->fb->depth != set->crtc->state->fb->depth) {
 			mode_changed = true;
 		} else if (set->fb->bits_per_pixel !=
-			   set->crtc->fb->bits_per_pixel) {
+			   set->crtc->state->fb->bits_per_pixel) {
 			mode_changed = true;
 		} else
 			fb_changed = true;
 	}
 
-	if (set->x != set->crtc->x || set->y != set->crtc->y)
+	if (set->x != set->crtc->state->x || set->y != set->crtc->state->y)
 		fb_changed = true;
 
 	if (set->mode && !drm_mode_equal(set->mode, &set->crtc->mode)) {
@@ -704,14 +704,14 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			DRM_DEBUG_KMS("attempting to set mode from"
 					" userspace\n");
 			drm_mode_debug_printmodeline(set->mode);
-			old_fb = set->crtc->fb;
-			set->crtc->fb = set->fb;
+			old_fb = set->crtc->state->fb;
+			set->crtc->state->fb = set->fb;
 			if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
 						      set->x, set->y,
 						      old_fb)) {
 				DRM_ERROR("failed to set mode on [CRTC:%d]\n",
 					  set->crtc->base.id);
-				set->crtc->fb = old_fb;
+				set->crtc->state->fb = old_fb;
 				ret = -EINVAL;
 				goto fail;
 			}
@@ -724,16 +724,16 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		}
 		drm_helper_disable_unused_functions(dev);
 	} else if (fb_changed) {
-		set->crtc->x = set->x;
-		set->crtc->y = set->y;
+		set->crtc->state->x = set->x;
+		set->crtc->state->y = set->y;
 
-		old_fb = set->crtc->fb;
-		if (set->crtc->fb != set->fb)
-			set->crtc->fb = set->fb;
+		old_fb = set->crtc->state->fb;
+		if (set->crtc->state->fb != set->fb)
+			set->crtc->state->fb = set->fb;
 		ret = crtc_funcs->mode_set_base(set->crtc,
 						set->x, set->y, old_fb);
 		if (ret != 0) {
-			set->crtc->fb = old_fb;
+			set->crtc->state->fb = old_fb;
 			goto fail;
 		}
 	}
@@ -888,7 +888,8 @@ int drm_helper_resume_force_mode(struct drm_device *dev)
 			continue;
 
 		ret = drm_crtc_helper_set_mode(crtc, &crtc->mode,
-					       crtc->x, crtc->y, crtc->fb);
+					       crtc->state->x, crtc->state->y,
+					       crtc->state->fb);
 
 		if (ret == false)
 			DRM_ERROR("failed to set mode on crtc %p\n", crtc);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f546d1e..d70b787 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -185,7 +185,7 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 
 	list_for_each_entry(c, &dev->mode_config.crtc_list, head) {
 		if (crtc->base.id == c->base.id)
-			return c->fb;
+			return c->state->fb;
 	}
 
 	return NULL;
@@ -214,8 +214,9 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
 		}
 
 		drm_fb_helper_restore_lut_atomic(mode_set->crtc);
-		funcs->mode_set_base_atomic(mode_set->crtc, fb, crtc->x,
-					    crtc->y, LEAVE_ATOMIC_MODE_SET);
+		funcs->mode_set_base_atomic(mode_set->crtc, fb,
+				crtc->state->x, crtc->state->y,
+				LEAVE_ATOMIC_MODE_SET);
 	}
 
 	return 0;
@@ -1361,9 +1362,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
 
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		if (crtc->fb)
+		if (crtc->state->fb)
 			crtcs_bound++;
-		if (crtc->fb == fb_helper->fb)
+		if (crtc->state->fb == fb_helper->fb)
 			bound++;
 	}
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 73a8a14..92e6bf8 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -363,18 +363,43 @@ struct drm_crtc_funcs {
 };
 
 /**
+ * drm_crtc_state - mutable crtc state
+ * @fb: the framebuffer that the CRTC is currently bound to
+ * @invert_dimensions: for purposes of error checking crtc vs fb sizes,
+ *    invert the width/height of the crtc.  This is used if the driver
+ *    is performing 90 or 270 degree rotated scanout
+ * @x: x position on screen
+ * @y: y position on screen
+ * @propvals: property values
+ */
+struct drm_crtc_state {
+	/*
+	 * NOTE: more should move from 'struct drm_crtc' into here as
+	 * part of the atomic-modeset support:
+	 *   + enabled
+	 *   + mode
+	 *   + hwmode
+	 *   + framedur_ns
+	 *   + linedur_ns
+	 *   + pixeldur_ns
+	 *
+	 * For now, I'm just moving what is needed for atomic-pageflip
+	 */
+	struct drm_framebuffer *fb;
+	bool invert_dimensions;
+	int x, y;
+	struct drm_object_property_values propvals;
+};
+
+/**
  * drm_crtc - central CRTC control structure
  * @dev: parent DRM device
  * @head: list management
  * @base: base KMS object for ID tracking etc.
+ * @state: the mutable state
  * @enabled: is this CRTC enabled?
  * @mode: current mode timings
  * @hwmode: mode timings as programmed to hw regs
- * @invert_dimensions: for purposes of error checking crtc vs fb sizes,
- *    invert the width/height of the crtc.  This is used if the driver
- *    is performing 90 or 270 degree rotated scanout
- * @x: x position on screen
- * @y: y position on screen
  * @funcs: CRTC control functions
  * @gamma_size: size of gamma ramp
  * @gamma_store: gamma ramp values
@@ -393,8 +418,7 @@ struct drm_crtc {
 
 	struct drm_mode_object base;
 
-	/* framebuffer the connector is currently bound to */
-	struct drm_framebuffer *fb;
+	struct drm_crtc_state *state;
 
 	bool enabled;
 
@@ -406,9 +430,6 @@ struct drm_crtc {
 	 */
 	struct drm_display_mode hwmode;
 
-	bool invert_dimensions;
-
-	int x, y;
 	const struct drm_crtc_funcs *funcs;
 
 	/* CRTC gamma size for reporting to userspace */
@@ -422,7 +443,6 @@ struct drm_crtc {
 	void *helper_private;
 
 	struct drm_object_properties properties;
-	struct drm_object_property_values propvals;
 };
 
 
@@ -882,6 +902,13 @@ extern int drm_crtc_init(struct drm_device *dev,
 			 struct drm_crtc *crtc,
 			 const struct drm_crtc_funcs *funcs);
 extern void drm_crtc_cleanup(struct drm_crtc *crtc);
+extern int drm_crtc_check_state(struct drm_crtc *crtc,
+		struct drm_crtc_state *state);
+extern void drm_crtc_commit_state(struct drm_crtc *crtc,
+		struct drm_crtc_state *state);
+extern int drm_crtc_set_property(struct drm_crtc *crtc,
+		struct drm_crtc_state *state,
+		struct drm_property *property, uint64_t value);
 
 extern int drm_connector_init(struct drm_device *dev,
 			      struct drm_connector *connector,
-- 
1.7.9.5

Re: [RFC 01/11] drm: add atomic fxns

[Laurent Pinchart]

Hi Rob,

On Wednesday 12 September 2012 22:49:47 Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> The 'atomic' mechanism allows for multiple properties to be updated,
> checked, and commited atomically.  This will be the basis of atomic-
> modeset and nuclear-pageflip.
> 
> The basic flow is:
> 
>    state = dev->atomic_begin();
>    for (... one or more ...)
>       obj->set_property(obj, state, prop, value);
>    if (dev->atomic_check(state))
>       dev->atomic_commit(state, event);
>    dev->atomic_end(state);
> 
> The split of check and commit steps is to allow for ioctls with a
> test-only flag (which would skip the commit step).
> 
> The atomic functions are mandatory, as they will end up getting
> called from enough places that it is easier not to have to bother
> with if-null checks everywhere.
> 
> TODO:
>   + add some stub atomic functions that can be used by drivers
>     until they are updated to support atomic operations natively
>   + roll-back previous property values if check/commit fails, so
>     userspace doesn't see incorrect values.

I have to confess that I have trouble understanding how the various pieces fit 
together. The call stacks are quite deep, and pointers to objects are passed 
around in a way to makes it difficult to understand what objects are accessed 
without going through the whole stack. Reviewing your proposal would be easier 
with proper documentation, including a functional overview of the atomic 
properties architecture. Renaming some methods would also be worth it: 
drm_connector_property_set_value() and drm_mode_connector_set_obj_prop() are 
confusing, and I would need pretty long to get used to them without having to 
look at the kerneldoc all the time.

Speaking of kerneldoc, there's none :-) Library functions need to be 
documented, especially with such a complex code flow.

Before your patches the code currently called the drivers set_property methods 
to apply the state to the hardware, and then, if those calls were successful, 
called drm_object_property_set_value() to store the property values 
permanently.

Your patches modify that behaviour and changes the meaning of set_property. 
The method now just updates the new state object without touching the 
hardware. However, you still call drm_object_property_set_value() which stores 
the new property value in propvals (struct drm_object_property_values).

Rolling back the values is a possible solution. As the mode_config.mutex is 
taken around all accesses to propvals, no other thread will be able to access 
incorrect values before they're rolled back. However, I wonder whether we 
couldn't come up with a more simple solution.

First of all, do we really need dynamic states ? All accesses to properties 
are serialized using a single common mutex. We could just have two static 
states (current and new) instead of allocating the state at runtime.

Then, do we really need to parse the properties and compute the state values 
so early ? We're storing the properties values in two separate locations, in 
the propvals structure as "raw" unprocessed values, and also in the state 
structures as processed values. Is that really necessary ? Wouldn't it be 
simpler to first validate all the new properties and then compute the state 
values at commit time ?

I can't really pinpoint the exact reason, but I feel like this is all getting 
a bit too messy for my taste. That might be partly caused by me being familiar 
with the V4L2 control framework, which solves some of the same issues (such as 
modifying several controls atomically) in a (in my opinion) cleaner way. The 
problem at hand in DRM might very well be more complex though.

Could you please include more documentation in the next version of this RFC ? 
I'd like text documentation that explains the design principles and how the 
pieces fit together (including code flows) (in plain text or DocBook, it 
doesn't matter much at this point), and kerneldoc for the exported functions.

> ---
>  drivers/gpu/drm/drm_crtc.c |  126 +++++++++++++++++++++++++----------------
>  include/drm/drmP.h         |   52 ++++++++++++++++++
>  include/drm/drm_crtc.h     |    8 +--
>  3 files changed, 135 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7552675..3a087cf 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3227,12 +3227,11 @@ int drm_mode_connector_property_set_ioctl(struct
> drm_device *dev, return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop,
> file_priv); }
> 
> -static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> -					   struct drm_property *property,
> +static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
> +					   void *state, struct drm_property *property,
>  					   uint64_t value)
>  {
>  	int ret = -EINVAL;
> -	struct drm_connector *connector = obj_to_connector(obj);
> 
>  	/* Do DPMS ourselves */
>  	if (property == connector->dev->mode_config.dpms_property) {
> @@ -3240,7 +3239,7 @@ static int drm_mode_connector_set_obj_prop(struct
> drm_mode_object *obj, (*connector->funcs->dpms)(connector, (int)value);
>  		ret = 0;
>  	} else if (connector->funcs->set_property)
> -		ret = connector->funcs->set_property(connector, property, value);
> +		ret = connector->funcs->set_property(connector, state, property, 
value);
> 
>  	/* store the property value if successful */
>  	if (!ret)
> @@ -3248,36 +3247,87 @@ static int drm_mode_connector_set_obj_prop(struct
> drm_mode_object *obj, return ret;
>  }
> 
> -static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
> -				      struct drm_property *property,
> +static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
> +				      void *state, struct drm_property *property,
>  				      uint64_t value)
>  {
>  	int ret = -EINVAL;
> -	struct drm_crtc *crtc = obj_to_crtc(obj);
> 
>  	if (crtc->funcs->set_property)
> -		ret = crtc->funcs->set_property(crtc, property, value);
> +		ret = crtc->funcs->set_property(crtc, state, property, value);
>  	if (!ret)
> -		drm_object_property_set_value(obj, property, value);
> +		drm_object_property_set_value(&crtc->base, property, value);
> 
>  	return ret;
>  }
> 
> -static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
> -				      struct drm_property *property,
> +static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> +				      void *state, struct drm_property *property,
>  				      uint64_t value)
>  {
>  	int ret = -EINVAL;
> -	struct drm_plane *plane = obj_to_plane(obj);
> 
>  	if (plane->funcs->set_property)
> -		ret = plane->funcs->set_property(plane, property, value);
> +		ret = plane->funcs->set_property(plane, state, property, value);
>  	if (!ret)
> -		drm_object_property_set_value(obj, property, value);
> +		drm_object_property_set_value(&plane->base, property, value);
> 
>  	return ret;
>  }
> 
> +static int drm_mode_set_obj_prop(struct drm_device *dev,
> +		struct drm_mode_object *obj, void *state,
> +		struct drm_property *property, uint64_t value)
> +{
> +	if (drm_property_change_is_valid(property, value)) {
> +		switch (obj->type) {
> +		case DRM_MODE_OBJECT_CONNECTOR:
> +			return drm_mode_connector_set_obj_prop(obj_to_connector(obj),
> +					state, property, value);
> +		case DRM_MODE_OBJECT_CRTC:
> +			return drm_mode_crtc_set_obj_prop(obj_to_crtc(obj),
> +					state, property, value);
> +		case DRM_MODE_OBJECT_PLANE:
> +			return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
> +					state, property, value);
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/* call with mode_config mutex held */
> +static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
> +		uint32_t obj_id, uint32_t obj_type,
> +		uint32_t prop_id, uint64_t value)
> +{
> +	struct drm_mode_object *arg_obj;
> +	struct drm_mode_object *prop_obj;
> +	struct drm_property *property;
> +	int i;
> +
> +	arg_obj = drm_mode_object_find(dev, obj_id, obj_type);
> +	if (!arg_obj)
> +		return -EINVAL;
> +	if (!arg_obj->properties)
> +		return -EINVAL;
> +
> +	for (i = 0; i < arg_obj->properties->count; i++)
> +		if (arg_obj->properties->ids[i] == prop_id)
> +			break;
> +
> +	if (i == arg_obj->properties->count)
> +		return -EINVAL;
> +
> +	prop_obj = drm_mode_object_find(dev, prop_id,
> +					DRM_MODE_OBJECT_PROPERTY);
> +	if (!prop_obj)
> +		return -EINVAL;
> +	property = obj_to_property(prop_obj);
> +
> +	return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);
> +}
> +
>  int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>  				      struct drm_file *file_priv)
>  {
> @@ -3338,53 +3388,35 @@ int drm_mode_obj_set_property_ioctl(struct
> drm_device *dev, void *data, struct drm_file *file_priv)
>  {
>  	struct drm_mode_obj_set_property *arg = data;
> -	struct drm_mode_object *arg_obj;
> -	struct drm_mode_object *prop_obj;
> -	struct drm_property *property;
> +	void *state = NULL;
>  	int ret = -EINVAL;
> -	int i;
> 
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> 
>  	mutex_lock(&dev->mode_config.mutex);
> 
> -	arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
> -	if (!arg_obj)
> -		goto out;
> -	if (!arg_obj->properties)
> -		goto out;
> -
> -	for (i = 0; i < arg_obj->properties->count; i++)
> -		if (arg_obj->properties->ids[i] == arg->prop_id)
> -			break;
> +	state = dev->driver->atomic_begin(dev, NULL);
> +	if (IS_ERR(state)) {
> +		ret = PTR_ERR(state);
> +		goto out_unlock;
> +	}
> 
> -	if (i == arg_obj->properties->count)
> +	ret = drm_mode_set_obj_prop_id(dev, state,
> +			arg->obj_id, arg->obj_type,
> +			arg->prop_id, arg->value);
> +	if (ret)
>  		goto out;
> 
> -	prop_obj = drm_mode_object_find(dev, arg->prop_id,
> -					DRM_MODE_OBJECT_PROPERTY);
> -	if (!prop_obj)
> -		goto out;
> -	property = obj_to_property(prop_obj);
> -
> -	if (!drm_property_change_is_valid(property, arg->value))
> +	ret = dev->driver->atomic_check(dev, state);
> +	if (ret)
>  		goto out;
> 
> -	switch (arg_obj->type) {
> -	case DRM_MODE_OBJECT_CONNECTOR:
> -		ret = drm_mode_connector_set_obj_prop(arg_obj, property,
> -						      arg->value);
> -		break;
> -	case DRM_MODE_OBJECT_CRTC:
> -		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
> -		break;
> -	case DRM_MODE_OBJECT_PLANE:
> -		ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);
> -		break;
> -	}
> +	ret = dev->driver->atomic_commit(dev, state, NULL);
> 
>  out:
> +	dev->driver->atomic_end(dev, state);
> +out_unlock:
>  	mutex_unlock(&dev->mode_config.mutex);
>  	return ret;
>  }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d6b67bb..f719c4d 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -933,6 +933,58 @@ struct drm_driver {
>  			    struct drm_device *dev,
>  			    uint32_t handle);
> 
> +	/*
> +	 * Atomic functions:
> +	 */
> +
> +	/**
> +	 * Begin a sequence of atomic property sets.  Returns a driver
> +	 * private state object that is passed back into the various
> +	 * object's set_property() fxns, and into the remainder of the
> +	 * atomic funcs.  The state object should accumulate the changes
> +	 * from one o more set_property()'s.  At the end, the state can
> +	 * be checked, and optionally committed.
> +	 *
> +	 * \param dev dev DRM device handle.
> +	 * \param crtc for asynchronous page-flip operations, the crtc
> +	 *   that is being updated.  (The driver should return -EBUSY if
> +	 *   a page-flip is still pending.)  Otherwise, NULL.
> +	 * \returns a driver private state object, which is passed
> +	 *   back in to the various other atomic fxns
> +	 */
> +	void *(*atomic_begin)(struct drm_device *dev, struct drm_crtc *crtc);
> +
> +	/**
> +	 * Check the state object to see if the requested state is
> +	 * physically possible.
> +	 *
> +	 * \param dev dev DRM device handle.
> +	 * \param state the driver private state object
> +	 */
> +	int (*atomic_check)(struct drm_device *dev, void *state);
> +
> +	/**
> +	 * Commit the state.  This will only be called if atomic_check()
> +	 * succeeds.
> +	 *
> +	 * \param dev dev DRM device handle.
> +	 * \param state the driver private state object
> +	 * \param event for asynchronous page-flip operations, the
> +	 *   userspace has requested an event to be sent when the
> +	 *   page-flip completes, or NULL.  Will always be NULL for
> +	 *   non-page-flip operations
> +	 */
> +	int (*atomic_commit)(struct drm_device *dev, void *state,
> +			struct drm_pending_vblank_event *event);
> +
> +	/**
> +	 * Release resources associated with the state object.
> +	 *
> +	 * \param dev dev DRM device handle.
> +	 * \param state the driver private state object
> +	 */
> +	void (*atomic_end)(struct drm_device *dev, void *state);
> +
>  	/* Driver private ops for this object */
>  	const struct vm_operations_struct *gem_vm_ops;
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1422b36..a609190 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -357,7 +357,7 @@ struct drm_crtc_funcs {
>  			 struct drm_framebuffer *fb,
>  			 struct drm_pending_vblank_event *event);
> 
> -	int (*set_property)(struct drm_crtc *crtc,
> +	int (*set_property)(struct drm_crtc *crtc, void *state,
>  			    struct drm_property *property, uint64_t val);
>  };
> 
> @@ -455,8 +455,8 @@ struct drm_connector_funcs {
>  	enum drm_connector_status (*detect)(struct drm_connector *connector,
>  					    bool force);
>  	int (*fill_modes)(struct drm_connector *connector, uint32_t max_width,
> uint32_t max_height); -	int (*set_property)(struct drm_connector
> *connector, struct drm_property *property, -			     uint64_t val);
> +	int (*set_property)(struct drm_connector *connector, void *state,
> +			struct drm_property *property, uint64_t val);
>  	void (*destroy)(struct drm_connector *connector);
>  	void (*force)(struct drm_connector *connector);
>  };
> @@ -627,7 +627,7 @@ struct drm_plane_funcs {
>  	int (*disable_plane)(struct drm_plane *plane);
>  	void (*destroy)(struct drm_plane *plane);
> 
> -	int (*set_property)(struct drm_plane *plane,
> +	int (*set_property)(struct drm_plane *plane, void *state,
>  			    struct drm_property *property, uint64_t val);
>  };
-- 
Regards,

Laurent Pinchart

Re: [RFC 01/11] drm: add atomic fxns

[Rob Clark]

On Thu, Oct 11, 2012 at 2:26 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rob,
>
> On Wednesday 12 September 2012 22:49:47 Rob Clark wrote:
>> From: Rob Clark <rob@ti.com>
>>
>> The 'atomic' mechanism allows for multiple properties to be updated,
>> checked, and commited atomically.  This will be the basis of atomic-
>> modeset and nuclear-pageflip.
>>
>> The basic flow is:
>>
>>    state = dev->atomic_begin();
>>    for (... one or more ...)
>>       obj->set_property(obj, state, prop, value);
>>    if (dev->atomic_check(state))
>>       dev->atomic_commit(state, event);
>>    dev->atomic_end(state);
>>
>> The split of check and commit steps is to allow for ioctls with a
>> test-only flag (which would skip the commit step).
>>
>> The atomic functions are mandatory, as they will end up getting
>> called from enough places that it is easier not to have to bother
>> with if-null checks everywhere.
>>
>> TODO:
>>   + add some stub atomic functions that can be used by drivers
>>     until they are updated to support atomic operations natively
>>   + roll-back previous property values if check/commit fails, so
>>     userspace doesn't see incorrect values.
>
> I have to confess that I have trouble understanding how the various pieces fit
> together. The call stacks are quite deep, and pointers to objects are passed
> around in a way to makes it difficult to understand what objects are accessed
> without going through the whole stack. Reviewing your proposal would be easier
> with proper documentation, including a functional overview of the atomic
> properties architecture. Renaming some methods would also be worth it:
> drm_connector_property_set_value() and drm_mode_connector_set_obj_prop() are
> confusing, and I would need pretty long to get used to them without having to
> look at the kerneldoc all the time.

Yeah, the drm_connector_property_* stuff is a bit of confusing
legacy.. let's just get rid of it.  I sent a patchset to do this, and
I'll rebase next round of the atomic pagefip series on top.

> Speaking of kerneldoc, there's none :-) Library functions need to be
> documented, especially with such a complex code flow.

Let me make an attempt to explain the code flow via email.. admittedly
I tend to understand things better from code so maybe writing docs
isn't my best strength.  But if this makes sense then I'll fold it
into the docs.  If not let me know and I'll keep trying until it does
make sense.

First, I should note, that I don't expect it will be possible to
update all drivers in one go, so there will be a transition period
with some drivers working the old way, and some supporting the
"atomic" way.  For old drivers, the .set_property() fxns would
continue to function the way they do today (touching hw immediately,
most likely).  They will have the additional state parameter added,
but will just ignore this.  I'm going to add back in the .page_flip(),
.set_plane(), .disable_plane().  For drivers that implement these fxn
ptrs, instead of the atomic fxns, they won't see common crtc/plane
attributes set via property.  For old drivers, userspace will not see
properties for common crtc/plane attributes (fb_id, crtc_x/y/w/h,
etc).  That isn't the case for the last version of the series I sent,
but I think it is the only sane way to avoid having to port all
drivers to "atomic" at once.

---

For drivers that have been atomic-ified, they no longer need to
implement .page_flip(), .set_plane(), .disable_plane().  Instead they
should implement the atomic fxns, and their various .set_property()
functions would not actually touch the hw, but instead update state
objects.  State objects should hold all the state which could
potentially be applied to the hw.  The .set_property() functions
should not actually update the hw, because it might be the users
intention to only test a new configuration.

In drm_crtc / drm_plane, the userspace visible attributes are split
out into separate drm_crtc_state / drm_plane_state structs.  (Note
that this is only partially done for crtc, only the page-flip related
attributes are split out.  I wanted to do this first, and then add the
modeset related attributes in a later patch to split things up into
smaller pieces.)  The intention is that drivers can wrap the state
structs, and add whatever other information they need.  For example:

struct omap_plane_state {
       struct drm_plane_state base;
       uint8_t rotation;
       uint8_t zorder;
       uint8_t enabled;
};

It doesn't strictly need to be just property values.  If driver needs
to calculate some clock settings, fifo thresholds, or other internal
state, based the external state set from userspace, it could stuff
that stuff into it's state structs as well if it wanted.  But at a
minimum the state objects should encapsulate the userspace visible
state.

For atomic-ified drivers, all updates, whether it be userspace
setproperty ioctl updating a single property, or legacy setplane or
pageflip ioctls, or new atomic ioctl(s), all go through the same path
from the driver's perspective:

    state = dev->atomic_begin();
    for (... one or more ...)
       obj->set_property(obj, state, prop, value);
    if (dev->atomic_check(state))
       dev->atomic_commit(state, event);
    dev->atomic_end(state);

The global driver state token/object returned from .atomic_begin() is
opaque from drm core perspective.  It somehow encapsulates the state
of all crtc/plane/etc.  Inside the driver's different .set_property()
fxns, this global state object gets mapped to per crtc/plane state
objects.  Core drm helpers for dealing with common attributes (fb_id,
crtc_x/y/w/h, etc) are provided.  (ie. drm_plane_set_property(),
drm_plane_check_state(), etc.)   This is to avoid forcing each driver
to duplicate code for setting common properties and sanity checking.

After all the property update have been passed to the driver via
.set_property() calls, .atomic_check() is called.  This should check
all the modified crtc/plane's (using drm_{plane,crtc}_check_state()
for common check), and do any needed global sanity checks for the hw
(ie. can this combination of planes meet hw bandwidth limitations,
etc).

For ioctls with a 'test' flag, the .atomic_commit() step might be
skipped if userspace is only testing the new configuration.  In either
case, .atomic_commit() is only called if .atomic_check() succeeds, so
at this point it is already known that the new configuration is
"good", so .atomic_commit() should really only fail for catastrophic
things (unable to allocate memory, hardware is on fire, etc).

The .atomic_end() is called at the end if the driver needs to do some cleanup.


> Before your patches the code currently called the drivers set_property methods
> to apply the state to the hardware, and then, if those calls were successful,
> called drm_object_property_set_value() to store the property values
> permanently.
>
> Your patches modify that behaviour and changes the meaning of set_property.
> The method now just updates the new state object without touching the
> hardware. However, you still call drm_object_property_set_value() which stores
> the new property value in propvals (struct drm_object_property_values).

The property values array is moved into the state object.  So the
userspace visible property values only change as a result of
.atomic_commit().  This way, each driver does not have to care about
rollback or knowing when to update the userspace visible property
values.  It is all automatic, based on the current state object.

One of my initial concerns about using properties for everything is
that I didn't want to make each driver have to deal w/ the "paperwork"
of keeping userspace visible property values in sync with current hw
state.  Splitting the prop vals out into the state objects makes that
problem go away.

> Rolling back the values is a possible solution. As the mode_config.mutex is
> taken around all accesses to propvals, no other thread will be able to access
> incorrect values before they're rolled back. However, I wonder whether we
> couldn't come up with a more simple solution.

yup, putting prop vals into the state struct is that more simple solution :-P

> First of all, do we really need dynamic states ? All accesses to properties
> are serialized using a single common mutex. We could just have two static
> states (current and new) instead of allocating the state at runtime.

The driver is free to implement things this way, since it is the one
allocating state structs.  I happened to pick dynamic allocation in
omapdrm, but nothing in the API forces this.

> Then, do we really need to parse the properties and compute the state values
> so early ? We're storing the properties values in two separate locations, in
> the propvals structure as "raw" unprocessed values, and also in the state
> structures as processed values. Is that really necessary ? Wouldn't it be
> simpler to first validate all the new properties and then compute the state
> values at commit time ?

the 'raw' propvals array is really just intended for getproperty
ioclt, so that we aren't needing to have .get_property() fxn ptrs in
the drivers.

I think you *somehow* need to parse the property values in order to
validate the state.  I guess the other alternative is ditch the
.set_property() and instead update all the propval's and then have a
.one_or_more_of_your_properties_has_changed() fxns to call in the
driver.  This seems more awkward for me, because then I have to go
figure out in the driver *which* properties have changed.  So I don't
really see that as an improvement.

> I can't really pinpoint the exact reason, but I feel like this is all getting
> a bit too messy for my taste. That might be partly caused by me being familiar
> with the V4L2 control framework, which solves some of the same issues (such as
> modifying several controls atomically) in a (in my opinion) cleaner way. The
> problem at hand in DRM might very well be more complex though.
>
> Could you please include more documentation in the next version of this RFC ?
> I'd like text documentation that explains the design principles and how the
> pieces fit together (including code flows) (in plain text or DocBook, it
> doesn't matter much at this point), and kerneldoc for the exported functions.
>

Yeah, I knew you would ask for that.  If my attempt at explaining this
makes things clearer to you (and hopefully not more confusing), then
I'll somehow re-work it into some text for the docbook

BR,
-R

>> ---
>>  drivers/gpu/drm/drm_crtc.c |  126 +++++++++++++++++++++++++----------------
>>  include/drm/drmP.h         |   52 ++++++++++++++++++
>>  include/drm/drm_crtc.h     |    8 +--
>>  3 files changed, 135 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 7552675..3a087cf 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -3227,12 +3227,11 @@ int drm_mode_connector_property_set_ioctl(struct
>> drm_device *dev, return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop,
>> file_priv); }
>>
>> -static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>> -                                        struct drm_property *property,
>> +static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
>> +                                        void *state, struct drm_property *property,
>>                                          uint64_t value)
>>  {
>>       int ret = -EINVAL;
>> -     struct drm_connector *connector = obj_to_connector(obj);
>>
>>       /* Do DPMS ourselves */
>>       if (property == connector->dev->mode_config.dpms_property) {
>> @@ -3240,7 +3239,7 @@ static int drm_mode_connector_set_obj_prop(struct
>> drm_mode_object *obj, (*connector->funcs->dpms)(connector, (int)value);
>>               ret = 0;
>>       } else if (connector->funcs->set_property)
>> -             ret = connector->funcs->set_property(connector, property, value);
>> +             ret = connector->funcs->set_property(connector, state, property,
> value);
>>
>>       /* store the property value if successful */
>>       if (!ret)
>> @@ -3248,36 +3247,87 @@ static int drm_mode_connector_set_obj_prop(struct
>> drm_mode_object *obj, return ret;
>>  }
>>
>> -static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
>> -                                   struct drm_property *property,
>> +static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
>> +                                   void *state, struct drm_property *property,
>>                                     uint64_t value)
>>  {
>>       int ret = -EINVAL;
>> -     struct drm_crtc *crtc = obj_to_crtc(obj);
>>
>>       if (crtc->funcs->set_property)
>> -             ret = crtc->funcs->set_property(crtc, property, value);
>> +             ret = crtc->funcs->set_property(crtc, state, property, value);
>>       if (!ret)
>> -             drm_object_property_set_value(obj, property, value);
>> +             drm_object_property_set_value(&crtc->base, property, value);
>>
>>       return ret;
>>  }
>>
>> -static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
>> -                                   struct drm_property *property,
>> +static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
>> +                                   void *state, struct drm_property *property,
>>                                     uint64_t value)
>>  {
>>       int ret = -EINVAL;
>> -     struct drm_plane *plane = obj_to_plane(obj);
>>
>>       if (plane->funcs->set_property)
>> -             ret = plane->funcs->set_property(plane, property, value);
>> +             ret = plane->funcs->set_property(plane, state, property, value);
>>       if (!ret)
>> -             drm_object_property_set_value(obj, property, value);
>> +             drm_object_property_set_value(&plane->base, property, value);
>>
>>       return ret;
>>  }
>>
>> +static int drm_mode_set_obj_prop(struct drm_device *dev,
>> +             struct drm_mode_object *obj, void *state,
>> +             struct drm_property *property, uint64_t value)
>> +{
>> +     if (drm_property_change_is_valid(property, value)) {
>> +             switch (obj->type) {
>> +             case DRM_MODE_OBJECT_CONNECTOR:
>> +                     return drm_mode_connector_set_obj_prop(obj_to_connector(obj),
>> +                                     state, property, value);
>> +             case DRM_MODE_OBJECT_CRTC:
>> +                     return drm_mode_crtc_set_obj_prop(obj_to_crtc(obj),
>> +                                     state, property, value);
>> +             case DRM_MODE_OBJECT_PLANE:
>> +                     return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
>> +                                     state, property, value);
>> +             }
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +/* call with mode_config mutex held */
>> +static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
>> +             uint32_t obj_id, uint32_t obj_type,
>> +             uint32_t prop_id, uint64_t value)
>> +{
>> +     struct drm_mode_object *arg_obj;
>> +     struct drm_mode_object *prop_obj;
>> +     struct drm_property *property;
>> +     int i;
>> +
>> +     arg_obj = drm_mode_object_find(dev, obj_id, obj_type);
>> +     if (!arg_obj)
>> +             return -EINVAL;
>> +     if (!arg_obj->properties)
>> +             return -EINVAL;
>> +
>> +     for (i = 0; i < arg_obj->properties->count; i++)
>> +             if (arg_obj->properties->ids[i] == prop_id)
>> +                     break;
>> +
>> +     if (i == arg_obj->properties->count)
>> +             return -EINVAL;
>> +
>> +     prop_obj = drm_mode_object_find(dev, prop_id,
>> +                                     DRM_MODE_OBJECT_PROPERTY);
>> +     if (!prop_obj)
>> +             return -EINVAL;
>> +     property = obj_to_property(prop_obj);
>> +
>> +     return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);
>> +}
>> +
>>  int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>>                                     struct drm_file *file_priv)
>>  {
>> @@ -3338,53 +3388,35 @@ int drm_mode_obj_set_property_ioctl(struct
>> drm_device *dev, void *data, struct drm_file *file_priv)
>>  {
>>       struct drm_mode_obj_set_property *arg = data;
>> -     struct drm_mode_object *arg_obj;
>> -     struct drm_mode_object *prop_obj;
>> -     struct drm_property *property;
>> +     void *state = NULL;
>>       int ret = -EINVAL;
>> -     int i;
>>
>>       if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>               return -EINVAL;
>>
>>       mutex_lock(&dev->mode_config.mutex);
>>
>> -     arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
>> -     if (!arg_obj)
>> -             goto out;
>> -     if (!arg_obj->properties)
>> -             goto out;
>> -
>> -     for (i = 0; i < arg_obj->properties->count; i++)
>> -             if (arg_obj->properties->ids[i] == arg->prop_id)
>> -                     break;
>> +     state = dev->driver->atomic_begin(dev, NULL);
>> +     if (IS_ERR(state)) {
>> +             ret = PTR_ERR(state);
>> +             goto out_unlock;
>> +     }
>>
>> -     if (i == arg_obj->properties->count)
>> +     ret = drm_mode_set_obj_prop_id(dev, state,
>> +                     arg->obj_id, arg->obj_type,
>> +                     arg->prop_id, arg->value);
>> +     if (ret)
>>               goto out;
>>
>> -     prop_obj = drm_mode_object_find(dev, arg->prop_id,
>> -                                     DRM_MODE_OBJECT_PROPERTY);
>> -     if (!prop_obj)
>> -             goto out;
>> -     property = obj_to_property(prop_obj);
>> -
>> -     if (!drm_property_change_is_valid(property, arg->value))
>> +     ret = dev->driver->atomic_check(dev, state);
>> +     if (ret)
>>               goto out;
>>
>> -     switch (arg_obj->type) {
>> -     case DRM_MODE_OBJECT_CONNECTOR:
>> -             ret = drm_mode_connector_set_obj_prop(arg_obj, property,
>> -                                                   arg->value);
>> -             break;
>> -     case DRM_MODE_OBJECT_CRTC:
>> -             ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
>> -             break;
>> -     case DRM_MODE_OBJECT_PLANE:
>> -             ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);
>> -             break;
>> -     }
>> +     ret = dev->driver->atomic_commit(dev, state, NULL);
>>
>>  out:
>> +     dev->driver->atomic_end(dev, state);
>> +out_unlock:
>>       mutex_unlock(&dev->mode_config.mutex);
>>       return ret;
>>  }
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index d6b67bb..f719c4d 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -933,6 +933,58 @@ struct drm_driver {
>>                           struct drm_device *dev,
>>                           uint32_t handle);
>>
>> +     /*
>> +      * Atomic functions:
>> +      */
>> +
>> +     /**
>> +      * Begin a sequence of atomic property sets.  Returns a driver
>> +      * private state object that is passed back into the various
>> +      * object's set_property() fxns, and into the remainder of the
>> +      * atomic funcs.  The state object should accumulate the changes
>> +      * from one o more set_property()'s.  At the end, the state can
>> +      * be checked, and optionally committed.
>> +      *
>> +      * \param dev dev DRM device handle.
>> +      * \param crtc for asynchronous page-flip operations, the crtc
>> +      *   that is being updated.  (The driver should return -EBUSY if
>> +      *   a page-flip is still pending.)  Otherwise, NULL.
>> +      * \returns a driver private state object, which is passed
>> +      *   back in to the various other atomic fxns
>> +      */
>> +     void *(*atomic_begin)(struct drm_device *dev, struct drm_crtc *crtc);
>> +
>> +     /**
>> +      * Check the state object to see if the requested state is
>> +      * physically possible.
>> +      *
>> +      * \param dev dev DRM device handle.
>> +      * \param state the driver private state object
>> +      */
>> +     int (*atomic_check)(struct drm_device *dev, void *state);
>> +
>> +     /**
>> +      * Commit the state.  This will only be called if atomic_check()
>> +      * succeeds.
>> +      *
>> +      * \param dev dev DRM device handle.
>> +      * \param state the driver private state object
>> +      * \param event for asynchronous page-flip operations, the
>> +      *   userspace has requested an event to be sent when the
>> +      *   page-flip completes, or NULL.  Will always be NULL for
>> +      *   non-page-flip operations
>> +      */
>> +     int (*atomic_commit)(struct drm_device *dev, void *state,
>> +                     struct drm_pending_vblank_event *event);
>> +
>> +     /**
>> +      * Release resources associated with the state object.
>> +      *
>> +      * \param dev dev DRM device handle.
>> +      * \param state the driver private state object
>> +      */
>> +     void (*atomic_end)(struct drm_device *dev, void *state);
>> +
>>       /* Driver private ops for this object */
>>       const struct vm_operations_struct *gem_vm_ops;
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 1422b36..a609190 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -357,7 +357,7 @@ struct drm_crtc_funcs {
>>                        struct drm_framebuffer *fb,
>>                        struct drm_pending_vblank_event *event);
>>
>> -     int (*set_property)(struct drm_crtc *crtc,
>> +     int (*set_property)(struct drm_crtc *crtc, void *state,
>>                           struct drm_property *property, uint64_t val);
>>  };
>>
>> @@ -455,8 +455,8 @@ struct drm_connector_funcs {
>>       enum drm_connector_status (*detect)(struct drm_connector *connector,
>>                                           bool force);
>>       int (*fill_modes)(struct drm_connector *connector, uint32_t max_width,
>> uint32_t max_height); -       int (*set_property)(struct drm_connector
>> *connector, struct drm_property *property, -                       uint64_t val);
>> +     int (*set_property)(struct drm_connector *connector, void *state,
>> +                     struct drm_property *property, uint64_t val);
>>       void (*destroy)(struct drm_connector *connector);
>>       void (*force)(struct drm_connector *connector);
>>  };
>> @@ -627,7 +627,7 @@ struct drm_plane_funcs {
>>       int (*disable_plane)(struct drm_plane *plane);
>>       void (*destroy)(struct drm_plane *plane);
>>
>> -     int (*set_property)(struct drm_plane *plane,
>> +     int (*set_property)(struct drm_plane *plane, void *state,
>>                           struct drm_property *property, uint64_t val);
>>  };
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 01/11] drm: add atomic fxns

[Rob Clark]

From: Rob Clark <rob@ti.com>

The 'atomic' mechanism allows for multiple properties to be updated,
checked, and commited atomically.  This will be the basis of atomic-
modeset and nuclear-pageflip.

The basic flow is:

   state = dev->atomic_begin();
   for (... one or more ...)
      obj->set_property(obj, state, prop, value);
   if (dev->atomic_check(state))
      dev->atomic_commit(state, event);
   dev->atomic_end(state);

The split of check and commit steps is to allow for ioctls with a
test-only flag (which would skip the commit step).

The atomic functions are mandatory, as they will end up getting
called from enough places that it is easier not to have to bother
with if-null checks everywhere.
---
 drivers/gpu/drm/drm_crtc.c           |  126 +++++++++++++++++++++-------------
 drivers/staging/omapdrm/omap_crtc.c  |    4 +-
 drivers/staging/omapdrm/omap_drv.h   |    2 +-
 drivers/staging/omapdrm/omap_plane.c |    2 +-
 include/drm/drmP.h                   |   52 ++++++++++++++
 include/drm/drm_crtc.h               |    8 +--
 6 files changed, 139 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cee96f4..a236acf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3208,12 +3208,11 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 	return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv);
 }
 
-static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
-					   struct drm_property *property,
+static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
+					   void *state, struct drm_property *property,
 					   uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_connector *connector = obj_to_connector(obj);
 
 	/* Do DPMS ourselves */
 	if (property == connector->dev->mode_config.dpms_property) {
@@ -3221,7 +3220,7 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 			(*connector->funcs->dpms)(connector, (int)value);
 		ret = 0;
 	} else if (connector->funcs->set_property)
-		ret = connector->funcs->set_property(connector, property, value);
+		ret = connector->funcs->set_property(connector, state, property, value);
 
 	/* store the property value if successful */
 	if (!ret)
@@ -3229,36 +3228,87 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
 	return ret;
 }
 
-static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
-				      struct drm_property *property,
+static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
+				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_crtc *crtc = obj_to_crtc(obj);
 
 	if (crtc->funcs->set_property)
-		ret = crtc->funcs->set_property(crtc, property, value);
+		ret = crtc->funcs->set_property(crtc, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(obj, property, value);
+		drm_object_property_set_value(&crtc->base, property, value);
 
 	return ret;
 }
 
-static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
-				      struct drm_property *property,
+static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
+				      void *state, struct drm_property *property,
 				      uint64_t value)
 {
 	int ret = -EINVAL;
-	struct drm_plane *plane = obj_to_plane(obj);
 
 	if (plane->funcs->set_property)
-		ret = plane->funcs->set_property(plane, property, value);
+		ret = plane->funcs->set_property(plane, state, property, value);
 	if (!ret)
-		drm_object_property_set_value(obj, property, value);
+		drm_object_property_set_value(&plane->base, property, value);
 
 	return ret;
 }
 
+static int drm_mode_set_obj_prop(struct drm_device *dev,
+		struct drm_mode_object *obj, void *state,
+		struct drm_property *property, uint64_t value)
+{
+	if (drm_property_change_is_valid(property, value)) {
+		switch (obj->type) {
+		case DRM_MODE_OBJECT_CONNECTOR:
+			return drm_mode_connector_set_obj_prop(obj_to_connector(obj),
+					state, property, value);
+		case DRM_MODE_OBJECT_CRTC:
+			return drm_mode_crtc_set_obj_prop(obj_to_crtc(obj),
+					state, property, value);
+		case DRM_MODE_OBJECT_PLANE:
+			return drm_mode_plane_set_obj_prop(obj_to_plane(obj),
+					state, property, value);
+		}
+	}
+
+	return -EINVAL;
+}
+
+/* call with mode_config mutex held */
+static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
+		uint32_t obj_id, uint32_t obj_type,
+		uint32_t prop_id, uint64_t value)
+{
+	struct drm_mode_object *arg_obj;
+	struct drm_mode_object *prop_obj;
+	struct drm_property *property;
+	int i;
+
+	arg_obj = drm_mode_object_find(dev, obj_id, obj_type);
+	if (!arg_obj)
+		return -EINVAL;
+	if (!arg_obj->properties)
+		return -EINVAL;
+
+	for (i = 0; i < arg_obj->properties->count; i++)
+		if (arg_obj->properties->ids[i] == prop_id)
+			break;
+
+	if (i == arg_obj->properties->count)
+		return -EINVAL;
+
+	prop_obj = drm_mode_object_find(dev, prop_id,
+					DRM_MODE_OBJECT_PROPERTY);
+	if (!prop_obj)
+		return -EINVAL;
+	property = obj_to_property(prop_obj);
+
+	return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);
+}
+
 int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv)
 {
@@ -3319,53 +3369,35 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
 {
 	struct drm_mode_obj_set_property *arg = data;
-	struct drm_mode_object *arg_obj;
-	struct drm_mode_object *prop_obj;
-	struct drm_property *property;
+	void *state = NULL;
 	int ret = -EINVAL;
-	int i;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
 	mutex_lock(&dev->mode_config.mutex);
 
-	arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
-	if (!arg_obj)
-		goto out;
-	if (!arg_obj->properties)
-		goto out;
-
-	for (i = 0; i < arg_obj->properties->count; i++)
-		if (arg_obj->properties->ids[i] == arg->prop_id)
-			break;
+	state = dev->driver->atomic_begin(dev, NULL);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto out_unlock;
+	}
 
-	if (i == arg_obj->properties->count)
+	ret = drm_mode_set_obj_prop_id(dev, state,
+			arg->obj_id, arg->obj_type,
+			arg->prop_id, arg->value);
+	if (ret)
 		goto out;
 
-	prop_obj = drm_mode_object_find(dev, arg->prop_id,
-					DRM_MODE_OBJECT_PROPERTY);
-	if (!prop_obj)
-		goto out;
-	property = obj_to_property(prop_obj);
-
-	if (!drm_property_change_is_valid(property, arg->value))
+	ret = dev->driver->atomic_check(dev, state);
+	if (ret)
 		goto out;
 
-	switch (arg_obj->type) {
-	case DRM_MODE_OBJECT_CONNECTOR:
-		ret = drm_mode_connector_set_obj_prop(arg_obj, property,
-						      arg->value);
-		break;
-	case DRM_MODE_OBJECT_CRTC:
-		ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);
-		break;
-	case DRM_MODE_OBJECT_PLANE:
-		ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);
-		break;
-	}
+	ret = dev->driver->atomic_commit(dev, state, NULL);
 
 out:
+	dev->driver->atomic_end(dev, state);
+out_unlock:
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
index e430559..eddbb2f 100644
--- a/drivers/staging/omapdrm/omap_crtc.c
+++ b/drivers/staging/omapdrm/omap_crtc.c
@@ -327,7 +327,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 	return 0;
 }
 
-static int omap_crtc_set_property(struct drm_crtc *crtc,
+static int omap_crtc_set_property(struct drm_crtc *crtc, void *state,
 		struct drm_property *property, uint64_t val)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
@@ -338,7 +338,7 @@ static int omap_crtc_set_property(struct drm_crtc *crtc,
 				!!(val & ((1LL << DRM_ROTATE_90) | (1LL << DRM_ROTATE_270)));
 	}
 
-	return omap_plane_set_property(omap_crtc->plane, property, val);
+	return omap_plane_set_property(omap_crtc->plane, state, property, val);
 }
 
 static const struct drm_crtc_funcs omap_crtc_funcs = {
diff --git a/drivers/staging/omapdrm/omap_drv.h b/drivers/staging/omapdrm/omap_drv.h
index fec3c75..c20ed7e 100644
--- a/drivers/staging/omapdrm/omap_drv.h
+++ b/drivers/staging/omapdrm/omap_drv.h
@@ -171,7 +171,7 @@ int omap_plane_mode_set(struct drm_plane *plane,
 		void (*fxn)(void *), void *arg);
 void omap_plane_install_properties(struct drm_plane *plane,
 		struct drm_mode_object *obj);
-int omap_plane_set_property(struct drm_plane *plane,
+int omap_plane_set_property(struct drm_plane *plane, void *state,
 		struct drm_property *property, uint64_t val);
 
 struct drm_encoder *omap_encoder_init(struct drm_device *dev);
diff --git a/drivers/staging/omapdrm/omap_plane.c b/drivers/staging/omapdrm/omap_plane.c
index c9098fc..854fdce 100644
--- a/drivers/staging/omapdrm/omap_plane.c
+++ b/drivers/staging/omapdrm/omap_plane.c
@@ -327,7 +327,7 @@ void omap_plane_install_properties(struct drm_plane *plane,
 	drm_object_attach_property(obj, prop, 0);
 }
 
-int omap_plane_set_property(struct drm_plane *plane,
+int omap_plane_set_property(struct drm_plane *plane, void *state,
 		struct drm_property *property, uint64_t val)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ee8f927..9c41d36 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -940,6 +940,58 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/*
+	 * Atomic functions:
+	 */
+
+	/**
+	 * Begin a sequence of atomic property sets.  Returns a driver
+	 * private state object that is passed back into the various
+	 * object's set_property() fxns, and into the remainder of the
+	 * atomic funcs.  The state object should accumulate the changes
+	 * from one o more set_property()'s.  At the end, the state can
+	 * be checked, and optionally committed.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param crtc for asynchronous page-flip operations, the crtc
+	 *   that is being updated.  (The driver should return -EBUSY if
+	 *   a page-flip is still pending.)  Otherwise, NULL.
+	 * \returns a driver private state object, which is passed
+	 *   back in to the various other atomic fxns
+	 */
+	void *(*atomic_begin)(struct drm_device *dev, struct drm_crtc *crtc);
+
+	/**
+	 * Check the state object to see if the requested state is
+	 * physically possible.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 */
+	int (*atomic_check)(struct drm_device *dev, void *state);
+
+	/**
+	 * Commit the state.  This will only be called if atomic_check()
+	 * succeeds.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 * \param event for asynchronous page-flip operations, the
+	 *   userspace has requested an event to be sent when the
+	 *   page-flip completes, or NULL.  Will always be NULL for
+	 *   non-page-flip operations
+	 */
+	int (*atomic_commit)(struct drm_device *dev, void *state,
+			struct drm_pending_vblank_event *event);
+
+	/**
+	 * Release resources associated with the state object.
+	 *
+	 * \param dev dev DRM device handle.
+	 * \param state the driver private state object
+	 */
+	void (*atomic_end)(struct drm_device *dev, void *state);
+
 	/* Driver private ops for this object */
 	const struct vm_operations_struct *gem_vm_ops;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a45cdc2..4ae5295 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -356,7 +356,7 @@ struct drm_crtc_funcs {
 			 struct drm_framebuffer *fb,
 			 struct drm_pending_vblank_event *event);
 
-	int (*set_property)(struct drm_crtc *crtc,
+	int (*set_property)(struct drm_crtc *crtc, void *state,
 			    struct drm_property *property, uint64_t val);
 };
 
@@ -454,8 +454,8 @@ struct drm_connector_funcs {
 	enum drm_connector_status (*detect)(struct drm_connector *connector,
 					    bool force);
 	int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height);
-	int (*set_property)(struct drm_connector *connector, struct drm_property *property,
-			     uint64_t val);
+	int (*set_property)(struct drm_connector *connector, void *state,
+			struct drm_property *property, uint64_t val);
 	void (*destroy)(struct drm_connector *connector);
 	void (*force)(struct drm_connector *connector);
 };
@@ -627,7 +627,7 @@ struct drm_plane_funcs {
 	int (*disable_plane)(struct drm_plane *plane);
 	void (*destroy)(struct drm_plane *plane);
 
-	int (*set_property)(struct drm_plane *plane,
+	int (*set_property)(struct drm_plane *plane, void *state,
 			    struct drm_property *property, uint64_t val);
 };
 
-- 
1.7.9.5