[PATCH 1/3] drm/lease: debug output for lease creation

[PATCH 1/3] drm/lease: debug output for lease creation

[Daniel Vetter]

I spent a bit of time scratching heads and figuring out why the igts
don't work. Probably useful to keep this work.

Cc: Keith Packard <keithp@keithp.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_lease.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 9ab88db8fad0..46408278c5b1 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -419,14 +419,17 @@ static int fill_object_idr(struct drm_device *dev,
 		}
 
 		if (!drm_mode_object_lease_required(objects[o]->type)) {
+			DRM_DEBUG_KMS("invalid object for lease\n");
 			ret = -EINVAL;
 			goto out_free_objects;
 		}
 	}
 
 	ret = validate_lease(dev, lessor_priv, object_count, objects);
-	if (ret)
+	if (ret) {
+		DRM_DEBUG_LEASE("lease validation failed\n");
 		goto out_free_objects;
+	}
 
 	/* add their IDs to the lease request - taking into account
 	   universal planes */
@@ -509,15 +512,21 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 		return -EOPNOTSUPP;
 
 	/* Do not allow sub-leases */
-	if (lessor->lessor)
+	if (lessor->lessor) {
+		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
 		return -EINVAL;
+	}
 
 	/* need some objects */
-	if (cl->object_count == 0)
+	if (cl->object_count == 0) {
+		DRM_DEBUG_LEASE("no objects in lease\n");
 		return -EINVAL;
+	}
 
-	if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK)))
+	if (cl->flags && (cl->flags & ~(O_CLOEXEC | O_NONBLOCK))) {
+		DRM_DEBUG_LEASE("invalid flags\n");
 		return -EINVAL;
+	}
 
 	object_count = cl->object_count;
 
@@ -532,6 +541,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 			      object_count, object_ids);
 	kfree(object_ids);
 	if (ret) {
+		DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret);
 		idr_destroy(&leases);
 		return ret;
 	}
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/file: Uncompact the feature flags

[Daniel Vetter]

This essentially undoes

commit 39868bd7668bd47308b1dfd97c212757caee764f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Oct 29 08:55:58 2013 +0000

    drm: Compact booleans within struct drm_file

We do lockless access to these flags everywhere, and it's kinda not a
great idea to mix lockless and bitfields. Aside from that gcc isn't
generating great code for these.

If this ever becomes an issue size-wise, I think we need atomic_t here
and atomic bitflag ops.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_file.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 26485acc51d7..84ac79219e4c 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -164,14 +164,14 @@ struct drm_file {
 	 * See also the :ref:`section on primary nodes and authentication
 	 * <drm_primary_node>`.
 	 */
-	unsigned authenticated :1;
+	bool authenticated;
 
 	/**
 	 * @stereo_allowed:
 	 *
 	 * True when the client has asked us to expose stereo 3D mode flags.
 	 */
-	unsigned stereo_allowed :1;
+	bool stereo_allowed;
 
 	/**
 	 * @universal_planes:
@@ -179,10 +179,10 @@ struct drm_file {
 	 * True if client understands CRTC primary planes and cursor planes
 	 * in the plane list. Automatically set when @atomic is set.
 	 */
-	unsigned universal_planes:1;
+	bool universal_planes;
 
 	/** @atomic: True if client understands atomic properties. */
-	unsigned atomic:1;
+	bool atomic;
 
 	/**
 	 * @aspect_ratio_allowed:
@@ -190,14 +190,14 @@ struct drm_file {
 	 * True, if client can handle picture aspect ratios, and has requested
 	 * to pass this information along with the mode.
 	 */
-	unsigned aspect_ratio_allowed:1;
+	bool aspect_ratio_allowed;
 
 	/**
 	 * @writeback_connectors:
 	 *
 	 * True if client understands writeback connectors
 	 */
-	unsigned writeback_connectors:1;
+	bool writeback_connectors;
 
 	/**
 	 * @is_master:
@@ -208,7 +208,7 @@ struct drm_file {
 	 * See also the :ref:`section on primary nodes and authentication
 	 * <drm_primary_node>`.
 	 */
-	unsigned is_master:1;
+	bool is_master;
 
 	/**
 	 * @master:
-- 
2.14.4

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

[PATCH 3/3] drm/lease: look at ->universal_planes only once

[Daniel Vetter]

It's lockless, and userspace might chance it underneath us. That's not
really a problem, all userspace gets is a slightly dysfunctional
lease with the current code. But this might change, and gcc might
decide to reload a few too many times, and then boom. So better safe
than sorry.

Cc: Keith Packard <keithp@keithp.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_lease.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 46408278c5b1..ce49d1919214 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -359,7 +359,8 @@ void drm_lease_revoke(struct drm_master *top)
 static int validate_lease(struct drm_device *dev,
 			  struct drm_file *lessor_priv,
 			  int object_count,
-			  struct drm_mode_object **objects)
+			  struct drm_mode_object **objects,
+			  bool universal_planes)
 {
 	int o;
 	int has_crtc = -1;
@@ -376,14 +377,14 @@ static int validate_lease(struct drm_device *dev,
 		if (objects[o]->type == DRM_MODE_OBJECT_CONNECTOR && has_connector == -1)
 			has_connector = o;
 
-		if (lessor_priv->universal_planes) {
+		if (universal_planes) {
 			if (objects[o]->type == DRM_MODE_OBJECT_PLANE && has_plane == -1)
 				has_plane = o;
 		}
 	}
 	if (has_crtc == -1 || has_connector == -1)
 		return -EINVAL;
-	if (lessor_priv->universal_planes && has_plane == -1)
+	if (universal_planes && has_plane == -1)
 		return -EINVAL;
 	return 0;
 }
@@ -397,6 +398,8 @@ static int fill_object_idr(struct drm_device *dev,
 	struct drm_mode_object **objects;
 	u32 o;
 	int ret;
+	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
+
 	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
 			  GFP_KERNEL);
 	if (!objects)
@@ -425,7 +428,8 @@ static int fill_object_idr(struct drm_device *dev,
 		}
 	}
 
-	ret = validate_lease(dev, lessor_priv, object_count, objects);
+	ret = validate_lease(dev, lessor_priv, object_count, objects,
+			     universal_planes);
 	if (ret) {
 		DRM_DEBUG_LEASE("lease validation failed\n");
 		goto out_free_objects;
@@ -452,7 +456,7 @@ static int fill_object_idr(struct drm_device *dev,
 					object_id, ret);
 			goto out_free_objects;
 		}
-		if (obj->type == DRM_MODE_OBJECT_CRTC && !lessor_priv->universal_planes) {
+		if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
 			struct drm_crtc *crtc = obj_to_crtc(obj);
 			ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL);
 			if (ret < 0) {
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/lease: debug output for lease creation

[Keith Packard]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> I spent a bit of time scratching heads and figuring out why the igts
> don't work. Probably useful to keep this work.

Acked-by: Keith Packard <keithp@keithp.com>

>  	/* Do not allow sub-leases */
> -	if (lessor->lessor)
> +	if (lessor->lessor) {
> +		DRM_DEBUG_LEASE("recursive leasing not allowed\n");
>  		return -EINVAL;
> +	}

(not review about this patch, just noticing that the relevant code is here:-)

We may want to revisit this restriction as it looks like multi-seat may
end up using leases from a manager to multiple X servers and letting
those X servers support leases would be a good thing.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/lease: look at ->universal_planes only once

[Keith Packard]

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

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> @@ -359,7 +359,8 @@ void drm_lease_revoke(struct drm_master *top)
>  static int validate_lease(struct drm_device *dev,
>  			  struct drm_file *lessor_priv,
>  			  int object_count,
> -			  struct drm_mode_object **objects)
> +			  struct drm_mode_object **objects,
> +			  bool universal_planes)

It looks like you can remove the lessor_priv argument here, which would
ensure that we didn't reference any fields in it in the future.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 2/3] drm/file: Uncompact the feature flags

[Chris Wilson]

Quoting Daniel Vetter (2018-11-02 13:25:42)
> This essentially undoes
> 
> commit 39868bd7668bd47308b1dfd97c212757caee764f
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Oct 29 08:55:58 2013 +0000
> 
>     drm: Compact booleans within struct drm_file
> 
> We do lockless access to these flags everywhere, and it's kinda not a
> great idea to mix lockless and bitfields. Aside from that gcc isn't
> generating great code for these.
> 
> If this ever becomes an issue size-wise, I think we need atomic_t here
> and atomic bitflag ops.

(you don't need atomic_t to use atomic bitflag ops, more so since the
bit ops work on unsigned long!)

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

A bit of a nuisance since bools are not well specified in terms of
alignment and size, so the growth is as bad as you would expect, and if
atomic access to these individual members was strictly required we
would already be up the proverbial creek.

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/lease: look at ->universal_planes only once

[Daniel Vetter]

It's lockless, and userspace might chance it underneath us. That's not
really a problem, all userspace gets is a slightly dysfunctional
lease with the current code. But this might change, and gcc might
decide to reload a few too many times, and then boom. So better safe
than sorry.

v2: Remove the now unused lessor_priv argument from validate_lease()
(Keith).

Cc: Keith Packard <keithp@keithp.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_lease.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 3b0342a45ae9..739d3830173f 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -353,9 +353,9 @@ void drm_lease_revoke(struct drm_master *top)
 }
 
 static int validate_lease(struct drm_device *dev,
-			  struct drm_file *lessor_priv,
 			  int object_count,
-			  struct drm_mode_object **objects)
+			  struct drm_mode_object **objects,
+			  bool universal_planes)
 {
 	int o;
 	int has_crtc = -1;
@@ -372,14 +372,14 @@ static int validate_lease(struct drm_device *dev,
 		if (objects[o]->type == DRM_MODE_OBJECT_CONNECTOR && has_connector == -1)
 			has_connector = o;
 
-		if (lessor_priv->universal_planes) {
+		if (universal_planes) {
 			if (objects[o]->type == DRM_MODE_OBJECT_PLANE && has_plane == -1)
 				has_plane = o;
 		}
 	}
 	if (has_crtc == -1 || has_connector == -1)
 		return -EINVAL;
-	if (lessor_priv->universal_planes && has_plane == -1)
+	if (universal_planes && has_plane == -1)
 		return -EINVAL;
 	return 0;
 }
@@ -393,6 +393,8 @@ static int fill_object_idr(struct drm_device *dev,
 	struct drm_mode_object **objects;
 	u32 o;
 	int ret;
+	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
+
 	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
 			  GFP_KERNEL);
 	if (!objects)
@@ -421,7 +423,8 @@ static int fill_object_idr(struct drm_device *dev,
 		}
 	}
 
-	ret = validate_lease(dev, lessor_priv, object_count, objects);
+	ret = validate_lease(dev, lessor_priv, object_count, objects,
+			     universal_planes);
 	if (ret) {
 		DRM_DEBUG_LEASE("lease validation failed\n");
 		goto out_free_objects;
@@ -448,7 +451,7 @@ static int fill_object_idr(struct drm_device *dev,
 					object_id, ret);
 			goto out_free_objects;
 		}
-		if (obj->type == DRM_MODE_OBJECT_CRTC && !lessor_priv->universal_planes) {
+		if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
 			struct drm_crtc *crtc = obj_to_crtc(obj);
 			ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL);
 			if (ret < 0) {
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/lease: look at ->universal_planes only once

[Daniel Vetter]

It's lockless, and userspace might chance it underneath us. That's not
really a problem, all userspace gets is a slightly dysfunctional
lease with the current code. But this might change, and gcc might
decide to reload a few too many times, and then boom. So better safe
than sorry.

v2: Remove the now unused lessor_priv argument from validate_lease()
(Keith).

v3: Actually add everything ... silly me.

Cc: Keith Packard <keithp@keithp.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_lease.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 3b0342a45ae9..3650d3c46718 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -353,9 +353,9 @@ void drm_lease_revoke(struct drm_master *top)
 }
 
 static int validate_lease(struct drm_device *dev,
-			  struct drm_file *lessor_priv,
 			  int object_count,
-			  struct drm_mode_object **objects)
+			  struct drm_mode_object **objects,
+			  bool universal_planes)
 {
 	int o;
 	int has_crtc = -1;
@@ -372,14 +372,14 @@ static int validate_lease(struct drm_device *dev,
 		if (objects[o]->type == DRM_MODE_OBJECT_CONNECTOR && has_connector == -1)
 			has_connector = o;
 
-		if (lessor_priv->universal_planes) {
+		if (universal_planes) {
 			if (objects[o]->type == DRM_MODE_OBJECT_PLANE && has_plane == -1)
 				has_plane = o;
 		}
 	}
 	if (has_crtc == -1 || has_connector == -1)
 		return -EINVAL;
-	if (lessor_priv->universal_planes && has_plane == -1)
+	if (universal_planes && has_plane == -1)
 		return -EINVAL;
 	return 0;
 }
@@ -393,6 +393,8 @@ static int fill_object_idr(struct drm_device *dev,
 	struct drm_mode_object **objects;
 	u32 o;
 	int ret;
+	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
+
 	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
 			  GFP_KERNEL);
 	if (!objects)
@@ -421,7 +423,7 @@ static int fill_object_idr(struct drm_device *dev,
 		}
 	}
 
-	ret = validate_lease(dev, lessor_priv, object_count, objects);
+	ret = validate_lease(dev, object_count, objects, universal_planes);
 	if (ret) {
 		DRM_DEBUG_LEASE("lease validation failed\n");
 		goto out_free_objects;
@@ -448,7 +450,7 @@ static int fill_object_idr(struct drm_device *dev,
 					object_id, ret);
 			goto out_free_objects;
 		}
-		if (obj->type == DRM_MODE_OBJECT_CRTC && !lessor_priv->universal_planes) {
+		if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
 			struct drm_crtc *crtc = obj_to_crtc(obj);
 			ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL);
 			if (ret < 0) {
-- 
2.14.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/lease: look at ->universal_planes only once

[Daniel Vetter]

On Mon, Nov 05, 2018 at 11:12:11AM +0100, Daniel Vetter wrote:
> It's lockless, and userspace might chance it underneath us. That's not
> really a problem, all userspace gets is a slightly dysfunctional
> lease with the current code. But this might change, and gcc might
> decide to reload a few too many times, and then boom. So better safe
> than sorry.
> 
> v2: Remove the now unused lessor_priv argument from validate_lease()
> (Keith).
> 
> v3: Actually add everything ... silly me.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Dave Airlie <airlied@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

All pulled in with CI approving and Keith's irc-ack on this one here.
-Daniel

> ---
>  drivers/gpu/drm/drm_lease.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 3b0342a45ae9..3650d3c46718 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -353,9 +353,9 @@ void drm_lease_revoke(struct drm_master *top)
>  }
>  
>  static int validate_lease(struct drm_device *dev,
> -			  struct drm_file *lessor_priv,
>  			  int object_count,
> -			  struct drm_mode_object **objects)
> +			  struct drm_mode_object **objects,
> +			  bool universal_planes)
>  {
>  	int o;
>  	int has_crtc = -1;
> @@ -372,14 +372,14 @@ static int validate_lease(struct drm_device *dev,
>  		if (objects[o]->type == DRM_MODE_OBJECT_CONNECTOR && has_connector == -1)
>  			has_connector = o;
>  
> -		if (lessor_priv->universal_planes) {
> +		if (universal_planes) {
>  			if (objects[o]->type == DRM_MODE_OBJECT_PLANE && has_plane == -1)
>  				has_plane = o;
>  		}
>  	}
>  	if (has_crtc == -1 || has_connector == -1)
>  		return -EINVAL;
> -	if (lessor_priv->universal_planes && has_plane == -1)
> +	if (universal_planes && has_plane == -1)
>  		return -EINVAL;
>  	return 0;
>  }
> @@ -393,6 +393,8 @@ static int fill_object_idr(struct drm_device *dev,
>  	struct drm_mode_object **objects;
>  	u32 o;
>  	int ret;
> +	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
> +
>  	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
>  			  GFP_KERNEL);
>  	if (!objects)
> @@ -421,7 +423,7 @@ static int fill_object_idr(struct drm_device *dev,
>  		}
>  	}
>  
> -	ret = validate_lease(dev, lessor_priv, object_count, objects);
> +	ret = validate_lease(dev, object_count, objects, universal_planes);
>  	if (ret) {
>  		DRM_DEBUG_LEASE("lease validation failed\n");
>  		goto out_free_objects;
> @@ -448,7 +450,7 @@ static int fill_object_idr(struct drm_device *dev,
>  					object_id, ret);
>  			goto out_free_objects;
>  		}
> -		if (obj->type == DRM_MODE_OBJECT_CRTC && !lessor_priv->universal_planes) {
> +		if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
>  			struct drm_crtc *crtc = obj_to_crtc(obj);
>  			ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL);
>  			if (ret < 0) {
> -- 
> 2.14.4
> 

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