[PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

No one looks at the major/minor versions except the unique/busid
stuff. If we protect that with the master_mutex (since it also affects
the unique of each master, oh well) we can mark these two IOCTL with
DRM_UNLOCKED.

While doing this I realized that the comment for the magic_map is
outdated, I've forgotten to update it in:

commit d2b34ee62b409a03c6fe43c07b779983be51d017
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jun 17 09:33:21 2016 +0200

    drm: Protect authmagic with master_mutex

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 12 +++++++++---
 include/drm/drm_auth.h      | 17 +++++++++++++----
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 17e941a533bd..42a17ea5d801 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -115,11 +115,15 @@ static int drm_getunique(struct drm_device *dev, void *data,
 	struct drm_unique *u = data;
 	struct drm_master *master = file_priv->master;
 
+	mutex_lock(&master->dev->master_mutex);
 	if (u->unique_len >= master->unique_len) {
-		if (copy_to_user(u->unique, master->unique, master->unique_len))
+		if (copy_to_user(u->unique, master->unique, master->unique_len)) {
+			mutex_unlock(&master->dev->master_mutex);
 			return -EFAULT;
+		}
 	}
 	u->unique_len = master->unique_len;
+	mutex_unlock(&master->dev->master_mutex);
 
 	return 0;
 }
@@ -340,6 +344,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	struct drm_set_version *sv = data;
 	int if_version, retcode = 0;
 
+	mutex_lock(&dev->master_mutex);
 	if (sv->drm_di_major != -1) {
 		if (sv->drm_di_major != DRM_IF_MAJOR ||
 		    sv->drm_di_minor < 0 || sv->drm_di_minor > DRM_IF_MINOR) {
@@ -374,6 +379,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 	sv->drm_di_minor = DRM_IF_MINOR;
 	sv->drm_dd_major = dev->driver->major;
 	sv->drm_dd_minor = dev->driver->minor;
+	mutex_unlock(&dev->master_mutex);
 
 	return retcode;
 }
@@ -528,7 +534,7 @@ EXPORT_SYMBOL(drm_ioctl_permit);
 static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW|DRM_CONTROL_ALLOW),
-	DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0),
+	DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_MAGIC, drm_getmagic, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_IRQ_BUSID, drm_irq_by_busid, DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_MAP, drm_legacy_getmap_ioctl, DRM_UNLOCKED),
@@ -536,7 +542,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
-	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
index 610223b0481b..155588eb8ccf 100644
--- a/include/drm/drm_auth.h
+++ b/include/drm/drm_auth.h
@@ -33,10 +33,7 @@
  *
  * @refcount: Refcount for this master object.
  * @dev: Link back to the DRM device
- * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex.
- * @unique_len: Length of unique field. Protected by drm_global_mutex.
- * @magic_map: Map of used authentication tokens. Protected by struct_mutex.
- * @lock: DRI lock information.
+ * @lock: DRI1 lock information.
  * @driver_priv: Pointer to driver-private information.
  *
  * Note that master structures are only relevant for the legacy/primary device
@@ -45,8 +42,20 @@
 struct drm_master {
 	struct kref refcount;
 	struct drm_device *dev;
+	/**
+	 * @unique: Unique identifier: e.g. busid. Protected by struct
+	 * &drm_device master_mutex.
+	 */
 	char *unique;
+	/**
+	 * @unique_len: Length of unique field. Protected by struct &drm_device
+	 * master_mutex.
+	 */
 	int unique_len;
+	/**
+	 * @magic_map: Map of used authentication tokens. Protected by struct
+	 * &drm_device master_mutex.
+	 */
 	struct idr magic_map;
 	struct drm_lock_data lock;
 	void *driver_priv;
-- 
2.10.2

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

[PATCH 2/4] drm: setclientcap doesn't need the drm BKL

[Daniel Vetter]

It only updates per-file feature flags. And all the ioctl which change
behaviour depending upon these flags (they're all kms features) do
_not_ hold the BKL. Therefor this is pure cargo-cult and can be
removed.

Note that there's a risk that the ioctl will behave inconsistently,
but that's ok. The only thing it's not allowed to do is oops the
kernel, and from an audit all places are safe.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 42a17ea5d801..e1b2c372f021 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -541,7 +541,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENT, drm_getclient, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_STATS, drm_getstats, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CAP, drm_getcap, DRM_UNLOCKED|DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, 0),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_CLIENT_CAP, drm_setclientcap, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_VERSION, drm_setversion, DRM_UNLOCKED | DRM_MASTER),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
-- 
2.10.2

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

Re: [Intel-gfx] [PATCH 2/4] drm: setclientcap doesn't need the drm BKL

[Chris Wilson]

On Sat, Dec 10, 2016 at 10:52:53PM +0100, Daniel Vetter wrote:
> It only updates per-file feature flags. And all the ioctl which change
> behaviour depending upon these flags (they're all kms features) do
> _not_ hold the BKL. Therefor this is pure cargo-cult and can be
> removed.
> 
> Note that there's a risk that the ioctl will behave inconsistently,

+ if userspace is racing with itself,

> but that's ok. The only thing it's not allowed to do is oops the
> kernel, and from an audit all places are safe.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm: Enforce BKL-less ioctls for modern drivers

[Daniel Vetter]

With the last round of changes all ioctls called by modern drivers now
have their own locking. Everything else is only allowed for legacy
drivers and hence the lack of locking doesn't matter.

One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag.
But that only works its magic on the context and bufs ioctls. And
drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by
the same and dev->ctxlist_mutex. That should be all safe, and we can
finally mandata drm-bkl-less ioctls for everyone!

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e1b2c372f021..e21a18ac968e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -735,9 +735,8 @@ long drm_ioctl(struct file *filp,
 	if (ksize > in_size)
 		memset(kdata + in_size, 0, ksize - in_size);
 
-	/* Enforce sane locking for modern driver ioctls. Core ioctls are
-	 * too messy still. */
-	if ((!drm_core_check_feature(dev, DRIVER_LEGACY) && is_driver_ioctl) ||
+	/* Enforce sane locking for modern driver ioctls. */
+	if (!drm_core_check_feature(dev, DRIVER_LEGACY) ||
 	    (ioctl->flags & DRM_UNLOCKED))
 		retcode = func(dev, kdata, file_priv);
 	else {
-- 
2.10.2

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

Re: [Intel-gfx] [PATCH 3/4] drm: Enforce BKL-less ioctls for modern drivers

[Chris Wilson]

On Sat, Dec 10, 2016 at 10:52:54PM +0100, Daniel Vetter wrote:
> With the last round of changes all ioctls called by modern drivers now
> have their own locking. Everything else is only allowed for legacy
> drivers and hence the lack of locking doesn't matter.
> 
> One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag.
> But that only works its magic on the context and bufs ioctls. And
> drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by
> the same and dev->ctxlist_mutex. That should be all safe, and we can
> finally mandata drm-bkl-less ioctls for everyone!
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Meh, I'd much rather if this was just data driven rather than overriding
based on its own rules. However, it is a small simple step cleaning up
after the core changes, so in that regard

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I'd just rather see DRM_GLOBAL_LOCK instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Enforce BKL-less ioctls for modern drivers

[Daniel Vetter]

On Mon, Dec 12, 2016 at 09:50:29AM +0000, Chris Wilson wrote:
> On Sat, Dec 10, 2016 at 10:52:54PM +0100, Daniel Vetter wrote:
> > With the last round of changes all ioctls called by modern drivers now
> > have their own locking. Everything else is only allowed for legacy
> > drivers and hence the lack of locking doesn't matter.
> > 
> > One exception is nouveau, due to the DRIVER_KMS_LEGACY_CONTEXT flag.
> > But that only works its magic on the context and bufs ioctls. And
> > drm_bufs.c is protected with dev->struct_mutex, and drm_context.c by
> > the same and dev->ctxlist_mutex. That should be all safe, and we can
> > finally mandata drm-bkl-less ioctls for everyone!
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Meh, I'd much rather if this was just data driven rather than overriding
> based on its own rules. However, it is a small simple step cleaning up
> after the core changes, so in that regard
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I'd just rather see DRM_GLOBAL_LOCK instead.

Yeah, same idea, but functional change first, then the large-scale
prettification.
-Daniel
-- 
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

[PATCH 4/4] drm: Simplify GETRESOURCES ioctl

[Daniel Vetter]

Looping twice when we can do it once is silly. Also use a consistent
style. Note that there's a good race with the connector list walking,
since that is no longer protected by mode_config.mutex. But that's for
a later patch to fix.

v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.

v3:
- squash all drm_mode_getresources cleanups into one
- use consistent style for walking objects (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 119 +++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 2735a5847ffa..54e371dac694 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
 	struct drm_mode_card_res *card_res = data;
-	struct list_head *lh;
 	struct drm_framebuffer *fb;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
-	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
-	int fb_count = 0;
-	int encoder_count = 0;
-	int copied = 0;
+	int count, ret = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
@@ -105,89 +99,70 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 
 
 	mutex_lock(&file_priv->fbs_lock);
-	/*
-	 * For the non-control nodes we need to limit the list of resources
-	 * by IDs in the group list for this node
-	 */
-	list_for_each(lh, &file_priv->fbs)
-		fb_count++;
-
-	/* handle this in 4 parts */
-	/* FBs */
-	if (card_res->count_fbs >= fb_count) {
-		copied = 0;
-		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
-		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
-			if (put_user(fb->base.id, fb_id + copied)) {
-				mutex_unlock(&file_priv->fbs_lock);
-				return -EFAULT;
-			}
-			copied++;
+	count = 0;
+	fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
+	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
+		count++;
+		if (count > card_res->count_fbs)
+			continue;
+
+		if (put_user(fb->base.id, fb_id + count)) {
+			mutex_unlock(&file_priv->fbs_lock);
+			return -EFAULT;
 		}
 	}
-	card_res->count_fbs = fb_count;
+	card_res->count_fbs = count;
 	mutex_unlock(&file_priv->fbs_lock);
 
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
 	card_res->min_width = dev->mode_config.min_width;
 
-	/* CRTCs */
-	if (card_res->count_crtcs >= crtc_count) {
-		copied = 0;
-		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		drm_for_each_crtc(crtc, dev) {
-			if (put_user(crtc->base.id, crtc_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	count = 0;
+	crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
+	drm_for_each_crtc(crtc, dev) {
+		count++;
+		if (count > card_res->count_crtcs)
+			continue;
+
+		if (put_user(crtc->base.id, crtc_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_crtcs = crtc_count;
-
-	/* Encoders */
-	if (card_res->count_encoders >= encoder_count) {
-		copied = 0;
-		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		drm_for_each_encoder(encoder, dev) {
-			if (put_user(encoder->base.id, encoder_id +
-				     copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_crtcs = count;
+
+	count = 0;
+	encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
+	drm_for_each_encoder(encoder, dev) {
+		count++;
+		if (count > card_res->count_encoders)
+			continue;
+
+		if (put_user(encoder->base.id, encoder_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_encoders = encoder_count;
-
-	/* Connectors */
-	if (card_res->count_connectors >= connector_count) {
-		copied = 0;
-		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		drm_for_each_connector(connector, dev) {
-			if (put_user(connector->base.id,
-				     connector_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_encoders = count;
+
+	count = 0;
+	connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
+	drm_for_each_connector(connector, dev) {
+		count++;
+		if (count >= card_res->count_connectors)
+			continue;
+
+		if (put_user(connector->base.id, connector_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_connectors = connector_count;
+	card_res->count_connectors = count;
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
-- 
2.10.2

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

Re: [PATCH 4/4] drm: Simplify GETRESOURCES ioctl

[Chris Wilson]

On Sat, Dec 10, 2016 at 10:52:55PM +0100, Daniel Vetter wrote:
> Looping twice when we can do it once is silly. Also use a consistent
> style. Note that there's a good race with the connector list walking,
> since that is no longer protected by mode_config.mutex. But that's for
> a later patch to fix.
> 
> v2: Actually try to not blow up, somehow I lost the hunk that checks
> we don't copy too much. Noticed by Chris.
> 
> v3:
> - squash all drm_mode_getresources cleanups into one
> - use consistent style for walking objects (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c | 119 +++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 2735a5847ffa..54e371dac694 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  			  struct drm_file *file_priv)
>  {
>  	struct drm_mode_card_res *card_res = data;
> -	struct list_head *lh;
>  	struct drm_framebuffer *fb;
>  	struct drm_connector *connector;
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
> -	int ret = 0;
> -	int connector_count = 0;
> -	int crtc_count = 0;
> -	int fb_count = 0;
> -	int encoder_count = 0;
> -	int copied = 0;
> +	int count, ret = 0;
>  	uint32_t __user *fb_id;
>  	uint32_t __user *crtc_id;
>  	uint32_t __user *connector_id;
> @@ -105,89 +99,70 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  
>  
>  	mutex_lock(&file_priv->fbs_lock);
> -	/*
> -	 * For the non-control nodes we need to limit the list of resources
> -	 * by IDs in the group list for this node
> -	 */
> -	list_for_each(lh, &file_priv->fbs)
> -		fb_count++;
> -
> -	/* handle this in 4 parts */
> -	/* FBs */
> -	if (card_res->count_fbs >= fb_count) {
> -		copied = 0;
> -		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
> -		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> -			if (put_user(fb->base.id, fb_id + copied)) {
> -				mutex_unlock(&file_priv->fbs_lock);
> -				return -EFAULT;
> -			}
> -			copied++;
> +	count = 0;
> +	fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;

u64_to_user_ptr() please :)

> +	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> +		count++;
> +		if (count > card_res->count_fbs)
> +			continue;
> +
> +		if (put_user(fb->base.id, fb_id + count)) {

In this style increment of count has to happen after the copy.

i.e.
	if (count < card_res->count_fbs &&
	    put_user(fb->base.id, fb_id + count) {
	    mutex_unlock()
	    return -EFAULT;
	}
	count++;
 

> +			mutex_unlock(&file_priv->fbs_lock);
> +			return -EFAULT;
>  		}
>  	}
> -	card_res->count_fbs = fb_count;
> +	card_res->count_fbs = count;
>  	mutex_unlock(&file_priv->fbs_lock);

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Simplify GETRESOURCES ioctl

[Daniel Vetter]

On Sat, Dec 10, 2016 at 11:04 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> +     list_for_each_entry(fb, &file_priv->fbs, filp_head) {
>> +             count++;
>> +             if (count > card_res->count_fbs)
>> +                     continue;
>> +
>> +             if (put_user(fb->base.id, fb_id + count)) {
>
> In this style increment of count has to happen after the copy.
>
> i.e.
>         if (count < card_res->count_fbs &&
>             put_user(fb->base.id, fb_id + count) {
>             mutex_unlock()
>             return -EFAULT;
>         }
>         count++;

Note I also have > instead of  <, so I think it should be equivalent.
Oops except for the connector lop, silly me that lost one.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Simplify GETRESOURCES ioctl

[Daniel Vetter]

Looping twice when we can do it once is silly. Also use a consistent
style. Note that there's a good race with the connector list walking,
since that is no longer protected by mode_config.mutex. But that's for
a later patch to fix.

v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.

v3:
- squash all drm_mode_getresources cleanups into one
- use consistent style for walking objects (Chris)

v4:
- Use u64_to_user_ptr (Chris)
- Don't forget to copy the last connector (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 119 +++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 2735a5847ffa..3e6952142974 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
 	struct drm_mode_card_res *card_res = data;
-	struct list_head *lh;
 	struct drm_framebuffer *fb;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
-	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
-	int fb_count = 0;
-	int encoder_count = 0;
-	int copied = 0;
+	int count, ret = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
@@ -105,89 +99,70 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 
 
 	mutex_lock(&file_priv->fbs_lock);
-	/*
-	 * For the non-control nodes we need to limit the list of resources
-	 * by IDs in the group list for this node
-	 */
-	list_for_each(lh, &file_priv->fbs)
-		fb_count++;
-
-	/* handle this in 4 parts */
-	/* FBs */
-	if (card_res->count_fbs >= fb_count) {
-		copied = 0;
-		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
-		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
-			if (put_user(fb->base.id, fb_id + copied)) {
-				mutex_unlock(&file_priv->fbs_lock);
-				return -EFAULT;
-			}
-			copied++;
+	count = 0;
+	fb_id = u64_to_user_ptr(card_res->fb_id_ptr);
+	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
+		count++;
+		if (count > card_res->count_fbs)
+			continue;
+
+		if (put_user(fb->base.id, fb_id + count)) {
+			mutex_unlock(&file_priv->fbs_lock);
+			return -EFAULT;
 		}
 	}
-	card_res->count_fbs = fb_count;
+	card_res->count_fbs = count;
 	mutex_unlock(&file_priv->fbs_lock);
 
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
 	card_res->min_width = dev->mode_config.min_width;
 
-	/* CRTCs */
-	if (card_res->count_crtcs >= crtc_count) {
-		copied = 0;
-		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		drm_for_each_crtc(crtc, dev) {
-			if (put_user(crtc->base.id, crtc_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	count = 0;
+	crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr);
+	drm_for_each_crtc(crtc, dev) {
+		count++;
+		if (count > card_res->count_crtcs)
+			continue;
+
+		if (put_user(crtc->base.id, crtc_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_crtcs = crtc_count;
-
-	/* Encoders */
-	if (card_res->count_encoders >= encoder_count) {
-		copied = 0;
-		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		drm_for_each_encoder(encoder, dev) {
-			if (put_user(encoder->base.id, encoder_id +
-				     copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_crtcs = count;
+
+	count = 0;
+	encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr);
+	drm_for_each_encoder(encoder, dev) {
+		count++;
+		if (count > card_res->count_encoders)
+			continue;
+
+		if (put_user(encoder->base.id, encoder_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_encoders = encoder_count;
-
-	/* Connectors */
-	if (card_res->count_connectors >= connector_count) {
-		copied = 0;
-		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		drm_for_each_connector(connector, dev) {
-			if (put_user(connector->base.id,
-				     connector_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_encoders = count;
+
+	count = 0;
+	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
+	drm_for_each_connector(connector, dev) {
+		count++;
+		if (count > card_res->count_connectors)
+			continue;
+
+		if (put_user(connector->base.id, connector_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
 	}
-	card_res->count_connectors = connector_count;
+	card_res->count_connectors = count;
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
-- 
2.10.2

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

Re: [PATCH] drm: Simplify GETRESOURCES ioctl

[Chris Wilson]

On Sun, Dec 11, 2016 at 01:39:15PM +0100, Daniel Vetter wrote:
> +	count = 0;
> +	fb_id = u64_to_user_ptr(card_res->fb_id_ptr);
> +	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
> +		count++;
> +		if (count > card_res->count_fbs)
> +			continue;
> +
> +		if (put_user(fb->base.id, fb_id + count)) {

On the first iteration what is the value of count here?
What should it be?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Simplify GETRESOURCES ioctl

[Daniel Vetter]

Looping twice when we can do it once is silly. Also use a consistent
style. Note that there's a good race with the connector list walking,
since that is no longer protected by mode_config.mutex. But that's for
a later patch to fix.

v2: Actually try to not blow up, somehow I lost the hunk that checks
we don't copy too much. Noticed by Chris.

v3:
- squash all drm_mode_getresources cleanups into one
- use consistent style for walking objects (Chris)

v4:
- Use u64_to_user_ptr (Chris)
- Don't forget to copy the last connector (Chris)

v5: Chris was right ...

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------
 1 file changed, 39 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 2735a5847ffa..b1e8bbceaf39 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 			  struct drm_file *file_priv)
 {
 	struct drm_mode_card_res *card_res = data;
-	struct list_head *lh;
 	struct drm_framebuffer *fb;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
-	int ret = 0;
-	int connector_count = 0;
-	int crtc_count = 0;
-	int fb_count = 0;
-	int encoder_count = 0;
-	int copied = 0;
+	int count, ret = 0;
 	uint32_t __user *fb_id;
 	uint32_t __user *crtc_id;
 	uint32_t __user *connector_id;
@@ -105,89 +99,62 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 
 
 	mutex_lock(&file_priv->fbs_lock);
-	/*
-	 * For the non-control nodes we need to limit the list of resources
-	 * by IDs in the group list for this node
-	 */
-	list_for_each(lh, &file_priv->fbs)
-		fb_count++;
-
-	/* handle this in 4 parts */
-	/* FBs */
-	if (card_res->count_fbs >= fb_count) {
-		copied = 0;
-		fb_id = (uint32_t __user *)(unsigned long)card_res->fb_id_ptr;
-		list_for_each_entry(fb, &file_priv->fbs, filp_head) {
-			if (put_user(fb->base.id, fb_id + copied)) {
-				mutex_unlock(&file_priv->fbs_lock);
-				return -EFAULT;
-			}
-			copied++;
+	count = 0;
+	fb_id = u64_to_user_ptr(card_res->fb_id_ptr);
+	list_for_each_entry(fb, &file_priv->fbs, filp_head) {
+		if (count < card_res->count_fbs &&
+		    put_user(fb->base.id, fb_id + count)) {
+			mutex_unlock(&file_priv->fbs_lock);
+			return -EFAULT;
 		}
+		count++;
 	}
-	card_res->count_fbs = fb_count;
+	card_res->count_fbs = count;
 	mutex_unlock(&file_priv->fbs_lock);
 
 	/* mode_config.mutex protects the connector list against e.g. DP MST
 	 * connector hot-adding. CRTC/Plane lists are invariant. */
 	mutex_lock(&dev->mode_config.mutex);
-	drm_for_each_crtc(crtc, dev)
-		crtc_count++;
-
-	drm_for_each_connector(connector, dev)
-		connector_count++;
-
-	drm_for_each_encoder(encoder, dev)
-		encoder_count++;
-
 	card_res->max_height = dev->mode_config.max_height;
 	card_res->min_height = dev->mode_config.min_height;
 	card_res->max_width = dev->mode_config.max_width;
 	card_res->min_width = dev->mode_config.min_width;
 
-	/* CRTCs */
-	if (card_res->count_crtcs >= crtc_count) {
-		copied = 0;
-		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		drm_for_each_crtc(crtc, dev) {
-			if (put_user(crtc->base.id, crtc_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	count = 0;
+	crtc_id = u64_to_user_ptr(card_res->crtc_id_ptr);
+	drm_for_each_crtc(crtc, dev) {
+		if (count < card_res->count_crtcs &&
+		    put_user(crtc->base.id, crtc_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
+		count++;
 	}
-	card_res->count_crtcs = crtc_count;
-
-	/* Encoders */
-	if (card_res->count_encoders >= encoder_count) {
-		copied = 0;
-		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		drm_for_each_encoder(encoder, dev) {
-			if (put_user(encoder->base.id, encoder_id +
-				     copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_crtcs = count;
+
+	count = 0;
+	encoder_id = u64_to_user_ptr(card_res->encoder_id_ptr);
+	drm_for_each_encoder(encoder, dev) {
+		if (count < card_res->count_encoders &&
+		    put_user(encoder->base.id, encoder_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
+		count++;
 	}
-	card_res->count_encoders = encoder_count;
-
-	/* Connectors */
-	if (card_res->count_connectors >= connector_count) {
-		copied = 0;
-		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		drm_for_each_connector(connector, dev) {
-			if (put_user(connector->base.id,
-				     connector_id + copied)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			copied++;
+	card_res->count_encoders = count;
+
+	count = 0;
+	connector_id = u64_to_user_ptr(card_res->connector_id_ptr);
+	drm_for_each_connector(connector, dev) {
+		if (count < card_res->count_connectors &&
+		    put_user(connector->base.id, connector_id + count)) {
+			ret = -EFAULT;
+			goto out;
 		}
+		count++;
 	}
-	card_res->count_connectors = connector_count;
+	card_res->count_connectors = count;
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
-- 
2.10.2

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

Re: [PATCH] drm: Simplify GETRESOURCES ioctl

[Chris Wilson]

On Sun, Dec 11, 2016 at 08:20:19PM +0100, Daniel Vetter wrote:
> Looping twice when we can do it once is silly. Also use a consistent
> style. Note that there's a good race with the connector list walking,
> since that is no longer protected by mode_config.mutex. But that's for
> a later patch to fix.
> 
> v2: Actually try to not blow up, somehow I lost the hunk that checks
> we don't copy too much. Noticed by Chris.
> 
> v3:
> - squash all drm_mode_getresources cleanups into one
> - use consistent style for walking objects (Chris)
> 
> v4:
> - Use u64_to_user_ptr (Chris)
> - Don't forget to copy the last connector (Chris)
> 
> v5: Chris was right ...
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 2735a5847ffa..b1e8bbceaf39 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
>  			  struct drm_file *file_priv)
>  {
>  	struct drm_mode_card_res *card_res = data;
> -	struct list_head *lh;
>  	struct drm_framebuffer *fb;
>  	struct drm_connector *connector;
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
> -	int ret = 0;
> -	int connector_count = 0;
> -	int crtc_count = 0;
> -	int fb_count = 0;
> -	int encoder_count = 0;
> -	int copied = 0;
> +	int count, ret = 0;

I'm down to a minor int but uABI uses u32. This being C, it all comes
out in the wash. One day we may have -Wsign-compare, but not today!

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Simplify GETRESOURCES ioctl

[Daniel Vetter]

On Sun, Dec 11, 2016 at 07:53:42PM +0000, Chris Wilson wrote:
> On Sun, Dec 11, 2016 at 08:20:19PM +0100, Daniel Vetter wrote:
> > Looping twice when we can do it once is silly. Also use a consistent
> > style. Note that there's a good race with the connector list walking,
> > since that is no longer protected by mode_config.mutex. But that's for
> > a later patch to fix.
> > 
> > v2: Actually try to not blow up, somehow I lost the hunk that checks
> > we don't copy too much. Noticed by Chris.
> > 
> > v3:
> > - squash all drm_mode_getresources cleanups into one
> > - use consistent style for walking objects (Chris)
> > 
> > v4:
> > - Use u64_to_user_ptr (Chris)
> > - Don't forget to copy the last connector (Chris)
> > 
> > v5: Chris was right ...

And CI finally concurred that v5 works ;-)

> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_mode_config.c | 111 ++++++++++++++------------------------
> >  1 file changed, 39 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 2735a5847ffa..b1e8bbceaf39 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -84,17 +84,11 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
> >  			  struct drm_file *file_priv)
> >  {
> >  	struct drm_mode_card_res *card_res = data;
> > -	struct list_head *lh;
> >  	struct drm_framebuffer *fb;
> >  	struct drm_connector *connector;
> >  	struct drm_crtc *crtc;
> >  	struct drm_encoder *encoder;
> > -	int ret = 0;
> > -	int connector_count = 0;
> > -	int crtc_count = 0;
> > -	int fb_count = 0;
> > -	int encoder_count = 0;
> > -	int copied = 0;
> > +	int count, ret = 0;
> 
> I'm down to a minor int but uABI uses u32. This being C, it all comes
> out in the wash. One day we may have -Wsign-compare, but not today!
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the review, applied to -misc. Can I volunteer you for 1-3 too
please?

Thanks, Daniel
-- 
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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm: Protect master->unique with dev->master_mutex

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm: Protect master->unique with dev->master_mutex
URL   : https://patchwork.freedesktop.org/series/16661/
State : failure

== Summary ==

Series 16661v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16661/revisions/1/mbox/

Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-kbl-7500u)
        Subgroup basic-flip-default-b:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-kbl-7500u)
        Subgroup basic-flip-default-c:
                pass       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-skl-6770hq)
                skip       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bdw-5557u)
                skip       -> FAIL       (fi-ilk-650)
                skip       -> FAIL       (fi-byt-j1900)
                skip       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-kbl-7500u)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                skip       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-byt-n2820)
                skip       -> FAIL       (fi-hsw-4770r)
                skip       -> FAIL       (fi-skl-6770hq)
                skip       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-bsw-n3050)
                skip       -> FAIL       (fi-skl-6700hq)
                skip       -> FAIL       (fi-bdw-5557u)
                skip       -> FAIL       (fi-ilk-650)
                skip       -> FAIL       (fi-byt-j1900)
                skip       -> FAIL       (fi-snb-2600)
                skip       -> FAIL       (fi-hsw-4770)
                skip       -> FAIL       (fi-ivb-3520m)
                skip       -> FAIL       (fi-skl-6260u)
                skip       -> FAIL       (fi-skl-6700k)
                skip       -> FAIL       (fi-kbl-7500u)
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-kbl-7500u)
        Subgroup basic-busy-flip-before-cursor-varying-size:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-ilk-650)
WARNING: Long output truncated

f6a248e2507f98d7eb1f4fec8cfcbf685a33d289 drm-tip: 2016y-12m-10d-21h-47m-23s UTC integration manifest
6af9b5883 drm: Simplify GETRESOURCES ioctl
36bf578 drm: Enforce BKL-less ioctls for modern drivers
8ceb0fe drm: setclientcap doesn't need the drm BKL
a797343 drm: Protect master->unique with dev->master_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3261/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/4] drm: Protect master->unique with dev->master_mutex (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm: Protect master->unique with dev->master_mutex (rev2)
URL   : https://patchwork.freedesktop.org/series/16661/
State : failure

== Summary ==

Series 16661v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16661/revisions/2/mbox/

Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-skl-6260u)
                skip       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-bxt-t5700)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6700hq)
        Subgroup basic-flip-default-b:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-skl-6260u)
                skip       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-bxt-t5700)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6700hq)
        Subgroup basic-flip-default-c:
                pass       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-skl-6770hq)
                skip       -> FAIL       (fi-snb-2600)
                skip       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-bxt-t5700)
                skip       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-bdw-5557u)
                skip       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6700hq)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                skip       -> FAIL       (fi-ivb-3770)
                skip       -> FAIL       (fi-ilk-650)
                skip       -> FAIL       (fi-skl-6260u)
                skip       -> FAIL       (fi-bsw-n3050)
                skip       -> FAIL       (fi-hsw-4770)
                skip       -> FAIL       (fi-skl-6770hq)
                skip       -> FAIL       (fi-snb-2600)
                skip       -> FAIL       (fi-byt-j1900)
                skip       -> FAIL       (fi-skl-6700k)
                skip       -> FAIL       (fi-hsw-4770r)
                skip       -> FAIL       (fi-kbl-7500u)
                skip       -> FAIL       (fi-bxt-t5700)
                skip       -> FAIL       (fi-byt-n2820)
                skip       -> FAIL       (fi-bdw-5557u)
                skip       -> FAIL       (fi-snb-2520m)
                skip       -> FAIL       (fi-ivb-3520m)
                skip       -> FAIL       (fi-skl-6700hq)
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-byt-j1900)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-bxt-t5700)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-skl-6700hq)
        Subgroup basic-busy-flip-before-cursor-varying-size:
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-ilk-650)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-bsw-n3050)
WARNING: Long output truncated

f6a248e2507f98d7eb1f4fec8cfcbf685a33d289 drm-tip: 2016y-12m-10d-21h-47m-23s UTC integration manifest
c25f986 drm: Simplify GETRESOURCES ioctl
51ba21c drm: Enforce BKL-less ioctls for modern drivers
5acb428 drm: setclientcap doesn't need the drm BKL
8fd08d9 drm: Protect master->unique with dev->master_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3262/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/4] drm: Protect master->unique with dev->master_mutex (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm: Protect master->unique with dev->master_mutex (rev3)
URL   : https://patchwork.freedesktop.org/series/16661/
State : success

== Summary ==

Series 16661v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16661/revisions/3/mbox/


fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

f6a248e2507f98d7eb1f4fec8cfcbf685a33d289 drm-tip: 2016y-12m-10d-21h-47m-23s UTC integration manifest
d1ca8ca drm: Simplify GETRESOURCES ioctl
0c7bda4 drm: Enforce BKL-less ioctls for modern drivers
a29527a drm: setclientcap doesn't need the drm BKL
dbca2a1 drm: Protect master->unique with dev->master_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3264/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Chris Wilson]

On Sat, Dec 10, 2016 at 10:52:52PM +0100, Daniel Vetter wrote:
> No one looks at the major/minor versions except the unique/busid
> stuff. If we protect that with the master_mutex (since it also affects
> the unique of each master, oh well) we can mark these two IOCTL with
> DRM_UNLOCKED.
> 
> While doing this I realized that the comment for the magic_map is
> outdated, I've forgotten to update it in:
> 
> commit d2b34ee62b409a03c6fe43c07b779983be51d017
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Jun 17 09:33:21 2016 +0200
> 
>     drm: Protect authmagic with master_mutex
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Ok, you have to jump around a bit to confirm the locking is correct.

(Mainly didn't get the connection between getunique and set_busid).
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

On Mon, Dec 12, 2016 at 09:39:55AM +0000, Chris Wilson wrote:
> On Sat, Dec 10, 2016 at 10:52:52PM +0100, Daniel Vetter wrote:
> > No one looks at the major/minor versions except the unique/busid
> > stuff. If we protect that with the master_mutex (since it also affects
> > the unique of each master, oh well) we can mark these two IOCTL with
> > DRM_UNLOCKED.
> > 
> > While doing this I realized that the comment for the magic_map is
> > outdated, I've forgotten to update it in:
> > 
> > commit d2b34ee62b409a03c6fe43c07b779983be51d017
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Jun 17 09:33:21 2016 +0200
> > 
> >     drm: Protect authmagic with master_mutex
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Ok, you have to jump around a bit to confirm the locking is correct.
> 
> (Mainly didn't get the connection between getunique and set_busid).

Yeah the naming is hilarious in this area. There's a kerneldoc comment
that tries to explain it all.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the review, applied the small update for patch 2 and merged
them all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Emil Velikov]

On 10 December 2016 at 21:52, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> No one looks at the major/minor versions except the unique/busid
> stuff. If we protect that with the master_mutex (since it also affects
> the unique of each master, oh well) we can mark these two IOCTL with
> DRM_UNLOCKED.
>
> While doing this I realized that the comment for the magic_map is
> outdated, I've forgotten to update it in:
>
> commit d2b34ee62b409a03c6fe43c07b779983be51d017
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Jun 17 09:33:21 2016 +0200
>
>     drm: Protect authmagic with master_mutex
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at
~25 patches. Admittedly it includes some related fixes/cleanups.
At some point we want to address the other KMS ddx - armsoc (ahem),
ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of
course modesetting ;-)
Regardless if some are superseded and/or barely used.

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

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

On Mon, Dec 12, 2016 at 01:23:46PM +0000, Emil Velikov wrote:
> On 10 December 2016 at 21:52, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > No one looks at the major/minor versions except the unique/busid
> > stuff. If we protect that with the master_mutex (since it also affects
> > the unique of each master, oh well) we can mark these two IOCTL with
> > DRM_UNLOCKED.
> >
> > While doing this I realized that the comment for the magic_map is
> > outdated, I've forgotten to update it in:
> >
> > commit d2b34ee62b409a03c6fe43c07b779983be51d017
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Jun 17 09:33:21 2016 +0200
> >
> >     drm: Protect authmagic with master_mutex
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> 
> Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at
> ~25 patches. Admittedly it includes some related fixes/cleanups.
> At some point we want to address the other KMS ddx - armsoc (ahem),
> ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of
> course modesetting ;-)
> Regardless if some are superseded and/or barely used.

Hm, I thought the grand plan is to use -modesetting almost everywhere and
forget about all the others?
-Daniel
-- 
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

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Michel Dänzer]

On 13/12/16 05:35 PM, Daniel Vetter wrote:
> On Mon, Dec 12, 2016 at 01:23:46PM +0000, Emil Velikov wrote:
>> On 10 December 2016 at 21:52, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> No one looks at the major/minor versions except the unique/busid
>>> stuff. If we protect that with the master_mutex (since it also affects
>>> the unique of each master, oh well) we can mark these two IOCTL with
>>> DRM_UNLOCKED.
>>>
>>> While doing this I realized that the comment for the magic_map is
>>> outdated, I've forgotten to update it in:
>>>
>>> commit d2b34ee62b409a03c6fe43c07b779983be51d017
>>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Date:   Fri Jun 17 09:33:21 2016 +0200
>>>
>>>     drm: Protect authmagic with master_mutex
>>>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> Side note: I've looked at "fixing" xf86-video-amdgpu and so far I'm at
>> ~25 patches. Admittedly it includes some related fixes/cleanups.
>> At some point we want to address the other KMS ddx - armsoc (ahem),
>> ati, freedreno, intel, nouveau, omap, opentegra, vmwgfx, qxl and of
>> course modesetting ;-)
>> Regardless if some are superseded and/or barely used.
> 
> Hm, I thought the grand plan is to use -modesetting almost everywhere and
> forget about all the others?

Maybe if you mean s/grand plan/pipe dream/ ...

Specifically, wrt to DDX drivers for ATI/AMD GPUs:

xf86-video-ati supports old GPUs on which glamor can't work, so
-modesetting can never be a complete replacement.

xf86-video-amdgpu is also used by the amdgpu-pro stack, with
modifications which are probably unsuitable for -modesetting. There's
also still a feature gap to -modesetting, e.g. the latter doesn't
support TearFree yet, and there doesn't seem to be any interest to fix that.

So I'm afraid we can't get rid of those anytime soon, if ever.


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

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Daniel Vetter]

On Tue, Dec 13, 2016 at 10:42 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> Hm, I thought the grand plan is to use -modesetting almost everywhere and
>> forget about all the others?
>
> Maybe if you mean s/grand plan/pipe dream/ ...

I said "almost everywhere", not "everywhere". I'm fully aware that
there's tons of old chips that need dedicated drivers, and that amd
wants to do some fancy stuff in their -pro version. But besides those
I do see a fairly clear push towards having standardized/generic kms
userspace, so don't really see that much value in updating all the
vendor specific drivers. And outside of X the push is even stronger.

So no pipe dream at all, it's happening.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Protect master->unique with dev->master_mutex

[Emil Velikov]

On 13 December 2016 at 09:46, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 13, 2016 at 10:42 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>> Hm, I thought the grand plan is to use -modesetting almost everywhere and
>>> forget about all the others?
>>
>> Maybe if you mean s/grand plan/pipe dream/ ...
>
> I said "almost everywhere", not "everywhere". I'm fully aware that
> there's tons of old chips that need dedicated drivers, and that amd
> wants to do some fancy stuff in their -pro version. But besides those
> I do see a fairly clear push towards having standardized/generic kms
> userspace, so don't really see that much value in updating all the
> vendor specific drivers. And outside of X the push is even stronger.
>
> So no pipe dream at all, it's happening.
Precisely.

As Michel pointed out:
There will [always] be compelling reasons why users/distros prefer
xf86-video-foo over the modesetting one.
So, barring time concerns, updating those is good idea, imho.

Pretty we don't want another DRIVER_KMS_LEGACY_CONTEXT, because user X
decided to stay with the final version of xf86-video-foo which still
uses nasty libdrm/ioctl Y ;-)

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