All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/14] Cruft removal around drm_master
@ 2016-06-14 18:50 Daniel Vetter
  2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
                   ` (14 more replies)
  0 siblings, 15 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development

Hi all,

These are the final bits of my dev->struct_mutex crusade, taking care of a few
things related to drm_master. Since I just couldnt' grok this dense web of
legacy dungeons, also includes some cleanups. On top of that also patches to
mark auth and master ioctls as DRM_UNLOCKED, which means the other drm subsystem
BKL is also much reduced (and only still needed to paper over the midlayer fail
for drivers that use the ->load callback).

Feedbacks, review and testing highly welcome.

Cheers, Daniel

Daniel Vetter (14):
  drm: Nuke legacy maps debugfs files
  drm: Hide hw.lock cleanup in filp->release better
  drm: Link directly from drm_master to drm_device
  drm: Move master functions into drm_auth.c
  drm: Extract drm_master_open
  drm: Extract drm_master_relase
  drm: Only do the hw.lock cleanup in master_relase for !MODESET
  drm: Move authmagic cleanup into drm_master_release
  drm: Protect authmagic with master_mutex
  drm: Mark authmagic ioctls as unlocked
  drm: Mark set/drop master ioctl as unlocked.
  drm: Move master pointer from drm_minor to drm_device
  drm: Clean up drm_crtc.h
  drm: Use dev->name as fallback for dev->unique

 drivers/gpu/drm/drm_auth.c          | 231 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_bufs.c          |   8 +-
 drivers/gpu/drm/drm_crtc.c          |   7 ++
 drivers/gpu/drm/drm_crtc_internal.h |  86 ++++++++++++-
 drivers/gpu/drm/drm_debugfs.c       |   3 -
 drivers/gpu/drm/drm_drv.c           | 119 +-----------------
 drivers/gpu/drm/drm_fb_helper.c     |   2 +-
 drivers/gpu/drm/drm_fops.c          | 125 ++-----------------
 drivers/gpu/drm/drm_info.c          |  97 ++-------------
 drivers/gpu/drm/drm_internal.h      |  18 +--
 drivers/gpu/drm/drm_ioctl.c         |  16 +--
 drivers/gpu/drm/drm_legacy.h        |   8 +-
 drivers/gpu/drm/drm_lock.c          | 238 ++++++++++++++++++------------------
 drivers/gpu/drm/drm_vm.c            |  54 --------
 drivers/gpu/drm/sis/sis_mm.c        |   2 +-
 drivers/gpu/drm/via/via_mm.c        |   2 +-
 include/drm/drmP.h                  |  17 +--
 include/drm/drm_crtc.h              | 188 +++++++++-------------------
 18 files changed, 547 insertions(+), 674 deletions(-)

-- 
2.8.1

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 01/14] drm: Nuke legacy maps debugfs files
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
@ 2016-06-14 18:50 ` Daniel Vetter
  2016-06-15 11:23   ` Chris Wilson
  2016-06-15 12:00   ` Emil Velikov
  2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
                   ` (13 subsequent siblings)
  14 siblings, 2 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development

GEM stopped using those a while ago, and no one should ever
need to use them again to debug legacy horror show drivers.

Nuke it all. Aside: It would kinda be nice if we'd have some
generic debugfs dumps for at least kms ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_debugfs.c  |  3 --
 drivers/gpu/drm/drm_info.c     | 89 ------------------------------------------
 drivers/gpu/drm/drm_internal.h |  5 ---
 drivers/gpu/drm/drm_vm.c       | 54 -------------------------
 4 files changed, 151 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 3bcf8e6a85b3..fa10cef2ba37 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -46,11 +46,8 @@
 
 static const struct drm_info_list drm_debugfs_list[] = {
 	{"name", drm_name_info, 0},
-	{"vm", drm_vm_info, 0},
 	{"clients", drm_clients_info, 0},
-	{"bufs", drm_bufs_info, 0},
 	{"gem_names", drm_gem_name_info, DRIVER_GEM},
-	{"vma", drm_vma_info, 0},
 };
 #define DRM_DEBUGFS_ENTRIES ARRAY_SIZE(drm_debugfs_list)
 
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 5d469b2f26f4..0090d5987801 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -66,94 +66,6 @@ int drm_name_info(struct seq_file *m, void *data)
 }
 
 /**
- * Called when "/proc/dri/.../vm" is read.
- *
- * Prints information about all mappings in drm_device::maplist.
- */
-int drm_vm_info(struct seq_file *m, void *data)
-{
-	struct drm_info_node *node = (struct drm_info_node *) m->private;
-	struct drm_device *dev = node->minor->dev;
-	struct drm_local_map *map;
-	struct drm_map_list *r_list;
-
-	/* Hardcoded from _DRM_FRAME_BUFFER,
-	   _DRM_REGISTERS, _DRM_SHM, _DRM_AGP, and
-	   _DRM_SCATTER_GATHER and _DRM_CONSISTENT */
-	const char *types[] = { "FB", "REG", "SHM", "AGP", "SG", "PCI" };
-	const char *type;
-	int i;
-
-	mutex_lock(&dev->struct_mutex);
-	seq_printf(m, "slot	 offset	      size type flags	 address mtrr\n\n");
-	i = 0;
-	list_for_each_entry(r_list, &dev->maplist, head) {
-		map = r_list->map;
-		if (!map)
-			continue;
-		if (map->type < 0 || map->type > 5)
-			type = "??";
-		else
-			type = types[map->type];
-
-		seq_printf(m, "%4d 0x%016llx 0x%08lx %4.4s  0x%02x 0x%08lx ",
-			   i,
-			   (unsigned long long)map->offset,
-			   map->size, type, map->flags,
-			   (unsigned long) r_list->user_token);
-		if (map->mtrr < 0)
-			seq_printf(m, "none\n");
-		else
-			seq_printf(m, "%4d\n", map->mtrr);
-		i++;
-	}
-	mutex_unlock(&dev->struct_mutex);
-	return 0;
-}
-
-/**
- * Called when "/proc/dri/.../bufs" is read.
- */
-int drm_bufs_info(struct seq_file *m, void *data)
-{
-	struct drm_info_node *node = (struct drm_info_node *) m->private;
-	struct drm_device *dev = node->minor->dev;
-	struct drm_device_dma *dma;
-	int i, seg_pages;
-
-	mutex_lock(&dev->struct_mutex);
-	dma = dev->dma;
-	if (!dma) {
-		mutex_unlock(&dev->struct_mutex);
-		return 0;
-	}
-
-	seq_printf(m, " o     size count  free	 segs pages    kB\n\n");
-	for (i = 0; i <= DRM_MAX_ORDER; i++) {
-		if (dma->bufs[i].buf_count) {
-			seg_pages = dma->bufs[i].seg_count * (1 << dma->bufs[i].page_order);
-			seq_printf(m, "%2d %8d %5d %5d %5d %5d %5ld\n",
-				   i,
-				   dma->bufs[i].buf_size,
-				   dma->bufs[i].buf_count,
-				   0,
-				   dma->bufs[i].seg_count,
-				   seg_pages,
-				   seg_pages * PAGE_SIZE / 1024);
-		}
-	}
-	seq_printf(m, "\n");
-	for (i = 0; i < dma->buf_count; i++) {
-		if (i && !(i % 32))
-			seq_printf(m, "\n");
-		seq_printf(m, " %d", dma->buflist[i]->list);
-	}
-	seq_printf(m, "\n");
-	mutex_unlock(&dev->struct_mutex);
-	return 0;
-}
-
-/**
  * Called when "/proc/dri/.../clients" is read.
  *
  */
@@ -194,7 +106,6 @@ int drm_clients_info(struct seq_file *m, void *data)
 	return 0;
 }
 
-
 static int drm_gem_one_name_info(int id, void *ptr, void *data)
 {
 	struct drm_gem_object *obj = ptr;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 902cf6a15212..56a9b1cf99d7 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -35,9 +35,6 @@ int drm_pci_set_unique(struct drm_device *dev,
 int drm_irq_by_busid(struct drm_device *dev, void *data,
 		     struct drm_file *file_priv);
 
-/* drm_vm.c */
-int drm_vma_info(struct seq_file *m, void *data);
-
 /* drm_prime.c */
 int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *file_priv);
@@ -51,8 +48,6 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
 
 /* drm_info.c */
 int drm_name_info(struct seq_file *m, void *data);
-int drm_vm_info(struct seq_file *m, void *data);
-int drm_bufs_info(struct seq_file *m, void *data);
 int drm_clients_info(struct seq_file *m, void* data);
 int drm_gem_name_info(struct seq_file *m, void *data);
 
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index ac9f4b3ec615..43ff44a2b8e7 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -670,57 +670,3 @@ void drm_legacy_vma_flush(struct drm_device *dev)
 		kfree(vma);
 	}
 }
-
-int drm_vma_info(struct seq_file *m, void *data)
-{
-	struct drm_info_node *node = (struct drm_info_node *) m->private;
-	struct drm_device *dev = node->minor->dev;
-	struct drm_vma_entry *pt;
-	struct vm_area_struct *vma;
-	unsigned long vma_count = 0;
-#if defined(__i386__)
-	unsigned int pgprot;
-#endif
-
-	mutex_lock(&dev->struct_mutex);
-	list_for_each_entry(pt, &dev->vmalist, head)
-		vma_count++;
-
-	seq_printf(m, "vma use count: %lu, high_memory = %pK, 0x%pK\n",
-		   vma_count, high_memory,
-		   (void *)(unsigned long)virt_to_phys(high_memory));
-
-	list_for_each_entry(pt, &dev->vmalist, head) {
-		vma = pt->vma;
-		if (!vma)
-			continue;
-		seq_printf(m,
-			   "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000",
-			   pt->pid,
-			   (void *)vma->vm_start, (void *)vma->vm_end,
-			   vma->vm_flags & VM_READ ? 'r' : '-',
-			   vma->vm_flags & VM_WRITE ? 'w' : '-',
-			   vma->vm_flags & VM_EXEC ? 'x' : '-',
-			   vma->vm_flags & VM_MAYSHARE ? 's' : 'p',
-			   vma->vm_flags & VM_LOCKED ? 'l' : '-',
-			   vma->vm_flags & VM_IO ? 'i' : '-',
-			   vma->vm_pgoff);
-
-#if defined(__i386__)
-		pgprot = pgprot_val(vma->vm_page_prot);
-		seq_printf(m, " %c%c%c%c%c%c%c%c%c",
-			   pgprot & _PAGE_PRESENT ? 'p' : '-',
-			   pgprot & _PAGE_RW ? 'w' : 'r',
-			   pgprot & _PAGE_USER ? 'u' : 's',
-			   pgprot & _PAGE_PWT ? 't' : 'b',
-			   pgprot & _PAGE_PCD ? 'u' : 'c',
-			   pgprot & _PAGE_ACCESSED ? 'a' : '-',
-			   pgprot & _PAGE_DIRTY ? 'd' : '-',
-			   pgprot & _PAGE_PSE ? 'm' : 'k',
-			   pgprot & _PAGE_GLOBAL ? 'g' : 'l');
-#endif
-		seq_printf(m, "\n");
-	}
-	mutex_unlock(&dev->struct_mutex);
-	return 0;
-}
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
  2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
@ 2016-06-14 18:50 ` Daniel Vetter
  2016-06-15 11:26   ` [Intel-gfx] " Chris Wilson
  2016-06-15 12:10   ` Emil Velikov
  2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
                   ` (12 subsequent siblings)
  14 siblings, 2 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

A few things:
- Rename the cleanup function from drm_master_release to
  drm_legacy_lock_release. It doesn't relase any master stuff, but
  just the legacy hw lock.
- Hide it in drm_lock.c, which allows us to make a few more functions
  static in there. To avoid forward decl we need to shuffle the code a
  bit though.
- Push the check for ->master into the function itself.
- Only call this for !DRIVER_MODESET.

End result: Another place that takes struct_mutex gone for good for
modern drivers.

v2: Remove leftover comment.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_fops.c   |  17 +---
 drivers/gpu/drm/drm_legacy.h |   8 +-
 drivers/gpu/drm/drm_lock.c   | 238 ++++++++++++++++++++++---------------------
 3 files changed, 126 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index a27bc7cda975..2fd4f42b907a 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -338,18 +338,6 @@ out_prime_destroy:
 	return ret;
 }
 
-static void drm_master_release(struct drm_device *dev, struct file *filp)
-{
-	struct drm_file *file_priv = filp->private_data;
-
-	if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
-		DRM_DEBUG("File %p released, freeing lock for context %d\n",
-			  filp, _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
-		drm_legacy_lock_free(&file_priv->master->lock,
-				     _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
-	}
-}
-
 static void drm_events_release(struct drm_file *file_priv)
 {
 	struct drm_device *dev = file_priv->minor->dev;
@@ -468,9 +456,8 @@ int drm_release(struct inode *inode, struct file *filp)
 		  (long)old_encode_dev(file_priv->minor->kdev->devt),
 		  dev->open_count);
 
-	/* if the master has gone away we can't do anything with the lock */
-	if (file_priv->minor->master)
-		drm_master_release(dev, filp);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		drm_legacy_lock_release(dev, filp);
 
 	if (drm_core_check_feature(dev, DRIVER_HAVE_DMA))
 		drm_legacy_reclaim_buffers(dev, file_priv);
diff --git a/drivers/gpu/drm/drm_legacy.h b/drivers/gpu/drm/drm_legacy.h
index d3b6ee357a2b..c6f422e879dd 100644
--- a/drivers/gpu/drm/drm_legacy.h
+++ b/drivers/gpu/drm/drm_legacy.h
@@ -88,14 +88,10 @@ struct drm_agp_mem {
 	struct list_head head;
 };
 
-/*
- * Generic Userspace Locking-API
- */
-
-int drm_legacy_i_have_hw_lock(struct drm_device *d, struct drm_file *f);
+/* drm_lock.c */
 int drm_legacy_lock(struct drm_device *d, void *v, struct drm_file *f);
 int drm_legacy_unlock(struct drm_device *d, void *v, struct drm_file *f);
-int drm_legacy_lock_free(struct drm_lock_data *lock, unsigned int ctx);
+void drm_legacy_lock_release(struct drm_device *dev, struct file *filp);
 
 /* DMA support */
 int drm_legacy_dma_setup(struct drm_device *dev);
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index daa2ff12101b..0fb8f9e73486 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -41,6 +41,110 @@
 static int drm_lock_take(struct drm_lock_data *lock_data, unsigned int context);
 
 /**
+ * Take the heavyweight lock.
+ *
+ * \param lock lock pointer.
+ * \param context locking context.
+ * \return one if the lock is held, or zero otherwise.
+ *
+ * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
+ */
+static
+int drm_lock_take(struct drm_lock_data *lock_data,
+		  unsigned int context)
+{
+	unsigned int old, new, prev;
+	volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+	spin_lock_bh(&lock_data->spinlock);
+	do {
+		old = *lock;
+		if (old & _DRM_LOCK_HELD)
+			new = old | _DRM_LOCK_CONT;
+		else {
+			new = context | _DRM_LOCK_HELD |
+				((lock_data->user_waiters + lock_data->kernel_waiters > 1) ?
+				 _DRM_LOCK_CONT : 0);
+		}
+		prev = cmpxchg(lock, old, new);
+	} while (prev != old);
+	spin_unlock_bh(&lock_data->spinlock);
+
+	if (_DRM_LOCKING_CONTEXT(old) == context) {
+		if (old & _DRM_LOCK_HELD) {
+			if (context != DRM_KERNEL_CONTEXT) {
+				DRM_ERROR("%d holds heavyweight lock\n",
+					  context);
+			}
+			return 0;
+		}
+	}
+
+	if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
+		/* Have lock */
+		return 1;
+	}
+	return 0;
+}
+
+/**
+ * This takes a lock forcibly and hands it to context.	Should ONLY be used
+ * inside *_unlock to give lock to kernel before calling *_dma_schedule.
+ *
+ * \param dev DRM device.
+ * \param lock lock pointer.
+ * \param context locking context.
+ * \return always one.
+ *
+ * Resets the lock file pointer.
+ * Marks the lock as held by the given context, via the \p cmpxchg instruction.
+ */
+static int drm_lock_transfer(struct drm_lock_data *lock_data,
+			     unsigned int context)
+{
+	unsigned int old, new, prev;
+	volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+	lock_data->file_priv = NULL;
+	do {
+		old = *lock;
+		new = context | _DRM_LOCK_HELD;
+		prev = cmpxchg(lock, old, new);
+	} while (prev != old);
+	return 1;
+}
+
+static int drm_legacy_lock_free(struct drm_lock_data *lock_data,
+				unsigned int context)
+{
+	unsigned int old, new, prev;
+	volatile unsigned int *lock = &lock_data->hw_lock->lock;
+
+	spin_lock_bh(&lock_data->spinlock);
+	if (lock_data->kernel_waiters != 0) {
+		drm_lock_transfer(lock_data, 0);
+		lock_data->idle_has_lock = 1;
+		spin_unlock_bh(&lock_data->spinlock);
+		return 1;
+	}
+	spin_unlock_bh(&lock_data->spinlock);
+
+	do {
+		old = *lock;
+		new = _DRM_LOCKING_CONTEXT(old);
+		prev = cmpxchg(lock, old, new);
+	} while (prev != old);
+
+	if (_DRM_LOCK_IS_HELD(old) && _DRM_LOCKING_CONTEXT(old) != context) {
+		DRM_ERROR("%d freed heavyweight lock held by %d\n",
+			  context, _DRM_LOCKING_CONTEXT(old));
+		return 1;
+	}
+	wake_up_interruptible(&lock_data->lock_queue);
+	return 0;
+}
+
+/**
  * Lock ioctl.
  *
  * \param inode device inode.
@@ -165,120 +269,6 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 }
 
 /**
- * Take the heavyweight lock.
- *
- * \param lock lock pointer.
- * \param context locking context.
- * \return one if the lock is held, or zero otherwise.
- *
- * Attempt to mark the lock as held by the given context, via the \p cmpxchg instruction.
- */
-static
-int drm_lock_take(struct drm_lock_data *lock_data,
-		  unsigned int context)
-{
-	unsigned int old, new, prev;
-	volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-	spin_lock_bh(&lock_data->spinlock);
-	do {
-		old = *lock;
-		if (old & _DRM_LOCK_HELD)
-			new = old | _DRM_LOCK_CONT;
-		else {
-			new = context | _DRM_LOCK_HELD |
-				((lock_data->user_waiters + lock_data->kernel_waiters > 1) ?
-				 _DRM_LOCK_CONT : 0);
-		}
-		prev = cmpxchg(lock, old, new);
-	} while (prev != old);
-	spin_unlock_bh(&lock_data->spinlock);
-
-	if (_DRM_LOCKING_CONTEXT(old) == context) {
-		if (old & _DRM_LOCK_HELD) {
-			if (context != DRM_KERNEL_CONTEXT) {
-				DRM_ERROR("%d holds heavyweight lock\n",
-					  context);
-			}
-			return 0;
-		}
-	}
-
-	if ((_DRM_LOCKING_CONTEXT(new)) == context && (new & _DRM_LOCK_HELD)) {
-		/* Have lock */
-		return 1;
-	}
-	return 0;
-}
-
-/**
- * This takes a lock forcibly and hands it to context.	Should ONLY be used
- * inside *_unlock to give lock to kernel before calling *_dma_schedule.
- *
- * \param dev DRM device.
- * \param lock lock pointer.
- * \param context locking context.
- * \return always one.
- *
- * Resets the lock file pointer.
- * Marks the lock as held by the given context, via the \p cmpxchg instruction.
- */
-static int drm_lock_transfer(struct drm_lock_data *lock_data,
-			     unsigned int context)
-{
-	unsigned int old, new, prev;
-	volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-	lock_data->file_priv = NULL;
-	do {
-		old = *lock;
-		new = context | _DRM_LOCK_HELD;
-		prev = cmpxchg(lock, old, new);
-	} while (prev != old);
-	return 1;
-}
-
-/**
- * Free lock.
- *
- * \param dev DRM device.
- * \param lock lock.
- * \param context context.
- *
- * Resets the lock file pointer.
- * Marks the lock as not held, via the \p cmpxchg instruction. Wakes any task
- * waiting on the lock queue.
- */
-int drm_legacy_lock_free(struct drm_lock_data *lock_data, unsigned int context)
-{
-	unsigned int old, new, prev;
-	volatile unsigned int *lock = &lock_data->hw_lock->lock;
-
-	spin_lock_bh(&lock_data->spinlock);
-	if (lock_data->kernel_waiters != 0) {
-		drm_lock_transfer(lock_data, 0);
-		lock_data->idle_has_lock = 1;
-		spin_unlock_bh(&lock_data->spinlock);
-		return 1;
-	}
-	spin_unlock_bh(&lock_data->spinlock);
-
-	do {
-		old = *lock;
-		new = _DRM_LOCKING_CONTEXT(old);
-		prev = cmpxchg(lock, old, new);
-	} while (prev != old);
-
-	if (_DRM_LOCK_IS_HELD(old) && _DRM_LOCKING_CONTEXT(old) != context) {
-		DRM_ERROR("%d freed heavyweight lock held by %d\n",
-			  context, _DRM_LOCKING_CONTEXT(old));
-		return 1;
-	}
-	wake_up_interruptible(&lock_data->lock_queue);
-	return 0;
-}
-
-/**
  * This function returns immediately and takes the hw lock
  * with the kernel context if it is free, otherwise it gets the highest priority when and if
  * it is eventually released.
@@ -330,11 +320,27 @@ void drm_legacy_idlelock_release(struct drm_lock_data *lock_data)
 }
 EXPORT_SYMBOL(drm_legacy_idlelock_release);
 
-int drm_legacy_i_have_hw_lock(struct drm_device *dev,
-			      struct drm_file *file_priv)
+static int drm_legacy_i_have_hw_lock(struct drm_device *dev,
+				     struct drm_file *file_priv)
 {
 	struct drm_master *master = file_priv->master;
 	return (file_priv->lock_count && master->lock.hw_lock &&
 		_DRM_LOCK_IS_HELD(master->lock.hw_lock->lock) &&
 		master->lock.file_priv == file_priv);
 }
+
+void drm_legacy_lock_release(struct drm_device *dev, struct file *filp)
+{
+	struct drm_file *file_priv = filp->private_data;
+
+	/* if the master has gone away we can't do anything with the lock */
+	if (!file_priv->minor->master)
+		return;
+
+	if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
+		DRM_DEBUG("File %p released, freeing lock for context %d\n",
+			  filp, _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
+		drm_legacy_lock_free(&file_priv->master->lock,
+				     _DRM_LOCKING_CONTEXT(file_priv->master->lock.hw_lock->lock));
+	}
+}
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 03/14] drm: Link directly from drm_master to drm_device
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
  2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
  2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
@ 2016-06-14 18:50 ` Daniel Vetter
  2016-06-15 11:29   ` Chris Wilson
  2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
  2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
                   ` (11 subsequent siblings)
  14 siblings, 2 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Master-based auth only exists for the legacy/primary drm_minor, hence
there can only be one per device. The goal here is to untangle the
epic dereference games of minor->master and master->minor which is
just massively confusing.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c      | 6 +++---
 drivers/gpu/drm/drm_fops.c     | 2 +-
 drivers/gpu/drm/drm_internal.h | 2 +-
 include/drm/drmP.h             | 7 +++++--
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8b2582aeaab6..3c01a16bbb77 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -93,7 +93,7 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...)
 }
 EXPORT_SYMBOL(drm_ut_debug_printk);
 
-struct drm_master *drm_master_create(struct drm_minor *minor)
+struct drm_master *drm_master_create(struct drm_device *dev)
 {
 	struct drm_master *master;
 
@@ -105,7 +105,7 @@ struct drm_master *drm_master_create(struct drm_minor *minor)
 	spin_lock_init(&master->lock.spinlock);
 	init_waitqueue_head(&master->lock.lock_queue);
 	idr_init(&master->magic_map);
-	master->minor = minor;
+	master->dev = dev;
 
 	return master;
 }
@@ -120,7 +120,7 @@ EXPORT_SYMBOL(drm_master_get);
 static void drm_master_destroy(struct kref *kref)
 {
 	struct drm_master *master = container_of(kref, struct drm_master, refcount);
-	struct drm_device *dev = master->minor->dev;
+	struct drm_device *dev = master->dev;
 
 	if (dev->driver->master_destroy)
 		dev->driver->master_destroy(dev, master);
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 2fd4f42b907a..bfbf1381f55d 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -185,7 +185,7 @@ int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 	lockdep_assert_held_once(&dev->master_mutex);
 
 	/* create a new master */
-	fpriv->minor->master = drm_master_create(fpriv->minor);
+	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
 	if (!fpriv->minor->master)
 		return -ENOMEM;
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 56a9b1cf99d7..f5c1d17fa51f 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -92,7 +92,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
-struct drm_master *drm_master_create(struct drm_minor *minor);
+struct drm_master *drm_master_create(struct drm_device *dev);
 
 /* drm_debugfs.c */
 #if defined(CONFIG_DEBUG_FS)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cd3995d82477..17dcbea4f259 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -379,16 +379,19 @@ struct drm_lock_data {
  * struct drm_master - drm master structure
  *
  * @refcount: Refcount for this master object.
- * @minor: Link back to minor char device we are master for. Immutable.
+ * @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.
  * @driver_priv: Pointer to driver-private information.
+ *
+ * Note that master structures are only relevant for the legacy/primary device
+ * nodes, hence there can only be one per device, not one per drm_minor.
  */
 struct drm_master {
 	struct kref refcount;
-	struct drm_minor *minor;
+	struct drm_device *dev;
 	char *unique;
 	int unique_len;
 	struct idr magic_map;
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 04/14] drm: Move master functions into drm_auth.c
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (2 preceding siblings ...)
  2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
@ 2016-06-14 18:50 ` Daniel Vetter
  2016-06-15 11:31   ` Chris Wilson
  2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
                   ` (10 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:50 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

For modern drivers pretty much the only thing drm_master does is
handling authentication for the primary/legacy drm_minor node. Instead
of having it all over drm files, move it all together into drm_auth.c.

This patch just does code-motion, follow up patches will also extract
the master logic from file open&release paths.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c     | 163 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c      | 108 ---------------------------
 drivers/gpu/drm/drm_fops.c     |  54 --------------
 drivers/gpu/drm/drm_internal.h |  12 ++-
 include/drm/drmP.h             |   1 -
 5 files changed, 168 insertions(+), 170 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 50d0baa06db0..1d314ae13560 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -30,6 +30,7 @@
 
 #include <drm/drmP.h>
 #include "drm_internal.h"
+#include "drm_legacy.h"
 
 /**
  * drm_getmagic - Get unique magic of a client
@@ -91,3 +92,165 @@ int drm_authmagic(struct drm_device *dev, void *data,
 
 	return file ? 0 : -EINVAL;
 }
+
+static struct drm_master *drm_master_create(struct drm_device *dev)
+{
+	struct drm_master *master;
+
+	master = kzalloc(sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return NULL;
+
+	kref_init(&master->refcount);
+	spin_lock_init(&master->lock.spinlock);
+	init_waitqueue_head(&master->lock.lock_queue);
+	idr_init(&master->magic_map);
+	master->dev = dev;
+
+	return master;
+}
+
+/*
+ * drm_new_set_master - Allocate a new master object and become master for the
+ * associated master realm.
+ *
+ * @dev: The associated device.
+ * @fpriv: File private identifying the client.
+ *
+ * This function must be called with dev::struct_mutex held.
+ * Returns negative error code on failure. Zero on success.
+ */
+int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
+{
+	struct drm_master *old_master;
+	int ret;
+
+	lockdep_assert_held_once(&dev->master_mutex);
+
+	/* create a new master */
+	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
+	if (!fpriv->minor->master)
+		return -ENOMEM;
+
+	/* take another reference for the copy in the local file priv */
+	old_master = fpriv->master;
+	fpriv->master = drm_master_get(fpriv->minor->master);
+
+	if (dev->driver->master_create) {
+		ret = dev->driver->master_create(dev, fpriv->master);
+		if (ret)
+			goto out_err;
+	}
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, fpriv, true);
+		if (ret)
+			goto out_err;
+	}
+
+	fpriv->is_master = 1;
+	fpriv->allowed_master = 1;
+	fpriv->authenticated = 1;
+	if (old_master)
+		drm_master_put(&old_master);
+
+	return 0;
+
+out_err:
+	/* drop both references and restore old master on failure */
+	drm_master_put(&fpriv->minor->master);
+	drm_master_put(&fpriv->master);
+	fpriv->master = old_master;
+
+	return ret;
+}
+
+int drm_setmaster_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file_priv)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->master_mutex);
+	if (file_priv->is_master)
+		goto out_unlock;
+
+	if (file_priv->minor->master) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (!file_priv->master) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (!file_priv->allowed_master) {
+		ret = drm_new_set_master(dev, file_priv);
+		goto out_unlock;
+	}
+
+	file_priv->minor->master = drm_master_get(file_priv->master);
+	file_priv->is_master = 1;
+	if (dev->driver->master_set) {
+		ret = dev->driver->master_set(dev, file_priv, false);
+		if (unlikely(ret != 0)) {
+			file_priv->is_master = 0;
+			drm_master_put(&file_priv->minor->master);
+		}
+	}
+
+out_unlock:
+	mutex_unlock(&dev->master_mutex);
+	return ret;
+}
+
+int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv)
+{
+	int ret = -EINVAL;
+
+	mutex_lock(&dev->master_mutex);
+	if (!file_priv->is_master)
+		goto out_unlock;
+
+	if (!file_priv->minor->master)
+		goto out_unlock;
+
+	ret = 0;
+	if (dev->driver->master_drop)
+		dev->driver->master_drop(dev, file_priv, false);
+	drm_master_put(&file_priv->minor->master);
+	file_priv->is_master = 0;
+
+out_unlock:
+	mutex_unlock(&dev->master_mutex);
+	return ret;
+}
+
+struct drm_master *drm_master_get(struct drm_master *master)
+{
+	kref_get(&master->refcount);
+	return master;
+}
+EXPORT_SYMBOL(drm_master_get);
+
+static void drm_master_destroy(struct kref *kref)
+{
+	struct drm_master *master = container_of(kref, struct drm_master, refcount);
+	struct drm_device *dev = master->dev;
+
+	if (dev->driver->master_destroy)
+		dev->driver->master_destroy(dev, master);
+
+	drm_legacy_master_rmmaps(dev, master);
+
+	idr_destroy(&master->magic_map);
+	kfree(master->unique);
+	kfree(master);
+}
+
+void drm_master_put(struct drm_master **master)
+{
+	kref_put(&(*master)->refcount, drm_master_destroy);
+	*master = NULL;
+}
+EXPORT_SYMBOL(drm_master_put);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3c01a16bbb77..8e67b4f4573e 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -93,114 +93,6 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...)
 }
 EXPORT_SYMBOL(drm_ut_debug_printk);
 
-struct drm_master *drm_master_create(struct drm_device *dev)
-{
-	struct drm_master *master;
-
-	master = kzalloc(sizeof(*master), GFP_KERNEL);
-	if (!master)
-		return NULL;
-
-	kref_init(&master->refcount);
-	spin_lock_init(&master->lock.spinlock);
-	init_waitqueue_head(&master->lock.lock_queue);
-	idr_init(&master->magic_map);
-	master->dev = dev;
-
-	return master;
-}
-
-struct drm_master *drm_master_get(struct drm_master *master)
-{
-	kref_get(&master->refcount);
-	return master;
-}
-EXPORT_SYMBOL(drm_master_get);
-
-static void drm_master_destroy(struct kref *kref)
-{
-	struct drm_master *master = container_of(kref, struct drm_master, refcount);
-	struct drm_device *dev = master->dev;
-
-	if (dev->driver->master_destroy)
-		dev->driver->master_destroy(dev, master);
-
-	drm_legacy_master_rmmaps(dev, master);
-
-	idr_destroy(&master->magic_map);
-	kfree(master->unique);
-	kfree(master);
-}
-
-void drm_master_put(struct drm_master **master)
-{
-	kref_put(&(*master)->refcount, drm_master_destroy);
-	*master = NULL;
-}
-EXPORT_SYMBOL(drm_master_put);
-
-int drm_setmaster_ioctl(struct drm_device *dev, void *data,
-			struct drm_file *file_priv)
-{
-	int ret = 0;
-
-	mutex_lock(&dev->master_mutex);
-	if (file_priv->is_master)
-		goto out_unlock;
-
-	if (file_priv->minor->master) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
-
-	if (!file_priv->master) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
-
-	if (!file_priv->allowed_master) {
-		ret = drm_new_set_master(dev, file_priv);
-		goto out_unlock;
-	}
-
-	file_priv->minor->master = drm_master_get(file_priv->master);
-	file_priv->is_master = 1;
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, file_priv, false);
-		if (unlikely(ret != 0)) {
-			file_priv->is_master = 0;
-			drm_master_put(&file_priv->minor->master);
-		}
-	}
-
-out_unlock:
-	mutex_unlock(&dev->master_mutex);
-	return ret;
-}
-
-int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
-			 struct drm_file *file_priv)
-{
-	int ret = -EINVAL;
-
-	mutex_lock(&dev->master_mutex);
-	if (!file_priv->is_master)
-		goto out_unlock;
-
-	if (!file_priv->minor->master)
-		goto out_unlock;
-
-	ret = 0;
-	if (dev->driver->master_drop)
-		dev->driver->master_drop(dev, file_priv, false);
-	drm_master_put(&file_priv->minor->master);
-	file_priv->is_master = 0;
-
-out_unlock:
-	mutex_unlock(&dev->master_mutex);
-	return ret;
-}
-
 /*
  * DRM Minors
  * A DRM device can provide several char-dev interfaces on the DRM-Major. Each
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index bfbf1381f55d..50b3de990aee 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -168,60 +168,6 @@ static int drm_cpu_valid(void)
 }
 
 /*
- * drm_new_set_master - Allocate a new master object and become master for the
- * associated master realm.
- *
- * @dev: The associated device.
- * @fpriv: File private identifying the client.
- *
- * This function must be called with dev::struct_mutex held.
- * Returns negative error code on failure. Zero on success.
- */
-int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
-{
-	struct drm_master *old_master;
-	int ret;
-
-	lockdep_assert_held_once(&dev->master_mutex);
-
-	/* create a new master */
-	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
-	if (!fpriv->minor->master)
-		return -ENOMEM;
-
-	/* take another reference for the copy in the local file priv */
-	old_master = fpriv->master;
-	fpriv->master = drm_master_get(fpriv->minor->master);
-
-	if (dev->driver->master_create) {
-		ret = dev->driver->master_create(dev, fpriv->master);
-		if (ret)
-			goto out_err;
-	}
-	if (dev->driver->master_set) {
-		ret = dev->driver->master_set(dev, fpriv, true);
-		if (ret)
-			goto out_err;
-	}
-
-	fpriv->is_master = 1;
-	fpriv->allowed_master = 1;
-	fpriv->authenticated = 1;
-	if (old_master)
-		drm_master_put(&old_master);
-
-	return 0;
-
-out_err:
-	/* drop both references and restore old master on failure */
-	drm_master_put(&fpriv->minor->master);
-	drm_master_put(&fpriv->master);
-	fpriv->master = old_master;
-
-	return ret;
-}
-
-/*
  * Called whenever a process opens /dev/drm.
  *
  * \param filp file pointer.
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f5c1d17fa51f..d29d426f633f 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -62,6 +62,11 @@ int drm_getmagic(struct drm_device *dev, void *data,
 		 struct drm_file *file_priv);
 int drm_authmagic(struct drm_device *dev, void *data,
 		  struct drm_file *file_priv);
+int drm_setmaster_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file_priv);
+int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_priv);
+int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
 
 /* drm_sysfs.c */
 extern struct class *drm_class;
@@ -87,13 +92,6 @@ int drm_gem_open_ioctl(struct drm_device *dev, void *data,
 void drm_gem_open(struct drm_device *dev, struct drm_file *file_private);
 void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);
 
-/* drm_drv.c */
-int drm_setmaster_ioctl(struct drm_device *dev, void *data,
-			struct drm_file *file_priv);
-int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
-			 struct drm_file *file_priv);
-struct drm_master *drm_master_create(struct drm_device *dev);
-
 /* drm_debugfs.c */
 #if defined(CONFIG_DEBUG_FS)
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 17dcbea4f259..e8788d7b29dc 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -928,7 +928,6 @@ int drm_open(struct inode *inode, struct file *filp);
 ssize_t drm_read(struct file *filp, char __user *buffer,
 		 size_t count, loff_t *offset);
 int drm_release(struct inode *inode, struct file *filp);
-int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
 unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
 int drm_event_reserve_init_locked(struct drm_device *dev,
 				  struct drm_file *file_priv,
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 05/14] drm: Extract drm_master_open
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (3 preceding siblings ...)
  2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:41   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development, Julia Lawall

And pull out the primary_client check to make it really obvious that
this can't happen on control/render nodes. Bonus that we can avoid the
master lock in this case.

v2: Don't leak locks on error path (and simplify control flow while
at it), reported by Julia.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c     | 19 ++++++++++++++++++-
 drivers/gpu/drm/drm_fops.c     | 13 ++-----------
 drivers/gpu/drm/drm_internal.h |  2 +-
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 1d314ae13560..9c1e2081cd58 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -120,7 +120,7 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
  * This function must be called with dev::struct_mutex held.
  * Returns negative error code on failure. Zero on success.
  */
-int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
+static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 {
 	struct drm_master *old_master;
 	int ret;
@@ -226,6 +226,23 @@ out_unlock:
 	return ret;
 }
 
+int drm_master_open(struct drm_file *file_priv)
+{
+	struct drm_device *dev = file_priv->minor->dev;
+	int ret = 0;
+
+	/* if there is no current master make this fd it, but do not create
+	 * any master object for render clients */
+	mutex_lock(&dev->master_mutex);
+	if (!file_priv->minor->master)
+		ret = drm_new_set_master(dev, file_priv);
+	else
+		file_priv->master = drm_master_get(file_priv->minor->master);
+	mutex_unlock(&dev->master_mutex);
+
+	return ret;
+}
+
 struct drm_master *drm_master_get(struct drm_master *master)
 {
 	kref_get(&master->refcount);
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 50b3de990aee..e2522672719a 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -229,19 +229,11 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 			goto out_prime_destroy;
 	}
 
-	/* if there is no current master make this fd it, but do not create
-	 * any master object for render clients */
-	mutex_lock(&dev->master_mutex);
-	if (drm_is_primary_client(priv) && !priv->minor->master) {
-		/* create a new master */
-		ret = drm_new_set_master(dev, priv);
+	if (drm_is_primary_client(priv)) {
+		ret = drm_master_open(priv);
 		if (ret)
 			goto out_close;
-	} else if (drm_is_primary_client(priv)) {
-		/* get a reference to the master */
-		priv->master = drm_master_get(priv->minor->master);
 	}
-	mutex_unlock(&dev->master_mutex);
 
 	mutex_lock(&dev->filelist_mutex);
 	list_add(&priv->lhead, &dev->filelist);
@@ -270,7 +262,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	return 0;
 
 out_close:
-	mutex_unlock(&dev->master_mutex);
 	if (dev->driver->postclose)
 		dev->driver->postclose(dev, priv);
 out_prime_destroy:
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index d29d426f633f..2b9a94f679ed 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -66,7 +66,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
-int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv);
+int drm_master_open(struct drm_file *file_priv);
 
 /* drm_sysfs.c */
 extern struct class *drm_class;
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 06/14] drm: Extract drm_master_relase
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (4 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:43   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Like with drm_master_open protect it with a check for primary_client
to make it clear that this can't happen on render/control nodes.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c     | 37 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_fops.c     | 35 ++---------------------------------
 drivers/gpu/drm/drm_internal.h |  1 +
 3 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 9c1e2081cd58..e015a7edb154 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -243,6 +243,43 @@ int drm_master_open(struct drm_file *file_priv)
 	return ret;
 }
 
+void drm_master_release(struct drm_file *file_priv)
+{
+	struct drm_device *dev = file_priv->minor->dev;
+
+	mutex_lock(&dev->master_mutex);
+	if (file_priv->is_master) {
+		struct drm_master *master = file_priv->master;
+
+		/*
+		 * Since the master is disappearing, so is the
+		 * possibility to lock.
+		 */
+		mutex_lock(&dev->struct_mutex);
+		if (master->lock.hw_lock) {
+			if (dev->sigdata.lock == master->lock.hw_lock)
+				dev->sigdata.lock = NULL;
+			master->lock.hw_lock = NULL;
+			master->lock.file_priv = NULL;
+			wake_up_interruptible_all(&master->lock.lock_queue);
+		}
+		mutex_unlock(&dev->struct_mutex);
+
+		if (file_priv->minor->master == file_priv->master) {
+			/* drop the reference held my the minor */
+			if (dev->driver->master_drop)
+				dev->driver->master_drop(dev, file_priv, true);
+			drm_master_put(&file_priv->minor->master);
+		}
+	}
+
+	/* drop the master reference held by the file priv */
+	if (file_priv->master)
+		drm_master_put(&file_priv->master);
+	file_priv->is_master = 0;
+	mutex_unlock(&dev->master_mutex);
+}
+
 struct drm_master *drm_master_get(struct drm_master *master)
 {
 	kref_get(&master->refcount);
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index e2522672719a..f3b2677de882 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -411,43 +411,12 @@ int drm_release(struct inode *inode, struct file *filp)
 
 	drm_legacy_ctxbitmap_flush(dev, file_priv);
 
-	mutex_lock(&dev->master_mutex);
-
-	if (file_priv->is_master) {
-		struct drm_master *master = file_priv->master;
-
-		/*
-		 * Since the master is disappearing, so is the
-		 * possibility to lock.
-		 */
-		mutex_lock(&dev->struct_mutex);
-		if (master->lock.hw_lock) {
-			if (dev->sigdata.lock == master->lock.hw_lock)
-				dev->sigdata.lock = NULL;
-			master->lock.hw_lock = NULL;
-			master->lock.file_priv = NULL;
-			wake_up_interruptible_all(&master->lock.lock_queue);
-		}
-		mutex_unlock(&dev->struct_mutex);
-
-		if (file_priv->minor->master == file_priv->master) {
-			/* drop the reference held my the minor */
-			if (dev->driver->master_drop)
-				dev->driver->master_drop(dev, file_priv, true);
-			drm_master_put(&file_priv->minor->master);
-		}
-	}
-
-	/* drop the master reference held by the file priv */
-	if (file_priv->master)
-		drm_master_put(&file_priv->master);
-	file_priv->is_master = 0;
-	mutex_unlock(&dev->master_mutex);
+	if (drm_is_primary_client(file_priv))
+		drm_master_release(file_priv);
 
 	if (dev->driver->postclose)
 		dev->driver->postclose(dev, file_priv);
 
-
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_destroy_file_private(&file_priv->prime);
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 2b9a94f679ed..38401d406532 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -67,6 +67,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
 int drm_master_open(struct drm_file *file_priv);
+void drm_master_release(struct drm_file *file_priv);
 
 /* drm_sysfs.c */
 extern struct class *drm_class;
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (5 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:48   ` Chris Wilson
  2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
                   ` (7 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Another place gone where modern drivers could have hit
dev->struct_mutex.

To avoid too deeply nesting control flow rework it a bit.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index e015a7edb154..a58b2eb0d004 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv)
 void drm_master_release(struct drm_file *file_priv)
 {
 	struct drm_device *dev = file_priv->minor->dev;
+	struct drm_master *master = file_priv->master;
+
 
 	mutex_lock(&dev->master_mutex);
-	if (file_priv->is_master) {
-		struct drm_master *master = file_priv->master;
+	if (!file_priv->is_master)
+		goto out_unlock;
 
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
 		/*
 		 * Since the master is disappearing, so is the
 		 * possibility to lock.
@@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv)
 			wake_up_interruptible_all(&master->lock.lock_queue);
 		}
 		mutex_unlock(&dev->struct_mutex);
+	}
 
-		if (file_priv->minor->master == file_priv->master) {
-			/* drop the reference held my the minor */
-			if (dev->driver->master_drop)
-				dev->driver->master_drop(dev, file_priv, true);
-			drm_master_put(&file_priv->minor->master);
-		}
+	if (file_priv->minor->master == file_priv->master) {
+		/* drop the reference held my the minor */
+		if (dev->driver->master_drop)
+			dev->driver->master_drop(dev, file_priv, true);
+		drm_master_put(&file_priv->minor->master);
 	}
 
 	/* drop the master reference held by the file priv */
 	if (file_priv->master)
 		drm_master_put(&file_priv->master);
 	file_priv->is_master = 0;
+out_unlock:
 	mutex_unlock(&dev->master_mutex);
 }
 
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (6 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:51   ` Chris Wilson
  2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
                   ` (6 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

It's related, and soon authmagic will also use the master_mutex.

There is an ever-so-slightly semantic change here:
- authmagic will only be cleaned up for primary_client drm_minors. But
  it's impossible to create authmagic on render/control nodes, so this
  is fine.
- The cleanup is moved down a bit in the release processing. Doesn't
  matter at all since authmagic is purely internal logic used by the
  core ioctl access checks, and when we're in a file's release
  callback no one can do ioctls any more.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c | 4 ++++
 drivers/gpu/drm/drm_fops.c | 5 -----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index a58b2eb0d004..e37d657dbf5f 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -248,6 +248,10 @@ void drm_master_release(struct drm_file *file_priv)
 	struct drm_device *dev = file_priv->minor->dev;
 	struct drm_master *master = file_priv->master;
 
+	mutex_lock(&dev->struct_mutex);
+	if (file_priv->magic)
+		idr_remove(&file_priv->master->magic_map, file_priv->magic);
+	mutex_unlock(&dev->struct_mutex);
 
 	mutex_lock(&dev->master_mutex);
 	if (!file_priv->is_master)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index f3b2677de882..f6dfdfcd018b 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -376,11 +376,6 @@ int drm_release(struct inode *inode, struct file *filp)
 	list_del(&file_priv->lhead);
 	mutex_unlock(&dev->filelist_mutex);
 
-	mutex_lock(&dev->struct_mutex);
-	if (file_priv->magic)
-		idr_remove(&file_priv->master->magic_map, file_priv->magic);
-	mutex_unlock(&dev->struct_mutex);
-
 	if (dev->driver->preclose)
 		dev->driver->preclose(dev, file_priv);
 
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 09/14] drm: Protect authmagic with master_mutex
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (7 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:54   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
                   ` (5 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Simplifies cleanup, and there's no reason drivers should ever care
about authmagic at all - it's all handled in the core.

And with that, Ladies and Gentlemen, it's time to pop the champagen
and celebrate: dev->struct_mutex is now officially gone from modern
drivers, and if a driver is using gem_free_object_unlocked and doesn't
do anything else silly it's positively impossible to ever touch
dev->struct_mutex at runtime, anywhere.

Well except for the mutex_init on driver load ;-)

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index e37d657dbf5f..94761e4bd1e5 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -49,7 +49,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 	struct drm_auth *auth = data;
 	int ret = 0;
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev->master_mutex);
 	if (!file_priv->magic) {
 		ret = idr_alloc(&file_priv->master->magic_map, file_priv,
 				1, 0, GFP_KERNEL);
@@ -57,7 +57,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			file_priv->magic = ret;
 	}
 	auth->magic = file_priv->magic;
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&dev->master_mutex);
 
 	DRM_DEBUG("%u\n", auth->magic);
 
@@ -82,13 +82,13 @@ int drm_authmagic(struct drm_device *dev, void *data,
 
 	DRM_DEBUG("%u\n", auth->magic);
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev->master_mutex);
 	file = idr_find(&file_priv->master->magic_map, auth->magic);
 	if (file) {
 		file->authenticated = 1;
 		idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
 	}
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&dev->master_mutex);
 
 	return file ? 0 : -EINVAL;
 }
@@ -117,7 +117,7 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
  * @dev: The associated device.
  * @fpriv: File private identifying the client.
  *
- * This function must be called with dev::struct_mutex held.
+ * This function must be called with dev::master_mutex held.
  * Returns negative error code on failure. Zero on success.
  */
 static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
@@ -248,12 +248,10 @@ void drm_master_release(struct drm_file *file_priv)
 	struct drm_device *dev = file_priv->minor->dev;
 	struct drm_master *master = file_priv->master;
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev->master_mutex);
 	if (file_priv->magic)
 		idr_remove(&file_priv->master->magic_map, file_priv->magic);
-	mutex_unlock(&dev->struct_mutex);
 
-	mutex_lock(&dev->master_mutex);
 	if (!file_priv->is_master)
 		goto out_unlock;
 
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 10/14] drm: Mark authmagic ioctls as unlocked
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (8 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:55   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
                   ` (4 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

All protected by dev->master_mutex. And there's no driver callbacks,
which means no need to sync with old dri1 horror show drivers at all.
Hence safe to drop the drm legacy BKL from these paths.

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

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index b7a39771c152..e4ccb5c3fdea 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -504,7 +504,7 @@ 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_MAGIC, drm_getmagic, 0),
+	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),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENT, drm_getclient, DRM_UNLOCKED),
@@ -516,7 +516,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_setunique, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
-	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_AUTH|DRM_MASTER),
+	DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_AUTH|DRM_UNLOCKED|DRM_MASTER),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_ADD_MAP, drm_legacy_addmap_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_RM_MAP, drm_legacy_rmmap_ioctl, DRM_AUTH),
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 11/14] drm: Mark set/drop master ioctl as unlocked.
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (9 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 11:56   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
                   ` (3 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Again this is neatly protected by the dev->master_mutex now. There is
a driver callback both for set and drop, but it's only used by vmwgfx.
And vmwgfx has it's own solid locking for shared resources (besides
dev->master_mutex), hence is all safe. Let's drop another place where
the drm legacy bkl is used.

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

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index e4ccb5c3fdea..11eda9050215 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -524,8 +524,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_SAREA_CTX, drm_legacy_setsareactx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH),
 
-	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_ROOT_ONLY),
-	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_ROOT_ONLY),
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
+	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (10 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15 12:10   ` [Intel-gfx] " Chris Wilson
  2016-06-14 18:51 ` [PATCH 13/14] drm: Clean up drm_crtc.h Daniel Vetter
                   ` (2 subsequent siblings)
  14 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development, Julia Lawall

There can only be one current master, and it's for the overall device.
Render/control minors don't support master-based auth at all.

This simplifies the master logic a lot, at least in my eyes: All these
additional pointer chases are just confusing.

While doing the conversion I spotted some locking fail:
- drm_lock/drm_auth check dev->master without holding the
  master_mutex. This is fallout from

  commit c996fd0b956450563454e7ccc97a82ca31f9d043
  Author: Thomas Hellstrom <thellstrom@vmware.com>
  Date:   Tue Feb 25 19:57:44 2014 +0100

      drm: Protect the master management with a drm_device::master_mutex v3

  but I honestly don't care one bit about those old legacy drivers
  using this.

- debugfs name info should just grab master_mutex.

- And the fbdev helper looked at it to figure out whether someone is
  using KMS. We just need a consistent value, so READ_ONCE. Aside: We
  should probably check if anyone has opened a control node too, but I
  guess current userspace doesn't really do that yet.

v2: Balance locking, reported by Julia.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_auth.c      | 26 +++++++++++++-------------
 drivers/gpu/drm/drm_bufs.c      |  8 ++++----
 drivers/gpu/drm/drm_fb_helper.c |  2 +-
 drivers/gpu/drm/drm_info.c      | 10 ++++++++--
 drivers/gpu/drm/drm_lock.c      |  2 +-
 drivers/gpu/drm/sis/sis_mm.c    |  2 +-
 drivers/gpu/drm/via/via_mm.c    |  2 +-
 include/drm/drmP.h              |  9 +++++----
 8 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 94761e4bd1e5..fcadd8e1de28 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 	lockdep_assert_held_once(&dev->master_mutex);
 
 	/* create a new master */
-	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
-	if (!fpriv->minor->master)
+	dev->master = drm_master_create(dev);
+	if (!dev->master)
 		return -ENOMEM;
 
 	/* take another reference for the copy in the local file priv */
 	old_master = fpriv->master;
-	fpriv->master = drm_master_get(fpriv->minor->master);
+	fpriv->master = drm_master_get(dev->master);
 
 	if (dev->driver->master_create) {
 		ret = dev->driver->master_create(dev, fpriv->master);
@@ -157,7 +157,7 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
 
 out_err:
 	/* drop both references and restore old master on failure */
-	drm_master_put(&fpriv->minor->master);
+	drm_master_put(&dev->master);
 	drm_master_put(&fpriv->master);
 	fpriv->master = old_master;
 
@@ -173,7 +173,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 	if (file_priv->is_master)
 		goto out_unlock;
 
-	if (file_priv->minor->master) {
+	if (dev->master) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -188,13 +188,13 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
 		goto out_unlock;
 	}
 
-	file_priv->minor->master = drm_master_get(file_priv->master);
+	dev->master = drm_master_get(file_priv->master);
 	file_priv->is_master = 1;
 	if (dev->driver->master_set) {
 		ret = dev->driver->master_set(dev, file_priv, false);
 		if (unlikely(ret != 0)) {
 			file_priv->is_master = 0;
-			drm_master_put(&file_priv->minor->master);
+			drm_master_put(&dev->master);
 		}
 	}
 
@@ -212,13 +212,13 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
 	if (!file_priv->is_master)
 		goto out_unlock;
 
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		goto out_unlock;
 
 	ret = 0;
 	if (dev->driver->master_drop)
 		dev->driver->master_drop(dev, file_priv, false);
-	drm_master_put(&file_priv->minor->master);
+	drm_master_put(&dev->master);
 	file_priv->is_master = 0;
 
 out_unlock:
@@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
 	/* if there is no current master make this fd it, but do not create
 	 * any master object for render clients */
 	mutex_lock(&dev->master_mutex);
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		ret = drm_new_set_master(dev, file_priv);
 	else
-		file_priv->master = drm_master_get(file_priv->minor->master);
+		file_priv->master = drm_master_get(dev->master);
 	mutex_unlock(&dev->master_mutex);
 
 	return ret;
@@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
-	if (file_priv->minor->master == file_priv->master) {
+	if (dev->master == file_priv->master) {
 		/* drop the reference held my the minor */
 		if (dev->driver->master_drop)
 			dev->driver->master_drop(dev, file_priv, true);
-		drm_master_put(&file_priv->minor->master);
+		drm_master_put(&dev->master);
 	}
 
 	/* drop the master reference held by the file priv */
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 9b34158c0f77..c3a12cd8bd0d 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -51,7 +51,7 @@ static struct drm_map_list *drm_find_matching_map(struct drm_device *dev,
 		 */
 		if (!entry->map ||
 		    map->type != entry->map->type ||
-		    entry->master != dev->primary->master)
+		    entry->master != dev->master)
 			continue;
 		switch (map->type) {
 		case _DRM_SHM:
@@ -245,12 +245,12 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 		map->offset = (unsigned long)map->handle;
 		if (map->flags & _DRM_CONTAINS_LOCK) {
 			/* Prevent a 2nd X Server from creating a 2nd lock */
-			if (dev->primary->master->lock.hw_lock != NULL) {
+			if (dev->master->lock.hw_lock != NULL) {
 				vfree(map->handle);
 				kfree(map);
 				return -EBUSY;
 			}
-			dev->sigdata.lock = dev->primary->master->lock.hw_lock = map->handle;	/* Pointer to lock */
+			dev->sigdata.lock = dev->master->lock.hw_lock = map->handle;	/* Pointer to lock */
 		}
 		break;
 	case _DRM_AGP: {
@@ -356,7 +356,7 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 	mutex_unlock(&dev->struct_mutex);
 
 	if (!(map->flags & _DRM_DRIVER))
-		list->master = dev->primary->master;
+		list->master = dev->master;
 	*maplist = list;
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 0bac5246e5a7..0a06f9120b5a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 
 	/* Sometimes user space wants everything disabled, so don't steal the
 	 * display if there's a master. */
-	if (dev->primary->master)
+	if (READ_ONCE(dev->master))
 		return false;
 
 	drm_for_each_crtc(crtc, dev) {
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 0090d5987801..24c80dd13837 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -50,9 +50,12 @@ int drm_name_info(struct seq_file *m, void *data)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_minor *minor = node->minor;
 	struct drm_device *dev = minor->dev;
-	struct drm_master *master = minor->master;
+	struct drm_master *master;
+
+	mutex_lock(&dev->master_mutex);
+	master = dev->master;
 	if (!master)
-		return 0;
+		goto out_unlock;
 
 	if (master->unique) {
 		seq_printf(m, "%s %s %s\n",
@@ -62,6 +65,9 @@ int drm_name_info(struct seq_file *m, void *data)
 		seq_printf(m, "%s %s\n",
 			   dev->driver->name, dev_name(dev->dev));
 	}
+out_unlock:
+	mutex_unlock(&dev->master_mutex);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index 0fb8f9e73486..ae0a4d39deec 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -334,7 +334,7 @@ void drm_legacy_lock_release(struct drm_device *dev, struct file *filp)
 	struct drm_file *file_priv = filp->private_data;
 
 	/* if the master has gone away we can't do anything with the lock */
-	if (!file_priv->minor->master)
+	if (!dev->master)
 		return;
 
 	if (drm_legacy_i_have_hw_lock(dev, file_priv)) {
diff --git a/drivers/gpu/drm/sis/sis_mm.c b/drivers/gpu/drm/sis/sis_mm.c
index 93ad8a5704d1..03defda77766 100644
--- a/drivers/gpu/drm/sis/sis_mm.c
+++ b/drivers/gpu/drm/sis/sis_mm.c
@@ -316,7 +316,7 @@ void sis_reclaim_buffers_locked(struct drm_device *dev,
 	struct sis_file_private *file_priv = file->driver_priv;
 	struct sis_memblock *entry, *next;
 
-	if (!(file->minor->master && file->master->lock.hw_lock))
+	if (!(dev->master && file->master->lock.hw_lock))
 		return;
 
 	drm_legacy_idlelock_take(&file->master->lock);
diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c
index 4f20742e7788..a04ef1c992d9 100644
--- a/drivers/gpu/drm/via/via_mm.c
+++ b/drivers/gpu/drm/via/via_mm.c
@@ -208,7 +208,7 @@ void via_reclaim_buffers_locked(struct drm_device *dev,
 	struct via_file_private *file_priv = file->driver_priv;
 	struct via_memblock *entry, *next;
 
-	if (!(file->minor->master && file->master->lock.hw_lock))
+	if (!(dev->master && file->master->lock.hw_lock))
 		return;
 
 	drm_legacy_idlelock_take(&file->master->lock);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index e8788d7b29dc..62e0e70cb5a2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -336,7 +336,7 @@ struct drm_file {
 	void *driver_priv;
 
 	struct drm_master *master; /* master this node is currently associated with
-				      N.B. not always minor->master */
+				      N.B. not always dev->master */
 	/**
 	 * fbs - List of framebuffers associated with this file.
 	 *
@@ -708,9 +708,6 @@ struct drm_minor {
 
 	struct list_head debugfs_list;
 	struct mutex debugfs_lock; /* Protects debugfs_list. */
-
-	/* currently active master for this node. Protected by master_mutex */
-	struct drm_master *master;
 };
 
 
@@ -759,6 +756,10 @@ struct drm_device {
 	struct drm_minor *control;		/**< Control node */
 	struct drm_minor *primary;		/**< Primary node */
 	struct drm_minor *render;		/**< Render node */
+
+	/* currently active master for this device. Protected by master_mutex */
+	struct drm_master *master;
+
 	atomic_t unplugged;			/**< Flag whether dev is dead */
 	struct inode *anon_inode;		/**< inode for private address-space */
 	char *unique;				/**< unique name of the device */
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 13/14] drm: Clean up drm_crtc.h
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (11 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-14 18:51 ` [PATCH 14/14] drm: Use dev->name as fallback for dev->unique Daniel Vetter
  2016-06-15  5:42 ` ✓ Ro.CI.BAT: success for Cruft removal around drm_master Patchwork
  14 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

- Group declarations for separate files (drm_bridge.c, drm_edid.c)
- Move declarations only used within drm.ko to drm_crtc_internal.h
- drm_property_type_valid to drm_crtc.c, its only callsite

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c          |   7 ++
 drivers/gpu/drm/drm_crtc_internal.h |  86 ++++++++++++++++-
 drivers/gpu/drm/drm_drv.c           |   1 +
 drivers/gpu/drm/drm_fops.c          |   1 +
 include/drm/drm_crtc.h              | 188 +++++++++++-------------------------
 5 files changed, 150 insertions(+), 133 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4ec35f9e6de5..4ff818ad34d7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3656,6 +3656,13 @@ void drm_fb_release(struct drm_file *priv)
 	}
 }
 
+static bool drm_property_type_valid(struct drm_property *property)
+{
+	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
+		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
+	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
+}
+
 /**
  * drm_property_create - create a new property type
  * @dev: drm device
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index a78c138282ea..8186c0e05c42 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -31,14 +31,98 @@
  * and are not exported to drivers.
  */
 
+
+/* drm_crtc.c */
+void drm_connector_ida_init(void);
+void drm_connector_ida_destroy(void);
 int drm_mode_object_get(struct drm_device *dev,
 			struct drm_mode_object *obj, uint32_t obj_type);
 void drm_mode_object_unregister(struct drm_device *dev,
 				struct drm_mode_object *object);
+bool drm_property_change_valid_get(struct drm_property *property,
+				   uint64_t value,
+				   struct drm_mode_object **ref);
+void drm_property_change_valid_put(struct drm_property *property,
+				   struct drm_mode_object *ref);
+
+int drm_plane_check_pixel_format(const struct drm_plane *plane,
+				 u32 format);
+int drm_crtc_check_viewport(const struct drm_crtc *crtc,
+			    int x, int y,
+			    const struct drm_display_mode *mode,
+			    const struct drm_framebuffer *fb);
+
+void drm_fb_release(struct drm_file *file_priv);
+void drm_property_destroy_user_blobs(struct drm_device *dev,
+				     struct drm_file *file_priv);
+
+/* dumb buffer support IOCTLs */
+int drm_mode_create_dumb_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
+				void *data, struct drm_file *file_priv);
+
+/* framebuffer IOCTLs */
+extern int drm_mode_addfb(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+extern int drm_mode_addfb2(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_rmfb(struct drm_device *dev,
+			 void *data, struct drm_file *file_priv);
+int drm_mode_getfb(struct drm_device *dev,
+		   void *data, struct drm_file *file_priv);
+int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+
+/* IOCTLs */
+int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
+				      struct drm_file *file_priv);
+int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
+				    struct drm_file *file_priv);
+
+int drm_mode_getresources(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_getplane_res(struct drm_device *dev, void *data,
+			  struct drm_file *file_priv);
+int drm_mode_getcrtc(struct drm_device *dev,
+		     void *data, struct drm_file *file_priv);
+int drm_mode_getconnector(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_setcrtc(struct drm_device *dev,
+		     void *data, struct drm_file *file_priv);
+int drm_mode_getplane(struct drm_device *dev,
+		      void *data, struct drm_file *file_priv);
+int drm_mode_setplane(struct drm_device *dev,
+		      void *data, struct drm_file *file_priv);
+int drm_mode_cursor_ioctl(struct drm_device *dev,
+			  void *data, struct drm_file *file_priv);
+int drm_mode_cursor2_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_getproperty_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_getblob_ioctl(struct drm_device *dev,
+			   void *data, struct drm_file *file_priv);
+int drm_mode_createblob_ioctl(struct drm_device *dev,
+			      void *data, struct drm_file *file_priv);
+int drm_mode_destroyblob_ioctl(struct drm_device *dev,
+			       void *data, struct drm_file *file_priv);
+int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
+					  void *data, struct drm_file *file_priv);
+int drm_mode_getencoder(struct drm_device *dev,
+			void *data, struct drm_file *file_priv);
+int drm_mode_gamma_get_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+int drm_mode_gamma_set_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
+
+int drm_mode_page_flip_ioctl(struct drm_device *dev,
+			     void *data, struct drm_file *file_priv);
 
 /* drm_atomic.c */
 int drm_atomic_get_property(struct drm_mode_object *obj,
-			   struct drm_property *property, uint64_t *val);
+			    struct drm_property *property, uint64_t *val);
 int drm_mode_atomic_ioctl(struct drm_device *dev,
 			  void *data, struct drm_file *file_priv);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8e67b4f4573e..10afa2539181 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -36,6 +36,7 @@
 #include <drm/drm_core.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 /*
  * drm_debug: Enable debug output.
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index f6dfdfcd018b..323c238fcac7 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -40,6 +40,7 @@
 #include <linux/module.h>
 #include "drm_legacy.h"
 #include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 /* from BKL pushdown */
 DEFINE_MUTEX(drm_global_mutex);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 914baa8c161d..c1177eb534b9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -44,6 +44,7 @@ struct drm_file;
 struct drm_clip_rect;
 struct device_node;
 struct fence;
+struct edid;
 
 struct drm_mode_object {
 	uint32_t id;
@@ -2467,12 +2468,10 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc)
 	return 1 << drm_crtc_index(crtc);
 }
 
-extern void drm_connector_ida_init(void);
-extern void drm_connector_ida_destroy(void);
-extern int drm_connector_init(struct drm_device *dev,
-			      struct drm_connector *connector,
-			      const struct drm_connector_funcs *funcs,
-			      int connector_type);
+int drm_connector_init(struct drm_device *dev,
+		       struct drm_connector *connector,
+		       const struct drm_connector_funcs *funcs,
+		       int connector_type);
 int drm_connector_register(struct drm_connector *connector);
 void drm_connector_unregister(struct drm_connector *connector);
 
@@ -2486,22 +2485,6 @@ static inline unsigned drm_connector_index(struct drm_connector *connector)
 extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
 
-extern int drm_bridge_add(struct drm_bridge *bridge);
-extern void drm_bridge_remove(struct drm_bridge *bridge);
-extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
-extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
-
-bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
-			const struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
-void drm_bridge_disable(struct drm_bridge *bridge);
-void drm_bridge_post_disable(struct drm_bridge *bridge);
-void drm_bridge_mode_set(struct drm_bridge *bridge,
-			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
-void drm_bridge_pre_enable(struct drm_bridge *bridge);
-void drm_bridge_enable(struct drm_bridge *bridge);
-
 extern __printf(5, 6)
 int drm_encoder_init(struct drm_device *dev,
 		     struct drm_encoder *encoder,
@@ -2563,14 +2546,8 @@ static inline unsigned int drm_plane_index(struct drm_plane *plane)
 }
 extern struct drm_plane * drm_plane_from_index(struct drm_device *dev, int idx);
 extern void drm_plane_force_disable(struct drm_plane *plane);
-extern int drm_plane_check_pixel_format(const struct drm_plane *plane,
-					u32 format);
 extern void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 				   int *hdisplay, int *vdisplay);
-extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
-				   int x, int y,
-				   const struct drm_display_mode *mode,
-				   const struct drm_framebuffer *fb);
 
 extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
@@ -2581,16 +2558,6 @@ extern const char *drm_get_dvi_i_subconnector_name(int val);
 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 bool drm_probe_ddc(struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
-extern struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
-					    struct i2c_adapter *adapter);
-extern struct edid *drm_edid_duplicate(const struct edid *edid);
-extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_config_init(struct drm_device *dev);
 extern void drm_mode_config_reset(struct drm_device *dev);
 extern void drm_mode_config_cleanup(struct drm_device *dev);
@@ -2614,13 +2581,6 @@ static inline bool drm_property_type_is(struct drm_property *property,
 	return property->flags & type;
 }
 
-static inline bool drm_property_type_valid(struct drm_property *property)
-{
-	if (property->flags & DRM_MODE_PROP_EXTENDED_TYPE)
-		return !(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
-	return !!(property->flags & DRM_MODE_PROP_LEGACY_TYPE);
-}
-
 extern int drm_object_property_set_value(struct drm_mode_object *obj,
 					 struct drm_property *property,
 					 uint64_t val);
@@ -2678,86 +2638,15 @@ extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
 extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
-extern bool drm_property_change_valid_get(struct drm_property *property,
-					 uint64_t value, struct drm_mode_object **ref);
-extern void drm_property_change_valid_put(struct drm_property *property,
-		struct drm_mode_object *ref);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
 extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
-extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
-		uint32_t id, uint32_t type);
-void drm_mode_object_reference(struct drm_mode_object *obj);
-void drm_mode_object_unreference(struct drm_mode_object *obj);
 
-/* IOCTLs */
-extern int drm_mode_getresources(struct drm_device *dev,
-				 void *data, struct drm_file *file_priv);
-extern int drm_mode_getplane_res(struct drm_device *dev, void *data,
-				   struct drm_file *file_priv);
-extern int drm_mode_getcrtc(struct drm_device *dev,
-			    void *data, struct drm_file *file_priv);
-extern int drm_mode_getconnector(struct drm_device *dev,
-			      void *data, struct drm_file *file_priv);
 extern int drm_mode_set_config_internal(struct drm_mode_set *set);
-extern int drm_mode_setcrtc(struct drm_device *dev,
-			    void *data, struct drm_file *file_priv);
-extern int drm_mode_getplane(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_setplane(struct drm_device *dev,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_cursor_ioctl(struct drm_device *dev,
-				void *data, struct drm_file *file_priv);
-extern int drm_mode_cursor2_ioctl(struct drm_device *dev,
-				void *data, struct drm_file *file_priv);
-extern int drm_mode_addfb(struct drm_device *dev,
-			  void *data, struct drm_file *file_priv);
-extern int drm_mode_addfb2(struct drm_device *dev,
-			   void *data, struct drm_file *file_priv);
+
 extern uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth);
-extern int drm_mode_rmfb(struct drm_device *dev,
-			 void *data, struct drm_file *file_priv);
-extern int drm_mode_getfb(struct drm_device *dev,
-			  void *data, struct drm_file *file_priv);
-extern int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
-				  void *data, struct drm_file *file_priv);
-
-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,
-			       void *data, struct drm_file *file_priv);
-extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
-extern enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
-extern bool drm_detect_hdmi_monitor(struct edid *edid);
-extern bool drm_detect_monitor_audio(struct edid *edid);
-extern bool drm_rgb_quant_range_selectable(struct edid *edid);
-extern int drm_mode_page_flip_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_add_modes_noedid(struct drm_connector *connector,
-				int hdisplay, int vdisplay);
-extern void drm_set_preferred_mode(struct drm_connector *connector,
-				   int hpref, int vpref);
-
-extern int drm_edid_header_is_valid(const u8 *raw_edid);
-extern bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
-				 bool *edid_corrupt);
-extern bool drm_edid_is_valid(struct edid *edid);
-extern void drm_edid_get_monitor_name(struct edid *edid, char *name,
-				      int buflen);
 
 extern struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
 							 char topology[8]);
@@ -2765,25 +2654,10 @@ extern struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
 					       char topology[8]);
 extern void drm_mode_put_tile_group(struct drm_device *dev,
 				   struct drm_tile_group *tg);
-struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
-					   int hsize, int vsize, int fresh,
-					   bool rb);
 
-extern int drm_mode_create_dumb_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
-				    void *data, struct drm_file *file_priv);
-extern int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
-				      void *data, struct drm_file *file_priv);
-extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
-					     struct drm_file *file_priv);
-extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
-					   struct drm_file *file_priv);
 extern int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
 				       struct drm_property *property,
 				       uint64_t value);
-extern int drm_mode_atomic_ioctl(struct drm_device *dev,
-				 void *data, struct drm_file *file_priv);
 
 extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
 							      unsigned int supported_rotations);
@@ -2794,6 +2668,10 @@ extern void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
 				       bool has_ctm,
 				       uint gamma_lut_size);
 /* Helpers */
+struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
+					     uint32_t id, uint32_t type);
+void drm_mode_object_reference(struct drm_mode_object *obj);
+void drm_mode_object_unreference(struct drm_mode_object *obj);
 
 static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 		uint32_t id)
@@ -2959,4 +2837,50 @@ assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
 	     &fb->head != (&(dev)->mode_config.fb_list);			\
 	     fb = list_next_entry(fb, head))
 
+/* drm_edid.c */
+bool drm_probe_ddc(struct i2c_adapter *adapter);
+struct edid *drm_get_edid(struct drm_connector *connector,
+			  struct i2c_adapter *adapter);
+struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+				     struct i2c_adapter *adapter);
+struct edid *drm_edid_duplicate(const struct edid *edid);
+int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
+
+u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
+enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
+bool drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_monitor_audio(struct edid *edid);
+bool drm_rgb_quant_range_selectable(struct edid *edid);
+int drm_add_modes_noedid(struct drm_connector *connector,
+			 int hdisplay, int vdisplay);
+void drm_set_preferred_mode(struct drm_connector *connector,
+			    int hpref, int vpref);
+
+int drm_edid_header_is_valid(const u8 *raw_edid);
+bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
+			  bool *edid_corrupt);
+bool drm_edid_is_valid(struct edid *edid);
+void drm_edid_get_monitor_name(struct edid *edid, char *name,
+			       int buflen);
+struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
+					   int hsize, int vsize, int fresh,
+					   bool rb);
+
+/* drm_bridge.c */
+extern int drm_bridge_add(struct drm_bridge *bridge);
+extern void drm_bridge_remove(struct drm_bridge *bridge);
+extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
+extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
+
+bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
+			const struct drm_display_mode *mode,
+			struct drm_display_mode *adjusted_mode);
+void drm_bridge_disable(struct drm_bridge *bridge);
+void drm_bridge_post_disable(struct drm_bridge *bridge);
+void drm_bridge_mode_set(struct drm_bridge *bridge,
+			struct drm_display_mode *mode,
+			struct drm_display_mode *adjusted_mode);
+void drm_bridge_pre_enable(struct drm_bridge *bridge);
+void drm_bridge_enable(struct drm_bridge *bridge);
+
 #endif /* __DRM_CRTC_H__ */
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH 14/14] drm: Use dev->name as fallback for dev->unique
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (12 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 13/14] drm: Clean up drm_crtc.h Daniel Vetter
@ 2016-06-14 18:51 ` Daniel Vetter
  2016-06-15  5:42 ` ✓ Ro.CI.BAT: success for Cruft removal around drm_master Patchwork
  14 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-14 18:51 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, Daniel Vetter

Lots of arm drivers get this wrong and for most arm boards this is the
right thing actually. And anyway with most loaders you want to chase
sysfs links anyway to figure out which dri device you want.

This will fix dmesg noise for rockchip and sti.

Also add a fallback to driver->name for entirely virtual drivers like
vgem.

v2: Rebase on top of

commit e112e593b215c394c0303dbf0534db0928e87967
Author: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Date:   Fri Dec 11 11:20:28 2015 +0100

    drm: use dev_name as default unique name in drm_dev_alloc()

and simplify a bit. Plus add a comment.

Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Reported-by: Ilia Mirkin <imirkin@alum.mit.edu>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c   | 10 +++++-----
 drivers/gpu/drm/drm_ioctl.c |  8 +-------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 10afa2539181..ecba2511ef5a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -523,11 +523,11 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 		}
 	}
 
-	if (parent) {
-		ret = drm_dev_set_unique(dev, dev_name(parent));
-		if (ret)
-			goto err_setunique;
-	}
+	/* Use the parent device name as DRM device unique identifier, but fall
+	 * back to the driver name for virtual devices like vgem. */
+	ret = drm_dev_set_unique(dev, parent ? dev_name(parent) : driver->name);
+	if (ret)
+		goto err_setunique;
 
 	return dev;
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 11eda9050215..83892b6b1e0d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -134,13 +134,7 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
 			drm_unset_busid(dev, master);
 			return ret;
 		}
-	} else {
-		if (WARN(dev->unique == NULL,
-			 "No drm_driver.set_busid() implementation provided by "
-			 "%ps. Use drm_dev_set_unique() to set the unique "
-			 "name explicitly.", dev->driver))
-			return -EINVAL;
-
+	} else if (dev->unique) {
 		master->unique = kstrdup(dev->unique, GFP_KERNEL);
 		if (master->unique)
 			master->unique_len = strlen(dev->unique);
-- 
2.8.1

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

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* ✓ Ro.CI.BAT: success for Cruft removal around drm_master
  2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
                   ` (13 preceding siblings ...)
  2016-06-14 18:51 ` [PATCH 14/14] drm: Use dev->name as fallback for dev->unique Daniel Vetter
@ 2016-06-15  5:42 ` Patchwork
  14 siblings, 0 replies; 40+ messages in thread
From: Patchwork @ 2016-06-15  5:42 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

== Series Details ==

Series: Cruft removal around drm_master
URL   : https://patchwork.freedesktop.org/series/8700/
State : success

== Summary ==

Series 8700v1 Cruft removal around drm_master
http://patchwork.freedesktop.org/api/1.0/series/8700/revisions/1/mbox

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-snb-i7-2620M)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-bdw-i7-5557u  total:213  pass:201  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i7-6700k  total:213  pass:188  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:213  pass:174  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:213  pass:197  dwarn:2   dfail:0   fail:0   skip:14 
ro-bdw-i7-5557U  total:213  pass:198  dwarn:0   dfail:0   fail:0   skip:15 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:213  pass:150  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-skl3-i5-6260u total:213  pass:201  dwarn:1   dfail:0   fail:0   skip:11 
ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
fi-hsw-i7-4770k failed to connect after reboot
fi-skl-i5-6260u failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1187/

5fb50cc drm-intel-nightly: 2016y-06m-14d-21h-33m-58s UTC integration manifest
6be0fb1 drm: Use dev->name as fallback for dev->unique
86d218b drm: Clean up drm_crtc.h
1282a65 drm: Move master pointer from drm_minor to drm_device
e1dfc93 drm: Mark set/drop master ioctl as unlocked.
d4389a5 drm: Mark authmagic ioctls as unlocked
4353438 drm: Protect authmagic with master_mutex
2ef7801 drm: Move authmagic cleanup into drm_master_release
0e69eda drm: Only do the hw.lock cleanup in master_relase for !MODESET
cce4b9a drm: Extract drm_master_relase
191f186 drm: Extract drm_master_open
bb49110 drm: Move master functions into drm_auth.c
f386c78 drm: Link directly from drm_master to drm_device
6f9c25f drm: Hide hw.lock cleanup in filp->release better
2259967 drm: Nuke legacy maps debugfs files

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 01/14] drm: Nuke legacy maps debugfs files
  2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
@ 2016-06-15 11:23   ` Chris Wilson
  2016-06-15 12:00   ` Emil Velikov
  1 sibling, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:23 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:50:56PM +0200, Daniel Vetter wrote:
> GEM stopped using those a while ago, and no one should ever
> need to use them again to debug legacy horror show drivers.
> 
> Nuke it all. Aside: It would kinda be nice if we'd have some
> generic debugfs dumps for at least kms ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

I guess mga will have the casting vote as to whether anybody still uses
these for debugging.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better
  2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
@ 2016-06-15 11:26   ` Chris Wilson
  2016-06-15 12:10   ` Emil Velikov
  1 sibling, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:26 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:50:57PM +0200, Daniel Vetter wrote:
> A few things:
> - Rename the cleanup function from drm_master_release to
>   drm_legacy_lock_release. It doesn't relase any master stuff, but
>   just the legacy hw lock.
> - Hide it in drm_lock.c, which allows us to make a few more functions
>   static in there. To avoid forward decl we need to shuffle the code a
>   bit though.
> - Push the check for ->master into the function itself.
> - Only call this for !DRIVER_MODESET.
> 
> End result: Another place that takes struct_mutex gone for good for
> modern drivers.
> 
> v2: Remove leftover comment.
> 
> 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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 03/14] drm: Link directly from drm_master to drm_device
  2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
@ 2016-06-15 11:29   ` Chris Wilson
  2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
  1 sibling, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:29 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:50:58PM +0200, Daniel Vetter wrote:
> Master-based auth only exists for the legacy/primary drm_minor, hence
> there can only be one per device. The goal here is to untangle the
> epic dereference games of minor->master and master->minor which is
> just massively confusing.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

There can only be one.

Looks sane,
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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 04/14] drm: Move master functions into drm_auth.c
  2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
@ 2016-06-15 11:31   ` Chris Wilson
  2016-06-15 15:47     ` Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:31 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:50:59PM +0200, Daniel Vetter wrote:
> For modern drivers pretty much the only thing drm_master does is
> handling authentication for the primary/legacy drm_minor node. Instead
> of having it all over drm files, move it all together into drm_auth.c.
> 
> This patch just does code-motion, follow up patches will also extract
> the master logic from file open&release paths.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

What else is to come? Wondering why drm_auth.c rather than drm_master.c?

Looks mechanical, so
Reviewed-by: Chris Wilson Mchris@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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 05/14] drm: Extract drm_master_open
  2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
@ 2016-06-15 11:41   ` Chris Wilson
  0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:41 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development, Julia Lawall

On Tue, Jun 14, 2016 at 08:51:00PM +0200, Daniel Vetter wrote:
> And pull out the primary_client check to make it really obvious that
> this can't happen on control/render nodes. Bonus that we can avoid the
> master lock in this case.

...as the minor->type is statically assigned on creation and so
inspection doesn't require the lock.

> v2: Don't leak locks on error path (and simplify control flow while
> at it), reported by Julia.
> 
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_auth.c     | 19 ++++++++++++++++++-
>  drivers/gpu/drm/drm_fops.c     | 13 ++-----------
>  drivers/gpu/drm/drm_internal.h |  2 +-
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1d314ae13560..9c1e2081cd58 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -120,7 +120,7 @@ static struct drm_master *drm_master_create(struct drm_device *dev)
>   * This function must be called with dev::struct_mutex held.
>   * Returns negative error code on failure. Zero on success.
>   */
> -int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> +static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>  {
>  	struct drm_master *old_master;
>  	int ret;
> @@ -226,6 +226,23 @@ out_unlock:
>  	return ret;
>  }
>  
> +int drm_master_open(struct drm_file *file_priv)
> +{
> +	struct drm_device *dev = file_priv->minor->dev;
> +	int ret = 0;
> +
> +	/* if there is no current master make this fd it, but do not create
> +	 * any master object for render clients */

The second half of this comment is out of place, it is referring to
drm_is_primary_client().

> +	mutex_lock(&dev->master_mutex);
> +	if (!file_priv->minor->master)
> +		ret = drm_new_set_master(dev, file_priv);
> +	else
> +		file_priv->master = drm_master_get(file_priv->minor->master);
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return ret;
> +}
> +
>  struct drm_master *drm_master_get(struct drm_master *master)
>  {
>  	kref_get(&master->refcount);
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 50b3de990aee..e2522672719a 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -229,19 +229,11 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
>  			goto out_prime_destroy;
>  	}
>  
> -	/* if there is no current master make this fd it, but do not create
> -	 * any master object for render clients */
> -	mutex_lock(&dev->master_mutex);
> -	if (drm_is_primary_client(priv) && !priv->minor->master) {
> -		/* create a new master */
> -		ret = drm_new_set_master(dev, priv);
> +	if (drm_is_primary_client(priv)) {
> +		ret = drm_master_open(priv);
>  		if (ret)

Both drm_is_primary_client() and drm_master_open() are acting on
drm_minor not drm_file. Idle speculation if that makes the code any
clearer.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 06/14] drm: Extract drm_master_relase
  2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
@ 2016-06-15 11:43   ` Chris Wilson
  2016-06-16  8:18     ` Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:43 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:01PM +0200, Daniel Vetter wrote:
> Like with drm_master_open protect it with a check for primary_client
> to make it clear that this can't happen on render/control nodes.
> 
> 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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET
  2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
@ 2016-06-15 11:48   ` Chris Wilson
  2016-06-15 15:49     ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:48 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:02PM +0200, Daniel Vetter wrote:
> Another place gone where modern drivers could have hit
> dev->struct_mutex.
> 
> To avoid too deeply nesting control flow rework it a bit.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_auth.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index e015a7edb154..a58b2eb0d004 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv)
>  void drm_master_release(struct drm_file *file_priv)
>  {
>  	struct drm_device *dev = file_priv->minor->dev;
> +	struct drm_master *master = file_priv->master;
> +

Spare newline.

>  	mutex_lock(&dev->master_mutex);
> -	if (file_priv->is_master) {
> -		struct drm_master *master = file_priv->master;
> +	if (!file_priv->is_master)
> +		goto out_unlock;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		/*
>  		 * Since the master is disappearing, so is the
>  		 * possibility to lock.
> @@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv)
>  			wake_up_interruptible_all(&master->lock.lock_queue);
>  		}
>  		mutex_unlock(&dev->struct_mutex);
> +	}
>  
> -		if (file_priv->minor->master == file_priv->master) {
> -			/* drop the reference held my the minor */
> -			if (dev->driver->master_drop)
> -				dev->driver->master_drop(dev, file_priv, true);
> -			drm_master_put(&file_priv->minor->master);
> -		}
> +	if (file_priv->minor->master == file_priv->master) {
> +		/* drop the reference held my the minor */
> +		if (dev->driver->master_drop)
> +			dev->driver->master_drop(dev, file_priv, true);
> +		drm_master_put(&file_priv->minor->master);
>  	}
>  
>  	/* drop the master reference held by the file priv */
>  	if (file_priv->master)
>  		drm_master_put(&file_priv->master);
>  	file_priv->is_master = 0;
> +out_unlock:

This changes the reference counting, and from my quick scan we can have
a reference on file_priv->master but file_priv->is_master == 0.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release
  2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
@ 2016-06-15 11:51   ` Chris Wilson
  0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:51 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:03PM +0200, Daniel Vetter wrote:
> It's related, and soon authmagic will also use the master_mutex.
> 
> There is an ever-so-slightly semantic change here:
> - authmagic will only be cleaned up for primary_client drm_minors. But
>   it's impossible to create authmagic on render/control nodes, so this
>   is fine.

For reference: file_priv->magic is created by the drm_getmagic() ioctl,
only available on the legacy device node.

> - The cleanup is moved down a bit in the release processing. Doesn't
>   matter at all since authmagic is purely internal logic used by the
>   core ioctl access checks, and when we're in a file's release
>   callback no one can do ioctls any more.
> 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 09/14] drm: Protect authmagic with master_mutex
  2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
@ 2016-06-15 11:54   ` Chris Wilson
  2016-06-15 15:52     ` Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:54 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:04PM +0200, Daniel Vetter wrote:
> Simplifies cleanup, and there's no reason drivers should ever care
> about authmagic at all - it's all handled in the core.
> 
> And with that, Ladies and Gentlemen, it's time to pop the champagen
> and celebrate: dev->struct_mutex is now officially gone from modern
> drivers, and if a driver is using gem_free_object_unlocked and doesn't
> do anything else silly it's positively impossible to ever touch
> dev->struct_mutex at runtime, anywhere.

Still keen to know what the fuss is all about.
 
> Well except for the mutex_init on driver load ;-)
> 
> 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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 10/14] drm: Mark authmagic ioctls as unlocked
  2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
@ 2016-06-15 11:55   ` Chris Wilson
  0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:55 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:05PM +0200, Daniel Vetter wrote:
> All protected by dev->master_mutex. And there's no driver callbacks,
> which means no need to sync with old dri1 horror show drivers at all.
> Hence safe to drop the drm legacy BKL from these paths.
> 
> 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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 11/14] drm: Mark set/drop master ioctl as unlocked.
  2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
@ 2016-06-15 11:56   ` Chris Wilson
  0 siblings, 0 replies; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 11:56 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On Tue, Jun 14, 2016 at 08:51:06PM +0200, Daniel Vetter wrote:
> Again this is neatly protected by the dev->master_mutex now. There is
> a driver callback both for set and drop, but it's only used by vmwgfx.
> And vmwgfx has it's own solid locking for shared resources (besides
> dev->master_mutex), hence is all safe. Let's drop another place where
> the drm legacy bkl is used.
> 
> 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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 01/14] drm: Nuke legacy maps debugfs files
  2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
  2016-06-15 11:23   ` Chris Wilson
@ 2016-06-15 12:00   ` Emil Velikov
  1 sibling, 0 replies; 40+ messages in thread
From: Emil Velikov @ 2016-06-15 12:00 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development, DRI Development

On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> GEM stopped using those a while ago, and no one should ever
> need to use them again to debug legacy horror show drivers.
>
> Nuke it all. Aside: It would kinda be nice if we'd have some
> generic debugfs dumps for at least kms ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Fwiw:
Acked-by: Emil Velikov <emil.l.velikov@gmail.com>

Aside: would be nice to nuke all those /proc/dri references. I believe
those haven't been a thing for a while now ?

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better
  2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
  2016-06-15 11:26   ` [Intel-gfx] " Chris Wilson
@ 2016-06-15 12:10   ` Emil Velikov
  1 sibling, 0 replies; 40+ messages in thread
From: Emil Velikov @ 2016-06-15 12:10 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> A few things:
> - Rename the cleanup function from drm_master_release to
>   drm_legacy_lock_release. It doesn't relase any master stuff, but
>   just the legacy hw lock.
> - Hide it in drm_lock.c, which allows us to make a few more functions
>   static in there. To avoid forward decl we need to shuffle the code a
>   bit though.
> - Push the check for ->master into the function itself.
> - Only call this for !DRIVER_MODESET.
>
As per the multiple points above - it would have been great to split
out the drm_lock.c code reshuffle as separate patch.
Regardless of my suggestion:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Side note: I think we don't need/bother with kernel doc for static
functions, do we ?

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
  2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
@ 2016-06-15 12:10   ` Chris Wilson
  2016-06-15 16:01     ` Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 12:10 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development, Julia Lawall

On Tue, Jun 14, 2016 at 08:51:07PM +0200, Daniel Vetter wrote:
> There can only be one current master, and it's for the overall device.
> Render/control minors don't support master-based auth at all.
> 
> This simplifies the master logic a lot, at least in my eyes: All these
> additional pointer chases are just confusing.

One master for the device, on the struct drm_device, as opposed to hidden
behind the first of three minors, makes sense.

> @@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>  	lockdep_assert_held_once(&dev->master_mutex);
>  
>  	/* create a new master */
> -	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
> -	if (!fpriv->minor->master)
> +	dev->master = drm_master_create(dev);
> +	if (!dev->master)
>  		return -ENOMEM;
>  
>  	/* take another reference for the copy in the local file priv */
>  	old_master = fpriv->master;
> -	fpriv->master = drm_master_get(fpriv->minor->master);
> +	fpriv->master = drm_master_get(dev->master);
>  
>  	if (dev->driver->master_create) {
>  		ret = dev->driver->master_create(dev, fpriv->master);

> @@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
>  	/* if there is no current master make this fd it, but do not create
>  	 * any master object for render clients */
>  	mutex_lock(&dev->master_mutex);
> -	if (!file_priv->minor->master)
> +	if (!dev->master)
>  		ret = drm_new_set_master(dev, file_priv);
>  	else
> -		file_priv->master = drm_master_get(file_priv->minor->master);
> +		file_priv->master = drm_master_get(dev->master);
>  	mutex_unlock(&dev->master_mutex);

You could take the opportunity to make this a bit simpler:

	if (!READ_ONCE(dev->master)) {
		int ret;

		ret = 0;
		mutex_lock(&dev->master_mutex);
		if (!dev->master)
			ret = drm_new_master(dev);
		mutex_unlock(&dev->master_mutex);
		if (ret)
			return ret;
	}

	file_priv->master = drm_master_get(dev->master);
	return 0;

Just to straighten out the kref dance.

>  
>  	return ret;
> @@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  
> -	if (file_priv->minor->master == file_priv->master) {
> +	if (dev->master == file_priv->master) {
>  		/* drop the reference held my the minor */
>  		if (dev->driver->master_drop)
>  			dev->driver->master_drop(dev, file_priv, true);
> -		drm_master_put(&file_priv->minor->master);
> +		drm_master_put(&dev->master);

This still makes me uneasy. This is not equivalent to dropmaster_ioctl
and subsequent setmaster_ioctl will fail as dev->master is still
assigned (but the owner has gone).
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 03/14] drm: Link directly from drm_master to drm_device
  2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
  2016-06-15 11:29   ` Chris Wilson
@ 2016-06-15 12:50   ` Emil Velikov
  2016-06-15 15:45     ` Daniel Vetter
  1 sibling, 1 reply; 40+ messages in thread
From: Emil Velikov @ 2016-06-15 12:50 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Master-based auth only exists for the legacy/primary drm_minor, hence
> there can only be one per device. The goal here is to untangle the
> epic dereference games of minor->master and master->minor which is
> just massively confusing.
>
Good riddance :-)
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Taking this a step further, have you considered:
 - having the master under drm_device, then
 - nuking the drm_file/drm_minor one and adding drm_minor::has_master.
The last one will be checked before accessing drm_minor::dev::master.

The above should work just fine, right ?

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 03/14] drm: Link directly from drm_master to drm_device
  2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
@ 2016-06-15 15:45     ` Daniel Vetter
  2016-06-16 20:04       ` Emil Velikov
  0 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 15:45 UTC (permalink / raw)
  To: Emil Velikov
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter

On Wed, Jun 15, 2016 at 01:50:02PM +0100, Emil Velikov wrote:
> On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Master-based auth only exists for the legacy/primary drm_minor, hence
> > there can only be one per device. The goal here is to untangle the
> > epic dereference games of minor->master and master->minor which is
> > just massively confusing.
> >
> Good riddance :-)
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> 
> Taking this a step further, have you considered:
>  - having the master under drm_device, then
>  - nuking the drm_file/drm_minor one and adding drm_minor::has_master.
> The last one will be checked before accessing drm_minor::dev::master.
> 
> The above should work just fine, right ?

We still need a pointer for the dev->master link. A later patch will
simplify that, but entirely embedding doesn't work: drm_master must be
free-standing, since when you drop_master all the clients authenticated
against you will still be connected to your master. This is to make sure
that they loose their authentication when vt switching to another X
server. When you reacquire master with set_master, all the clients
authenticated against that master are again considered authenticated.

But yes, this will be simplified more.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 04/14] drm: Move master functions into drm_auth.c
  2016-06-15 11:31   ` Chris Wilson
@ 2016-06-15 15:47     ` Daniel Vetter
  0 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 15:47 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, DRI Development,
	Intel Graphics Development, Daniel Vetter

On Wed, Jun 15, 2016 at 12:31:20PM +0100, Chris Wilson wrote:
> On Tue, Jun 14, 2016 at 08:50:59PM +0200, Daniel Vetter wrote:
> > For modern drivers pretty much the only thing drm_master does is
> > handling authentication for the primary/legacy drm_minor node. Instead
> > of having it all over drm files, move it all together into drm_auth.c.
> > 
> > This patch just does code-motion, follow up patches will also extract
> > the master logic from file open&release paths.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> What else is to come? Wondering why drm_auth.c rather than drm_master.c?

Yeah, I guess we could rename it to drm_master, probably makes more sense.
Otoh the only reason we have a drm_master is to support the DRM_AUTH ioctl
restriction, so drm_auth seems to fit, too.

I went with the lazy approach of sticking to what's there already ;-)
-Daniel

> 
> Looks mechanical, so
> Reviewed-by: Chris Wilson Mchris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET
  2016-06-15 11:48   ` Chris Wilson
@ 2016-06-15 15:49     ` Daniel Vetter
  0 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 15:49 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, DRI Development,
	Intel Graphics Development, Daniel Vetter

On Wed, Jun 15, 2016 at 12:48:23PM +0100, Chris Wilson wrote:
> On Tue, Jun 14, 2016 at 08:51:02PM +0200, Daniel Vetter wrote:
> > Another place gone where modern drivers could have hit
> > dev->struct_mutex.
> > 
> > To avoid too deeply nesting control flow rework it a bit.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_auth.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > index e015a7edb154..a58b2eb0d004 100644
> > --- a/drivers/gpu/drm/drm_auth.c
> > +++ b/drivers/gpu/drm/drm_auth.c
> > @@ -246,11 +246,14 @@ int drm_master_open(struct drm_file *file_priv)
> >  void drm_master_release(struct drm_file *file_priv)
> >  {
> >  	struct drm_device *dev = file_priv->minor->dev;
> > +	struct drm_master *master = file_priv->master;
> > +
> 
> Spare newline.
> 
> >  	mutex_lock(&dev->master_mutex);
> > -	if (file_priv->is_master) {
> > -		struct drm_master *master = file_priv->master;
> > +	if (!file_priv->is_master)
> > +		goto out_unlock;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> >  		/*
> >  		 * Since the master is disappearing, so is the
> >  		 * possibility to lock.
> > @@ -264,19 +267,20 @@ void drm_master_release(struct drm_file *file_priv)
> >  			wake_up_interruptible_all(&master->lock.lock_queue);
> >  		}
> >  		mutex_unlock(&dev->struct_mutex);
> > +	}
> >  
> > -		if (file_priv->minor->master == file_priv->master) {
> > -			/* drop the reference held my the minor */
> > -			if (dev->driver->master_drop)
> > -				dev->driver->master_drop(dev, file_priv, true);
> > -			drm_master_put(&file_priv->minor->master);
> > -		}
> > +	if (file_priv->minor->master == file_priv->master) {
> > +		/* drop the reference held my the minor */
> > +		if (dev->driver->master_drop)
> > +			dev->driver->master_drop(dev, file_priv, true);
> > +		drm_master_put(&file_priv->minor->master);
> >  	}
> >  
> >  	/* drop the master reference held by the file priv */
> >  	if (file_priv->master)
> >  		drm_master_put(&file_priv->master);
> >  	file_priv->is_master = 0;
> > +out_unlock:
> 
> This changes the reference counting, and from my quick scan we can have
> a reference on file_priv->master but file_priv->is_master == 0.

Indeed, I've created a leak. Will fix.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 09/14] drm: Protect authmagic with master_mutex
  2016-06-15 11:54   ` [Intel-gfx] " Chris Wilson
@ 2016-06-15 15:52     ` Daniel Vetter
  0 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 15:52 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, DRI Development,
	Intel Graphics Development, Daniel Vetter

On Wed, Jun 15, 2016 at 12:54:24PM +0100, Chris Wilson wrote:
> On Tue, Jun 14, 2016 at 08:51:04PM +0200, Daniel Vetter wrote:
> > Simplifies cleanup, and there's no reason drivers should ever care
> > about authmagic at all - it's all handled in the core.
> > 
> > And with that, Ladies and Gentlemen, it's time to pop the champagen
> > and celebrate: dev->struct_mutex is now officially gone from modern
> > drivers, and if a driver is using gem_free_object_unlocked and doesn't
> > do anything else silly it's positively impossible to ever touch
> > dev->struct_mutex at runtime, anywhere.
> 
> Still keen to know what the fuss is all about.

I hate sprawling locks, and go all ocd about them. There's not really
anything else. It also makes reviewing changes painful since you have to
re-read every code that somehow interacts with your lock. I much prefer
locking where each part fits into my brain, and that one is of pityful
capacity ;-)
-Daniel

>  
> > Well except for the mutex_init on driver load ;-)
> > 
> > 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

-- 
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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
  2016-06-15 12:10   ` [Intel-gfx] " Chris Wilson
@ 2016-06-15 16:01     ` Daniel Vetter
  2016-06-15 16:33       ` Chris Wilson
  0 siblings, 1 reply; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 16:01 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, DRI Development, Daniel Vetter,
	Intel Graphics Development, Julia Lawall

On Wed, Jun 15, 2016 at 01:10:35PM +0100, Chris Wilson wrote:
> On Tue, Jun 14, 2016 at 08:51:07PM +0200, Daniel Vetter wrote:
> > There can only be one current master, and it's for the overall device.
> > Render/control minors don't support master-based auth at all.
> > 
> > This simplifies the master logic a lot, at least in my eyes: All these
> > additional pointer chases are just confusing.
> 
> One master for the device, on the struct drm_device, as opposed to hidden
> behind the first of three minors, makes sense.
> 
> > @@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> >  	lockdep_assert_held_once(&dev->master_mutex);
> >  
> >  	/* create a new master */
> > -	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
> > -	if (!fpriv->minor->master)
> > +	dev->master = drm_master_create(dev);
> > +	if (!dev->master)
> >  		return -ENOMEM;
> >  
> >  	/* take another reference for the copy in the local file priv */
> >  	old_master = fpriv->master;
> > -	fpriv->master = drm_master_get(fpriv->minor->master);
> > +	fpriv->master = drm_master_get(dev->master);
> >  
> >  	if (dev->driver->master_create) {
> >  		ret = dev->driver->master_create(dev, fpriv->master);
> 
> > @@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
> >  	/* if there is no current master make this fd it, but do not create
> >  	 * any master object for render clients */
> >  	mutex_lock(&dev->master_mutex);
> > -	if (!file_priv->minor->master)
> > +	if (!dev->master)
> >  		ret = drm_new_set_master(dev, file_priv);
> >  	else
> > -		file_priv->master = drm_master_get(file_priv->minor->master);
> > +		file_priv->master = drm_master_get(dev->master);
> >  	mutex_unlock(&dev->master_mutex);
> 
> You could take the opportunity to make this a bit simpler:
> 
> 	if (!READ_ONCE(dev->master)) {
> 		int ret;
> 
> 		ret = 0;
> 		mutex_lock(&dev->master_mutex);
> 		if (!dev->master)
> 			ret = drm_new_master(dev);
> 		mutex_unlock(&dev->master_mutex);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	file_priv->master = drm_master_get(dev->master);

drm_master_get(dev->master) must be under the master_mutex, without it we
could race with a drm_master_put(&dev->master) and end up doing a kref_get
when the refcount already reached 0.

> 	return 0;
> 
> Just to straighten out the kref dance.
> 
> >  
> >  	return ret;
> > @@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
> >  		mutex_unlock(&dev->struct_mutex);
> >  	}
> >  
> > -	if (file_priv->minor->master == file_priv->master) {
> > +	if (dev->master == file_priv->master) {
> >  		/* drop the reference held my the minor */
> >  		if (dev->driver->master_drop)
> >  			dev->driver->master_drop(dev, file_priv, true);
> > -		drm_master_put(&file_priv->minor->master);
> > +		drm_master_put(&dev->master);
> 
> This still makes me uneasy. This is not equivalent to dropmaster_ioctl
> and subsequent setmaster_ioctl will fail as dev->master is still
> assigned (but the owner has gone).

drm_master_put clears the pointer passed to it, so dev->master will be set
to NULL. And it does the same as drop_master (wrt dev->master at least,
master_release also needs to clean up file_priv->master on top). Not sure
it's worth it to extract those 5 lines into a __drm_drop_master() helper
function? I can respin with that if you want. On the master_open/setmaster
side the shared code is already extracted in drm_new_set_master().
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
  2016-06-15 16:01     ` Daniel Vetter
@ 2016-06-15 16:33       ` Chris Wilson
  2016-06-15 19:54         ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 40+ messages in thread
From: Chris Wilson @ 2016-06-15 16:33 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, Julia Lawall,
	DRI Development, Daniel Vetter

On Wed, Jun 15, 2016 at 06:01:41PM +0200, Daniel Vetter wrote:
> On Wed, Jun 15, 2016 at 01:10:35PM +0100, Chris Wilson wrote:
> > On Tue, Jun 14, 2016 at 08:51:07PM +0200, Daniel Vetter wrote:
> > > There can only be one current master, and it's for the overall device.
> > > Render/control minors don't support master-based auth at all.
> > > 
> > > This simplifies the master logic a lot, at least in my eyes: All these
> > > additional pointer chases are just confusing.
> > 
> > One master for the device, on the struct drm_device, as opposed to hidden
> > behind the first of three minors, makes sense.
> > 
> > > @@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
> > >  	lockdep_assert_held_once(&dev->master_mutex);
> > >  
> > >  	/* create a new master */
> > > -	fpriv->minor->master = drm_master_create(fpriv->minor->dev);
> > > -	if (!fpriv->minor->master)
> > > +	dev->master = drm_master_create(dev);
> > > +	if (!dev->master)
> > >  		return -ENOMEM;
> > >  
> > >  	/* take another reference for the copy in the local file priv */
> > >  	old_master = fpriv->master;
> > > -	fpriv->master = drm_master_get(fpriv->minor->master);
> > > +	fpriv->master = drm_master_get(dev->master);
> > >  
> > >  	if (dev->driver->master_create) {
> > >  		ret = dev->driver->master_create(dev, fpriv->master);
> > 
> > > @@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
> > >  	/* if there is no current master make this fd it, but do not create
> > >  	 * any master object for render clients */
> > >  	mutex_lock(&dev->master_mutex);
> > > -	if (!file_priv->minor->master)
> > > +	if (!dev->master)
> > >  		ret = drm_new_set_master(dev, file_priv);
> > >  	else
> > > -		file_priv->master = drm_master_get(file_priv->minor->master);
> > > +		file_priv->master = drm_master_get(dev->master);
> > >  	mutex_unlock(&dev->master_mutex);
> > 
> > You could take the opportunity to make this a bit simpler:
> > 
> > 	if (!READ_ONCE(dev->master)) {
> > 		int ret;
> > 
> > 		ret = 0;
> > 		mutex_lock(&dev->master_mutex);
> > 		if (!dev->master)
> > 			ret = drm_new_master(dev);
> > 		mutex_unlock(&dev->master_mutex);
> > 		if (ret)
> > 			return ret;
> > 	}
> > 
> > 	file_priv->master = drm_master_get(dev->master);
> 
> drm_master_get(dev->master) must be under the master_mutex, without it we
> could race with a drm_master_put(&dev->master) and end up doing a kref_get
> when the refcount already reached 0.

Something is very fishy then. The behaviour of drm_new_master() would
appear to be to create a drm_master owned by the device, but really it is
owned by file_priv?

* checks back on drm_master

So drm_master is the authentication structure that needs to be unique to
a hierachy. So drm_new_set_master() and here really do appear backwards.

The old drm_new_set_master() makes more sense, it assigns to the
file_priv, and then performs a setmaster operation. In that ordering,
you could even do the file_priv local operation of creating the new
master structure before taking the lock to perform setmaster.


> > Just to straighten out the kref dance.
> > 
> > >  
> > >  	return ret;
> > > @@ -271,11 +271,11 @@ void drm_master_release(struct drm_file *file_priv)
> > >  		mutex_unlock(&dev->struct_mutex);
> > >  	}
> > >  
> > > -	if (file_priv->minor->master == file_priv->master) {
> > > +	if (dev->master == file_priv->master) {
> > >  		/* drop the reference held my the minor */
> > >  		if (dev->driver->master_drop)
> > >  			dev->driver->master_drop(dev, file_priv, true);
> > > -		drm_master_put(&file_priv->minor->master);
> > > +		drm_master_put(&dev->master);
> > 
> > This still makes me uneasy. This is not equivalent to dropmaster_ioctl
> > and subsequent setmaster_ioctl will fail as dev->master is still
> > assigned (but the owner has gone).
> 
> drm_master_put clears the pointer passed to it, so dev->master will be set
> to NULL. And it does the same as drop_master (wrt dev->master at least,
> master_release also needs to clean up file_priv->master on top). Not sure
> it's worth it to extract those 5 lines into a __drm_drop_master() helper
> function? I can respin with that if you want. On the master_open/setmaster
> side the shared code is already extracted in drm_new_set_master().

drm_master_put() nullifies, didn't expect that.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [Intel-gfx] [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device
  2016-06-15 16:33       ` Chris Wilson
@ 2016-06-15 19:54         ` Daniel Vetter
  0 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-15 19:54 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Daniel Vetter, DRI Development,
	Daniel Vetter, Intel Graphics Development, Julia Lawall

On Wed, Jun 15, 2016 at 6:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Jun 15, 2016 at 06:01:41PM +0200, Daniel Vetter wrote:
>> On Wed, Jun 15, 2016 at 01:10:35PM +0100, Chris Wilson wrote:
>> > On Tue, Jun 14, 2016 at 08:51:07PM +0200, Daniel Vetter wrote:
>> > > There can only be one current master, and it's for the overall device.
>> > > Render/control minors don't support master-based auth at all.
>> > >
>> > > This simplifies the master logic a lot, at least in my eyes: All these
>> > > additional pointer chases are just confusing.
>> >
>> > One master for the device, on the struct drm_device, as opposed to hidden
>> > behind the first of three minors, makes sense.
>> >
>> > > @@ -128,13 +128,13 @@ static int drm_new_set_master(struct drm_device *dev, struct drm_file *fpriv)
>> > >   lockdep_assert_held_once(&dev->master_mutex);
>> > >
>> > >   /* create a new master */
>> > > - fpriv->minor->master = drm_master_create(fpriv->minor->dev);
>> > > - if (!fpriv->minor->master)
>> > > + dev->master = drm_master_create(dev);
>> > > + if (!dev->master)
>> > >           return -ENOMEM;
>> > >
>> > >   /* take another reference for the copy in the local file priv */
>> > >   old_master = fpriv->master;
>> > > - fpriv->master = drm_master_get(fpriv->minor->master);
>> > > + fpriv->master = drm_master_get(dev->master);
>> > >
>> > >   if (dev->driver->master_create) {
>> > >           ret = dev->driver->master_create(dev, fpriv->master);
>> >
>> > > @@ -234,10 +234,10 @@ int drm_master_open(struct drm_file *file_priv)
>> > >   /* if there is no current master make this fd it, but do not create
>> > >    * any master object for render clients */
>> > >   mutex_lock(&dev->master_mutex);
>> > > - if (!file_priv->minor->master)
>> > > + if (!dev->master)
>> > >           ret = drm_new_set_master(dev, file_priv);
>> > >   else
>> > > -         file_priv->master = drm_master_get(file_priv->minor->master);
>> > > +         file_priv->master = drm_master_get(dev->master);
>> > >   mutex_unlock(&dev->master_mutex);
>> >
>> > You could take the opportunity to make this a bit simpler:
>> >
>> >     if (!READ_ONCE(dev->master)) {
>> >             int ret;
>> >
>> >             ret = 0;
>> >             mutex_lock(&dev->master_mutex);
>> >             if (!dev->master)
>> >                     ret = drm_new_master(dev);
>> >             mutex_unlock(&dev->master_mutex);
>> >             if (ret)
>> >                     return ret;
>> >     }
>> >
>> >     file_priv->master = drm_master_get(dev->master);
>>
>> drm_master_get(dev->master) must be under the master_mutex, without it we
>> could race with a drm_master_put(&dev->master) and end up doing a kref_get
>> when the refcount already reached 0.
>
> Something is very fishy then. The behaviour of drm_new_master() would
> appear to be to create a drm_master owned by the device, but really it is
> owned by file_priv?
>
> * checks back on drm_master
>
> So drm_master is the authentication structure that needs to be unique to
> a hierachy. So drm_new_set_master() and here really do appear backwards.
>
> The old drm_new_set_master() makes more sense, it assigns to the
> file_priv, and then performs a setmaster operation. In that ordering,
> you could even do the file_priv local operation of creating the new
> master structure before taking the lock to perform setmaster.

I didn't change the logic, the old new_set_master didn't first create
the master for file_priv, it first created it for file_priv->minor,
and that drm_minor is the device-unique drm_minor for the
primary/legacy node. The change only moves from that drm_minor for the
legacy node to the drm_device.

Wrt refences, every file_priv holds a ref for it's master. On top of
that the current master structure (after this patch stored in
dev->master, before in dev->minors[legacy]->master. The only tricky
bit is that when the current master closes, we do an implicit
dropmaster first.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 06/14] drm: Extract drm_master_relase
  2016-06-15 11:43   ` [Intel-gfx] " Chris Wilson
@ 2016-06-16  8:18     ` Daniel Vetter
  0 siblings, 0 replies; 40+ messages in thread
From: Daniel Vetter @ 2016-06-16  8:18 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, DRI Development,
	Intel Graphics Development, Daniel Vetter

On Wed, Jun 15, 2016 at 12:43:11PM +0100, Chris Wilson wrote:
> On Tue, Jun 14, 2016 at 08:51:01PM +0200, Daniel Vetter wrote:
> > Like with drm_master_open protect it with a check for primary_client
> > to make it clear that this can't happen on render/control nodes.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged up to this one here to drm-misc, thanks for the review. I'll resend
the remaining ones anew, there's a trickle of small conflicts due to the
leak fix.
-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

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 03/14] drm: Link directly from drm_master to drm_device
  2016-06-15 15:45     ` Daniel Vetter
@ 2016-06-16 20:04       ` Emil Velikov
  0 siblings, 0 replies; 40+ messages in thread
From: Emil Velikov @ 2016-06-16 20:04 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter

On 15 June 2016 at 16:45, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jun 15, 2016 at 01:50:02PM +0100, Emil Velikov wrote:
>> On 14 June 2016 at 19:50, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > Master-based auth only exists for the legacy/primary drm_minor, hence
>> > there can only be one per device. The goal here is to untangle the
>> > epic dereference games of minor->master and master->minor which is
>> > just massively confusing.
>> >
>> Good riddance :-)
>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> Taking this a step further, have you considered:
>>  - having the master under drm_device, then
>>  - nuking the drm_file/drm_minor one and adding drm_minor::has_master.
>> The last one will be checked before accessing drm_minor::dev::master.
>>
>> The above should work just fine, right ?
>
> We still need a pointer for the dev->master link. A later patch will
> simplify that, but entirely embedding doesn't work: drm_master must be
> free-standing, since when you drop_master all the clients authenticated
> against you will still be connected to your master. This is to make sure
> that they loose their authentication when vt switching to another X
> server. When you reacquire master with set_master, all the clients
> authenticated against that master are again considered authenticated.
>
Just to double check:

+ * Note that master structures are only relevant for the legacy/primary device
+ * nodes, hence there can only be one per device, not one per drm_minor.

In the above does "one per device" reference the "legacy/primary
device node" or the "drm_master struct" ? I was thinking about the
latter then again, reading your reply I'm not sure any more :-(

Or perhaps it is the latter but it should read "one active/associated
per device" ? In which case, yes drm_master should be standalone. Can
you please shed some light ?

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

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2016-06-16 20:04 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-14 18:50 [PATCH 00/14] Cruft removal around drm_master Daniel Vetter
2016-06-14 18:50 ` [PATCH 01/14] drm: Nuke legacy maps debugfs files Daniel Vetter
2016-06-15 11:23   ` Chris Wilson
2016-06-15 12:00   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 02/14] drm: Hide hw.lock cleanup in filp->release better Daniel Vetter
2016-06-15 11:26   ` [Intel-gfx] " Chris Wilson
2016-06-15 12:10   ` Emil Velikov
2016-06-14 18:50 ` [PATCH 03/14] drm: Link directly from drm_master to drm_device Daniel Vetter
2016-06-15 11:29   ` Chris Wilson
2016-06-15 12:50   ` [Intel-gfx] " Emil Velikov
2016-06-15 15:45     ` Daniel Vetter
2016-06-16 20:04       ` Emil Velikov
2016-06-14 18:50 ` [PATCH 04/14] drm: Move master functions into drm_auth.c Daniel Vetter
2016-06-15 11:31   ` Chris Wilson
2016-06-15 15:47     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 05/14] drm: Extract drm_master_open Daniel Vetter
2016-06-15 11:41   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 06/14] drm: Extract drm_master_relase Daniel Vetter
2016-06-15 11:43   ` [Intel-gfx] " Chris Wilson
2016-06-16  8:18     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 07/14] drm: Only do the hw.lock cleanup in master_relase for !MODESET Daniel Vetter
2016-06-15 11:48   ` Chris Wilson
2016-06-15 15:49     ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 08/14] drm: Move authmagic cleanup into drm_master_release Daniel Vetter
2016-06-15 11:51   ` Chris Wilson
2016-06-14 18:51 ` [PATCH 09/14] drm: Protect authmagic with master_mutex Daniel Vetter
2016-06-15 11:54   ` [Intel-gfx] " Chris Wilson
2016-06-15 15:52     ` Daniel Vetter
2016-06-14 18:51 ` [PATCH 10/14] drm: Mark authmagic ioctls as unlocked Daniel Vetter
2016-06-15 11:55   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 11/14] drm: Mark set/drop master ioctl " Daniel Vetter
2016-06-15 11:56   ` [Intel-gfx] " Chris Wilson
2016-06-14 18:51 ` [PATCH 12/14] drm: Move master pointer from drm_minor to drm_device Daniel Vetter
2016-06-15 12:10   ` [Intel-gfx] " Chris Wilson
2016-06-15 16:01     ` Daniel Vetter
2016-06-15 16:33       ` Chris Wilson
2016-06-15 19:54         ` [Intel-gfx] " Daniel Vetter
2016-06-14 18:51 ` [PATCH 13/14] drm: Clean up drm_crtc.h Daniel Vetter
2016-06-14 18:51 ` [PATCH 14/14] drm: Use dev->name as fallback for dev->unique Daniel Vetter
2016-06-15  5:42 ` ✓ Ro.CI.BAT: success for Cruft removal around drm_master Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.