[PATCH 00/11] CRTC state mode property

[PATCH 00/11] CRTC state mode property

[Daniel Stone]

Hi,
Building on the blob-property submission series[0], this provides a
MODE_ID property that can be used to set the current mode for a CRTC
under atomic modesetting, plus a way for users to create blob
properties so they can specify their own modes.

Using this[1] I've been able to validate the atomic userspace API with
a ported version of Weston[2]. This is currently undergoing review, but
seems pretty OK. Both modesetting for every output together, and
per-output async pageflips with events, works as well as the driver
does.

Also tested with fbcon and X11, on Broadwell.

Cheers,
Daniel

[0]: <1429554176-9865-1-git-send-email-daniels@collabora.com>
[1]: And a pile of other work in the i915 driver from Maarten/Ander, and
     my own bits on top; see
     git://git.collabora.co.uk/git/user/daniels/linux in the
     wip/4.1.x/blob-modes-i915 branch.
[2]: http://lists.freedesktop.org/archives/wayland-devel/2015-May/022106.html

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

[PATCH 01/11] drm: kerneldoc fixes for blob properties

[Daniel Stone]

Change '@param foo' to '@foo:' to fit kerneldoc style.

672cb1d6ae mistakenly added an extra parameter to the kerneldoc for
drm_property_unreference_blob which wasn't actually present.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_crtc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4059f06..7e5085f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4270,7 +4270,7 @@ EXPORT_SYMBOL(drm_property_create_blob);
  *
  * Internal free function for blob properties; must not be used directly.
  *
- * @param kref Reference
+ * @kref: Reference
  */
 static void drm_property_free_blob(struct kref *kref)
 {
@@ -4290,7 +4290,7 @@ static void drm_property_free_blob(struct kref *kref)
  *
  * Drop a reference on a blob property. May free the object.
  *
- * @param blob Pointer to blob property
+ * @blob: Pointer to blob property
  */
 void drm_property_unreference_blob(struct drm_property_blob *blob)
 {
@@ -4318,8 +4318,7 @@ EXPORT_SYMBOL(drm_property_unreference_blob);
  * Drop a reference on a blob property. May free the object. This must be
  * called with blob_lock held.
  *
- * @param dev  Device the blob was created on
- * @param blob Pointer to blob property
+ * @blob: Pointer to blob property
  */
 static void drm_property_unreference_blob_locked(struct drm_property_blob *blob)
 {
@@ -4336,7 +4335,7 @@ static void drm_property_unreference_blob_locked(struct drm_property_blob *blob)
  *
  * Take a new reference on an existing blob property.
  *
- * @param blob Pointer to blob property
+ * @blob: Pointer to blob property
  */
 struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob)
 {
-- 
2.4.1

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

[PATCH 02/11] drm/crtc_helper: Replace open-coded CRTC state helpers

[Daniel Stone]

Rather than open-coding our own CRTC state helpers, use the atomic helpers
added in f5e7840b0c, and make our freeing behaviour consistent as well.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc_helper.c | 42 ++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index f2f42b1..9297a61 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -927,14 +927,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 
 	if (crtc->funcs->atomic_duplicate_state)
 		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
-	else if (crtc->state)
-		crtc_state = kmemdup(crtc->state, sizeof(*crtc_state),
-				     GFP_KERNEL);
-	else
+	else {
 		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
-	if (!crtc_state)
-		return -ENOMEM;
-	crtc_state->crtc = crtc;
+		if (!crtc_state)
+			return -ENOMEM;
+		if (crtc->state)
+			__drm_atomic_helper_crtc_duplicate_state(crtc, crtc_state);
+		else
+			crtc_state->crtc = crtc;
+	}
 
 	crtc_state->enable = true;
 	crtc_state->planes_changed = true;
@@ -944,30 +945,25 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 
 	if (crtc_funcs->atomic_check) {
 		ret = crtc_funcs->atomic_check(crtc, crtc_state);
-		if (ret) {
-			if (crtc->funcs->atomic_destroy_state) {
-				crtc->funcs->atomic_destroy_state(crtc,
-				                                  crtc_state);
-			} else {
-				kfree(crtc_state);
-			}
-
-			return ret;
-		}
+		if (ret)
+			goto out;
 	}
 
 	swap(crtc->state, crtc_state);
 
 	crtc_funcs->mode_set_nofb(crtc);
 
-	if (crtc_state) {
-		if (crtc->funcs->atomic_destroy_state)
-			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
-		else
-			kfree(crtc_state);
+	ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
+
+out:
+	if (crtc->funcs->atomic_destroy_state)
+		crtc->funcs->atomic_destroy_state(crtc, crtc_state);
+	else {
+		__drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
+		kfree(crtc_state);
 	}
 
-	return drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
+	return ret;
 }
 EXPORT_SYMBOL(drm_helper_crtc_mode_set);
 
-- 
2.4.1

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

[PATCH 03/11] drm: Retain reference to blob properties in lookup

[Daniel Stone]

When we look up a blob property, make sure we retain a reference to the
blob for the lifetime.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7e5085f..da2b117 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4661,7 +4661,9 @@ bool drm_property_change_valid_get(struct drm_property *property,
 		for (i = 0; i < property->num_values; i++)
 			valid_mask |= (1ULL << property->values[i]);
 		return !(value & ~valid_mask);
-	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
+	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB) ||
+		   (drm_property_type_is(property, DRM_MODE_PROP_OBJECT) &&
+		    property->values[0] == DRM_MODE_OBJECT_BLOB)) {
 		struct drm_property_blob *blob;
 
 		if (value == 0)
@@ -4709,6 +4711,8 @@ void drm_property_change_valid_put(struct drm_property *property,
 	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
 		if (property->values[0] == DRM_MODE_OBJECT_FB)
 			drm_framebuffer_unreference(obj_to_fb(ref));
+		else if (property->values[0] == DRM_MODE_OBJECT_BLOB)
+			drm_property_unreference_blob(obj_to_blob(ref));
 	}
 }
 
-- 
2.4.1

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

Re: [PATCH 03/11] drm: Retain reference to blob properties in lookup

[Daniel Vetter]

On Fri, May 22, 2015 at 01:34:46PM +0100, Daniel Stone wrote:
> When we look up a blob property, make sure we retain a reference to the
> blob for the lifetime.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_crtc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7e5085f..da2b117 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4661,7 +4661,9 @@ bool drm_property_change_valid_get(struct drm_property *property,
>  		for (i = 0; i < property->num_values; i++)
>  			valid_mask |= (1ULL << property->values[i]);
>  		return !(value & ~valid_mask);
> -	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
> +	} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB) ||
> +		   (drm_property_type_is(property, DRM_MODE_PROP_OBJECT) &&
> +		    property->values[0] == DRM_MODE_OBJECT_BLOB)) {
>  		struct drm_property_blob *blob;
>  
>  		if (value == 0)
> @@ -4709,6 +4711,8 @@ void drm_property_change_valid_put(struct drm_property *property,
>  	if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) {
>  		if (property->values[0] == DRM_MODE_OBJECT_FB)
>  			drm_framebuffer_unreference(obj_to_fb(ref));
> +		else if (property->values[0] == DRM_MODE_OBJECT_BLOB)
> +			drm_property_unreference_blob(obj_to_blob(ref));

Hm why this? I know it's a bit inconsistent that for all the other atomic
obj props we use DRM_MODE_PROP_OBJ, but for blobs we have a special one. I
guess I'll see the reason later on, but without more details I'd vote to
not subsume props under the generic PROP_OBJECT.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/11] drm/mode: Validate modes inside drm_crtc_convert_umode

[Daniel Stone]

The only user of convert_umode was also performing mode validation, so
do that in the same place.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index da2b117..9ab2fc6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1785,11 +1785,15 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
 static int drm_crtc_convert_umode(struct drm_display_mode *out,
 				  const struct drm_mode_modeinfo *in)
 {
-	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
-		return -ERANGE;
+	int ret = -EINVAL;
+
+	if (in->clock > INT_MAX || in->vrefresh > INT_MAX) {
+		ret = -ERANGE;
+		goto out;
+	}
 
 	if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
-		return -EINVAL;
+		goto out;
 
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
@@ -1808,7 +1812,14 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out,
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
 
-	return 0;
+	out->status = drm_mode_validate_basic(out);
+	if (out->status != MODE_OK)
+		goto out;
+
+	ret = 0;
+
+out:
+	return ret;
 }
 
 /**
@@ -2821,12 +2832,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
-		mode->status = drm_mode_validate_basic(mode);
-		if (mode->status != MODE_OK) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
 
 		/*
-- 
2.4.1

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

[PATCH 05/11] drm/mode: Unstatic kernel-userspace mode conversion

[Daniel Stone]

Move the drm_display_mode <-> drm_mode_modeinfo conversion functions
from drm_crtc.c to drm_modes.c, and make them non-static so that others
can use them.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c  | 95 ++-------------------------------------------
 drivers/gpu/drm/drm_modes.c | 87 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_modes.h     |  4 ++
 3 files changed, 95 insertions(+), 91 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9ab2fc6..def7be8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1736,93 +1736,6 @@ void drm_reinit_primary_mode_group(struct drm_device *dev)
 EXPORT_SYMBOL(drm_reinit_primary_mode_group);
 
 /**
- * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
- * @out: drm_mode_modeinfo struct to return to the user
- * @in: drm_display_mode to use
- *
- * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
- * the user.
- */
-static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
-				      const struct drm_display_mode *in)
-{
-	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
-	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
-	     in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
-	     in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
-	     in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
-	     "timing values too large for mode info\n");
-
-	out->clock = in->clock;
-	out->hdisplay = in->hdisplay;
-	out->hsync_start = in->hsync_start;
-	out->hsync_end = in->hsync_end;
-	out->htotal = in->htotal;
-	out->hskew = in->hskew;
-	out->vdisplay = in->vdisplay;
-	out->vsync_start = in->vsync_start;
-	out->vsync_end = in->vsync_end;
-	out->vtotal = in->vtotal;
-	out->vscan = in->vscan;
-	out->vrefresh = in->vrefresh;
-	out->flags = in->flags;
-	out->type = in->type;
-	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
-	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
-}
-
-/**
- * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode
- * @out: drm_display_mode to return to the user
- * @in: drm_mode_modeinfo to use
- *
- * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to
- * the caller.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-static int drm_crtc_convert_umode(struct drm_display_mode *out,
-				  const struct drm_mode_modeinfo *in)
-{
-	int ret = -EINVAL;
-
-	if (in->clock > INT_MAX || in->vrefresh > INT_MAX) {
-		ret = -ERANGE;
-		goto out;
-	}
-
-	if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
-		goto out;
-
-	out->clock = in->clock;
-	out->hdisplay = in->hdisplay;
-	out->hsync_start = in->hsync_start;
-	out->hsync_end = in->hsync_end;
-	out->htotal = in->htotal;
-	out->hskew = in->hskew;
-	out->vdisplay = in->vdisplay;
-	out->vsync_start = in->vsync_start;
-	out->vsync_end = in->vsync_end;
-	out->vtotal = in->vtotal;
-	out->vscan = in->vscan;
-	out->vrefresh = in->vrefresh;
-	out->flags = in->flags;
-	out->type = in->type;
-	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
-	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
-
-	out->status = drm_mode_validate_basic(out);
-	if (out->status != MODE_OK)
-		goto out;
-
-	ret = 0;
-
-out:
-	return ret;
-}
-
-/**
  * drm_mode_getresources - get graphics configuration
  * @dev: drm device for the ioctl
  * @data: data pointer for the ioctl
@@ -2048,7 +1961,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->x = crtc->primary->state->src_x >> 16;
 		crtc_resp->y = crtc->primary->state->src_y >> 16;
 		if (crtc->state->enable) {
-			drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
+			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
 			crtc_resp->mode_valid = 1;
 
 		} else {
@@ -2058,7 +1971,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->x = crtc->x;
 		crtc_resp->y = crtc->y;
 		if (crtc->enabled) {
-			drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode);
+			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
 			crtc_resp->mode_valid = 1;
 
 		} else {
@@ -2215,7 +2128,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
 			if (!drm_mode_expose_to_userspace(mode, file_priv))
 				continue;
 
-			drm_crtc_convert_to_umode(&u_mode, mode);
+			drm_mode_convert_to_umode(&u_mode, mode);
 			if (copy_to_user(mode_ptr + copied,
 					 &u_mode, sizeof(u_mode))) {
 				ret = -EFAULT;
@@ -2826,7 +2739,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
-		ret = drm_crtc_convert_umode(mode, &crtc_req->mode);
+		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
 		if (ret) {
 			DRM_DEBUG_KMS("Invalid mode\n");
 			goto out;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 213b11e..cd74a09 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1405,3 +1405,90 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 	return mode;
 }
 EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
+
+/**
+ * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
+ * @out: drm_mode_modeinfo struct to return to the user
+ * @in: drm_display_mode to use
+ *
+ * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
+ * the user.
+ */
+void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
+			       const struct drm_display_mode *in)
+{
+	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
+	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
+	     in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
+	     in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
+	     in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
+	     "timing values too large for mode info\n");
+
+	out->clock = in->clock;
+	out->hdisplay = in->hdisplay;
+	out->hsync_start = in->hsync_start;
+	out->hsync_end = in->hsync_end;
+	out->htotal = in->htotal;
+	out->hskew = in->hskew;
+	out->vdisplay = in->vdisplay;
+	out->vsync_start = in->vsync_start;
+	out->vsync_end = in->vsync_end;
+	out->vtotal = in->vtotal;
+	out->vscan = in->vscan;
+	out->vrefresh = in->vrefresh;
+	out->flags = in->flags;
+	out->type = in->type;
+	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
+	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
+}
+
+/**
+ * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode
+ * @out: drm_display_mode to return to the user
+ * @in: drm_mode_modeinfo to use
+ *
+ * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to
+ * the caller.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_convert_umode(struct drm_display_mode *out,
+			   const struct drm_mode_modeinfo *in)
+{
+	int ret = -EINVAL;
+
+	if (in->clock > INT_MAX || in->vrefresh > INT_MAX) {
+		ret = -ERANGE;
+		goto out;
+	}
+
+	if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX)
+		goto out;
+
+	out->clock = in->clock;
+	out->hdisplay = in->hdisplay;
+	out->hsync_start = in->hsync_start;
+	out->hsync_end = in->hsync_end;
+	out->htotal = in->htotal;
+	out->hskew = in->hskew;
+	out->vdisplay = in->vdisplay;
+	out->vsync_start = in->vsync_start;
+	out->vsync_end = in->vsync_end;
+	out->vtotal = in->vtotal;
+	out->vscan = in->vscan;
+	out->vrefresh = in->vrefresh;
+	out->flags = in->flags;
+	out->type = in->type;
+	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
+	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
+
+	out->status = drm_mode_validate_basic(out);
+	if (out->status != MODE_OK)
+		goto out;
+
+	ret = 0;
+
+out:
+	return ret;
+}
\ No newline at end of file
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 0616188..08a8cac 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -182,6 +182,10 @@ struct drm_cmdline_mode;
 
 struct drm_display_mode *drm_mode_create(struct drm_device *dev);
 void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode);
+void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
+                               const struct drm_display_mode *in);
+int drm_mode_convert_umode(struct drm_display_mode *out,
+			   const struct drm_mode_modeinfo *in);
 void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
 
-- 
2.4.1

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

[PATCH 06/11] drm: Allow creating blob properties without copy

[Daniel Stone]

Make the data parameter to drm_property_create_blob optional; if
omitted, the copy will be skipped and the data will be empty.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@intel.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index def7be8..e4a48e0 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4145,6 +4145,16 @@ done:
 	return ret;
 }
 
+/**
+ * drm_property_create_blob - Create new blob property
+ *
+ * Creates a new blob property for a specified DRM device, optionally
+ * copying data.
+ *
+ * @dev: DRM device to create property for
+ * @length: Length to allocate for blob data
+ * @data: If specified, copies data into blob
+ */
 struct drm_property_blob *
 drm_property_create_blob(struct drm_device *dev, size_t length,
 			 const void *data)
@@ -4152,7 +4162,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	struct drm_property_blob *blob;
 	int ret;
 
-	if (!length || !data)
+	if (!length)
 		return NULL;
 
 	blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
@@ -4162,7 +4172,8 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	blob->length = length;
 	blob->dev = dev;
 
-	memcpy(blob->data, data, length);
+	if (data)
+		memcpy(blob->data, data, length);
 
 	mutex_lock(&dev->mode_config.blob_lock);
 
-- 
2.4.1

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

[PATCH 07/11] drm: Return error value from blob creation

[Daniel Stone]

Change drm_property_create_blob to return an ERR_PTR-encoded error on
failure, so we can pass the failure reason down.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@intel.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e4a48e0..494c4a0 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4154,6 +4154,10 @@ done:
  * @dev: DRM device to create property for
  * @length: Length to allocate for blob data
  * @data: If specified, copies data into blob
+ *
+ * Returns:
+ * New blob property with a single reference on success, or an ERR_PTR
+ * value on failure.
  */
 struct drm_property_blob *
 drm_property_create_blob(struct drm_device *dev, size_t length,
@@ -4163,11 +4167,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	int ret;
 
 	if (!length)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
 	if (!blob)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	blob->length = length;
 	blob->dev = dev;
@@ -4181,7 +4185,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	if (ret) {
 		kfree(blob);
 		mutex_unlock(&dev->mode_config.blob_lock);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	kref_init(&blob->refcount);
@@ -4370,8 +4374,8 @@ static int drm_property_replace_global_blob(struct drm_device *dev,
 
 	if (length && data) {
 		new_blob = drm_property_create_blob(dev, length, data);
-		if (!new_blob)
-			return -EINVAL;
+		if (IS_ERR(new_blob))
+			return PTR_ERR(new_blob);
 	}
 
 	/* This does not need to be synchronised with blob_lock, as the
-- 
2.4.1

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

[PATCH 08/11] drm/mode: Add user blob-creation ioctl

[Daniel Stone]

Add an ioctl which allows users to create blob properties from supplied
data. Currently this only supports modes, creating a drm_display_mode from
the userspace drm_mode_modeinfo.

v2: Removed size/type checks.
    Rebased on new patches to allow error propagation from create_blob,
    as well as avoiding double-allocation.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@intel.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c  | 139 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_fops.c  |   5 +-
 drivers/gpu/drm/drm_ioctl.c |   2 +
 include/drm/drmP.h          |   4 ++
 include/drm/drm_crtc.h      |   9 ++-
 include/uapi/drm/drm.h      |   2 +
 include/uapi/drm/drm_mode.h |  20 +++++++
 7 files changed, 176 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 494c4a0..9263b22 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4173,6 +4173,9 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	if (!blob)
 		return ERR_PTR(-ENOMEM);
 
+	/* This must be explicitly initialised, so we can safely call list_del
+	 * on it in the removal handler, even if it isn't in a file list. */
+	INIT_LIST_HEAD(&blob->head_file);
 	blob->length = length;
 	blob->dev = dev;
 
@@ -4190,7 +4193,8 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 
 	kref_init(&blob->refcount);
 
-	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
+	list_add_tail(&blob->head_global,
+	              &dev->mode_config.property_blob_list);
 
 	mutex_unlock(&dev->mode_config.blob_lock);
 
@@ -4212,7 +4216,8 @@ static void drm_property_free_blob(struct kref *kref)
 
 	WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock));
 
-	list_del(&blob->head);
+	list_del(&blob->head_global);
+	list_del(&blob->head_file);
 	drm_mode_object_put(blob->dev, &blob->base);
 
 	kfree(blob);
@@ -4264,6 +4269,26 @@ static void drm_property_unreference_blob_locked(struct drm_property_blob *blob)
 }
 
 /**
+ * drm_property_destroy_user_blobs - destroy all blobs created by this client
+ * @dev:       DRM device
+ * @file_priv: destroy all blobs owned by this file handle
+ */
+void drm_property_destroy_user_blobs(struct drm_device *dev,
+				     struct drm_file *file_priv)
+{
+	struct drm_property_blob *blob, *bt;
+
+	mutex_lock(&dev->mode_config.blob_lock);
+
+	list_for_each_entry_safe(blob, bt, &file_priv->blobs, head_file) {
+		list_del_init(&blob->head_file);
+		drm_property_unreference_blob_locked(blob);
+	}
+
+	mutex_unlock(&dev->mode_config.blob_lock);
+}
+
+/**
  * drm_property_reference_blob - Take a reference on an existing property
  *
  * Take a new reference on an existing blob property.
@@ -4453,6 +4478,114 @@ done:
 }
 
 /**
+ * drm_mode_createblob_ioctl - create a new blob property
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * This function creates a new blob property with user-defined values. In order
+ * to give us sensible validation and checking when creating, rather than at
+ * every potential use, we also require a type to be provided upfront.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_createblob_ioctl(struct drm_device *dev,
+			      void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_create_blob *out_resp = data;
+	struct drm_property_blob *blob;
+	void __user *blob_ptr;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	blob = drm_property_create_blob(dev, out_resp->length, NULL);
+	if (IS_ERR(blob))
+		return PTR_ERR(blob);
+
+	blob_ptr = (void __user *)(unsigned long)out_resp->data;
+	if (copy_from_user(blob->data, blob_ptr, out_resp->length)) {
+		ret = -EFAULT;
+		goto out_blob;
+	}
+
+	/* Dropping the lock between create_blob and our access here is safe
+	 * as only the same file_priv can remove the blob; at this point, it is
+	 * not associated with any file_priv. */
+	mutex_lock(&dev->mode_config.blob_lock);
+	out_resp->blob_id = blob->base.id;
+	list_add_tail(&file_priv->blobs, &blob->head_file);
+	mutex_unlock(&dev->mode_config.blob_lock);
+
+	return 0;
+
+out_blob:
+	drm_property_unreference_blob(blob);
+	return ret;
+}
+
+/**
+ * drm_mode_destroyblob_ioctl - destroy a user blob property
+ * @dev: DRM device
+ * @data: ioctl data
+ * @file_priv: DRM file info
+ *
+ * Destroy an existing user-defined blob property.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv)
+{
+	struct drm_mode_destroy_blob *out_resp = data;
+	struct drm_property_blob *blob = NULL, *bt;
+	bool found = false;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	mutex_lock(&dev->mode_config.blob_lock);
+	blob = __drm_property_lookup_blob(dev, out_resp->blob_id);
+	if (!blob) {
+		ret = -ENOENT;
+		goto err;
+	}
+
+	/* Ensure the property was actually created by this user. */
+	list_for_each_entry(bt, &file_priv->blobs, head_file) {
+		if (bt == blob) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		ret = -EPERM;
+		goto err;
+	}
+
+	/* We must drop head_file here, because we may not be the last
+	 * reference on the blob. */
+	list_del_init(&blob->head_file);
+	drm_property_unreference_blob_locked(blob);
+	mutex_unlock(&dev->mode_config.blob_lock);
+
+	return 0;
+
+err:
+	mutex_unlock(&dev->mode_config.blob_lock);
+	return ret;
+}
+
+/**
  * drm_mode_connector_set_path_property - set tile property on connector
  * @connector: connector to set property on.
  * @path: path to use for property; must not be NULL.
@@ -5659,7 +5792,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
-				 head) {
+				 head_global) {
 		drm_property_unreference_blob(blob);
 	}
 
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 0f6a5c8..c59ce4d 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -167,6 +167,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	INIT_LIST_HEAD(&priv->lhead);
 	INIT_LIST_HEAD(&priv->fbs);
 	mutex_init(&priv->fbs_lock);
+	INIT_LIST_HEAD(&priv->blobs);
 	INIT_LIST_HEAD(&priv->event_list);
 	init_waitqueue_head(&priv->event_wait);
 	priv->event_space = 4096; /* set aside 4k for event buffer */
@@ -405,8 +406,10 @@ int drm_release(struct inode *inode, struct file *filp)
 
 	drm_events_release(file_priv);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_fb_release(file_priv);
+		drm_property_destroy_user_blobs(dev, file_priv);
+	}
 
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, file_priv);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 266dcd6..9bac1b7 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -641,6 +641,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index df6d997..9fa6366 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -326,6 +326,10 @@ struct drm_file {
 	struct list_head fbs;
 	struct mutex fbs_lock;
 
+	/** User-created blob properties; this retains a reference on the
+	 *  property. */
+	struct list_head blobs;
+
 	wait_queue_head_t event_wait;
 	struct list_head event_list;
 	int event_space;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index dace1b6..72b60db 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -218,7 +218,8 @@ struct drm_property_blob {
 	struct drm_mode_object base;
 	struct drm_device *dev;
 	struct kref refcount;
-	struct list_head head;
+	struct list_head head_global;
+	struct list_head head_file;
 	size_t length;
 	unsigned char data[];
 };
@@ -1315,6 +1316,8 @@ extern const char *drm_get_dvi_i_select_name(int val);
 extern const char *drm_get_tv_subconnector_name(int val);
 extern const char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
+extern void drm_property_destroy_user_blobs(struct drm_device *dev,
+                                            struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern void drm_mode_group_destroy(struct drm_mode_group *group);
 extern void drm_reinit_primary_mode_group(struct drm_device *dev);
@@ -1460,6 +1463,10 @@ extern int drm_mode_getproperty_ioctl(struct drm_device *dev,
 				      void *data, struct drm_file *file_priv);
 extern int drm_mode_getblob_ioctl(struct drm_device *dev,
 				  void *data, struct drm_file *file_priv);
+extern int drm_mode_createblob_ioctl(struct drm_device *dev,
+				     void *data, struct drm_file *file_priv);
+extern int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+				      void *data, struct drm_file *file_priv);
 extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
 					      void *data, struct drm_file *file_priv);
 extern int drm_mode_getencoder(struct drm_device *dev,
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index ff6ef62..3801584 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -786,6 +786,8 @@ struct drm_prime_handle {
 #define DRM_IOCTL_MODE_OBJ_SETPROPERTY	DRM_IOWR(0xBA, struct drm_mode_obj_set_property)
 #define DRM_IOCTL_MODE_CURSOR2		DRM_IOWR(0xBB, struct drm_mode_cursor2)
 #define DRM_IOCTL_MODE_ATOMIC		DRM_IOWR(0xBC, struct drm_mode_atomic)
+#define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
+#define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
 
 /**
  * Device specific ioctls should only be in their respective headers
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index dbeba94..359107a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -558,4 +558,24 @@ struct drm_mode_atomic {
 	__u64 user_data;
 };
 
+/**
+ * Create a new 'blob' data property, copying length bytes from data pointer,
+ * and returning new blob ID.
+ */
+struct drm_mode_create_blob {
+	/** Pointer to data to copy. */
+	__u64 data;
+	/** Length of data to copy. */
+	__u32 length;
+	/** Return: new property ID. */
+	__u32 blob_id;
+};
+
+/**
+ * Destroy a user-created blob property.
+ */
+struct drm_mode_destroy_blob {
+	__u32 blob_id;
+};
+
 #endif
-- 
2.4.1

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

[PATCH 09/11] drm: Add drm_atomic_set_mode_for_crtc

[Daniel Stone]

Add a new helper, to be used later for blob property management, that
sets the mode for a CRTC state, as well as updating the CRTC enable/active
state at the same time.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic.c         | 42 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c  | 24 ++++++---------------
 drivers/gpu/drm/drm_crtc_helper.c    |  6 +++---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++-----
 include/drm/drm_atomic.h             |  6 ++++++
 5 files changed, 68 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3134f50..67c251f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -290,6 +290,48 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 
 /**
+ * drm_atomic_set_mode_for_crtc - set mode for CRTC
+ * @state: the CRTC whose incoming state to update
+ * @mode: kernel-internal mode to use for the CRTC, or NULL to disable
+ *
+ * Set a mode (originating from the kernel) on the desired CRTC state, and
+ * update the CRTC state's enable and mode_changed parameters as necessary.
+ * If disabling, it will also set active to false.
+ */
+int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
+				 struct drm_display_mode *mode)
+{
+	/* Early return for no change. */
+	if (!mode && !state->enable)
+		return 0;
+	if (mode && state->enable &&
+	    memcmp(&state->mode, mode, sizeof(*mode)) == 0)
+		return 0;
+
+	state->mode_changed = true;
+
+	if (mode) {
+		drm_mode_copy(&state->mode, mode);
+		state->enable = true;
+	} else {
+		memset(&state->mode, 0, sizeof(state->mode));
+		state->enable = false;
+		state->active = false;
+	}
+
+	if (state->enable)
+		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
+				 mode->name, state);
+	else
+		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
+				 state);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
+
+
+/**
  * drm_atomic_crtc_set_property - set property on CRTC
  * @crtc: the drm CRTC to set a property on
  * @state: the state object to update with the new property value
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a64bacd..70ce243 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -369,20 +369,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	struct drm_connector_state *connector_state;
 	int i, ret;
 
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n",
-					 crtc->base.id);
-			crtc_state->mode_changed = true;
-		}
-
-		if (crtc->state->enable != crtc_state->enable) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
-					 crtc->base.id);
-			crtc_state->mode_changed = true;
-		}
-	}
-
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		/*
 		 * This only sets crtc->mode_changed for routing changes,
@@ -1607,8 +1593,9 @@ retry:
 		WARN_ON(set->fb);
 		WARN_ON(set->num_connectors);
 
-		crtc_state->enable = false;
-		crtc_state->active = false;
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+		if (ret != 0)
+			goto fail;
 
 		ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
 		if (ret != 0)
@@ -1622,9 +1609,10 @@ retry:
 	WARN_ON(!set->fb);
 	WARN_ON(!set->num_connectors);
 
-	crtc_state->enable = true;
+	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
+	if (ret != 0)
+		goto fail;
 	crtc_state->active = true;
-	drm_mode_copy(&crtc_state->mode, set->mode);
 
 	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
 	if (ret != 0)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 9297a61..cc77a4f 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -937,10 +937,10 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 			crtc_state->crtc = crtc;
 	}
 
-	crtc_state->enable = true;
 	crtc_state->planes_changed = true;
-	crtc_state->mode_changed = true;
-	drm_mode_copy(&crtc_state->mode, mode);
+	ret = drm_atomic_set_mode_for_crtc(crtc_state, mode);
+	if (ret)
+		goto out;
 	drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
 
 	if (crtc_funcs->atomic_check) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 240092a..7e31972 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9896,7 +9896,9 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
 
 	if (intel_set_mode(crtc, state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
@@ -12494,8 +12496,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 
 		crtc_state->base.enable = intel_crtc->new_enabled;
 
-		if (&intel_crtc->base == crtc)
-			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
+		if (&intel_crtc->base == crtc) {
+			ret = drm_atomic_set_mode_for_crtc(&crtc_state->base,
+							   &crtc->mode);
+			WARN_ON(ret);
+		}
 	}
 
 	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
@@ -12633,8 +12638,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	if (IS_ERR(crtc_state))
 		return PTR_ERR(crtc_state);
 
-	if (set->mode)
-		drm_mode_copy(&crtc_state->mode, set->mode);
+	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
+	if (ret)
+		return ret;
 
 	if (set->num_connectors)
 		crtc_state->active = true;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e89db0c..55f4604 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -110,6 +110,12 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state,
 }
 
 int __must_check
+drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
+			     struct drm_display_mode *mode);
+int __must_check
+drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
+				  struct drm_property_blob *blob);
+int __must_check
 drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
 void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
-- 
2.4.1

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

Re: [PATCH 09/11] drm: Add drm_atomic_set_mode_for_crtc

[Daniel Vetter]

On Fri, May 22, 2015 at 01:34:52PM +0100, Daniel Stone wrote:
> Add a new helper, to be used later for blob property management, that
> sets the mode for a CRTC state, as well as updating the CRTC enable/active
> state at the same time.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>

A few small nits below.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c         | 42 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c  | 24 ++++++---------------
>  drivers/gpu/drm/drm_crtc_helper.c    |  6 +++---
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++-----
>  include/drm/drm_atomic.h             |  6 ++++++
>  5 files changed, 68 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3134f50..67c251f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -290,6 +290,48 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
>  /**
> + * drm_atomic_set_mode_for_crtc - set mode for CRTC
> + * @state: the CRTC whose incoming state to update
> + * @mode: kernel-internal mode to use for the CRTC, or NULL to disable
> + *
> + * Set a mode (originating from the kernel) on the desired CRTC state, and
> + * update the CRTC state's enable and mode_changed parameters as necessary.
> + * If disabling, it will also set active to false.

Kerneldoc for return values is missing. Fairly important since many atomic
functions can return -EDEADLK, but this one doesn (even not when all the
patches are applied).

> + */
> +int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> +				 struct drm_display_mode *mode)
> +{
> +	/* Early return for no change. */
> +	if (!mode && !state->enable)
> +		return 0;
> +	if (mode && state->enable &&
> +	    memcmp(&state->mode, mode, sizeof(*mode)) == 0)
> +		return 0;
> +
> +	state->mode_changed = true;

Layering violation: ->mode_changed is purely something for the benefit of
drivers/helpers and not something the core manadates. Hence this should
get removed imo.

> +
> +	if (mode) {
> +		drm_mode_copy(&state->mode, mode);
> +		state->enable = true;
> +	} else {
> +		memset(&state->mode, 0, sizeof(state->mode));
> +		state->enable = false;
> +		state->active = false;
> +	}
> +
> +	if (state->enable)
> +		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> +				 mode->name, state);
> +	else
> +		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
> +				 state);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
> +
> +
> +/**
>   * drm_atomic_crtc_set_property - set property on CRTC
>   * @crtc: the drm CRTC to set a property on
>   * @state: the state object to update with the new property value
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a64bacd..70ce243 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -369,20 +369,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	struct drm_connector_state *connector_state;
>  	int i, ret;
>  
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n",
> -					 crtc->base.id);
> -			crtc_state->mode_changed = true;
> -		}
> -
> -		if (crtc->state->enable != crtc_state->enable) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
> -					 crtc->base.id);
> -			crtc_state->mode_changed = true;
> -		}
> -	}

Ofc then this hunk here needs to stay.

> -
>  	for_each_connector_in_state(state, connector, connector_state, i) {
>  		/*
>  		 * This only sets crtc->mode_changed for routing changes,
> @@ -1607,8 +1593,9 @@ retry:
>  		WARN_ON(set->fb);
>  		WARN_ON(set->num_connectors);
>  
> -		crtc_state->enable = false;
> -		crtc_state->active = false;
> +		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> +		if (ret != 0)
> +			goto fail;
>  
>  		ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
>  		if (ret != 0)
> @@ -1622,9 +1609,10 @@ retry:
>  	WARN_ON(!set->fb);
>  	WARN_ON(!set->num_connectors);
>  
> -	crtc_state->enable = true;
> +	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
> +	if (ret != 0)
> +		goto fail;
>  	crtc_state->active = true;
> -	drm_mode_copy(&crtc_state->mode, set->mode);
>  
>  	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
>  	if (ret != 0)
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 9297a61..cc77a4f 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -937,10 +937,10 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>  			crtc_state->crtc = crtc;
>  	}
>  
> -	crtc_state->enable = true;
>  	crtc_state->planes_changed = true;
> -	crtc_state->mode_changed = true;
> -	drm_mode_copy(&crtc_state->mode, mode);
> +	ret = drm_atomic_set_mode_for_crtc(crtc_state, mode);
> +	if (ret)
> +		goto out;
>  	drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
>  
>  	if (crtc_funcs->atomic_check) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 240092a..7e31972 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9896,7 +9896,9 @@ retry:
>  	if (ret)
>  		goto fail;
>  
> -	drm_mode_copy(&crtc_state->base.mode, mode);
> +	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
> +	if (ret)
> +		goto fail;
>  
>  	if (intel_set_mode(crtc, state)) {
>  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
> @@ -12494,8 +12496,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  
>  		crtc_state->base.enable = intel_crtc->new_enabled;
>  
> -		if (&intel_crtc->base == crtc)
> -			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> +		if (&intel_crtc->base == crtc) {
> +			ret = drm_atomic_set_mode_for_crtc(&crtc_state->base,
> +							   &crtc->mode);
> +			WARN_ON(ret);
> +		}
>  	}
>  
>  	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
> @@ -12633,8 +12638,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	if (IS_ERR(crtc_state))
>  		return PTR_ERR(crtc_state);
>  
> -	if (set->mode)
> -		drm_mode_copy(&crtc_state->mode, set->mode);
> +	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
> +	if (ret)
> +		return ret;
>  
>  	if (set->num_connectors)
>  		crtc_state->active = true;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index e89db0c..55f4604 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -110,6 +110,12 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state,
>  }
>  
>  int __must_check
> +drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> +			     struct drm_display_mode *mode);
> +int __must_check
> +drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
> +				  struct drm_property_blob *blob);
> +int __must_check
>  drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  			      struct drm_crtc *crtc);
>  void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> -- 
> 2.4.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/11] drm/atomic: Add current-mode blob to CRTC state

[Daniel Stone]

Add a blob property tracking the current mode to the CRTC state, and
ensure it is properly updated and referenced.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic.c        | 62 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c | 11 ++++---
 drivers/gpu/drm/drm_crtc.c          |  3 +-
 include/drm/drm_crtc.h              |  3 ++
 4 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 67c251f..63fc58a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -301,6 +301,8 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 				 struct drm_display_mode *mode)
 {
+	struct drm_mode_modeinfo umode;
+
 	/* Early return for no change. */
 	if (!mode && !state->enable)
 		return 0;
@@ -308,9 +310,20 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 	    memcmp(&state->mode, mode, sizeof(*mode)) == 0)
 		return 0;
 
+	if (state->mode_blob)
+		drm_property_unreference_blob(state->mode_blob);
+	state->mode_blob = NULL;
 	state->mode_changed = true;
 
 	if (mode) {
+		drm_mode_convert_to_umode(&umode, mode);
+		state->mode_blob =
+			drm_property_create_blob(state->crtc->dev,
+		                                 sizeof(umode),
+		                                 &umode);
+		if (!state->mode_blob)
+			return -ENOMEM;
+
 		drm_mode_copy(&state->mode, mode);
 		state->enable = true;
 	} else {
@@ -330,6 +343,55 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
 
+/**
+ * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
+ * @state: the CRTC whose incoming state to update
+ * @blob: pointer to blob property to use for mode
+ *
+ * Set a mode (originating from a blob property) on the desired CRTC state.
+ * This function will take a reference on the blob property for the CRTC state,
+ * and release the reference held on the state's existing mode property, if any
+ * was set.
+ */
+int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
+                                      struct drm_property_blob *blob)
+{
+	if (blob == state->mode_blob)
+		return 0;
+
+	if (state->mode_blob)
+		drm_property_unreference_blob(state->mode_blob);
+	state->mode_blob = NULL;
+
+	if (blob) {
+		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
+		    drm_mode_convert_umode(&state->mode,
+		                           (const struct drm_mode_modeinfo *)
+		                            blob->data))
+			return -EINVAL;
+
+		state->mode_blob = drm_property_reference_blob(blob);
+		state->mode_changed = true;
+		state->enable = true;
+	} else {
+		memset(&state->mode, 0, sizeof(state->mode));
+
+		if (state->enable)
+			state->mode_changed = true;
+		state->enable = false;
+		state->active = false;
+	}
+
+	if (state->enable)
+		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
+				 state->mode.name, state);
+	else
+		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
+				 state);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
 
 /**
  * drm_atomic_crtc_set_property - set property on CRTC
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 70ce243..5d596d9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2027,6 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
  */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 {
+	if (crtc->state && crtc->state->mode_blob)
+		drm_property_unreference_blob(crtc->state->mode_blob);
 	kfree(crtc->state);
 	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
 
@@ -2048,6 +2050,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 {
 	memcpy(state, crtc->state, sizeof(*state));
 
+	if (state->mode_blob)
+		drm_property_reference_blob(state->mode_blob);
 	state->mode_changed = false;
 	state->active_changed = false;
 	state->planes_changed = false;
@@ -2090,11 +2094,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
 void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					    struct drm_crtc_state *state)
 {
-	/*
-	 * This is currently a placeholder so that drivers that subclass the
-	 * state will automatically do the right thing if code is ever added
-	 * to this function.
-	 */
+	if (state->mode_blob)
+		drm_property_unreference_blob(state->mode_blob);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9263b22..ee87045 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1961,7 +1961,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->x = crtc->primary->state->src_x >> 16;
 		crtc_resp->y = crtc->primary->state->src_y >> 16;
 		if (crtc->state->enable) {
-			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
+			memcpy(&crtc_resp->mode, &crtc->state->mode_blob->data,
+			       sizeof(crtc_resp->mode));
 			crtc_resp->mode_valid = 1;
 
 		} else {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 72b60db..c54fa4a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -299,6 +299,9 @@ struct drm_crtc_state {
 
 	struct drm_display_mode mode;
 
+	/* blob property to expose current mode to atomic userspace */
+	struct drm_property_blob *mode_blob;
+
 	struct drm_pending_vblank_event *event;
 
 	struct drm_atomic_state *state;
-- 
2.4.1

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

Re: [PATCH 10/11] drm/atomic: Add current-mode blob to CRTC state

[Daniel Vetter]

On Fri, May 22, 2015 at 01:34:53PM +0100, Daniel Stone wrote:
> Add a blob property tracking the current mode to the CRTC state, and
> ensure it is properly updated and referenced.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 62 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c | 11 ++++---
>  drivers/gpu/drm/drm_crtc.c          |  3 +-
>  include/drm/drm_crtc.h              |  3 ++
>  4 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 67c251f..63fc58a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -301,6 +301,8 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  				 struct drm_display_mode *mode)
>  {
> +	struct drm_mode_modeinfo umode;
> +
>  	/* Early return for no change. */
>  	if (!mode && !state->enable)
>  		return 0;
> @@ -308,9 +310,20 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  	    memcmp(&state->mode, mode, sizeof(*mode)) == 0)
>  		return 0;
>  
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
> +	state->mode_blob = NULL;
>  	state->mode_changed = true;
>  
>  	if (mode) {
> +		drm_mode_convert_to_umode(&umode, mode);
> +		state->mode_blob =
> +			drm_property_create_blob(state->crtc->dev,
> +		                                 sizeof(umode),
> +		                                 &umode);
> +		if (!state->mode_blob)
> +			return -ENOMEM;

IS_ERR and PTR_ERR? Dan Carpenter will catch this as soon as I'll apply
;-)

> +
>  		drm_mode_copy(&state->mode, mode);
>  		state->enable = true;
>  	} else {
> @@ -330,6 +343,55 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
>  
> +/**
> + * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
> + * @state: the CRTC whose incoming state to update
> + * @blob: pointer to blob property to use for mode
> + *
> + * Set a mode (originating from a blob property) on the desired CRTC state.
> + * This function will take a reference on the blob property for the CRTC state,
> + * and release the reference held on the state's existing mode property, if any
> + * was set.
> + */
> +int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
> +                                      struct drm_property_blob *blob)
> +{
> +	if (blob == state->mode_blob)
> +		return 0;
> +
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
> +	state->mode_blob = NULL;
> +
> +	if (blob) {
> +		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> +		    drm_mode_convert_umode(&state->mode,
> +		                           (const struct drm_mode_modeinfo *)
> +		                            blob->data))
> +			return -EINVAL;
> +
> +		state->mode_blob = drm_property_reference_blob(blob);
> +		state->mode_changed = true;
> +		state->enable = true;
> +	} else {
> +		memset(&state->mode, 0, sizeof(state->mode));
> +
> +		if (state->enable)
> +			state->mode_changed = true;

Same comment as in the previous patch: mode_changed isnt' the core atomic
code's business.

> +		state->enable = false;
> +		state->active = false;
> +	}
> +
> +	if (state->enable)
> +		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> +				 state->mode.name, state);
> +	else
> +		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
> +				 state);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
>  
>  /**
>   * drm_atomic_crtc_set_property - set property on CRTC
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 70ce243..5d596d9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2027,6 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
>   */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>  {
> +	if (crtc->state && crtc->state->mode_blob)
> +		drm_property_unreference_blob(crtc->state->mode_blob);
>  	kfree(crtc->state);
>  	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
>  
> @@ -2048,6 +2050,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  {
>  	memcpy(state, crtc->state, sizeof(*state));
>  
> +	if (state->mode_blob)
> +		drm_property_reference_blob(state->mode_blob);
>  	state->mode_changed = false;
>  	state->active_changed = false;
>  	state->planes_changed = false;
> @@ -2090,11 +2094,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>  					    struct drm_crtc_state *state)
>  {
> -	/*
> -	 * This is currently a placeholder so that drivers that subclass the
> -	 * state will automatically do the right thing if code is ever added
> -	 * to this function.
> -	 */
> +	if (state->mode_blob)
> +		drm_property_unreference_blob(state->mode_blob);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9263b22..ee87045 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1961,7 +1961,8 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->x = crtc->primary->state->src_x >> 16;
>  		crtc_resp->y = crtc->primary->state->src_y >> 16;
>  		if (crtc->state->enable) {
> -			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> +			memcpy(&crtc_resp->mode, &crtc->state->mode_blob->data,
> +			       sizeof(crtc_resp->mode));

Why this change? While transitioning I'd expect that some driver (probably
the one with hw state readout) will get this wrong, imo safer to keep
looking at crtc->state->mode.

lgtm otherwise.
-Daniel

>  			crtc_resp->mode_valid = 1;
>  
>  		} else {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 72b60db..c54fa4a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -299,6 +299,9 @@ struct drm_crtc_state {
>  
>  	struct drm_display_mode mode;
>  
> +	/* blob property to expose current mode to atomic userspace */
> +	struct drm_property_blob *mode_blob;
> +
>  	struct drm_pending_vblank_event *event;
>  
>  	struct drm_atomic_state *state;
> -- 
> 2.4.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Stone]

Atomic modesetting: now with modesetting support.

Signed-off-by: Daniel Stone <daniels@collabora.com>
Tested-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_atomic.c | 12 +++++++++++-
 drivers/gpu/drm/drm_crtc.c   |  7 +++++++
 include/drm/drm_crtc.h       |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 63fc58a..a4ab03a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_mode_config *config = &dev->mode_config;
+	int ret;
 
-	/* FIXME: Mode prop is missing, which also controls ->enable. */
 	if (property == config->prop_active)
 		state->active = val;
+	else if (property == config->prop_mode_id) {
+		struct drm_property_blob *mode =
+			drm_property_lookup_blob(dev, val);
+		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
+		if (mode)
+			drm_property_unreference_blob(mode);
+		return ret;
+	}
 	else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	else
@@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 
 	if (property == config->prop_active)
 		*val = state->active;
+	else if (property == config->prop_mode_id)
+		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ee87045..8f18412 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 
 	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
 		drm_object_attach_property(&crtc->base, config->prop_active, 0);
+		drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);
 	}
 
 	return 0;
@@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_active = prop;
 
+	prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
+			"MODE_ID", DRM_MODE_OBJECT_BLOB);
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_mode_id = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c54fa4a..3b4d8a4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1146,6 +1146,7 @@ struct drm_mode_config {
 	struct drm_property *prop_fb_id;
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
+	struct drm_property *prop_mode_id;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
-- 
2.4.1

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

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

[Daniel Vetter]

On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
> Atomic modesetting: now with modesetting support.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> Tested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_atomic.c | 12 +++++++++++-
>  drivers/gpu/drm/drm_crtc.c   |  7 +++++++
>  include/drm/drm_crtc.h       |  1 +
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 63fc58a..a4ab03a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	int ret;
>  
> -	/* FIXME: Mode prop is missing, which also controls ->enable. */
>  	if (property == config->prop_active)
>  		state->active = val;
> +	else if (property == config->prop_mode_id) {
> +		struct drm_property_blob *mode =
> +			drm_property_lookup_blob(dev, val);
> +		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> +		if (mode)
> +			drm_property_unreference_blob(mode);

Hm, maybe I need to revisit whether auto-clamping ->active is a good idea.
We need it for legacy helpers, but for atomic userspace this code means
depending upon whether active or mode_id is first in the prop list it will
get clamped or not, which isn't awesome.

Imo that's a good reason to remove the ->active clamping from
set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since
that's only used internally and in legacy paths. Perhaps with a comment as
to why (and why not in set_mode_prop).

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

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

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Stone]

Hi,

On 22 May 2015 at 15:34, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
>> -     /* FIXME: Mode prop is missing, which also controls ->enable. */
>>       if (property == config->prop_active)
>>               state->active = val;
>> +     else if (property == config->prop_mode_id) {
>> +             struct drm_property_blob *mode =
>> +                     drm_property_lookup_blob(dev, val);
>> +             ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>> +             if (mode)
>> +                     drm_property_unreference_blob(mode);
>
> Hm, maybe I need to revisit whether auto-clamping ->active is a good idea.
> We need it for legacy helpers, but for atomic userspace this code means
> depending upon whether active or mode_id is first in the prop list it will
> get clamped or not, which isn't awesome.
>
> Imo that's a good reason to remove the ->active clamping from
> set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since
> that's only used internally and in legacy paths. Perhaps with a comment as
> to why (and why not in set_mode_prop).

No argument to not touching mode_changed, but I'm a bit confused about this one.

We don't touch ->active when setting a mode (i.e. if active is true
and you change MODE_ID without changing the ACTIVE prop, active
remains true; if active is false and you change MODE_ID, active
remains false, but it gains a mode).

I've been working on the following assumption:
  - enable is a proxy for having a valid mode (enable == !!MODE_ID)
  - active cannot be true without enable also being true

Setting MODE_ID to 0 removes the current mode, and when it becomes 0,
we can no longer report back a mode that we're scanning out. So how
would we have active == true (a particular mode is enabled and being
displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in
the same request is a broken configuration which should be rejected.
Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not
only pretty pathological but impossible with current libdrm, but
you're right that it should be respected.

So, I guess the way forward would be to not clamp either active or
enable, check that the dependencies (active -> enable -> MODE_ID) are
satisfied in drm_atomic_helper_check, and hope that everyone
implementing their own check gets it right too.

Sound good?

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

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

[Daniel Vetter]

On Mon, May 25, 2015 at 02:06:13PM +0100, Daniel Stone wrote:
> On 22 May 2015 at 15:34, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
> >> -     /* FIXME: Mode prop is missing, which also controls ->enable. */
> >>       if (property == config->prop_active)
> >>               state->active = val;
> >> +     else if (property == config->prop_mode_id) {
> >> +             struct drm_property_blob *mode =
> >> +                     drm_property_lookup_blob(dev, val);
> >> +             ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >> +             if (mode)
> >> +                     drm_property_unreference_blob(mode);
> >
> > Hm, maybe I need to revisit whether auto-clamping ->active is a good idea.
> > We need it for legacy helpers, but for atomic userspace this code means
> > depending upon whether active or mode_id is first in the prop list it will
> > get clamped or not, which isn't awesome.
> >
> > Imo that's a good reason to remove the ->active clamping from
> > set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since
> > that's only used internally and in legacy paths. Perhaps with a comment as
> > to why (and why not in set_mode_prop).
> 
> No argument to not touching mode_changed, but I'm a bit confused about this one.
> 
> We don't touch ->active when setting a mode (i.e. if active is true
> and you change MODE_ID without changing the ACTIVE prop, active
> remains true; if active is false and you change MODE_ID, active
> remains false, but it gains a mode).
> 
> I've been working on the following assumption:
>   - enable is a proxy for having a valid mode (enable == !!MODE_ID)
>   - active cannot be true without enable also being true
> 
> Setting MODE_ID to 0 removes the current mode, and when it becomes 0,
> we can no longer report back a mode that we're scanning out. So how
> would we have active == true (a particular mode is enabled and being
> displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in
> the same request is a broken configuration which should be rejected.
> Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not
> only pretty pathological but impossible with current libdrm, but
> you're right that it should be respected.
> 
> So, I guess the way forward would be to not clamp either active or
> enable, check that the dependencies (active -> enable -> MODE_ID) are
> satisfied in drm_atomic_helper_check, and hope that everyone
> implementing their own check gets it right too.

Just to summarize the irc discussion:
- ->enable is a derived state and should == !!MODE_ID
- ->active is it's own independent atomic property, and as a rule we
  shouldn't try to "intelligently" patch up what userspace passes in, but
  instead just reject invalid configurations. Hence no auto-clamping for
  active, but proper computation of enable
- There's no risk that some drivers will get the active vs. enable checks
  wrong, they're core drm code: Everything in drm_atomic.c is non-optional
  (and that's the hunk where you've removed the check). Maybe that was
  part of the confusion, since you're description above sounds like you
  put this in the helper library?

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/11] CRTC state mode property

[Daniel Vetter]

On Fri, May 22, 2015 at 01:34:43PM +0100, Daniel Stone wrote:
> Hi,
> Building on the blob-property submission series[0], this provides a
> MODE_ID property that can be used to set the current mode for a CRTC
> under atomic modesetting, plus a way for users to create blob
> properties so they can specify their own modes.
> 
> Using this[1] I've been able to validate the atomic userspace API with
> a ported version of Weston[2]. This is currently undergoing review, but
> seems pretty OK. Both modesetting for every output together, and
> per-output async pageflips with events, works as well as the driver
> does.
> 
> Also tested with fbcon and X11, on Broadwell.

A few nitpicks on some patches, all the others merged to topic/drm-misc.
-Daniel

> 
> Cheers,
> Daniel
> 
> [0]: <1429554176-9865-1-git-send-email-daniels@collabora.com>
> [1]: And a pile of other work in the i915 driver from Maarten/Ander, and
>      my own bits on top; see
>      git://git.collabora.co.uk/git/user/daniels/linux in the
>      wip/4.1.x/blob-modes-i915 branch.
> [2]: http://lists.freedesktop.org/archives/wayland-devel/2015-May/022106.html
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel