[RFC][PATCH 0/10] Atomic modesetting v2

[RFC][PATCH 0/10] Atomic modesetting v2

[ville.syrjala]

Second version of the atomic mode setting work. Still very much
work in progress.

I decided that I can't afford to fight the drm_crtc_helper
architecture due to the sheer amount of driver code depending on it.
So I changed the code to do things in way that more closely matches
drm_crtc_helper.

Next I'll be moving the buffer pinning to happen before any hw state
is clobbered. This will avoid having to do actual rollbacks when
pinning fails. And a similar treatment needs to be done to the PLL
calculations (those should be done before buffer pinning).

I didn't re-post all the i915 specific bits I have in my tree since those
didn't really change. I pushed the whole work to the drm_atomic_2 branch
at https://gitorious.org/vsyrjala/linux.

[RFC][PATCH 01/10] drm: Export drm_property_create_blob() and drm_property_destroy_blob()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    8 +++++---
 include/drm/drm_crtc.h     |    4 ++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6dafb99..ce0f446 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3044,8 +3044,8 @@ done:
 	return ret;
 }
 
-static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
-							  void *data)
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int length,
+						   void *data)
 {
 	struct drm_property_blob *blob;
 	int ret;
@@ -3070,14 +3070,16 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
 	return blob;
 }
+EXPORT_SYMBOL(drm_property_create_blob);
 
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
 			       struct drm_property_blob *blob)
 {
 	drm_mode_object_put(dev, &blob->base);
 	list_del(&blob->head);
 	kfree(blob);
 }
+EXPORT_SYMBOL(drm_property_destroy_blob);
 
 int drm_mode_getblob_ioctl(struct drm_device *dev,
 			   void *data, struct drm_file *file_priv)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 14bc68d..0a2ebb4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -952,6 +952,10 @@ struct drm_property *drm_property_create_range(struct drm_device *dev, int flags
 					 const char *name,
 					 uint64_t min, uint64_t max);
 extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
+extern struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
+							  int length, void *data);
+extern void drm_property_destroy_blob(struct drm_device *dev,
+				      struct drm_property_blob *blob);
 extern int drm_property_add_enum(struct drm_property *property, int index,
 				 uint64_t value, const char *name);
 extern int drm_mode_create_dvi_i_properties(struct drm_device *dev);
-- 
1.7.3.4

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

[RFC][PATCH 02/10] drm: Allow signed values for range properties

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Treat a range property as signed when the unsigned minimum value is
larger than the unsigned maximum value.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ce0f446..cc18ae6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3143,14 +3143,25 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
 
-static bool drm_property_change_is_valid(struct drm_property *property,
+static bool range_property_is_signed(const struct drm_property *property)
+{
+	return property->values[0] > property->values[1];
+}
+
+static bool drm_property_change_is_valid(const struct drm_property *property,
 					 uint64_t value)
 {
 	if (property->flags & DRM_MODE_PROP_IMMUTABLE)
 		return false;
 	if (property->flags & DRM_MODE_PROP_RANGE) {
-		if (value < property->values[0] || value > property->values[1])
-			return false;
+		if (range_property_is_signed(property)) {
+			if ((int64_t)value < (int64_t)property->values[0] ||
+			    (int64_t)value > (int64_t)property->values[1])
+				return false;
+		} else {
+			if (value < property->values[0] || value > property->values[1])
+				return false;
+		}
 		return true;
 	} else if (property->flags & DRM_MODE_PROP_BITMASK) {
 		int i;
-- 
1.7.3.4

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

[RFC][PATCH 03/10] drm: Allow drm_mode_object_find() to look up an object of any type

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To avoid having to pass object types from userspace for atomic mode
setting ioctl, allow drm_mode_object_find() to look up an object of any
type. This will only work as long as the all object types share the ID
space.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    3 ++-
 include/drm/drm_crtc.h     |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cc18ae6..9945e4b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -268,7 +268,8 @@ struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
 
 	mutex_lock(&dev->mode_config.idr_mutex);
 	obj = idr_find(&dev->mode_config.crtc_idr, id);
-	if (!obj || (obj->type != type) || (obj->id != id))
+	if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
+	    (obj->id != id))
 		obj = NULL;
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 0a2ebb4..99bd489 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -47,6 +47,7 @@ struct drm_object_properties;
 #define DRM_MODE_OBJECT_FB 0xfbfbfbfb
 #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb
 #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee
+#define DRM_MODE_OBJECT_ANY 0
 
 struct drm_mode_object {
 	uint32_t id;
-- 
1.7.3.4

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

[RFC][PATCH 04/10] drm: Export drm_encoder_crtc_ok

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

---
 drivers/gpu/drm/drm_crtc_helper.c |    5 +++--
 include/drm/drm_crtc_helper.h     |    3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 3252e70..c4f7e7c 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -279,8 +279,8 @@ EXPORT_SYMBOL(drm_helper_disable_unused_functions);
  *
  * Return false if @encoder can't be driven by @crtc, true otherwise.
  */
-static bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
-				struct drm_crtc *crtc)
+bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
+			 struct drm_crtc *crtc)
 {
 	struct drm_device *dev;
 	struct drm_crtc *tmp;
@@ -300,6 +300,7 @@ static bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
 		return true;
 	return false;
 }
+EXPORT_SYMBOL(drm_encoder_crtc_ok);
 
 /*
  * Check the CRTC we're going to map each output to vs. its current
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 7988e55..79bc76b 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -166,4 +166,7 @@ extern void drm_helper_hpd_irq_event(struct drm_device *dev);
 extern void drm_kms_helper_poll_disable(struct drm_device *dev);
 extern void drm_kms_helper_poll_enable(struct drm_device *dev);
 
+extern bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
+				struct drm_crtc *crtc);
+
 #endif
-- 
1.7.3.4

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

[RFC][PATCH 05/10] drm: Export drm_crtc_prepare_encoders()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

---
 drivers/gpu/drm/drm_crtc_helper.c |    3 ++-
 include/drm/drm_crtc_helper.h     |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index c4f7e7c..ebbfcc6 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -307,7 +307,7 @@ EXPORT_SYMBOL(drm_encoder_crtc_ok);
  * CRTC.  If they don't match, we have to disable the output and the CRTC
  * since the driver will have to re-route things.
  */
-static void
+void
 drm_crtc_prepare_encoders(struct drm_device *dev)
 {
 	struct drm_encoder_helper_funcs *encoder_funcs;
@@ -324,6 +324,7 @@ drm_crtc_prepare_encoders(struct drm_device *dev)
 			drm_encoder_disable(encoder);
 	}
 }
+EXPORT_SYMBOL(drm_crtc_prepare_encoders);
 
 /**
  * drm_crtc_set_mode - set a mode
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 79bc76b..c88202d 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -168,5 +168,6 @@ extern void drm_kms_helper_poll_enable(struct drm_device *dev);
 
 extern bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
 				struct drm_crtc *crtc);
+extern void drm_crtc_prepare_encoders(struct drm_device *dev);
 
 #endif
-- 
1.7.3.4

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

[RFC][PATCH 06/10] drm: Refactor object property check code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Refactor the code to check whether an object has a specific property
to a new function.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9945e4b..958a4b0 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3301,6 +3301,19 @@ out:
 	return ret;
 }
 
+static bool object_has_prop(const struct drm_mode_object *obj, u32 prop_id)
+{
+	int i;
+
+	if (!obj->properties)
+		return false;
+
+	for (i = 0; i < obj->properties->count; i++)
+		if (obj->properties->ids[i] == prop_id)
+			return true;
+	return false;
+}
+
 int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv)
 {
@@ -3309,7 +3322,6 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	struct drm_mode_object *prop_obj;
 	struct drm_property *property;
 	int ret = -EINVAL;
-	int i;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -3322,11 +3334,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 	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;
-
-	if (i == arg_obj->properties->count)
+	if (!object_has_prop(arg_obj, arg->prop_id))
 		goto out;
 
 	prop_obj = drm_mode_object_find(dev, arg->prop_id,
-- 
1.7.3.4

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

[RFC][PATCH 07/10] drm: Add MODE_IDS property to connectors

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a new blob property MODE_IDS to connectors. This property contains
a list a mode object IDs attached to the connector (either probed modes,
or user attached modes).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c        |   77 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc_helper.c |    1 +
 include/drm/drm_crtc.h            |    2 +
 3 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 958a4b0..cfef9de 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -508,6 +508,9 @@ int drm_connector_init(struct drm_device *dev,
 	drm_connector_attach_property(connector,
 				      dev->mode_config.dpms_property, 0);
 
+	drm_connector_attach_property(connector,
+				      dev->mode_config.mode_ids_property, 0);
+
  out:
 	mutex_unlock(&dev->mode_config.mutex);
 
@@ -713,6 +716,7 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 {
 	struct drm_property *edid;
 	struct drm_property *dpms;
+	struct drm_property *mode_ids;
 
 	/*
 	 * Standard properties (apply to all connectors)
@@ -727,6 +731,11 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 				   ARRAY_SIZE(drm_dpms_enum_list));
 	dev->mode_config.dpms_property = dpms;
 
+	mode_ids = drm_property_create(dev, DRM_MODE_PROP_BLOB |
+				       DRM_MODE_PROP_IMMUTABLE,
+				       "MODE_IDS", 0);
+	dev->mode_config.mode_ids_property = mode_ids;
+
 	return 0;
 }
 
@@ -2648,6 +2657,10 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev,
 	}
 
 	drm_mode_attachmode(dev, connector, mode);
+
+	ret = drm_mode_connector_update_mode_ids_property(connector);
+	if (ret)
+		drm_mode_remove(connector, mode);
 out:
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
@@ -3144,6 +3157,70 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
 
+int drm_mode_connector_update_mode_ids_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property_blob *blob = NULL;
+	struct drm_property_blob *old_blob = NULL;
+	struct drm_display_mode *mode;
+	uint64_t value;
+	int i = 0;
+	int ret;
+
+	ret = drm_connector_property_get_value(connector,
+					       dev->mode_config.mode_ids_property,
+					       &value);
+	if (ret)
+		return ret;
+
+	if (value) {
+		struct drm_mode_object *obj = drm_mode_object_find(dev, value, DRM_MODE_OBJECT_BLOB);
+		if (!obj)
+			return -ENOENT;
+
+		old_blob = obj_to_blob(obj);
+	}
+
+	list_for_each_entry(mode, &connector->modes, head)
+		i++;
+	list_for_each_entry(mode, &connector->user_modes, head)
+		i++;
+
+	if (i) {
+		uint32_t *mode_ids = kcalloc(i, sizeof mode_ids[0], GFP_KERNEL);
+		if (!mode_ids)
+			return -ENOMEM;
+
+		i = 0;
+		list_for_each_entry(mode, &connector->modes, head)
+			mode_ids[i++] = mode->base.id;
+		list_for_each_entry(mode, &connector->user_modes, head)
+			mode_ids[i++] = mode->base.id;
+
+		blob = drm_property_create_blob(dev, i * sizeof mode_ids[0], mode_ids);
+
+		kfree(mode_ids);
+
+		if (!blob)
+			return -ENOMEM;
+	}
+
+	ret = drm_connector_property_set_value(connector,
+					       dev->mode_config.mode_ids_property,
+					       blob ? blob->base.id : 0);
+	if (ret) {
+		if (blob)
+			drm_property_destroy_blob(dev, blob);
+		return ret;
+	}
+
+	if (old_blob)
+		drm_property_destroy_blob(dev, old_blob);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_mode_connector_update_mode_ids_property);
+
 static bool range_property_is_signed(const struct drm_property *property)
 {
 	return property->values[0] > property->values[1];
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index ebbfcc6..bc24c0e 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -131,6 +131,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 
 	drm_mode_connector_list_update(connector);
+	drm_mode_connector_update_mode_ids_property(connector);
 
 	if (maxX && maxY)
 		drm_mode_validate_size(dev, &connector->modes, maxX,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 99bd489..c8bfdf1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -790,6 +790,7 @@ struct drm_mode_config {
 	struct list_head property_blob_list;
 	struct drm_property *edid_property;
 	struct drm_property *dpms_property;
+	struct drm_property *mode_ids_property;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
@@ -910,6 +911,7 @@ extern void drm_mode_set_crtcinfo(struct drm_display_mode *p,
 extern void drm_mode_connector_list_update(struct drm_connector *connector);
 extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
 						struct edid *edid);
+extern int drm_mode_connector_update_mode_ids_property(struct drm_connector *connector);
 extern int drm_connector_property_set_value(struct drm_connector *connector,
 					 struct drm_property *property,
 					 uint64_t value);
-- 
1.7.3.4

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

[RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

First draft.

The ioctl simply takes a list of object IDs and property IDs and their
values. For setting values to blob properties, the property value
indicates the length of the data, and the actual data is passed via
another blob pointer.

Detailed error reporting is missing, as is completion event support.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c  |    1 +
 include/drm/drm.h          |    1 +
 include/drm/drmP.h         |    2 +
 include/drm/drm_crtc.h     |   12 ++++
 include/drm/drm_mode.h     |   14 +++++
 6 files changed, 163 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cfef9de..35f8882 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4201,3 +4201,136 @@ int drm_calc_vscale(struct drm_region *src, struct drm_region *dst,
 	return vscale;
 }
 EXPORT_SYMBOL(drm_calc_vscale);
+
+int drm_mode_atomic_ioctl(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_atomic *arg = data;
+	uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
+	uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
+	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
+	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
+	uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
+	unsigned int copied_objs = 0;
+	unsigned int copied_props = 0;
+	unsigned int copied_blobs = 0;
+	void *state;
+	int ret = 0;
+	unsigned int i, j;
+
+	if (!dev->driver->atomic_funcs ||
+	    !dev->driver->atomic_funcs->begin ||
+	    !dev->driver->atomic_funcs->set ||
+	    !dev->driver->atomic_funcs->check ||
+	    !dev->driver->atomic_funcs->commit ||
+	    !dev->driver->atomic_funcs->end)
+		return -ENOSYS;
+
+	if (arg->flags & ~DRM_ATOMIC_TEST_ONLY)
+		return -EINVAL;
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	state = dev->driver->atomic_funcs->begin(dev, arg->flags);
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto unlock;
+	}
+
+	for (i = 0; i < arg->count_objs; i++) {
+		uint32_t obj_id, count_props;
+		struct drm_mode_object *obj;
+
+		if (get_user(obj_id, objs_ptr + copied_objs)) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
+		if (!obj || !obj->properties) {
+			ret = -ENOENT;
+			goto out;
+		}
+
+		if (get_user(count_props, count_props_ptr + copied_objs)) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		copied_objs++;
+
+		for (j = 0; j < count_props; j++) {
+			uint32_t prop_id;
+			uint64_t prop_value;
+			struct drm_mode_object *prop_obj;
+			struct drm_property *prop;
+			void *blob_data = NULL;
+
+			if (get_user(prop_id, props_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto out;
+			}
+
+			if (!object_has_prop(obj, prop_id)) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			prop_obj = drm_mode_object_find(dev, prop_id, DRM_MODE_OBJECT_PROPERTY);
+			if (!prop_obj) {
+				ret = -ENOENT;
+				goto out;
+			}
+			prop = obj_to_property(prop_obj);
+
+			if (get_user(prop_value, prop_values_ptr + copied_props)) {
+				ret = -EFAULT;
+				goto out;
+			}
+
+			if (!drm_property_change_is_valid(prop, prop_value)) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			if (prop->flags & DRM_MODE_PROP_BLOB && prop_value) {
+				blob_data = kmalloc(prop_value, GFP_KERNEL);
+				if (!blob_data) {
+					ret = -ENOMEM;
+					goto out;
+				}
+
+				if (copy_from_user(blob_data, blob_values_ptr + copied_blobs, prop_value)) {
+					kfree(blob_data);
+					ret = -EFAULT;
+					goto out;
+				}
+
+				copied_blobs++;
+			}
+
+			/* The driver will be in charge of blob_data from now on. */
+			ret = dev->driver->atomic_funcs->set(dev, state, obj, prop, prop_value, blob_data);
+			if (ret)
+				goto out;
+
+			copied_props++;
+		}
+	}
+
+	ret = dev->driver->atomic_funcs->check(dev, state);
+	if (ret)
+		goto out;
+
+	if (arg->flags & DRM_ATOMIC_TEST_ONLY)
+		goto out;
+
+	ret = dev->driver->atomic_funcs->commit(dev, state);
+
+ out:
+	dev->driver->atomic_funcs->end(dev, state);
+ unlock:
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 946bd91..6ca936a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -166,6 +166,7 @@ static struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drm.h b/include/drm/drm.h
index e51035a..b179169 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -732,6 +732,7 @@ struct drm_prime_handle {
 #define DRM_IOCTL_MODE_ADDFB2		DRM_IOWR(0xB8, struct drm_mode_fb_cmd2)
 #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES	DRM_IOWR(0xB9, struct drm_mode_obj_get_properties)
 #define DRM_IOCTL_MODE_OBJ_SETPROPERTY	DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
+#define DRM_IOCTL_MODE_ATOMIC	DRM_IOWR(0xBB, struct drm_mode_atomic)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 31ad880..7bff96d 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -964,6 +964,8 @@ struct drm_driver {
 
 	/* List of devices hanging off this driver */
 	struct list_head device_list;
+
+	const struct drm_atomic_funcs *atomic_funcs;
 };
 
 #define DRM_MINOR_UNASSIGNED 0
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c8bfdf1..a27b70f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1065,6 +1065,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
 					     struct drm_file *file_priv);
 extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
 					   struct drm_file *file_priv);
+extern int drm_mode_atomic_ioctl(struct drm_device *dev,
+				 void *data, struct drm_file *file_priv);
 
 extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
 				 int *bpp);
@@ -1101,4 +1103,14 @@ extern int drm_calc_hscale(struct drm_region *src, struct drm_region *dst,
 extern int drm_calc_vscale(struct drm_region *src, struct drm_region *dst,
 			   int min_vscale, int max_vscale);
 
+struct drm_atomic_funcs {
+	void *(*begin)(struct drm_device *dev, uint32_t flags);
+	int (*set)(struct drm_device *dev, void *state,
+		   struct drm_mode_object *obj, struct drm_property *prop,
+		   uint64_t value, void *blob_data);
+	int (*check)(struct drm_device *dev, void *state);
+	int (*commit)(struct drm_device *dev, void *state);
+	void (*end)(struct drm_device *dev, void *state);
+};
+
 #endif /* __DRM_CRTC_H__ */
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 5581980..ed776b4 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -459,4 +459,18 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+
+#define DRM_ATOMIC_TEST_ONLY (1<<0)
+
+/* FIXME come up with some sane error reporting mechanism? */
+struct drm_mode_atomic {
+	__u32 flags;
+	__u32 count_objs;
+	__u64 objs_ptr;
+	__u64 count_props_ptr;
+	__u64 props_ptr;
+	__u64 prop_values_ptr;
+	__u64 blob_values_ptr;
+};
+
 #endif
-- 
1.7.3.4

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

[RFC][PATCH 09/10] drm/i915: Split clipping and checking from update_plane hook

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split the update_plane() codepath into two separate steps. The first
step checkis and clips the plane, and the second step actually commits
the changes to the hardware. This allows the atomic modesetting code
to perform all checks before clobering hardware state.

The update_plane() hook is reduced to a thin wrapper calling both check
and commit functions.

Buffer (un)pinning is still being performed in the commit step. This
needs to be changed as well, so that the atomic modesetting code can
try to pin all new buffers before touching the hardware.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |   15 +-
 drivers/gpu/drm/i915/intel_sprite.c |  372 ++++++++++++++++++++++-------------
 2 files changed, 249 insertions(+), 138 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 08fa52d..3e9506c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -187,6 +187,15 @@ struct intel_crtc {
 	struct intel_pch_pll *pch_pll;
 };
 
+struct intel_plane_coords {
+	/* disabled or fully clipped? */
+	bool visible;
+	/* coordinates clipped against pipe dimensions */
+	int32_t crtc_x, crtc_y;
+	uint32_t crtc_w, crtc_h;
+	uint32_t src_x, src_y, src_w, src_h;
+};
+
 struct intel_plane {
 	struct drm_plane base;
 	enum pipe pipe;
@@ -196,11 +205,7 @@ struct intel_plane {
 	u32 lut_r[1024], lut_g[1024], lut_b[1024];
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_framebuffer *fb,
-			     struct drm_i915_gem_object *obj,
-			     int crtc_x, int crtc_y,
-			     unsigned int crtc_w, unsigned int crtc_h,
-			     uint32_t x, uint32_t y,
-			     uint32_t src_w, uint32_t src_h);
+			     const struct intel_plane_coords *clip);
 	void (*disable_plane)(struct drm_plane *plane);
 	int (*update_colorkey)(struct drm_plane *plane,
 			       struct drm_intel_sprite_colorkey *key);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e2abae6..f2cdb05 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -36,16 +36,173 @@
 #include "i915_drm.h"
 #include "i915_drv.h"
 
+static bool
+format_is_yuv(uint32_t format)
+{
+	switch (format) {
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_YVYU:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static void intel_clip_plane(const struct drm_plane *plane,
+			     const struct drm_crtc *crtc,
+			     const struct drm_framebuffer *fb,
+			     struct intel_plane_coords *coords)
+{
+	const struct intel_plane *intel_plane = to_intel_plane(plane);
+	const struct drm_display_mode *mode = &crtc->mode;
+	int hscale, vscale;
+	struct drm_region src = {
+		.x1 = coords->src_x,
+		.x2 = coords->src_x + coords->src_w,
+		.y1 = coords->src_y,
+		.y2 = coords->src_y + coords->src_h,
+	};
+	struct drm_region dst = {
+		.x1 = coords->crtc_x,
+		.x2 = coords->crtc_x + coords->crtc_w,
+		.y1 = coords->crtc_y,
+		.y2 = coords->crtc_y + coords->crtc_h,
+	};
+	const struct drm_region clip = {
+		.x2 = mode->hdisplay,
+		.y2 = mode->vdisplay,
+	};
+
+	hscale = drm_calc_hscale(&src, &dst, 1, intel_plane->max_downscale << 16);
+	vscale = drm_calc_vscale(&src, &dst, 1, intel_plane->max_downscale << 16);
+
+	coords->visible = drm_region_clip_scaled(&src, &dst, &clip, hscale, vscale);
+
+	coords->crtc_x = dst.x1;
+	coords->crtc_y = dst.y1;
+	coords->crtc_w = drm_region_width(&dst);
+	coords->crtc_h = drm_region_height(&dst);
+
+	/* HW doesn't seem to like smaller sprite, even when scaling */
+	/* FIXME return an error instead? */
+	if (coords->crtc_w < 3 || coords->crtc_h < 3)
+		coords->visible = false;
+
+	/*
+	 * Hardware doesn't handle subpixel coordinates.
+	 * Round to nearest (macro)pixel boundary.
+	 */
+	if (format_is_yuv(fb->pixel_format)) {
+		coords->src_x = ((src.x1 + 0x10000) >> 17) << 1;
+		coords->src_w = (((src.x2 + 0x10000) >> 17) << 1) - coords->src_x;
+	} else {
+		coords->src_x = (src.x1 + 0x8000) >> 16;
+		coords->src_w = ((src.x2 + 0x8000) >> 16) - coords->src_x;
+	}
+	coords->src_y = (src.y1 + 0x8000) >> 16;
+	coords->src_h = ((src.y2 + 0x8000) >> 16) - coords->src_y;
+
+	/* Account for minimum source size when scaling */
+	if (coords->visible && (coords->src_w != coords->crtc_w || coords->src_h != coords->crtc_h)) {
+		unsigned int min_w = format_is_yuv(fb->pixel_format) ? 4 : 3;
+
+		if (coords->src_w < min_w) {
+			coords->src_w = min_w;
+			if (coords->src_x > fb->width - coords->src_w)
+				coords->src_x = fb->width - coords->src_w;
+		}
+
+		/* FIXME interlacing */
+		if (coords->src_h < 3) {
+			coords->src_h = 3;
+			if (coords->src_y > fb->height - coords->src_h)
+				coords->src_y = fb->height - coords->src_h;
+		}
+	}
+}
+
+int intel_check_plane(const struct drm_plane *plane,
+		      const struct drm_crtc *crtc,
+		      const struct drm_framebuffer *fb,
+		      struct intel_plane_coords *coords)
+{
+	const struct intel_plane *intel_plane = to_intel_plane(plane);
+
+	if (fb) {
+		/* FIXME copy-pasted. refactor common code in drm_crtc.c */
+		uint32_t fb_width = fb->width << 16;
+		uint32_t fb_height = fb->height << 16;
+		int i;
+
+		for (i = 0; i < plane->format_count; i++) {
+			if (plane->format_types[i] == fb->pixel_format)
+				break;
+		}
+		if (i == plane->format_count)
+			return -EINVAL;
+
+		if (coords->src_w > fb_width ||
+		    coords->src_x > fb_width - coords->src_w ||
+		    coords->src_h > fb_height ||
+		    coords->src_y > fb_height - coords->src_h)
+			return -ENOSPC;
+
+		if (coords->crtc_w > INT_MAX ||
+		    coords->crtc_x > INT_MAX - (int32_t) coords->crtc_w ||
+		    coords->crtc_h > INT_MAX ||
+		    coords->crtc_y > INT_MAX - (int32_t) coords->crtc_h)
+			return -ERANGE;
+
+		if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384)
+			return -EINVAL;
+	}
+
+	if (crtc) {
+		const struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		/* Don't modify another pipe's plane */
+		if (intel_plane->pipe != intel_crtc->pipe)
+			return -EINVAL;
+	}
+
+	if (!fb || !crtc || !crtc->enabled) {
+		coords->visible = false;
+		return 0;
+	}
+
+	intel_clip_plane(plane, crtc, fb, coords);
+
+	/* Check size restrictions when scaling */
+	if (coords->visible && (coords->src_w != coords->crtc_w || coords->src_h != coords->crtc_h)) {
+		unsigned int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
+
+		if (coords->src_w > 2048 || coords->src_h > 2048 ||
+		    coords->src_w * cpp > 4096 - 64 || fb->pitches[0] > 4096)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void
-ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
-		 struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
-		 unsigned int crtc_w, unsigned int crtc_h,
-		 uint32_t x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+ivb_update_plane(struct drm_plane *plane,
+		 struct drm_framebuffer *fb,
+		 const struct intel_plane_coords *coords)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	const struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
+	int crtc_x = coords->crtc_x;
+	int crtc_y = coords->crtc_y;
+	unsigned int crtc_w = coords->crtc_w;
+	unsigned int crtc_h = coords->crtc_h;
+	uint32_t x = coords->src_x;
+	uint32_t y = coords->src_y;
+	uint32_t src_w = coords->src_w;
+	uint32_t src_h = coords->src_h;
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
@@ -209,15 +366,22 @@ ivb_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 }
 
 static void
-ilk_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
-		 struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
-		 unsigned int crtc_w, unsigned int crtc_h,
-		 uint32_t x, uint32_t y,
-		 uint32_t src_w, uint32_t src_h)
+ilk_update_plane(struct drm_plane *plane,
+		 struct drm_framebuffer *fb,
+		 const struct intel_plane_coords *coords)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	const struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
+	int crtc_x = coords->crtc_x;
+	int crtc_y = coords->crtc_y;
+	unsigned int crtc_w = coords->crtc_w;
+	unsigned int crtc_h = coords->crtc_h;
+	uint32_t x = coords->src_x;
+	uint32_t y = coords->src_y;
+	uint32_t src_w = coords->src_w;
+	uint32_t src_h = coords->src_h;
 	int pipe = intel_plane->pipe;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
@@ -383,129 +547,70 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 		key->flags = I915_SET_COLORKEY_NONE;
 }
 
-static bool
-format_is_yuv(uint32_t format)
-{
-	switch (format) {
-	case DRM_FORMAT_YUYV:
-	case DRM_FORMAT_UYVY:
-	case DRM_FORMAT_VYUY:
-	case DRM_FORMAT_YVYU:
-		return true;
-	default:
-		return false;
-	}
-}
-
 static int
-intel_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)
+intel_disable_plane(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int pipe = intel_plane->pipe;
 	int ret = 0;
-	int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
-	bool disable_primary = false;
-	bool visible;
-	int hscale, vscale;
-	int cpp = drm_format_plane_cpp(fb->pixel_format, 0);
-	struct drm_region src = {
-		.x1 = src_x,
-		.x2 = src_x + src_w,
-		.y1 = src_y,
-		.y2 = src_y + src_h,
-	};
-	struct drm_region dst = {
-		.x1 = crtc_x,
-		.x2 = crtc_x + crtc_w,
-		.y1 = crtc_y,
-		.y2 = crtc_y + crtc_h,
-	};
-	const struct drm_region clip = {
-		.x2 = crtc->mode.hdisplay,
-		.y2 = crtc->mode.vdisplay,
-	};
-
-	/* Don't modify another pipe's plane */
-	if (intel_plane->pipe != intel_crtc->pipe)
-		return -EINVAL;
-
-	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384)
-		return -EINVAL;
-
-	hscale = drm_calc_hscale(&src, &dst, 1, intel_plane->max_downscale << 16);
-	vscale = drm_calc_vscale(&src, &dst, 1, intel_plane->max_downscale << 16);
-
-	visible = drm_region_clip_scaled(&src, &dst, &clip, hscale, vscale);
 
-	crtc_x = dst.x1;
-	crtc_y = dst.y1;
-	crtc_w = drm_region_width(&dst);
-	crtc_h = drm_region_height(&dst);
+	if (intel_plane->primary_disabled) {
+		intel_enable_primary(plane->crtc);
+		intel_plane->primary_disabled = false;
+	}
 
-	/* HW doesn't seem to like smaller sprite, even when scaling */
-	/* FIXME return an error instead? */
-	if (crtc_w < 3 || crtc_h < 3)
-		visible = false;
+	intel_plane->disable_plane(plane);
 
-	/*
-	 * Hardware doesn't handle subpixel coordinates.
-	 * Round to nearest (macro)pixel boundary.
-	 */
-	if (format_is_yuv(fb->pixel_format)) {
-		src_x = ((src.x1 + 0x10000) >> 17) << 1;
-		src_w = (((src.x2 + 0x10000) >> 17) << 1) - src_x;
-	} else {
-		src_x = (src.x1 + 0x8000) >> 16;
-		src_w = ((src.x2 + 0x8000) >> 16) - src_x;
-	}
-	src_y = (src.y1 + 0x8000) >> 16;
-	src_h = ((src.y2 + 0x8000) >> 16) - src_y;
+	if (!intel_plane->obj)
+		goto out;
 
-	/* Account for minimum source size when scaling */
-	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
-		unsigned int min_w = format_is_yuv(fb->pixel_format) ? 4 : 3;
+	mutex_lock(&dev->struct_mutex);
+	intel_unpin_fb_obj(intel_plane->obj);
+	intel_plane->obj = NULL;
+	mutex_unlock(&dev->struct_mutex);
+out:
 
-		if (src_w < min_w) {
-			src_w = min_w;
-			if (src_x > fb->width - src_w)
-				src_x = fb->width - src_w;
-		}
+	return ret;
+}
 
-		/* FIXME interlacing */
-		if (src_h < 3) {
-			src_h = 3;
-			if (src_y > fb->height - src_h)
-				src_y = fb->height - src_h;
-		}
-	}
+int
+intel_commit_plane(struct drm_plane *plane,
+		   struct drm_crtc *crtc,
+		   struct drm_framebuffer *fb,
+		   const struct intel_plane_coords *coords)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_framebuffer *intel_fb;
+	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	int pipe = intel_plane->pipe;
+	int ret;
+	int primary_w, primary_h;
+	bool disable_primary = false;
 
-	/* Check size restrictions when scaling */
-	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
-		if (src_w > 2048 || src_h > 2048 ||
-		    src_w * cpp > 4096 - 64 || fb->pitches[0] > 4096)
-			return -EINVAL;
+	if (!coords->visible) {
+		intel_disable_plane(plane);
+		return 0;
 	}
 
+	/* FIXME this should happen anymore I suppose */
 	/* Pipe must be running... */
 	if (!(I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE))
 		return 0;
 
+	intel_fb = to_intel_framebuffer(fb);
+	obj = intel_fb->obj;
+	primary_w = crtc->mode.hdisplay;
+	primary_h = crtc->mode.vdisplay;
+
 	/*
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	if ((crtc_x == 0) && (crtc_y == 0) &&
-	    (crtc_w == primary_w) && (crtc_h == primary_h))
+	if ((coords->crtc_x == 0) && (coords->crtc_y == 0) &&
+	    (coords->crtc_w == primary_w) && (coords->crtc_h == primary_h))
 		disable_primary = true;
 
 	mutex_lock(&dev->struct_mutex);
@@ -525,9 +630,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		intel_plane->primary_disabled = false;
 	}
 
-	if (visible) {
-		intel_plane->update_plane(plane, fb, obj, crtc_x, crtc_y,
-					  crtc_w, crtc_h, src_x, src_y, src_w, src_h);
+	if (coords->visible) {
+		intel_plane->update_plane(plane, fb, coords);
 
 		if (disable_primary) {
 			intel_disable_primary(crtc);
@@ -558,29 +662,31 @@ out_unlock:
 }
 
 static int
-intel_disable_plane(struct drm_plane *plane)
+intel_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)
 {
-	struct drm_device *dev = plane->dev;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	int ret = 0;
-
-	if (intel_plane->primary_disabled) {
-		intel_enable_primary(plane->crtc);
-		intel_plane->primary_disabled = false;
-	}
-
-	intel_plane->disable_plane(plane);
+	int ret;
+	struct intel_plane_coords coords = {
+		.crtc_x = crtc_x,
+		.crtc_y = crtc_y,
+		.crtc_w = crtc_w,
+		.crtc_h = crtc_h,
+		.src_x = src_x,
+		.src_y = src_y,
+		.src_w = src_w,
+		.src_h = src_h,
+	};
 
-	if (!intel_plane->obj)
-		goto out;
+	ret = intel_check_plane(plane, crtc, fb, &coords);
+	if (ret)
+		return ret;
 
-	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(intel_plane->obj);
-	intel_plane->obj = NULL;
-	mutex_unlock(&dev->struct_mutex);
-out:
+	intel_commit_plane(plane, crtc, fb, &coords);
 
-	return ret;
+	return 0;
 }
 
 static void intel_destroy_plane(struct drm_plane *plane)
-- 
1.7.3.4

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

[RFC][PATCH 10/10] WIP drm/i915: "Atomic" modeset test implementation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Second attempt at the atomic modeset implementation. This versions does
things in a way that's closer to the drm_crtc_helper way. Not exactly
pretty, but there's too much code depending on that design to change it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/Makefile        |    1 +
 drivers/gpu/drm/i915/intel_atomic.c  | 1101 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |    7 +
 3 files changed, 1109 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_atomic.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 2e9268d..f97e7a4 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -15,6 +15,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
 	  i915_gem_tiling.o \
 	  i915_sysfs.o \
 	  i915_trace_points.o \
+	  intel_atomic.o \
 	  intel_display.o \
 	  intel_crt.o \
 	  intel_lvds.o \
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
new file mode 100644
index 0000000..173721a
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -0,0 +1,1101 @@
+/*
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+
+#include "intel_drv.h"
+
+static struct drm_property *prop_src_x;
+static struct drm_property *prop_src_y;
+static struct drm_property *prop_src_w;
+static struct drm_property *prop_src_h;
+static struct drm_property *prop_crtc_x;
+static struct drm_property *prop_crtc_y;
+static struct drm_property *prop_crtc_w;
+static struct drm_property *prop_crtc_h;
+static struct drm_property *prop_fb_id;
+static struct drm_property *prop_crtc_id;
+static struct drm_property *prop_mode_id;
+static struct drm_property *prop_connector_ids;
+
+struct intel_plane_state {
+	struct drm_plane *plane;
+	struct intel_plane_coords coords;
+	bool dirty;
+};
+
+struct intel_crtc_state {
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *old_fb;
+	bool mode_dirty;
+	bool fb_dirty;
+	bool active_dirty;
+	unsigned long connectors_bitmask;
+	unsigned long encoders_bitmask;
+};
+
+struct intel_atomic_state {
+	struct intel_plane_state *plane;
+	struct intel_crtc_state *crtc;
+	bool dirty;
+	bool restore_hw;
+	struct drm_plane *saved_planes;
+	struct drm_crtc *saved_crtcs;
+	struct drm_connector *saved_connectors;
+	struct drm_encoder *saved_encoders;
+};
+
+static void update_connectors_bitmask(struct intel_crtc_state *st)
+{
+	struct drm_device *dev = st->crtc->dev;
+	struct drm_connector *connector;
+	unsigned int i;
+
+	st->connectors_bitmask = 0;
+
+	i = 0;
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		if (connector->encoder && connector->encoder->crtc == st->crtc)
+			__set_bit(i, &st->connectors_bitmask);
+
+		i++;
+	}
+}
+
+static void update_encoders_bitmask(struct intel_crtc_state *st)
+{
+	struct drm_device *dev = st->crtc->dev;
+	struct drm_encoder *encoder;
+	unsigned int i;
+
+	st->encoders_bitmask = 0;
+
+	i = 0;
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		if (encoder->crtc == st->crtc)
+			__set_bit(i, &st->encoders_bitmask);
+
+		i++;
+	}
+}
+
+static int process_connectors(struct intel_crtc_state *s, const uint32_t *ids, int count_ids)
+{
+	struct drm_crtc *crtc = s->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_connector *connectors[count_ids];
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	int i;
+
+	for (i = 0; i < count_ids; i++) {
+		struct drm_encoder *encoder;
+		const struct drm_connector_helper_funcs *connector_funcs;
+		struct drm_mode_object *obj;
+		int j;
+
+		/* don't accept duplicates */
+		for (j = i + 1; j < count_ids; j++)
+			if (ids[i] == ids[j])
+				return -EINVAL;
+
+		obj = drm_mode_object_find(dev, ids[i], DRM_MODE_OBJECT_CONNECTOR);
+		if (!obj)
+			return -ENOENT;
+
+		connector = obj_to_connector(obj);
+		connector_funcs = connector->helper_private;
+
+		encoder = connector_funcs->best_encoder(connector);
+
+		if (!drm_encoder_crtc_ok(encoder, crtc))
+			return -EINVAL;
+
+		connectors[i] = connector;
+	}
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		const struct drm_connector_helper_funcs *connector_funcs =
+			connector->helper_private;
+
+		for (i = 0; i < count_ids; i++) {
+			if (connector == connectors[i])
+				break;
+		}
+
+		/* this connector isn't in the set */
+		if (i == count_ids) {
+			/* remove the link to the encoder if this crtc was driving it previously */
+			if (connector->encoder && connector->encoder->crtc == crtc) {
+				s->mode_dirty = true;
+				connector->encoder = NULL;
+			}
+			continue;
+		}
+
+		encoder = connector_funcs->best_encoder(connector);
+
+		if (encoder != connector->encoder) {
+			s->mode_dirty = true;
+			connector->encoder = encoder;
+		}
+
+		if (crtc != encoder->crtc) {
+			s->mode_dirty = true;
+			encoder->crtc = crtc;
+		}
+	}
+
+	/* prune dangling encoder->crtc links pointing to this crtc  */
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		if (encoder->crtc == crtc && !drm_helper_encoder_in_use(encoder))
+			encoder->crtc = NULL;
+	}
+
+	update_connectors_bitmask(s);
+	update_encoders_bitmask(s);
+
+	return 0;
+}
+
+static size_t intel_atomic_state_size(const struct drm_device *dev)
+{
+	struct intel_atomic_state *state;
+	unsigned int num_connector = dev->mode_config.num_connector;
+	unsigned int num_encoder = dev->mode_config.num_encoder;
+	unsigned int num_crtc = dev->mode_config.num_crtc;
+	unsigned int num_plane = dev->mode_config.num_plane;
+
+	return sizeof *state +
+		num_crtc * sizeof state->crtc[0] +
+		num_plane * sizeof state->plane[0] +
+		num_connector * sizeof state->saved_connectors[0] +
+		num_encoder * sizeof state->saved_encoders[0] +
+		num_crtc * sizeof state->saved_crtcs[0] +
+		num_plane * sizeof state->saved_planes[0];
+}
+
+static void *intel_atomic_begin(struct drm_device *dev, uint32_t flags)
+{
+	struct intel_atomic_state *state;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	unsigned int num_connector = dev->mode_config.num_connector;
+	unsigned int num_encoder = dev->mode_config.num_encoder;
+	unsigned int num_crtc = dev->mode_config.num_crtc;
+	unsigned int num_plane = dev->mode_config.num_plane;
+	int i;
+
+	state = kzalloc(intel_atomic_state_size(dev), GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->crtc = (struct intel_crtc_state *)(state + 1);
+	state->plane = (struct intel_plane_state  *)(state->crtc + num_crtc);
+
+	state->saved_connectors = (struct drm_connector *)(state->plane + num_plane);
+	state->saved_encoders = (struct drm_encoder *)(state->saved_connectors + num_connector);
+	state->saved_crtcs = (struct drm_crtc *)(state->saved_encoders + num_encoder);
+	state->saved_planes = (struct drm_plane *)(state->saved_crtcs + num_crtc);
+
+	i = 0;
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc_state *s = &state->crtc[i++];
+
+		s->crtc = crtc;
+		s->old_fb = crtc->fb;
+
+		update_connectors_bitmask(s);
+		update_encoders_bitmask(s);
+	}
+
+	i = 0;
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		struct intel_plane_state *s = &state->plane[i++];
+
+		s->plane = plane;
+	}
+
+	i = 0;
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+		state->saved_connectors[i++] = *connector;
+	i = 0;
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
+		state->saved_encoders[i++] = *encoder;
+	i = 0;
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		state->saved_crtcs[i++] = *crtc;
+	i = 0;
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		state->saved_planes[i++] = *plane;
+
+	return state;
+}
+
+static int plane_set(struct intel_atomic_state *s,
+		     struct intel_plane_state *state,
+		     struct drm_property *prop,
+		     uint64_t value)
+{
+	struct drm_plane *plane = state->plane;
+	struct drm_mode_object *obj;
+
+	if (prop == prop_src_x) {
+		if (plane->src_x == value)
+			return 0;
+		plane->src_x = value;
+	} else if (prop == prop_src_y) {
+		if (plane->src_y == value)
+			return 0;
+		plane->src_y = value;
+	} else if (prop == prop_src_w) {
+		if (plane->src_w == value)
+			return 0;
+		plane->src_w = value;
+	} else if (prop == prop_src_h) {
+		if (plane->src_h == value)
+			return 0;
+		plane->src_h = value;
+	} else if (prop == prop_crtc_x) {
+		if (plane->crtc_x == value)
+			return 0;
+		plane->crtc_x = value;
+	} else if (prop == prop_crtc_y) {
+		if (plane->crtc_y == value)
+			return 0;
+		plane->crtc_y = value;
+	} else if (prop == prop_crtc_w) {
+		if (plane->crtc_w == value)
+			return 0;
+		plane->crtc_w = value;
+	} else if (prop == prop_crtc_h) {
+		if (plane->crtc_h == value)
+			return 0;
+		plane->crtc_h = value;
+	} else if (prop == prop_crtc_id) {
+		struct drm_crtc *crtc = NULL;
+
+		if (plane->crtc) {
+			if (value == plane->crtc->base.id)
+				return 0;
+		} else {
+			if (value == 0)
+				return 0;
+		}
+
+		if (value) {
+			obj = drm_mode_object_find(plane->dev, value, DRM_MODE_OBJECT_CRTC);
+			if (!obj) {
+				printk("Unknown CRTC ID %llu\n", value);
+				return -ENOENT;
+			}
+			crtc = obj_to_crtc(obj);
+		}
+
+		plane->crtc = crtc;
+	} else if (prop == prop_fb_id) {
+		struct drm_framebuffer *fb = NULL;
+
+		if (plane->fb) {
+			if (value == plane->fb->base.id)
+				return 0;
+		} else {
+			if (value == 0)
+				return 0;
+		}
+
+		if (value) {
+			obj = drm_mode_object_find(plane->dev, value, DRM_MODE_OBJECT_FB);
+			if (!obj) {
+				printk("Unknown framebuffer ID %llu\n", value);
+				return -ENOENT;
+			}
+			fb = obj_to_fb(obj);
+		}
+
+		plane->fb = fb;
+	} else
+		return -ENOENT;
+
+	state->dirty = true;
+	s->dirty = true;
+
+	return 0;
+}
+
+static int crtc_set(struct intel_atomic_state *s,
+		    struct intel_crtc_state *state,
+		    struct drm_property *prop,
+		    uint64_t value, void *blob_data)
+{
+	struct drm_crtc *crtc = state->crtc;
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	struct drm_mode_object *obj;
+
+	if (prop == prop_src_x) {
+		if (crtc->x == value)
+			return 0;
+		crtc->x = value;
+		if (crtc_funcs->mode_set_base)
+			state->fb_dirty = true;
+		else
+			state->mode_dirty = true;
+	} else if (prop == prop_src_y) {
+		if (crtc->y == value)
+			return 0;
+		crtc->y = value;
+		if (crtc_funcs->mode_set_base)
+			state->fb_dirty = true;
+		else
+			state->mode_dirty = true;
+	} else if (prop == prop_mode_id) {
+		struct drm_display_mode *mode = NULL;
+
+		if (!crtc->enabled) {
+			if (value == 0)
+				return 0;
+		}
+
+		if (value) {
+			obj = drm_mode_object_find(crtc->dev, value, DRM_MODE_OBJECT_MODE);
+			if (!obj) {
+				printk("Unknown mode ID %llu\n", value);
+				return -ENOENT;
+			}
+			mode = obj_to_mode(obj);
+		}
+
+		/* FIXME should check just the user timings? */
+		if (crtc->enabled && mode && !memcmp(&crtc->mode, mode, sizeof *mode))
+			return 0;
+
+		/* turn on/off or active area changed? */
+		if (!crtc->enabled || !mode ||
+		    crtc->mode.hdisplay != mode->hdisplay ||
+		    crtc->mode.vdisplay != mode->vdisplay)
+			state->active_dirty = true;
+
+		if (mode) {
+			crtc->mode = *mode;
+			crtc->enabled = true;
+		} else
+			crtc->enabled = false;
+		state->mode_dirty = true;
+	} else if (prop == prop_fb_id) {
+		struct drm_framebuffer *fb = NULL;
+
+		if (crtc->fb) {
+			if (value == crtc->fb->base.id)
+				return 0;
+		} else {
+			if (value == 0)
+				return 0;
+		}
+
+		if (value) {
+			obj = drm_mode_object_find(crtc->dev, value, DRM_MODE_OBJECT_FB);
+			if (!obj) {
+				printk("Unknown framebuffer ID %llu\n", value);
+				return -ENOENT;
+			}
+			fb = obj_to_fb(obj);
+		}
+
+		crtc->fb = fb;
+		if (crtc_funcs->mode_set_base)
+			state->fb_dirty = true;
+		else
+			state->mode_dirty = true;
+	} else if (prop == prop_connector_ids) {
+		const uint32_t *ids = blob_data;
+		uint32_t count_ids = value / sizeof(uint32_t);
+		int ret;
+
+		if (value & 3)
+			return -EINVAL;
+
+		if (count_ids > crtc->dev->mode_config.num_connector)
+			return -ERANGE;
+
+		ret = process_connectors(state, ids, count_ids);
+		if (ret)
+			return ret;
+	} else
+		return -ENOENT;
+
+	s->dirty = true;
+
+	return 0;
+}
+
+static struct intel_plane_state *get_plane_state(const struct drm_device *dev,
+						 struct intel_atomic_state *state,
+						 const struct drm_plane *plane)
+{
+	int i;
+
+	for (i = 0; i < dev->mode_config.num_plane; i++)
+		if (plane == state->plane[i].plane)
+			return &state->plane[i];
+
+	return NULL;
+}
+
+static struct intel_crtc_state *get_crtc_state(const struct drm_device *dev,
+					       struct intel_atomic_state *state,
+					       const struct drm_crtc *crtc)
+{
+	int i;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++)
+		if (crtc == state->crtc[i].crtc)
+			return &state->crtc[i];
+
+	return NULL;
+}
+
+static void crtc_prepare(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	struct drm_encoder *encoder;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
+
+		if (encoder->crtc != crtc)
+			continue;
+
+		encoder_funcs->prepare(encoder);
+	}
+
+	drm_crtc_prepare_encoders(dev);
+
+	crtc_funcs->prepare(crtc);
+}
+
+static int crtc_set_base(struct drm_crtc *crtc,
+			 struct drm_framebuffer *old_fb)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	return crtc_funcs->mode_set_base(crtc, crtc->x, crtc->y, old_fb);
+}
+
+static int crtc_mode_set(struct drm_crtc *crtc,
+			 struct drm_framebuffer *old_fb)
+{
+	struct drm_device *dev = crtc->dev;
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	struct drm_encoder *encoder;
+	int ret;
+
+	ret = crtc_funcs->mode_set(crtc, &crtc->mode, &crtc->hwmode,
+				   crtc->x, crtc->y, old_fb);
+	if (ret)
+		return ret;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
+
+		if (encoder->crtc != crtc)
+			continue;
+
+		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->hwmode);
+	}
+
+	return 0;
+}
+
+static void crtc_commit(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	struct drm_encoder *encoder;
+
+	crtc_funcs->commit(crtc);
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
+
+		if (encoder->crtc != crtc)
+			continue;
+
+		encoder_funcs->commit(encoder);
+	}
+}
+
+int intel_commit_plane(struct drm_plane *plane,
+		       struct drm_crtc *crtc,
+		       struct drm_framebuffer *fb,
+		       const struct intel_plane_coords *st);
+
+static int apply_config(struct drm_device *dev,
+			struct intel_atomic_state *s)
+{
+	int i, ret;
+	struct drm_plane *plane;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (!st->mode_dirty)
+			continue;
+
+		crtc_prepare(st->crtc);
+	}
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (st->mode_dirty) {
+			ret = crtc_mode_set(st->crtc, st->old_fb);
+			if (ret)
+				return ret;
+		} else if (st->fb_dirty) {
+			ret = crtc_set_base(st->crtc, st->old_fb);
+			if (ret)
+				return ret;
+		} else
+			continue;
+
+		/*
+		 * Old fb was unpinned and new fb pinned.
+		 * Update the old_fb pointer accordingingly
+		 * in case we need to roll back later.
+		 */
+		st->old_fb = st->crtc->fb;
+	}
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *st = &s->plane[i];
+		struct intel_crtc_state *cst;
+		struct drm_plane *plane = st->plane;
+
+		if (!s->plane[i].dirty)
+			continue;
+
+		if (!plane->crtc)
+			continue;
+
+		cst = get_crtc_state(dev, s, plane->crtc);
+		/* already handled by the CRTC mode set? */
+		if (cst->mode_dirty)
+			continue;
+
+		ret = intel_commit_plane(plane, plane->crtc, plane->fb, &st->coords);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (!st->mode_dirty)
+			continue;
+
+		crtc_commit(st->crtc);
+	}
+
+	/*
+	 * FIXME perhaps better order would be
+	 * 1. prepare all current objects
+	 * 2. disable unused objects
+	 * 3. set mode for current objects
+	 * 4. commit current objects
+	 */
+	drm_helper_disable_unused_functions(dev);
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		/* planes attached to crtcs already handled in mode_set() */
+		if (!plane->crtc || !plane->fb)
+			plane->funcs->disable_plane(plane);
+	}
+
+	/* don't restore the old state in end() */
+	s->dirty = false;
+
+	return 0;
+}
+
+static void restore_state(struct drm_device *dev,
+			  struct intel_atomic_state *s)
+{
+	int i;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+
+	i = 0;
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+		*connector = s->saved_connectors[i++];
+	i = 0;
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
+		*encoder = s->saved_encoders[i++];
+	i = 0;
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		*crtc = s->saved_crtcs[i++];
+	i = 0;
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		*plane = s->saved_planes[i++];
+
+	/* FIXME props etc. */
+
+	/* was the hardware state clobbered? */
+	if (s->restore_hw)
+		apply_config(dev, s);
+}
+
+static int intel_atomic_set(struct drm_device *dev, void *state,
+			    struct drm_mode_object *obj,
+			    struct drm_property *prop,
+			    uint64_t value, void *blob_data)
+{
+	struct intel_atomic_state *s = state;
+	int ret = -EINVAL;
+
+	switch (obj->type) {
+	case DRM_MODE_OBJECT_PLANE:
+		ret = plane_set(s, get_plane_state(dev, s, obj_to_plane(obj)), prop, value);
+		break;
+	case DRM_MODE_OBJECT_CRTC:
+		ret = crtc_set(s, get_crtc_state(dev, s, obj_to_crtc(obj)), prop, value, blob_data);
+		break;
+	default:
+		break;
+	}
+
+	kfree(blob_data);
+
+	return ret;
+}
+
+int intel_check_plane(const struct drm_plane *plane,
+		      const struct drm_crtc *crtc,
+		      const struct drm_framebuffer *fb,
+		      struct intel_plane_coords *st);
+
+static void dirty_planes(const struct drm_device *dev,
+			 struct intel_atomic_state *state,
+			 const struct drm_crtc *crtc)
+{
+	int i;
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *s = &state->plane[i];
+
+		if (s->plane->crtc == crtc)
+			s->dirty = true;
+	}
+}
+
+static int check_crtc(struct intel_crtc_state *s, bool mode_dirty)
+{
+	struct drm_crtc *crtc = s->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_encoder *encoder;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct drm_display_mode mode, adjusted_mode;
+	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+
+	/* must have a fb and connectors if we have a mode, and vice versa */
+	if (crtc->enabled) {
+		if (!fb)
+			return -EINVAL;
+		if (!drm_helper_crtc_in_use(crtc))
+			return -EINVAL;
+	} else {
+		if (fb)
+			return -EINVAL;
+		if (drm_helper_crtc_in_use(crtc))
+			return -EINVAL;
+	}
+
+	if (crtc->enabled) {
+		if (crtc->mode.hdisplay > fb->width ||
+		    crtc->mode.vdisplay > fb->height ||
+		    crtc->x > fb->width - crtc->mode.hdisplay ||
+		    crtc->y > fb->height - crtc->mode.vdisplay)
+			return -ENOSPC;
+	}
+
+	if (!crtc->enabled || !mode_dirty)
+		return 0;
+
+	mode = adjusted_mode = crtc->mode;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+		const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
+
+		if (!encoder_funcs->mode_fixup(encoder, &mode, &adjusted_mode))
+			return -EINVAL;
+	}
+
+	if (!crtc_funcs->mode_fixup(crtc, &mode, &adjusted_mode))
+		return -EINVAL;
+
+	adjusted_mode.clock = mode.clock;
+
+	crtc->hwmode = adjusted_mode;
+
+	return 0;
+}
+
+static int intel_atomic_check(struct drm_device *dev, void *state)
+{
+	struct intel_atomic_state *s = state;
+	int ret;
+	int i;
+
+	if (!s->dirty)
+		return 0;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (!st->fb_dirty && !st->mode_dirty)
+			continue;
+
+		ret = check_crtc(st, st->mode_dirty);
+		if (ret)
+			return ret;
+
+		/*
+		 * Mark all planes on this CRTC as dirty if the active video
+		 * area changed so that the planes will get reclipped correctly.
+		 */
+		if (st->active_dirty)
+			dirty_planes(dev, s, st->crtc);
+	}
+
+	/* check for conflicts in encoder/connector assignment */
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+		int j;
+
+		for (j = i + 1; j < dev->mode_config.num_crtc; j++) {
+			struct intel_crtc_state *st2 = &s->crtc[j];
+
+			if (st->connectors_bitmask & st2->connectors_bitmask)
+				return -EINVAL;
+
+			if (st->encoders_bitmask & st2->encoders_bitmask)
+				return -EINVAL;
+		}
+	}
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *st = &s->plane[i];
+		const struct drm_plane *plane = st->plane;
+
+		if (!st->dirty)
+			continue;
+
+		st->coords.crtc_x = plane->crtc_x;
+		st->coords.crtc_y = plane->crtc_y;
+		st->coords.crtc_w = plane->crtc_w;
+		st->coords.crtc_h = plane->crtc_h;
+
+		st->coords.src_x = plane->src_x;
+		st->coords.src_y = plane->src_y;
+		st->coords.src_w = plane->src_w;
+		st->coords.src_h = plane->src_h;
+
+		ret = intel_check_plane(plane, plane->crtc, plane->fb, &st->coords);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void update_plane_props(struct drm_plane *plane)
+{
+	struct drm_mode_object *obj = &plane->base;
+
+	drm_object_property_set_value(obj, prop_src_x, plane->src_x);
+	drm_object_property_set_value(obj, prop_src_y, plane->src_y);
+	drm_object_property_set_value(obj, prop_src_w, plane->src_w);
+	drm_object_property_set_value(obj, prop_src_h, plane->src_h);
+
+	drm_object_property_set_value(obj, prop_crtc_x, plane->crtc_x);
+	drm_object_property_set_value(obj, prop_crtc_y, plane->crtc_y);
+	drm_object_property_set_value(obj, prop_crtc_w, plane->crtc_w);
+	drm_object_property_set_value(obj, prop_crtc_h, plane->crtc_h);
+
+	drm_object_property_set_value(obj, prop_fb_id, plane->fb ? plane->fb->base.id : 0);
+	drm_object_property_set_value(obj, prop_crtc_id, plane->crtc ? plane->crtc->base.id : 0);
+}
+
+static int free_connector_ids(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_object *obj;
+	uint64_t value;
+	int ret;
+
+	ret = drm_object_property_get_value(&crtc->base, prop_connector_ids, &value);
+	if (ret)
+		return ret;
+
+	if (!value)
+		return 0;
+
+	obj = drm_mode_object_find(dev, value, DRM_MODE_PROP_BLOB);
+	if (!obj)
+		return -ENOENT;
+
+	drm_property_destroy_blob(dev, obj_to_blob(obj));
+
+	return 0;
+}
+
+static int update_connector_ids(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_connector *connector;
+	struct drm_property_blob *blob;
+	uint64_t value = 0;
+	int i = 0;
+	uint32_t connector_ids[dev->mode_config.num_connector];
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		if (connector->encoder && connector->encoder->crtc == crtc)
+			connector_ids[i++] = connector->base.id;
+	}
+
+	if (i) {
+		/*
+		 * FIXME make blobs into vectors, and pre-allocate using worst case estimate.
+		 * That way this function could never fail.
+		 */
+		blob = drm_property_create_blob(dev, i * sizeof connector_ids[0], connector_ids);
+		if (!blob)
+			return -ENOMEM;
+
+		value = blob->base.id;
+	}
+
+	free_connector_ids(crtc);
+
+	drm_object_property_set_value(&crtc->base, prop_connector_ids, value);
+
+	return 0;
+}
+
+static void update_crtc_props(struct drm_crtc *crtc)
+{
+	struct drm_mode_object *obj = &crtc->base;
+
+	drm_object_property_set_value(obj, prop_src_x, crtc->x);
+	drm_object_property_set_value(obj, prop_src_y, crtc->y);
+
+	drm_object_property_set_value(obj, prop_fb_id, crtc->fb ? crtc->fb->base.id : 0);
+
+	/*
+	 * FIXME if mode is user specifiec via old setcrtc ioctl prop value should be -1
+	 * or perhaps setcrtc should insert an imlicitly created mode into some list...
+	 */
+	drm_object_property_set_value(obj, prop_mode_id,
+				      crtc->enabled ? crtc->mode.base.id : 0);
+
+	update_connector_ids(crtc);
+}
+
+static void update_props(const struct drm_device *dev,
+			 struct intel_atomic_state *s)
+{
+	int i;
+
+	for (i = 0; i < dev->mode_config.num_crtc; i++) {
+		struct intel_crtc_state *st = &s->crtc[i];
+
+		if (!st->fb_dirty && !st->mode_dirty)
+			continue;
+
+		update_crtc_props(st->crtc);
+	}
+
+	for (i = 0; i < dev->mode_config.num_plane; i++) {
+		struct intel_plane_state *st = &s->plane[i];
+
+		if (!st->dirty)
+			continue;
+
+		update_plane_props(st->plane);
+	}
+}
+
+static int intel_atomic_commit(struct drm_device *dev, void *state)
+{
+	struct intel_atomic_state *s = state;
+	int ret;
+
+	if (!s->dirty)
+		return 0;
+
+	ret = apply_config(dev, s);
+	if (ret) {
+		s->restore_hw = true;
+		return ret;
+	}
+
+	update_props(dev, s);
+
+	return 0;
+}
+
+static void intel_atomic_end(struct drm_device *dev, void *state)
+{
+	struct intel_atomic_state *s = state;
+
+	/* restore the state of all objects */
+	if (s->dirty)
+		restore_state(dev, state);
+
+	kfree(state);
+}
+
+static const struct drm_atomic_funcs intel_atomic_funcs = {
+	.begin = intel_atomic_begin,
+	.set = intel_atomic_set,
+	.check = intel_atomic_check,
+	.commit = intel_atomic_commit,
+	.end = intel_atomic_end,
+};
+
+int intel_atomic_init(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+	int ret = -ENOMEM;
+
+	prop_src_x = drm_property_create_range(dev, 0, "SRC_X", 0, UINT_MAX);
+	if (!prop_src_x)
+		goto out;
+	prop_src_y = drm_property_create_range(dev, 0, "SRC_Y", 0, UINT_MAX);
+	if (!prop_src_y)
+		goto destroy_src_x;
+	prop_src_w = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
+	if (!prop_src_w)
+		goto destroy_src_y;
+	prop_src_h = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
+	if (!prop_src_h)
+		goto destroy_src_w;
+
+	prop_crtc_x = drm_property_create_range(dev, 0, "CRTC_X", INT_MIN, INT_MAX);
+	if (!prop_crtc_x)
+		goto destroy_src_h;
+	prop_crtc_y = drm_property_create_range(dev, 0, "CRTC_Y", INT_MIN, INT_MAX);
+	if (!prop_crtc_y)
+		goto destroy_crtc_x;
+	prop_crtc_w = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
+	if (!prop_crtc_w)
+		goto destroy_crtc_y;
+	prop_crtc_h = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
+	if (!prop_crtc_h)
+		goto destroy_crtc_w;
+
+	/* FIXME create special object ID property type? */
+	prop_fb_id = drm_property_create_range(dev, 0, "FB_ID", 0, UINT_MAX);
+	if (!prop_fb_id)
+		goto destroy_crtc_h;
+	prop_crtc_id = drm_property_create_range(dev, 0, "CRTC_ID", 0, UINT_MAX);
+	if (!prop_crtc_id)
+		goto destroy_fb_id;
+	prop_mode_id = drm_property_create_range(dev, 0, "MODE_ID", 0, UINT_MAX);
+	if (!prop_mode_id)
+		goto destroy_crtc_id;
+
+	/* FIXME create special object ID list property type? */
+	prop_connector_ids = drm_property_create(dev, DRM_MODE_PROP_BLOB, "CONNECTOR_IDS", 0);
+	if (!prop_connector_ids)
+		goto destroy_mode_id;
+
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		struct drm_mode_object *obj = &plane->base;
+
+		drm_object_attach_property(obj, prop_src_x, 0);
+		drm_object_attach_property(obj, prop_src_y, 0);
+		drm_object_attach_property(obj, prop_src_w, 0);
+		drm_object_attach_property(obj, prop_src_h, 0);
+
+		drm_object_attach_property(obj, prop_crtc_x, 0);
+		drm_object_attach_property(obj, prop_crtc_y, 0);
+		drm_object_attach_property(obj, prop_crtc_w, 0);
+		drm_object_attach_property(obj, prop_crtc_h, 0);
+
+		drm_object_attach_property(obj, prop_fb_id, 0);
+		drm_object_attach_property(obj, prop_crtc_id, 0);
+
+		update_plane_props(plane);
+	}
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct drm_mode_object *obj = &crtc->base;
+
+		drm_object_attach_property(obj, prop_src_x, 0);
+		drm_object_attach_property(obj, prop_src_y, 0);
+
+		drm_object_attach_property(obj, prop_fb_id, 0);
+		drm_object_attach_property(obj, prop_mode_id, 0);
+		drm_object_attach_property(obj, prop_connector_ids, 0);
+
+		update_crtc_props(crtc);
+	}
+
+	dev->driver->atomic_funcs = &intel_atomic_funcs;
+
+	return 0;
+
+ destroy_mode_id:
+	drm_property_destroy(dev, prop_mode_id);
+ destroy_crtc_id:
+	drm_property_destroy(dev, prop_crtc_id);
+ destroy_fb_id:
+	drm_property_destroy(dev, prop_fb_id);
+ destroy_crtc_h:
+	drm_property_destroy(dev, prop_crtc_h);
+ destroy_crtc_w:
+	drm_property_destroy(dev, prop_crtc_w);
+ destroy_crtc_y:
+	drm_property_destroy(dev, prop_crtc_y);
+ destroy_crtc_x:
+	drm_property_destroy(dev, prop_crtc_x);
+ destroy_src_h:
+	drm_property_destroy(dev, prop_src_h);
+ destroy_src_w:
+	drm_property_destroy(dev, prop_src_w);
+ destroy_src_y:
+	drm_property_destroy(dev, prop_src_y);
+ destroy_src_x:
+	drm_property_destroy(dev, prop_src_x);
+ out:
+	return ret;
+}
+
+void intel_atomic_fini(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		free_connector_ids(crtc);
+
+	drm_property_destroy(dev, prop_connector_ids);
+	drm_property_destroy(dev, prop_mode_id);
+	drm_property_destroy(dev, prop_crtc_id);
+	drm_property_destroy(dev, prop_fb_id);
+
+	drm_property_destroy(dev, prop_crtc_h);
+	drm_property_destroy(dev, prop_crtc_w);
+	drm_property_destroy(dev, prop_crtc_y);
+	drm_property_destroy(dev, prop_crtc_x);
+
+	drm_property_destroy(dev, prop_src_h);
+	drm_property_destroy(dev, prop_src_w);
+	drm_property_destroy(dev, prop_src_y);
+	drm_property_destroy(dev, prop_src_x);
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 83437ee..4b186d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7053,6 +7053,9 @@ void intel_modeset_init_hw(struct drm_device *dev)
 		ivb_pch_pwm_override(dev);
 }
 
+void intel_atomic_init(struct drm_device *dev);
+void intel_atomic_fini(struct drm_device *dev);
+
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -7107,6 +7110,8 @@ void intel_modeset_init(struct drm_device *dev)
 	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
 	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
 		    (unsigned long)dev);
+
+	intel_atomic_init(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -7122,6 +7127,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
 
+	intel_atomic_fini(dev);
+
 	drm_kms_helper_poll_fini(dev);
 	mutex_lock(&dev->struct_mutex);
 
-- 
1.7.3.4

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

Re: [RFC][PATCH 0/10] Atomic modesetting v2

[Jesse Barnes]

On Wed, 27 Jun 2012 13:24:02 +0300
ville.syrjala@linux.intel.com wrote:

> Second version of the atomic mode setting work. Still very much
> work in progress.
> 
> I decided that I can't afford to fight the drm_crtc_helper
> architecture due to the sheer amount of driver code depending on it.
> So I changed the code to do things in way that more closely matches
> drm_crtc_helper.

:(

> Next I'll be moving the buffer pinning to happen before any hw state
> is clobbered. This will avoid having to do actual rollbacks when
> pinning fails. And a similar treatment needs to be done to the PLL
> calculations (those should be done before buffer pinning).
> 
> I didn't re-post all the i915 specific bits I have in my tree since those
> didn't really change. I pushed the whole work to the drm_atomic_2 branch
> at https://gitorious.org/vsyrjala/linux.

OTOH introducing an atomic alternative to the helper stuff and moving
drivers over would probably end up looking a lot nicer.  But there's no
doubt that's a huge chunk of work...

The other thing I'm worried about with atomic mode setting is handling
the legacy case properly.  We'll still need to handle apps that want to
change one CRTC at a time without altering the state of other CRTCs.
In an atomic context, drivers should be able to assume they can shut
anything off that doesn't come in as part of the atomic state, which
means when we build a compat atomic setup, we'll need to read the
current state of any existing objects and pass them in to the driver...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC][PATCH 0/10] Atomic modesetting v2

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 835 bytes --]

On Wed, 2012-06-27 at 09:48 -0700, Jesse Barnes wrote:

> The other thing I'm worried about with atomic mode setting is handling
> the legacy case properly.  We'll still need to handle apps that want to
> change one CRTC at a time without altering the state of other CRTCs.

Fortunately that's not true, you can flag-day it (per drm device even,
if we want).  If the first thing userspace asks for is atomic modeset
then you simply disallow subsequent non-atomic modesets, and the
reverse.  Userspace code will need to grow atomic support anyway, so can
simply treat -EINVAL from trying it as meaning "no kernel support,
revert to the older thing".

This does mean you have to update all of userspace to do the new call
instead of the old one, but that's fine, there's only a handful of
clients for this API.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC][PATCH 0/10] Atomic modesetting v2

[Dave Airlie]

On Wed, Jun 27, 2012 at 6:11 PM, Adam Jackson <ajax@redhat.com> wrote:
> On Wed, 2012-06-27 at 09:48 -0700, Jesse Barnes wrote:
>
>> The other thing I'm worried about with atomic mode setting is handling
>> the legacy case properly.  We'll still need to handle apps that want to
>> change one CRTC at a time without altering the state of other CRTCs.
>
> Fortunately that's not true, you can flag-day it (per drm device even,
> if we want).  If the first thing userspace asks for is atomic modeset
> then you simply disallow subsequent non-atomic modesets, and the
> reverse.  Userspace code will need to grow atomic support anyway, so can
> simply treat -EINVAL from trying it as meaning "no kernel support,
> revert to the older thing".
>
> This does mean you have to update all of userspace to do the new call
> instead of the old one, but that's fine, there's only a handful of
> clients for this API.
>

You'll also need to handles cases like render nodes where we split
resources for multi-seat on one gpu.

Dave.

Re: [RFC][PATCH 0/10] Atomic modesetting v2

[Ville Syrjälä]

On Wed, Jun 27, 2012 at 09:48:26AM -0700, Jesse Barnes wrote:
> On Wed, 27 Jun 2012 13:24:02 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > Second version of the atomic mode setting work. Still very much
> > work in progress.
> > 
> > I decided that I can't afford to fight the drm_crtc_helper
> > architecture due to the sheer amount of driver code depending on it.
> > So I changed the code to do things in way that more closely matches
> > drm_crtc_helper.
> 
> :(
> 
> > Next I'll be moving the buffer pinning to happen before any hw state
> > is clobbered. This will avoid having to do actual rollbacks when
> > pinning fails. And a similar treatment needs to be done to the PLL
> > calculations (those should be done before buffer pinning).
> > 
> > I didn't re-post all the i915 specific bits I have in my tree since those
> > didn't really change. I pushed the whole work to the drm_atomic_2 branch
> > at https://gitorious.org/vsyrjala/linux.
> 
> OTOH introducing an atomic alternative to the helper stuff and moving
> drivers over would probably end up looking a lot nicer.  But there's no
> doubt that's a huge chunk of work...

I'm thinking that it should be doable. Well, at least for all the "core"
features, but I'm not quite sure how it would handle all driver specific
properties in a nice way. Basically what I have now in intel_atomic.c
could become the new helper, but then it needs somehow to defer to the
driver for some of the props.

So either the driver needs to collect the state for those somehow, or we
have the core do it the same way as for other objects, ie. overwrite
the object state with the new values as we go on, and then roll back
later if necessary. But then we potentially need to save/restore all
properties in the beginning and end of the operation, or we could try
to do it in some lazy fashion.

> The other thing I'm worried about with atomic mode setting is handling
> the legacy case properly.  We'll still need to handle apps that want to
> change one CRTC at a time without altering the state of other CRTCs.
> In an atomic context, drivers should be able to assume they can shut
> anything off that doesn't come in as part of the atomic state, which
> means when we build a compat atomic setup, we'll need to read the
> current state of any existing objects and pass them in to the driver...

My current code collects the current state in the beginning (or rather
it saves the state of all the objects in the beginning) and starts
to modify the current state of the objects as new values are read in by
the ioctl handler. Anything not part of the atomic modeset is left
untouched.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[Rob Clark]

On Wed, Jun 27, 2012 at 5:24 AM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> First draft.
>
> The ioctl simply takes a list of object IDs and property IDs and their
> values. For setting values to blob properties, the property value
> indicates the length of the data, and the actual data is passed via
> another blob pointer.
>
> Detailed error reporting is missing, as is completion event support.

Hmm, I'm thinking we may need to define a bit in the way of
semantics..  ie. I think we really do want things like
enabling/disabling a plane to be asynchronous (after doing a
DRM_ATOMIC_TEST_ONLY call), otherwise performance will suck when
switching a window surface to/from a plane.  But maybe we should
define that another DRM_IOCTL_MODE_ATOMIC cannot come until after some
sort of event back to userspace?


> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |  133 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_drv.c  |    1 +
>  include/drm/drm.h          |    1 +
>  include/drm/drmP.h         |    2 +
>  include/drm/drm_crtc.h     |   12 ++++
>  include/drm/drm_mode.h     |   14 +++++
>  6 files changed, 163 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index cfef9de..35f8882 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4201,3 +4201,136 @@ int drm_calc_vscale(struct drm_region *src, struct drm_region *dst,
>         return vscale;
>  }
>  EXPORT_SYMBOL(drm_calc_vscale);
> +
> +int drm_mode_atomic_ioctl(struct drm_device *dev,
> +                         void *data, struct drm_file *file_priv)
> +{
> +       struct drm_mode_atomic *arg = data;
> +       uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
> +       uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
> +       uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
> +       uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
> +       uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
> +       unsigned int copied_objs = 0;
> +       unsigned int copied_props = 0;
> +       unsigned int copied_blobs = 0;
> +       void *state;
> +       int ret = 0;
> +       unsigned int i, j;
> +
> +       if (!dev->driver->atomic_funcs ||
> +           !dev->driver->atomic_funcs->begin ||
> +           !dev->driver->atomic_funcs->set ||
> +           !dev->driver->atomic_funcs->check ||
> +           !dev->driver->atomic_funcs->commit ||
> +           !dev->driver->atomic_funcs->end)
> +               return -ENOSYS;
> +
> +       if (arg->flags & ~DRM_ATOMIC_TEST_ONLY)
> +               return -EINVAL;
> +
> +       mutex_lock(&dev->mode_config.mutex);
> +
> +       state = dev->driver->atomic_funcs->begin(dev, arg->flags);
> +       if (IS_ERR(state)) {
> +               ret = PTR_ERR(state);
> +               goto unlock;
> +       }
> +
> +       for (i = 0; i < arg->count_objs; i++) {
> +               uint32_t obj_id, count_props;
> +               struct drm_mode_object *obj;
> +
> +               if (get_user(obj_id, objs_ptr + copied_objs)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +
> +               obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> +               if (!obj || !obj->properties) {
> +                       ret = -ENOENT;
> +                       goto out;
> +               }
> +
> +               if (get_user(count_props, count_props_ptr + copied_objs)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +
> +               copied_objs++;
> +
> +               for (j = 0; j < count_props; j++) {
> +                       uint32_t prop_id;
> +                       uint64_t prop_value;
> +                       struct drm_mode_object *prop_obj;
> +                       struct drm_property *prop;
> +                       void *blob_data = NULL;
> +
> +                       if (get_user(prop_id, props_ptr + copied_props)) {
> +                               ret = -EFAULT;
> +                               goto out;
> +                       }
> +
> +                       if (!object_has_prop(obj, prop_id)) {
> +                               ret = -EINVAL;
> +                               goto out;
> +                       }
> +
> +                       prop_obj = drm_mode_object_find(dev, prop_id, DRM_MODE_OBJECT_PROPERTY);
> +                       if (!prop_obj) {
> +                               ret = -ENOENT;
> +                               goto out;
> +                       }
> +                       prop = obj_to_property(prop_obj);
> +
> +                       if (get_user(prop_value, prop_values_ptr + copied_props)) {
> +                               ret = -EFAULT;
> +                               goto out;
> +                       }
> +
> +                       if (!drm_property_change_is_valid(prop, prop_value)) {
> +                               ret = -EINVAL;
> +                               goto out;
> +                       }
> +
> +                       if (prop->flags & DRM_MODE_PROP_BLOB && prop_value) {
> +                               blob_data = kmalloc(prop_value, GFP_KERNEL);
> +                               if (!blob_data) {
> +                                       ret = -ENOMEM;
> +                                       goto out;
> +                               }
> +
> +                               if (copy_from_user(blob_data, blob_values_ptr + copied_blobs, prop_value)) {
> +                                       kfree(blob_data);
> +                                       ret = -EFAULT;
> +                                       goto out;
> +                               }
> +
> +                               copied_blobs++;
> +                       }
> +
> +                       /* The driver will be in charge of blob_data from now on. */
> +                       ret = dev->driver->atomic_funcs->set(dev, state, obj, prop, prop_value, blob_data);
> +                       if (ret)
> +                               goto out;
> +
> +                       copied_props++;
> +               }
> +       }
> +
> +       ret = dev->driver->atomic_funcs->check(dev, state);
> +       if (ret)
> +               goto out;
> +
> +       if (arg->flags & DRM_ATOMIC_TEST_ONLY)
> +               goto out;
> +
> +       ret = dev->driver->atomic_funcs->commit(dev, state);
> +
> + out:
> +       dev->driver->atomic_funcs->end(dev, state);
> + unlock:
> +       mutex_unlock(&dev->mode_config.mutex);
> +
> +       return ret;
> +}
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 946bd91..6ca936a 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -166,6 +166,7 @@ static struct drm_ioctl_desc drm_ioctls[] = {
>         DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>         DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>         DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +       DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),

DRM_IOCTL_MODE_ATOMIC_SET?  DRM_IOCTL_MODE_SET_ATOMIC?  Seems like
there should somehow be a "SET" in the name..

BR,
-R

>  };
>
>  #define DRM_CORE_IOCTL_COUNT   ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm.h b/include/drm/drm.h
> index e51035a..b179169 100644
> --- a/include/drm/drm.h
> +++ b/include/drm/drm.h
> @@ -732,6 +732,7 @@ struct drm_prime_handle {
>  #define DRM_IOCTL_MODE_ADDFB2          DRM_IOWR(0xB8, struct drm_mode_fb_cmd2)
>  #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES       DRM_IOWR(0xB9, struct drm_mode_obj_get_properties)
>  #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
> +#define DRM_IOCTL_MODE_ATOMIC  DRM_IOWR(0xBB, struct drm_mode_atomic)
>
>  /**
>   * Device specific ioctls should only be in their respective headers
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 31ad880..7bff96d 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -964,6 +964,8 @@ struct drm_driver {
>
>         /* List of devices hanging off this driver */
>         struct list_head device_list;
> +
> +       const struct drm_atomic_funcs *atomic_funcs;
>  };
>
>  #define DRM_MINOR_UNASSIGNED 0
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c8bfdf1..a27b70f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1065,6 +1065,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
>                                              struct drm_file *file_priv);
>  extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
>                                            struct drm_file *file_priv);
> +extern int drm_mode_atomic_ioctl(struct drm_device *dev,
> +                                void *data, struct drm_file *file_priv);
>
>  extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
>                                  int *bpp);
> @@ -1101,4 +1103,14 @@ extern int drm_calc_hscale(struct drm_region *src, struct drm_region *dst,
>  extern int drm_calc_vscale(struct drm_region *src, struct drm_region *dst,
>                            int min_vscale, int max_vscale);
>
> +struct drm_atomic_funcs {
> +       void *(*begin)(struct drm_device *dev, uint32_t flags);
> +       int (*set)(struct drm_device *dev, void *state,
> +                  struct drm_mode_object *obj, struct drm_property *prop,
> +                  uint64_t value, void *blob_data);
> +       int (*check)(struct drm_device *dev, void *state);
> +       int (*commit)(struct drm_device *dev, void *state);
> +       void (*end)(struct drm_device *dev, void *state);
> +};
> +
>  #endif /* __DRM_CRTC_H__ */
> diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> index 5581980..ed776b4 100644
> --- a/include/drm/drm_mode.h
> +++ b/include/drm/drm_mode.h
> @@ -459,4 +459,18 @@ struct drm_mode_destroy_dumb {
>         uint32_t handle;
>  };
>
> +
> +#define DRM_ATOMIC_TEST_ONLY (1<<0)
> +
> +/* FIXME come up with some sane error reporting mechanism? */
> +struct drm_mode_atomic {
> +       __u32 flags;
> +       __u32 count_objs;
> +       __u64 objs_ptr;
> +       __u64 count_props_ptr;
> +       __u64 props_ptr;
> +       __u64 prop_values_ptr;
> +       __u64 blob_values_ptr;
> +};
> +
>  #endif
> --
> 1.7.3.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[Daniel Vetter]

On Fri, Aug 31, 2012 at 05:47:13PM -0500, Rob Clark wrote:
> On Wed, Jun 27, 2012 at 5:24 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > First draft.
> >
> > The ioctl simply takes a list of object IDs and property IDs and their
> > values. For setting values to blob properties, the property value
> > indicates the length of the data, and the actual data is passed via
> > another blob pointer.
> >
> > Detailed error reporting is missing, as is completion event support.
> 
> Hmm, I'm thinking we may need to define a bit in the way of
> semantics..  ie. I think we really do want things like
> enabling/disabling a plane to be asynchronous (after doing a
> DRM_ATOMIC_TEST_ONLY call), otherwise performance will suck when
> switching a window surface to/from a plane.  But maybe we should
> define that another DRM_IOCTL_MODE_ATOMIC cannot come until after some
> sort of event back to userspace?

I've discussed things with Kristian Høgsberg (and also Jesse now that he's
back from sabbatical) a lot and we've concluded on one big thing:

 We should separate the issue of modeset on mutliple crtcs (usually called
 atomic modeset on irc) from a (usually called nuclear) pageflip on a
 single crtc that exchanges a bunch of things (framebufferrs, plane
 parameters, other properties, cursor position, ...).

The reason is that these things solve in large parts separate problems:

- atomic modeset is first and foremost a solution to exposing constraints
  on global configurations due to shared resources like plls, link lanes
  (e.g. on many intel chipset if you enable multiple outputs you can use
  the full bw of the single output config) and similar things. The
  solution is to tell the kernel the entire config and let the kernel
  decide how to best make it possible with the hw it has (maybe even
  re-assigning crtcs internally to give the display pipe with the most
  bandwidth to the crtc with the highest dotclock*bpp requirement). To be
  able to expose all this to userspace Jesse proposed a test_flag to check
  configurations without actually changing anything in the hw state. This
  way a modeset GUI could grey out everything that won't work (since
  describing the actual hw constraints is too complex, and users don't
  really care anyway in most cases why something doesn't work).

- a modeset also usually can take a few vblanks, since establishing new
  clocks and locking them in the various stages of the pipeline can take a
  while. And since modeset configuration happens seldomly, we imo can
  simply specify that a modeset always happens synchronously.

- Pageflip on the _must_ happen on a precise vblank, and be fully
  asynchronous, always. Furthermore we want a timestamp to know when the
  pageflip actually happened. Again, due to funny hw constraints we might
  need a test_flag so that a compositor can figure out whether it can use
  a plane for a given surface. Compared to the atomic modeset test-flag we
  have a few complications:
  - We don't want to walk the plane configuration space in the test mode
    for every frame. Hence the compositor will just try the current
    configuration (maybe with some surfaces moved around and a few
    properties changed). If a given plane config doesn't work out, the
    compositor could just fall back to gpu rendering and try to come up
    with a new optimal configuration for the next frame.
  - There's the issue that some changes require more than one vblank to
    take effect due to some prep work (e.g. switching a plane to a
    different crtc). My idea here is that the kernel could return a
    special "transient -EINVAL" for this case indicating to the compositor
    that it can't do this configuration for the next vblank, but it has
    set up things so that eventually (in a few vblanks) the sprite will be
    available on that crtc. The userspace compositor should then just keep
    on retrying for each frame with this configuration (until it either
    works or the kernel returns a "definit -EINVAL" indicating that
    something changed and it can't do this configuration). That way we
    should be able to support any steady-state plane/fb/cursor/whatever
    configuration that the hw can, without forcing the compositor to miss
    a frame. Obviously, these prepare steps _must_ _not_ result in a rouge
    plane eventually appearing ;-)

- Also a rather practical one: The lack of nuclear pageflip is the reason
  that Wayland/Weston can't use sprites atm (since set_plane is can be
  synchronous and can't be synced with a pageflip). Kristian therefore
  wants the atomic modeset (which I think is much more invasive) not to
  stall the nuclear pageflip support. And the implementation effort is imo
  really a big difference: For i915.ko I expect the nuclear pageflip to
  mostly boil down to wiring up async plane/cursor updates through the
  latch register (with keeping all the other setup code around since we
  only allow at most one outstanding pageflip per crtc currently anyway)
  and wiring that up with the existing crtc pageflip handler. atomic
  modeset otoh requires an invasive rewrite of the driver code to properly
  handle these shared resources (and support the test mode). The beginning
  of that is happening with the modeset rework, but that's just the first
  step.

I.e. I'm voting to reduce the scope of this ioctl a bit in the naming of
gaining simpler&clearer semantics. In any case I don't see the big&scary
work in the ioctl itself, but the actual low-level hw implementation in
drivers (imo the crtc helper framework is holy inadequate for this safe
for the simplest of drivers) and in testing the ioctl interfaces
exhaustively.

Especially the testing is very important, since the entire design hinges
on the test-flag to make complex configuration constraints discoverable at
run-time. I think we need a few tools that random-walk the configuration
space and check whether any configuration that returns -EINVAL in the test
mode actually fails (and vice-versa), maybe excluding any other failures
in the real modeset like -EIO (e.g. due to some stupid dp link failing to
properly lock onto the stream).

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[Rob Clark]

On Sat, Sep 1, 2012 at 6:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Aug 31, 2012 at 05:47:13PM -0500, Rob Clark wrote:
>> On Wed, Jun 27, 2012 at 5:24 AM,  <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > First draft.
>> >
>> > The ioctl simply takes a list of object IDs and property IDs and their
>> > values. For setting values to blob properties, the property value
>> > indicates the length of the data, and the actual data is passed via
>> > another blob pointer.
>> >
>> > Detailed error reporting is missing, as is completion event support.
>>
>> Hmm, I'm thinking we may need to define a bit in the way of
>> semantics..  ie. I think we really do want things like
>> enabling/disabling a plane to be asynchronous (after doing a
>> DRM_ATOMIC_TEST_ONLY call), otherwise performance will suck when
>> switching a window surface to/from a plane.  But maybe we should
>> define that another DRM_IOCTL_MODE_ATOMIC cannot come until after some
>> sort of event back to userspace?
>
> I've discussed things with Kristian Høgsberg (and also Jesse now that he's
> back from sabbatical) a lot and we've concluded on one big thing:
>
>  We should separate the issue of modeset on mutliple crtcs (usually called
>  atomic modeset on irc) from a (usually called nuclear) pageflip on a
>  single crtc that exchanges a bunch of things (framebufferrs, plane
>  parameters, other properties, cursor position, ...).

yeah, nuclear-pageflip would be associated only w/ a single crtc.
Actually I was kinda assuming atomic-modeset was too..  ie. moving a
plane from one crtc to another would be two ioctls, one to remove it
from first crtc, one to add to the other.  Although maybe one big
ioctl is better.

I do think there is a bit of a grey area between the two.  Ie.
enabling/disabling a crtc, or changing what encoder->connector it is
hooked to might be taking several vblank cycles, and is a relatively
infrequent operation so ok to block.  Enabling/disabling a plane could
be much more frequent and should not block.  Returning EINVAL or EBUSY
in the transient case where it isn't ready quite yet seems like a good
approach.. EBUSY might be better for "it might succeed if you try
again in next vblank" and EINVAL for "it won't ever work with current
setup, don't bother trying again".

Infrequent, can be slow:
 + enabling/disabling a new output

Semi-frequent, should not block but -EBUSY ok:
 + enabling/disabling a plane
 + resizing a plane

More frequent, should not block, should not need extra "test" ioctl
step, but should just work
 + page flip, update plane position

So for that middle group, I'm a bit fuzzy on where those operations
should fit in..

> The reason is that these things solve in large parts separate problems:
>
> - atomic modeset is first and foremost a solution to exposing constraints
>   on global configurations due to shared resources like plls, link lanes
>   (e.g. on many intel chipset if you enable multiple outputs you can use
>   the full bw of the single output config) and similar things. The
>   solution is to tell the kernel the entire config and let the kernel
>   decide how to best make it possible with the hw it has (maybe even
>   re-assigning crtcs internally to give the display pipe with the most
>   bandwidth to the crtc with the highest dotclock*bpp requirement). To be
>   able to expose all this to userspace Jesse proposed a test_flag to check
>   configurations without actually changing anything in the hw state. This
>   way a modeset GUI could grey out everything that won't work (since
>   describing the actual hw constraints is too complex, and users don't
>   really care anyway in most cases why something doesn't work).
>
> - a modeset also usually can take a few vblanks, since establishing new
>   clocks and locking them in the various stages of the pipeline can take a
>   while. And since modeset configuration happens seldomly, we imo can
>   simply specify that a modeset always happens synchronously.
>
> - Pageflip on the _must_ happen on a precise vblank, and be fully
>   asynchronous, always. Furthermore we want a timestamp to know when the
>   pageflip actually happened. Again, due to funny hw constraints we might
>   need a test_flag so that a compositor can figure out whether it can use
>   a plane for a given surface. Compared to the atomic modeset test-flag we
>   have a few complications:
>   - We don't want to walk the plane configuration space in the test mode
>     for every frame. Hence the compositor will just try the current
>     configuration (maybe with some surfaces moved around and a few
>     properties changed). If a given plane config doesn't work out, the
>     compositor could just fall back to gpu rendering and try to come up
>     with a new optimal configuration for the next frame.
>   - There's the issue that some changes require more than one vblank to
>     take effect due to some prep work (e.g. switching a plane to a
>     different crtc). My idea here is that the kernel could return a
>     special "transient -EINVAL" for this case indicating to the compositor
>     that it can't do this configuration for the next vblank, but it has
>     set up things so that eventually (in a few vblanks) the sprite will be
>     available on that crtc. The userspace compositor should then just keep
>     on retrying for each frame with this configuration (until it either
>     works or the kernel returns a "definit -EINVAL" indicating that
>     something changed and it can't do this configuration). That way we
>     should be able to support any steady-state plane/fb/cursor/whatever
>     configuration that the hw can, without forcing the compositor to miss
>     a frame. Obviously, these prepare steps _must_ _not_ result in a rouge
>     plane eventually appearing ;-)
>
> - Also a rather practical one: The lack of nuclear pageflip is the reason
>   that Wayland/Weston can't use sprites atm (since set_plane is can be
>   synchronous and can't be synced with a pageflip). Kristian therefore
>   wants the atomic modeset (which I think is much more invasive) not to
>   stall the nuclear pageflip support. And the implementation effort is imo
>   really a big difference: For i915.ko I expect the nuclear pageflip to
>   mostly boil down to wiring up async plane/cursor updates through the
>   latch register (with keeping all the other setup code around since we
>   only allow at most one outstanding pageflip per crtc currently anyway)
>   and wiring that up with the existing crtc pageflip handler. atomic
>   modeset otoh requires an invasive rewrite of the driver code to properly
>   handle these shared resources (and support the test mode). The beginning
>   of that is happening with the modeset rework, but that's just the first
>   step.

Hmm, I was thinking Ville's atomic fxns would be useful, but maybe I
can still bits from that and try and put nuclear-pageflip beneath
atomic-modeset in the stack.

BR,
-R

> I.e. I'm voting to reduce the scope of this ioctl a bit in the naming of
> gaining simpler&clearer semantics. In any case I don't see the big&scary
> work in the ioctl itself, but the actual low-level hw implementation in
> drivers (imo the crtc helper framework is holy inadequate for this safe
> for the simplest of drivers) and in testing the ioctl interfaces
> exhaustively.
>
> Especially the testing is very important, since the entire design hinges
> on the test-flag to make complex configuration constraints discoverable at
> run-time. I think we need a few tools that random-walk the configuration
> space and check whether any configuration that returns -EINVAL in the test
> mode actually fails (and vice-versa), maybe excluding any other failures
> in the real modeset like -EIO (e.g. due to some stupid dp link failing to
> properly lock onto the stream).
>
> Cheers, Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

Re: [RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[Daniel Vetter]

On Sat, Sep 1, 2012 at 5:58 PM, Rob Clark <robdclark@gmail.com> wrote:
> yeah, nuclear-pageflip would be associated only w/ a single crtc.
> Actually I was kinda assuming atomic-modeset was too..  ie. moving a
> plane from one crtc to another would be two ioctls, one to remove it
> from first crtc, one to add to the other.  Although maybe one big
> ioctl is better.
>
> I do think there is a bit of a grey area between the two.  Ie.
> enabling/disabling a crtc, or changing what encoder->connector it is
> hooked to might be taking several vblank cycles, and is a relatively
> infrequent operation so ok to block.  Enabling/disabling a plane could
> be much more frequent and should not block.  Returning EINVAL or EBUSY
> in the transient case where it isn't ready quite yet seems like a good
> approach.. EBUSY might be better for "it might succeed if you try
> again in next vblank" and EINVAL for "it won't ever work with current
> setup, don't bother trying again".
>
> Infrequent, can be slow:
>  + enabling/disabling a new output
>
> Semi-frequent, should not block but -EBUSY ok:
>  + enabling/disabling a plane
>  + resizing a plane
>
> More frequent, should not block, should not need extra "test" ioctl
> step, but should just work
>  + page flip, update plane position
>
> So for that middle group, I'm a bit fuzzy on where those operations
> should fit in..

My idea is that the nuclear pageflip should support those since they
only affect a single crtc. The compositor would simply assume it will
work out, and the kernel can then succeed, or return either the
transient or definite -EINVAL if it doesn't work out. In the later
case the compositor can retry in the next frame (and fall back to gpu
compositing for the current one).

[snip]

>> - Also a rather practical one: The lack of nuclear pageflip is the reason
>>   that Wayland/Weston can't use sprites atm (since set_plane is can be
>>   synchronous and can't be synced with a pageflip). Kristian therefore
>>   wants the atomic modeset (which I think is much more invasive) not to
>>   stall the nuclear pageflip support. And the implementation effort is imo
>>   really a big difference: For i915.ko I expect the nuclear pageflip to
>>   mostly boil down to wiring up async plane/cursor updates through the
>>   latch register (with keeping all the other setup code around since we
>>   only allow at most one outstanding pageflip per crtc currently anyway)
>>   and wiring that up with the existing crtc pageflip handler. atomic
>>   modeset otoh requires an invasive rewrite of the driver code to properly
>>   handle these shared resources (and support the test mode). The beginning
>>   of that is happening with the modeset rework, but that's just the first
>>   step.
>
> Hmm, I was thinking Ville's atomic fxns would be useful, but maybe I
> can still bits from that and try and put nuclear-pageflip beneath
> atomic-modeset in the stack.

At least in i915 I expect that the atomic modeset and the nuclear
pageflip would take rather different paths. I'm not even sure whether
we should support enabling planes in the amotic modeset (userspace can
always composite the first frame after a modeset with the gpu) and
handle enabling planes with only the nuclear pageflip.

I guess we /could/ add a few things to the crtc helper code to
facilitate drivers using them to support atomic modeset. I'm thinking
of a simple ->check callback at the beginning, which checks whether
the global configuration can be supported. This checking would be in
addition to the ->mode_fixup checking we already have in place at the
encoder/crtc layer.

Then we could add a simple atomic_modeset implementation for using
these interfaces.

For the nuclear pageflip I guess we'll just add a new crtc interface
(not in the helper func pointer) which gets all the new parameters and
let drivers sort things out. Since the flip needs to happen synced to
the same vblank for everything, I don't see how we could compose this
without some in-depth hw knowledge (which only the driver has) in some
generic framework. And we do not yet support pageflip on planes (at
least not in full glory with drm events and timestamps) anyway.

Cheers, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 08/10] WIP: drm: Atomic modeset ioctl

[Rob Clark]

On Sat, Sep 1, 2012 at 11:56 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Sep 1, 2012 at 5:58 PM, Rob Clark <robdclark@gmail.com> wrote:
>> yeah, nuclear-pageflip would be associated only w/ a single crtc.
>> Actually I was kinda assuming atomic-modeset was too..  ie. moving a
>> plane from one crtc to another would be two ioctls, one to remove it
>> from first crtc, one to add to the other.  Although maybe one big
>> ioctl is better.
>>
>> I do think there is a bit of a grey area between the two.  Ie.
>> enabling/disabling a crtc, or changing what encoder->connector it is
>> hooked to might be taking several vblank cycles, and is a relatively
>> infrequent operation so ok to block.  Enabling/disabling a plane could
>> be much more frequent and should not block.  Returning EINVAL or EBUSY
>> in the transient case where it isn't ready quite yet seems like a good
>> approach.. EBUSY might be better for "it might succeed if you try
>> again in next vblank" and EINVAL for "it won't ever work with current
>> setup, don't bother trying again".
>>
>> Infrequent, can be slow:
>>  + enabling/disabling a new output
>>
>> Semi-frequent, should not block but -EBUSY ok:
>>  + enabling/disabling a plane
>>  + resizing a plane
>>
>> More frequent, should not block, should not need extra "test" ioctl
>> step, but should just work
>>  + page flip, update plane position
>>
>> So for that middle group, I'm a bit fuzzy on where those operations
>> should fit in..
>
> My idea is that the nuclear pageflip should support those since they
> only affect a single crtc. The compositor would simply assume it will
> work out, and the kernel can then succeed, or return either the
> transient or definite -EINVAL if it doesn't work out. In the later
> case the compositor can retry in the next frame (and fall back to gpu
> compositing for the current one).

hmm, but this would imply that the nuclear pageflip must also have a
'test' step.. because the compositor needs to know whether or not it
will work, *then* kick rendering, and *then* issue the actual
pageflip.

But OTOH, some simple things like just flipping or perhaps
flipping+moving should not need a test step.  I guess userspace could
just know that certain operations don't need a test step.  Seems a bit
weird, though.

> [snip]
>
>>> - Also a rather practical one: The lack of nuclear pageflip is the reason
>>>   that Wayland/Weston can't use sprites atm (since set_plane is can be
>>>   synchronous and can't be synced with a pageflip). Kristian therefore
>>>   wants the atomic modeset (which I think is much more invasive) not to
>>>   stall the nuclear pageflip support. And the implementation effort is imo
>>>   really a big difference: For i915.ko I expect the nuclear pageflip to
>>>   mostly boil down to wiring up async plane/cursor updates through the
>>>   latch register (with keeping all the other setup code around since we
>>>   only allow at most one outstanding pageflip per crtc currently anyway)
>>>   and wiring that up with the existing crtc pageflip handler. atomic
>>>   modeset otoh requires an invasive rewrite of the driver code to properly
>>>   handle these shared resources (and support the test mode). The beginning
>>>   of that is happening with the modeset rework, but that's just the first
>>>   step.
>>
>> Hmm, I was thinking Ville's atomic fxns would be useful, but maybe I
>> can still bits from that and try and put nuclear-pageflip beneath
>> atomic-modeset in the stack.
>
> At least in i915 I expect that the atomic modeset and the nuclear
> pageflip would take rather different paths. I'm not even sure whether
> we should support enabling planes in the amotic modeset (userspace can
> always composite the first frame after a modeset with the gpu) and
> handle enabling planes with only the nuclear pageflip.

hmm, ok, yeah.. I guess we could just make nuclear_page_flip a fxnptr
on the crtc funcs.

I suppose I don't mind one way or another about the enabling/disabling
planes.  It seems ok to render first frame w/ gpu, so if it simplifies
things I guess it is ok.

> I guess we /could/ add a few things to the crtc helper code to
> facilitate drivers using them to support atomic modeset. I'm thinking
> of a simple ->check callback at the beginning, which checks whether
> the global configuration can be supported. This checking would be in
> addition to the ->mode_fixup checking we already have in place at the
> encoder/crtc layer.
>
> Then we could add a simple atomic_modeset implementation for using
> these interfaces.
>
> For the nuclear pageflip I guess we'll just add a new crtc interface
> (not in the helper func pointer) which gets all the new parameters and
> let drivers sort things out. Since the flip needs to happen synced to
> the same vblank for everything, I don't see how we could compose this
> without some in-depth hw knowledge (which only the driver has) in some
> generic framework. And we do not yet support pageflip on planes (at
> least not in full glory with drm events and timestamps) anyway.

Right, I was thinking to start without helper..  it is easy enough to
add helpers later if there are some common patterns emerging between
the different drivers.

BR,
-R

> Cheers, Daniel
> --
> Daniel Vetter
> daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch