[RFC v2] Revive the work on render-nodes branch

[RFC v2] Revive the work on render-nodes branch

[Ilija Hadzic]

The following set of patches is the reword of the series
sent two weeks ago [2] that will revive the drm-render-nodes [1]
branch. Details of the original series are described in [2].

Patches in this series have been reworked (and a few prep patches
have been added) to address the comment about the planes support
(planes can now be included in the resource list for the render
node).

Consequently, the ioctls have changed to include plane support
so the libdrm side is also affected (patches for libdrm will be sent
in the series following this one).

regards,
-- Ilija

----
[1] http://airlied.livejournal.com/72187.html

[2] http://lists.freedesktop.org/archives/dri-devel/2012-March/020765.html

[PATCH 01/19] drm: simplify dereferencing of node type

[Ilija Hadzic]

file_priv->master->minor is equivalent to file_priv->minor
http://lists.freedesktop.org/archives/dri-devel/2011-October/015511.html

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2d9dc5..d2e5560 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1160,8 +1160,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	list_for_each(lh, &file_priv->fbs)
 		fb_count++;
 
-	mode_group = &file_priv->master->minor->mode_group;
-	if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+	mode_group = &file_priv->minor->mode_group;
+	if (file_priv->minor->type == DRM_MINOR_CONTROL) {
 
 		list_for_each(lh, &dev->mode_config.crtc_list)
 			crtc_count++;
@@ -1202,7 +1202,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_crtcs >= crtc_count) {
 		copied = 0;
 		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type == DRM_MINOR_CONTROL) {
 			list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 					    head) {
 				DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
@@ -1229,7 +1229,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_encoders >= encoder_count) {
 		copied = 0;
 		encoder_id = (uint32_t __user *)(unsigned long)card_res->encoder_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type == DRM_MINOR_CONTROL) {
 			list_for_each_entry(encoder,
 					    &dev->mode_config.encoder_list,
 					    head) {
@@ -1260,7 +1260,7 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 	if (card_res->count_connectors >= connector_count) {
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
-		if (file_priv->master->minor->type == DRM_MINOR_CONTROL) {
+		if (file_priv->minor->type == DRM_MINOR_CONTROL) {
 			list_for_each_entry(connector,
 					    &dev->mode_config.connector_list,
 					    head) {
-- 
1.7.8.5

[PATCH 02/19] drm: track planes in drm_mode_group structure

[Ilija Hadzic]

drm_mode_group structure (meant for subgrouping) of display
resources didn't include planes, which made it impossible to
do any subgrouping of planes. This patch rectifies the problem.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |    8 ++++++++
 include/drm/drm_crtc.h     |    2 ++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2e5560..29ede0a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -940,6 +940,7 @@ int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 	total_objects += dev->mode_config.num_crtc;
 	total_objects += dev->mode_config.num_connector;
 	total_objects += dev->mode_config.num_encoder;
+	total_objects += dev->mode_config.num_plane;
 
 	group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL);
 	if (!group->id_list)
@@ -948,6 +949,7 @@ int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 	group->num_crtcs = 0;
 	group->num_connectors = 0;
 	group->num_encoders = 0;
+	group->num_planes = 0;
 	return 0;
 }
 
@@ -957,6 +959,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
+	struct drm_plane *plane;
 	int ret;
 
 	if ((ret = drm_mode_group_init(dev, group)))
@@ -973,6 +976,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
 		group->id_list[group->num_crtcs + group->num_encoders +
 			       group->num_connectors++] = connector->base.id;
 
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+		group->id_list[group->num_crtcs + group->num_encoders +
+			       group->num_connectors + group->num_planes++] =
+		plane->base.id;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 9595c2c..4aa793e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -696,6 +696,7 @@ struct drm_mode_config_funcs {
  * @num_crtcs: CRTC count
  * @num_encoders: encoder count
  * @num_connectors: connector count
+ * @num_planes: connector count
  * @id_list: list of KMS object IDs in this group
  *
  * Currently this simply tracks the global mode setting state.  But in the
@@ -708,6 +709,7 @@ struct drm_mode_group {
 	uint32_t num_crtcs;
 	uint32_t num_encoders;
 	uint32_t num_connectors;
+	uint32_t num_planes;
 
 	/* list of object IDs for this group */
 	uint32_t *id_list;
-- 
1.7.8.5

[PATCH 03/19] drm: use drm_mode_group in drm_mode_getplane_res

[Ilija Hadzic]

drm_mode_group structure now tracks planes, so use it in
in drm_mode_getplane_res(ources) IOCTL.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |   46 +++++++++++++++++++++++++++++++++----------
 1 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 29ede0a..eafb49d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1564,7 +1564,6 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 			    struct drm_file *file_priv)
 {
 	struct drm_mode_get_plane_res *plane_resp = data;
-	struct drm_mode_config *config;
 	struct drm_plane *plane;
 	uint32_t __user *plane_ptr;
 	int copied = 0, ret = 0;
@@ -1573,25 +1572,50 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 		return -EINVAL;
 
 	mutex_lock(&dev->mode_config.mutex);
-	config = &dev->mode_config;
 
 	/*
 	 * This ioctl is called twice, once to determine how much space is
 	 * needed, and the 2nd time to fill it.
 	 */
-	if (config->num_plane &&
-	    (plane_resp->count_planes >= config->num_plane)) {
-		plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
+	plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
+	if (file_priv->minor->type == DRM_MINOR_CONTROL) {
+		struct drm_mode_config *config = &dev->mode_config;
 
-		list_for_each_entry(plane, &config->plane_list, head) {
-			if (put_user(plane->base.id, plane_ptr + copied)) {
-				ret = -EFAULT;
-				goto out;
+		if (config->num_plane &&
+		    (plane_resp->count_planes >= config->num_plane)) {
+			list_for_each_entry(plane, &config->plane_list, head) {
+				if (put_user(plane->base.id,
+					     plane_ptr + copied)) {
+					ret = -EFAULT;
+					goto out;
+				}
+				copied++;
+			}
+		}
+		plane_resp->count_planes = config->num_plane;
+	} else {
+		int i;
+		struct drm_mode_group *mode_group =
+			&file_priv->minor->mode_group;
+
+		if (mode_group->num_planes &&
+		    (plane_resp->count_planes >= mode_group->num_planes)) {
+			int start;
+
+			start = mode_group->num_crtcs;
+			start += mode_group->num_encoders;
+			start += mode_group->num_connectors;
+			for (i = start; i < start + mode_group->num_planes; i++) {
+				if (put_user(mode_group->id_list[i],
+					     plane_ptr + copied)) {
+					ret = -EFAULT;
+					goto out;
+				}
+				copied++;
 			}
-			copied++;
 		}
+		plane_resp->count_planes = mode_group->num_planes;
 	}
-	plane_resp->count_planes = config->num_plane;
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
-- 
1.7.8.5

[PATCH 04/19] drm: do not push inode down into drm_open_helper

[Ilija Hadzic]

Push minor number instead. This is a preparatory
patch for introducing render nodes. It has been derived
from 7c5cc4f63556e351e9e5980ed22accad410e3fdc originally
created by Dave Airlie.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_fops.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 6263b01..98cb064 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -43,8 +43,8 @@
 DEFINE_MUTEX(drm_global_mutex);
 EXPORT_SYMBOL(drm_global_mutex);
 
-static int drm_open_helper(struct inode *inode, struct file *filp,
-			   struct drm_device * dev);
+static int drm_open_helper(struct file *filp,
+			   struct drm_minor *minor);
 
 static int drm_setup(struct drm_device * dev)
 {
@@ -133,7 +133,7 @@ int drm_open(struct inode *inode, struct file *filp)
 	if (!(dev = minor->dev))
 		return -ENODEV;
 
-	retcode = drm_open_helper(inode, filp, dev);
+	retcode = drm_open_helper(filp, minor);
 	if (!retcode) {
 		atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
 		if (!dev->open_count++)
@@ -226,10 +226,10 @@ static int drm_cpu_valid(void)
  * Creates and initializes a drm_file structure for the file private data in \p
  * filp and add it into the double linked list in \p dev.
  */
-static int drm_open_helper(struct inode *inode, struct file *filp,
-			   struct drm_device * dev)
+static int drm_open_helper(struct file *filp,
+			   struct drm_minor *minor)
 {
-	int minor_id = iminor(inode);
+	struct drm_device *dev = minor->dev;
 	struct drm_file *priv;
 	int ret;
 
@@ -240,7 +240,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
 		return -EINVAL;
 
-	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor_id);
+	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -250,7 +250,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 	priv->filp = filp;
 	priv->uid = current_euid();
 	priv->pid = task_pid_nr(current);
-	priv->minor = idr_find(&drm_minors_idr, minor_id);
+	priv->minor = minor;
 	priv->ioctl_count = 0;
 	/* for compatibility root is always authenticated */
 	priv->authenticated = capable(CAP_SYS_ADMIN);
@@ -558,7 +558,8 @@ int drm_release(struct inode *inode, struct file *filp)
 	}
 
 	/* drop the reference held my the file priv */
-	drm_master_put(&file_priv->master);
+	if (file_priv->master)
+		drm_master_put(&file_priv->master);
 	file_priv->is_master = 0;
 	list_del(&file_priv->lhead);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.7.8.5

[PATCH 05/19] drm: move dev_mapping to the minor node

[Ilija Hadzic]

Make dev_mapping per-minor instead of per device. This is
a preparatory patch for introducing render nodes. This
will allow per-node instead of per-device mapping range,
once we introduce render nodes.

Patch derived from 7c5cc4f63556e351e9e5980ed22accad410e3fdc
originally authored by Dave Airlie.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_drv.c              |    1 -
 drivers/gpu/drm/drm_fops.c             |    8 ++++----
 drivers/gpu/drm/drm_vm.c               |    9 +++++++++
 drivers/gpu/drm/i915/i915_gem.c        |    7 +++----
 drivers/gpu/drm/nouveau/nouveau_gem.c  |    4 ++--
 drivers/gpu/drm/radeon/radeon_object.c |    4 ++--
 drivers/gpu/drm/radeon/radeon_ttm.c    |    6 +++---
 drivers/gpu/drm/ttm/ttm_bo.c           |    7 ++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c    |    5 ++---
 drivers/staging/omapdrm/omap_gem.c     |   10 ++++------
 include/drm/drmP.h                     |    3 ++-
 include/drm/drm_mem_util.h             |    3 +++
 include/drm/ttm/ttm_bo_driver.h        |    3 ++-
 13 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index d166bd0..a4d7d44 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -236,7 +236,6 @@ int drm_lastclose(struct drm_device * dev)
 	    !drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_dma_takedown(dev);
 
-	dev->dev_mapping = NULL;
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG("lastclose completed\n");
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 98cb064..4498d76 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -141,10 +141,10 @@ int drm_open(struct inode *inode, struct file *filp)
 	}
 	if (!retcode) {
 		mutex_lock(&dev->struct_mutex);
-		if (minor->type == DRM_MINOR_LEGACY) {
-			if (dev->dev_mapping == NULL)
-				dev->dev_mapping = inode->i_mapping;
-			else if (dev->dev_mapping != inode->i_mapping)
+		if (minor->type == DRM_MINOR_LEGACY || minor->type == DRM_MINOR_RENDER) {
+			if (minor->dev_mapping == NULL)
+				minor->dev_mapping = inode->i_mapping;
+			else if (minor->dev_mapping != inode->i_mapping)
 				retcode = -ENODEV;
 		}
 		mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index 55cd615..bcd15b0 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -687,3 +687,12 @@ int drm_mmap(struct file *filp, struct vm_area_struct *vma)
 	return ret;
 }
 EXPORT_SYMBOL(drm_mmap);
+
+void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
+		       loff_t const holelen)
+{
+	if (dev->primary->dev_mapping)
+		unmap_mapping_range(dev->primary->dev_mapping,
+				    holebegin, holelen, 1);
+}
+EXPORT_SYMBOL(drm_unmap_mapping);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 19a06c2..5eb0294 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1215,10 +1215,9 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	if (!obj->fault_mappable)
 		return;
 
-	if (obj->base.dev->dev_mapping)
-		unmap_mapping_range(obj->base.dev->dev_mapping,
-				    (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
-				    obj->base.size, 1);
+	drm_unmap_mapping(obj->base.dev,
+			  (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
+			  obj->base.size);
 
 	obj->fault_mappable = false;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 7ce3fde..63521af 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -202,8 +202,8 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
 	struct nouveau_bo *nvbo = NULL;
 	int ret = 0;
 
-	if (unlikely(dev_priv->ttm.bdev.dev_mapping == NULL))
-		dev_priv->ttm.bdev.dev_mapping = dev_priv->dev->dev_mapping;
+	if (unlikely(dev_priv->ttm.bdev.mapping_priv == NULL))
+		dev_priv->ttm.bdev.mapping_priv = (void *)dev;
 
 	if (!dev_priv->engine.vram.flags_valid(dev, req->info.tile_flags)) {
 		NV_ERROR(dev, "bad page flags: 0x%08x\n", req->info.tile_flags);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 342deac..837c7eb 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -115,8 +115,8 @@ int radeon_bo_create(struct radeon_device *rdev,
 
 	size = ALIGN(size, PAGE_SIZE);
 
-	if (unlikely(rdev->mman.bdev.dev_mapping == NULL)) {
-		rdev->mman.bdev.dev_mapping = rdev->ddev->dev_mapping;
+	if (unlikely(rdev->mman.bdev.mapping_priv == NULL)) {
+		rdev->mman.bdev.mapping_priv = (void *)rdev->ddev;
 	}
 	if (kernel) {
 		type = ttm_bo_type_kernel;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index f493c64..7bedbf8 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -753,9 +753,9 @@ int radeon_ttm_init(struct radeon_device *rdev)
 	}
 	DRM_INFO("radeon: %uM of GTT memory ready.\n",
 		 (unsigned)(rdev->mc.gtt_size / (1024 * 1024)));
-	if (unlikely(rdev->mman.bdev.dev_mapping == NULL)) {
-		rdev->mman.bdev.dev_mapping = rdev->ddev->dev_mapping;
-	}
+
+	if (unlikely(rdev->mman.bdev.mapping_priv == NULL))
+		rdev->mman.bdev.mapping_priv = (void *)rdev->ddev;
 
 	r = radeon_ttm_debugfs_init(rdev);
 	if (r) {
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 7c3a57d..40bc5f6 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -38,6 +38,7 @@
 #include <linux/file.h>
 #include <linux/module.h>
 #include <linux/atomic.h>
+#include "drm/drm_mem_util.h"
 
 #define TTM_ASSERT_LOCKED(param)
 #define TTM_DEBUG(fmt, arg...)
@@ -1579,7 +1580,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
 	INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
 	bdev->nice_mode = true;
 	INIT_LIST_HEAD(&bdev->ddestroy);
-	bdev->dev_mapping = NULL;
+	bdev->mapping_priv = NULL;
 	bdev->glob = glob;
 	bdev->need_dma32 = need_dma32;
 	bdev->val_seq = 0;
@@ -1623,9 +1624,9 @@ void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
 	loff_t offset = (loff_t) bo->addr_space_offset;
 	loff_t holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
 
-	if (!bdev->dev_mapping)
+	if (!bdev->mapping_priv)
 		return;
-	unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
+	drm_unmap_mapping(bdev->mapping_priv, offset, holelen);
 	ttm_mem_io_free_vm(bo);
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 1760aba..6a4adc0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -770,9 +770,8 @@ static int vmw_driver_open(struct drm_device *dev, struct drm_file *file_priv)
 
 	file_priv->driver_priv = vmw_fp;
 
-	if (unlikely(dev_priv->bdev.dev_mapping == NULL))
-		dev_priv->bdev.dev_mapping =
-			file_priv->filp->f_path.dentry->d_inode->i_mapping;
+	if (unlikely(dev_priv->bdev.mapping_priv == NULL))
+		dev_priv->bdev.mapping_priv = (void *)dev;
 
 	return 0;
 
diff --git a/drivers/staging/omapdrm/omap_gem.c b/drivers/staging/omapdrm/omap_gem.c
index b7d6f88..22a5f39 100644
--- a/drivers/staging/omapdrm/omap_gem.c
+++ b/drivers/staging/omapdrm/omap_gem.c
@@ -150,13 +150,11 @@ static struct {
 static void evict_entry(struct drm_gem_object *obj,
 		enum tiler_fmt fmt, struct usergart_entry *entry)
 {
-	if (obj->dev->dev_mapping) {
-		size_t size = PAGE_SIZE * usergart[fmt].height;
-		loff_t off = mmap_offset(obj) +
-				(entry->obj_pgoff << PAGE_SHIFT);
-		unmap_mapping_range(obj->dev->dev_mapping, off, size, 1);
-	}
+	size_t size = PAGE_SIZE * usergart[fmt].height;
+	loff_t off = mmap_offset(obj) +
+		(entry->obj_pgoff << PAGE_SHIFT);
 
+	drm_unmap_mapping(obj->dev, off, size);
 	entry->obj = NULL;
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cfd921f..eeb377a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -997,6 +997,8 @@ struct drm_minor {
 	struct drm_master *master; /* currently active master for this node */
 	struct list_head master_list;
 	struct drm_mode_group mode_group;
+
+	struct address_space *dev_mapping;
 };
 
 /* mode specified on the command line */
@@ -1152,7 +1154,6 @@ struct drm_device {
 	unsigned int num_crtcs;                  /**< Number of CRTCs on this device */
 	void *dev_private;		/**< device private data */
 	void *mm_private;
-	struct address_space *dev_mapping;
 	struct drm_sigdata sigdata;	   /**< For block_all_signals */
 	sigset_t sigmask;
 
diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index 6bd325f..820afbb 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -62,4 +62,7 @@ static __inline void drm_free_large(void *ptr)
 	vfree(ptr);
 }
 
+struct drm_device;
+extern void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
+			      loff_t const holelen);
 #endif
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index d43e892..4a05aca 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -40,6 +40,7 @@
 #include "linux/spinlock.h"
 
 struct ttm_backend;
+struct drm_device;
 
 struct ttm_backend_func {
 	/**
@@ -558,7 +559,7 @@ struct ttm_bo_device {
 	 */
 
 	bool nice_mode;
-	struct address_space *dev_mapping;
+	struct drm_device *mapping_priv;
 
 	/*
 	 * Internal protection.
-- 
1.7.8.5

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Dave Airlie]

On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
> Make dev_mapping per-minor instead of per device. This is
> a preparatory patch for introducing render nodes. This
> will allow per-node instead of per-device mapping range,
> once we introduce render nodes.

One problem is this introduces a ttm/drm dependency that we don't
really have so far.

Dave

>
> Patch derived from 7c5cc4f63556e351e9e5980ed22accad410e3fdc
> originally authored by Dave Airlie.
>
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> ---
>  drivers/gpu/drm/drm_drv.c              |    1 -
>  drivers/gpu/drm/drm_fops.c             |    8 ++++----
>  drivers/gpu/drm/drm_vm.c               |    9 +++++++++
>  drivers/gpu/drm/i915/i915_gem.c        |    7 +++----
>  drivers/gpu/drm/nouveau/nouveau_gem.c  |    4 ++--
>  drivers/gpu/drm/radeon/radeon_object.c |    4 ++--
>  drivers/gpu/drm/radeon/radeon_ttm.c    |    6 +++---
>  drivers/gpu/drm/ttm/ttm_bo.c           |    7 ++++---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c    |    5 ++---
>  drivers/staging/omapdrm/omap_gem.c     |   10 ++++------
>  include/drm/drmP.h                     |    3 ++-
>  include/drm/drm_mem_util.h             |    3 +++
>  include/drm/ttm/ttm_bo_driver.h        |    3 ++-
>  13 files changed, 40 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index d166bd0..a4d7d44 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -236,7 +236,6 @@ int drm_lastclose(struct drm_device * dev)
>            !drm_core_check_feature(dev, DRIVER_MODESET))
>                drm_dma_takedown(dev);
>
> -       dev->dev_mapping = NULL;
>        mutex_unlock(&dev->struct_mutex);
>
>        DRM_DEBUG("lastclose completed\n");
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 98cb064..4498d76 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -141,10 +141,10 @@ int drm_open(struct inode *inode, struct file *filp)
>        }
>        if (!retcode) {
>                mutex_lock(&dev->struct_mutex);
> -               if (minor->type == DRM_MINOR_LEGACY) {
> -                       if (dev->dev_mapping == NULL)
> -                               dev->dev_mapping = inode->i_mapping;
> -                       else if (dev->dev_mapping != inode->i_mapping)
> +               if (minor->type == DRM_MINOR_LEGACY || minor->type == DRM_MINOR_RENDER) {
> +                       if (minor->dev_mapping == NULL)
> +                               minor->dev_mapping = inode->i_mapping;
> +                       else if (minor->dev_mapping != inode->i_mapping)
>                                retcode = -ENODEV;
>                }
>                mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index 55cd615..bcd15b0 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -687,3 +687,12 @@ int drm_mmap(struct file *filp, struct vm_area_struct *vma)
>        return ret;
>  }
>  EXPORT_SYMBOL(drm_mmap);
> +
> +void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
> +                      loff_t const holelen)
> +{
> +       if (dev->primary->dev_mapping)
> +               unmap_mapping_range(dev->primary->dev_mapping,
> +                                   holebegin, holelen, 1);
> +}
> +EXPORT_SYMBOL(drm_unmap_mapping);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 19a06c2..5eb0294 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1215,10 +1215,9 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>        if (!obj->fault_mappable)
>                return;
>
> -       if (obj->base.dev->dev_mapping)
> -               unmap_mapping_range(obj->base.dev->dev_mapping,
> -                                   (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
> -                                   obj->base.size, 1);
> +       drm_unmap_mapping(obj->base.dev,
> +                         (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
> +                         obj->base.size);
>
>        obj->fault_mappable = false;
>  }
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 7ce3fde..63521af 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -202,8 +202,8 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
>        struct nouveau_bo *nvbo = NULL;
>        int ret = 0;
>
> -       if (unlikely(dev_priv->ttm.bdev.dev_mapping == NULL))
> -               dev_priv->ttm.bdev.dev_mapping = dev_priv->dev->dev_mapping;
> +       if (unlikely(dev_priv->ttm.bdev.mapping_priv == NULL))
> +               dev_priv->ttm.bdev.mapping_priv = (void *)dev;
>
>        if (!dev_priv->engine.vram.flags_valid(dev, req->info.tile_flags)) {
>                NV_ERROR(dev, "bad page flags: 0x%08x\n", req->info.tile_flags);
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 342deac..837c7eb 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -115,8 +115,8 @@ int radeon_bo_create(struct radeon_device *rdev,
>
>        size = ALIGN(size, PAGE_SIZE);
>
> -       if (unlikely(rdev->mman.bdev.dev_mapping == NULL)) {
> -               rdev->mman.bdev.dev_mapping = rdev->ddev->dev_mapping;
> +       if (unlikely(rdev->mman.bdev.mapping_priv == NULL)) {
> +               rdev->mman.bdev.mapping_priv = (void *)rdev->ddev;
>        }
>        if (kernel) {
>                type = ttm_bo_type_kernel;
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index f493c64..7bedbf8 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -753,9 +753,9 @@ int radeon_ttm_init(struct radeon_device *rdev)
>        }
>        DRM_INFO("radeon: %uM of GTT memory ready.\n",
>                 (unsigned)(rdev->mc.gtt_size / (1024 * 1024)));
> -       if (unlikely(rdev->mman.bdev.dev_mapping == NULL)) {
> -               rdev->mman.bdev.dev_mapping = rdev->ddev->dev_mapping;
> -       }
> +
> +       if (unlikely(rdev->mman.bdev.mapping_priv == NULL))
> +               rdev->mman.bdev.mapping_priv = (void *)rdev->ddev;
>
>        r = radeon_ttm_debugfs_init(rdev);
>        if (r) {
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 7c3a57d..40bc5f6 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -38,6 +38,7 @@
>  #include <linux/file.h>
>  #include <linux/module.h>
>  #include <linux/atomic.h>
> +#include "drm/drm_mem_util.h"
>
>  #define TTM_ASSERT_LOCKED(param)
>  #define TTM_DEBUG(fmt, arg...)
> @@ -1579,7 +1580,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
>        INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
>        bdev->nice_mode = true;
>        INIT_LIST_HEAD(&bdev->ddestroy);
> -       bdev->dev_mapping = NULL;
> +       bdev->mapping_priv = NULL;
>        bdev->glob = glob;
>        bdev->need_dma32 = need_dma32;
>        bdev->val_seq = 0;
> @@ -1623,9 +1624,9 @@ void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
>        loff_t offset = (loff_t) bo->addr_space_offset;
>        loff_t holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
>
> -       if (!bdev->dev_mapping)
> +       if (!bdev->mapping_priv)
>                return;
> -       unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
> +       drm_unmap_mapping(bdev->mapping_priv, offset, holelen);
>        ttm_mem_io_free_vm(bo);
>  }
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 1760aba..6a4adc0 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -770,9 +770,8 @@ static int vmw_driver_open(struct drm_device *dev, struct drm_file *file_priv)
>
>        file_priv->driver_priv = vmw_fp;
>
> -       if (unlikely(dev_priv->bdev.dev_mapping == NULL))
> -               dev_priv->bdev.dev_mapping =
> -                       file_priv->filp->f_path.dentry->d_inode->i_mapping;
> +       if (unlikely(dev_priv->bdev.mapping_priv == NULL))
> +               dev_priv->bdev.mapping_priv = (void *)dev;
>
>        return 0;
>
> diff --git a/drivers/staging/omapdrm/omap_gem.c b/drivers/staging/omapdrm/omap_gem.c
> index b7d6f88..22a5f39 100644
> --- a/drivers/staging/omapdrm/omap_gem.c
> +++ b/drivers/staging/omapdrm/omap_gem.c
> @@ -150,13 +150,11 @@ static struct {
>  static void evict_entry(struct drm_gem_object *obj,
>                enum tiler_fmt fmt, struct usergart_entry *entry)
>  {
> -       if (obj->dev->dev_mapping) {
> -               size_t size = PAGE_SIZE * usergart[fmt].height;
> -               loff_t off = mmap_offset(obj) +
> -                               (entry->obj_pgoff << PAGE_SHIFT);
> -               unmap_mapping_range(obj->dev->dev_mapping, off, size, 1);
> -       }
> +       size_t size = PAGE_SIZE * usergart[fmt].height;
> +       loff_t off = mmap_offset(obj) +
> +               (entry->obj_pgoff << PAGE_SHIFT);
>
> +       drm_unmap_mapping(obj->dev, off, size);
>        entry->obj = NULL;
>  }
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index cfd921f..eeb377a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -997,6 +997,8 @@ struct drm_minor {
>        struct drm_master *master; /* currently active master for this node */
>        struct list_head master_list;
>        struct drm_mode_group mode_group;
> +
> +       struct address_space *dev_mapping;
>  };
>
>  /* mode specified on the command line */
> @@ -1152,7 +1154,6 @@ struct drm_device {
>        unsigned int num_crtcs;                  /**< Number of CRTCs on this device */
>        void *dev_private;              /**< device private data */
>        void *mm_private;
> -       struct address_space *dev_mapping;
>        struct drm_sigdata sigdata;        /**< For block_all_signals */
>        sigset_t sigmask;
>
> diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
> index 6bd325f..820afbb 100644
> --- a/include/drm/drm_mem_util.h
> +++ b/include/drm/drm_mem_util.h
> @@ -62,4 +62,7 @@ static __inline void drm_free_large(void *ptr)
>        vfree(ptr);
>  }
>
> +struct drm_device;
> +extern void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
> +                             loff_t const holelen);
>  #endif
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index d43e892..4a05aca 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -40,6 +40,7 @@
>  #include "linux/spinlock.h"
>
>  struct ttm_backend;
> +struct drm_device;
>
>  struct ttm_backend_func {
>        /**
> @@ -558,7 +559,7 @@ struct ttm_bo_device {
>         */
>
>        bool nice_mode;
> -       struct address_space *dev_mapping;
> +       struct drm_device *mapping_priv;
>
>        /*
>         * Internal protection.
> --
> 1.7.8.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Ilija Hadzic]

On Fri, 20 Apr 2012, Dave Airlie wrote:

> On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
> <ihadzic@research.bell-labs.com> wrote:
>> Make dev_mapping per-minor instead of per device. This is
>> a preparatory patch for introducing render nodes. This
>> will allow per-node instead of per-device mapping range,
>> once we introduce render nodes.
>
> One problem is this introduces a ttm/drm dependency that we don't
> really have so far.
>

Sorry for a belated follow-up (I was busy with something else). I 
understand the concern and I think that the patch can be reworked such 
that it does not introduce the dependency. What introduces the dependency 
is a call to drm_unmap_mapping in ttm_bo_unmap_virtual. If the patch is 
reworked such that the minor tracks i_mapping pointer instead of the whole 
drm_device structure, the need for drm_unmap_mapping may be eliminated, 
which in turn will take care of the dependency.

That brings me to a question for you. The patch from which I derived this 
is your patch 7c5cc4f63556e351e9e5980ed22accad410e3fdc (on your original 
render-nodes branch). That's where you introduced drm_unmap_mapping 
function, which calls unmap_mapping_range for every minor known to the 
system.

Why was that necessary at first place ? I mean we don't do the eqivalent 
of that currently when there are multiple physical GPUs in the system, so 
I don't quite get why introduction of multiple minors would require to go 
through the whole list of minors. If that's a "just in case"/"just to be 
safe" kind of thing, then I think we may be able to just call 
drm_unmap_mapping from the driver that needs to call it and only for the 
minor in question.

If we can do that, then the patch will in principle look the same to what 
the current code does, except that dev_mapping will be in the minor 
structure and there will ne no new dependencies introduced. The patch will 
also be simpler that way.

-- Ilija

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Dave Airlie]

On Mon, Apr 30, 2012 at 3:48 PM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
>
>
> On Fri, 20 Apr 2012, Dave Airlie wrote:
>
>> On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
>> <ihadzic@research.bell-labs.com> wrote:
>>>
>>> Make dev_mapping per-minor instead of per device. This is
>>> a preparatory patch for introducing render nodes. This
>>> will allow per-node instead of per-device mapping range,
>>> once we introduce render nodes.
>>
>>
>> One problem is this introduces a ttm/drm dependency that we don't
>> really have so far.
>>
>
> Sorry for a belated follow-up (I was busy with something else). I understand
> the concern and I think that the patch can be reworked such that it does not
> introduce the dependency. What introduces the dependency is a call to
> drm_unmap_mapping in ttm_bo_unmap_virtual. If the patch is reworked such
> that the minor tracks i_mapping pointer instead of the whole drm_device
> structure, the need for drm_unmap_mapping may be eliminated, which in turn
> will take care of the dependency.
>
> That brings me to a question for you. The patch from which I derived this is
> your patch 7c5cc4f63556e351e9e5980ed22accad410e3fdc (on your original
> render-nodes branch). That's where you introduced drm_unmap_mapping
> function, which calls unmap_mapping_range for every minor known to the
> system.
>
> Why was that necessary at first place ? I mean we don't do the eqivalent of
> that currently when there are multiple physical GPUs in the system, so I
> don't quite get why introduction of multiple minors would require to go
> through the whole list of minors. If that's a "just in case"/"just to be
> safe" kind of thing, then I think we may be able to just call
> drm_unmap_mapping from the driver that needs to call it and only for the
> minor in question.

When we move a buffer from VRAM->RAM we have to invalidate all
userspace mappings for it.

There could be userspace mappings on any of the device nodes, so we
need to get them all.

I think some of this code could be redone, to be less complex, Al Viro
questions why we weren't just setting the
inode mapping ourselves, citing some references in an fs somewhere
(maybe cifs), I haven't had time to follow up and check.

This would also make the case where you have something else create a
device node inside a chroot.

Dave.

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Ilija Hadzic]

On Mon, 30 Apr 2012, Dave Airlie wrote:

>
> When we move a buffer from VRAM->RAM we have to invalidate all
> userspace mappings for it.
>
> There could be userspace mappings on any of the device nodes, so we
> need to get them all.
>

Ah OK, I get it ... and the reason you don't have to do this when you have 
multiple physical cards, but only one node per card (i.e. current state of 
the code) is that VRAMs of each card are disjoint, so there can't possibly 
be mappings through different /dev/dri/whatever of the same piece of VRAM. 
Multiple render nodes, introduce that possibility.

> I think some of this code could be redone, to be less complex, Al Viro
> questions why we weren't just setting the
> inode mapping ourselves, citing some references in an fs somewhere
> (maybe cifs), I haven't had time to follow up and check.
>
> This would also make the case where you have something else create a
> device node inside a chroot.
>

Do you have pointers to that discussion (assuming it was on sime mailing 
list)? The least I can do, while I am at it, is try to understand it and
see if I can incorporate some ideas from there in the rework of the patch.

-- Ilija

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Dave Airlie]

>
> Do you have pointers to that discussion (assuming it was on sime mailing
> list)? The least I can do, while I am at it, is try to understand it and
> see if I can incorporate some ideas from there in the rework of the patch.

Nope it was an offhand discussion on irc while we were talking about
some other ugly code.

Dave.

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Dave Airlie]

On Mon, Apr 30, 2012 at 6:53 PM, Dave Airlie <airlied@gmail.com> wrote:
>>
>> Do you have pointers to that discussion (assuming it was on sime mailing
>> list)? The least I can do, while I am at it, is try to understand it and
>> see if I can incorporate some ideas from there in the rework of the patch.
>
> Nope it was an offhand discussion on irc while we were talking about
> some other ugly code.

Okay I think it was the code in coda,

       if (coda_inode->i_mapping == &coda_inode->i_data)
                coda_inode->i_mapping = host_inode->i_mapping;

        /* only allow additional mmaps as long as userspace isn't changing
         * the container file on us! */
        else if (coda_inode->i_mapping != host_inode->i_mapping) {
                spin_unlock(&cii->c_lock);
                return -EBUSY;
        }

We could use code like that to change the i_mapping, instead of failing.

Dave.

Re: [PATCH 05/19] drm: move dev_mapping to the minor node

[Ilija Hadzic]

On Mon, 30 Apr 2012, Dave Airlie wrote:

> On Mon, Apr 30, 2012 at 6:53 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>
>>> Do you have pointers to that discussion (assuming it was on sime mailing
>>> list)? The least I can do, while I am at it, is try to understand it and
>>> see if I can incorporate some ideas from there in the rework of the patch.
>>
>> Nope it was an offhand discussion on irc while we were talking about
>> some other ugly code.
>
> Okay I think it was the code in coda,
>
>       if (coda_inode->i_mapping == &coda_inode->i_data)
>                coda_inode->i_mapping = host_inode->i_mapping;
>
>        /* only allow additional mmaps as long as userspace isn't changing
>         * the container file on us! */
>        else if (coda_inode->i_mapping != host_inode->i_mapping) {
>                spin_unlock(&cii->c_lock);
>                return -EBUSY;
>        }
>
> We could use code like that to change the i_mapping, instead of failing.
>
> Dave.
>

I have just cut (and sent to the list) a patch that, I believe, implements 
this idea. I did it slightly differently from what coda fs does, but it's 
the same concept. I set i_mapping in the open() instead of moving it 
down to mmap(), because there are so many variants of mmap both in drm and 
GPU-specific drivers, that it would get too messy.

I traced the code quite extensively with printks and it looks like it's 
doing the right thing.

If the new patch seems right, then if and when we add render nodes, the 
original dev_mapping patch that started this thread won't be needed any 
more.

-- Ilija

[PATCH 06/19] drm: add support for render nodes

[Ilija Hadzic]

Add support for creating multiple render nodes on the same
physical GPU.

Patch derived from 7c5cc4f63556e351e9e5980ed22accad410e3fdc
originally authored by Dave Airlie.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |    1 +
 drivers/gpu/drm/drm_stub.c |   32 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_vm.c   |    7 +++++++
 include/drm/drmP.h         |    4 +++-
 include/drm/drm_crtc.h     |    1 +
 5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index eafb49d..4fc7c5f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -952,6 +952,7 @@ int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
 	group->num_planes = 0;
 	return 0;
 }
+EXPORT_SYMBOL(drm_mode_group_init);
 
 int drm_mode_group_init_legacy_group(struct drm_device *dev,
 				     struct drm_mode_group *group)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 6d7b083..53033d3 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -269,6 +269,7 @@ int drm_fill_in_dev(struct drm_device *dev,
 	INIT_LIST_HEAD(&dev->vmalist);
 	INIT_LIST_HEAD(&dev->maplist);
 	INIT_LIST_HEAD(&dev->vblank_event_list);
+	INIT_LIST_HEAD(&dev->render_minor_list);
 
 	spin_lock_init(&dev->count_lock);
 	spin_lock_init(&dev->event_lock);
@@ -355,6 +356,7 @@ int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type)
 	new_minor->dev = dev;
 	new_minor->index = minor_id;
 	INIT_LIST_HEAD(&new_minor->master_list);
+	INIT_LIST_HEAD(&new_minor->render_node_list);
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
 
@@ -398,6 +400,29 @@ err_idr:
 	return ret;
 }
 
+int drm_create_minor_render(struct drm_device *dev, struct drm_minor **minor_p)
+{
+	int ret;
+	struct drm_minor *minor;
+
+	ret = drm_get_minor(dev, &minor, DRM_MINOR_RENDER);
+	if (ret)
+		return ret;
+
+	list_add_tail(&minor->render_node_list, &dev->render_minor_list);
+	return 0;
+}
+
+int drm_destroy_minor_render(struct drm_device *dev)
+{
+	struct drm_minor *minor, *tmp;
+
+	list_for_each_entry_safe(minor, tmp, &dev->render_minor_list, render_node_list) {
+		drm_put_minor(&minor);
+	}
+	return 0;
+}
+
 /**
  * Put a secondary minor number.
  *
@@ -414,6 +439,8 @@ int drm_put_minor(struct drm_minor **minor_p)
 
 	DRM_DEBUG("release secondary minor %d\n", minor->index);
 
+	list_del(&minor->render_node_list);
+
 	if (minor->type == DRM_MINOR_LEGACY)
 		drm_proc_cleanup(minor, drm_proc_root);
 #if defined(CONFIG_DEBUG_FS)
@@ -476,9 +503,12 @@ void drm_put_dev(struct drm_device *dev)
 
 	drm_ctxbitmap_cleanup(dev);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_put_minor(&dev->control);
 
+		drm_destroy_minor_render(dev);
+	}
+
 	if (driver->driver_features & DRIVER_GEM)
 		drm_gem_destroy(dev);
 
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index bcd15b0..7cf67dd 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -691,8 +691,15 @@ EXPORT_SYMBOL(drm_mmap);
 void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
 		       loff_t const holelen)
 {
+	struct drm_minor *minor;
 	if (dev->primary->dev_mapping)
 		unmap_mapping_range(dev->primary->dev_mapping,
 				    holebegin, holelen, 1);
+
+	list_for_each_entry(minor, &dev->render_minor_list, render_node_list) {
+		if (minor->dev_mapping)
+			unmap_mapping_range(minor->dev_mapping,
+					    holebegin, holelen, 1);
+	}
 }
 EXPORT_SYMBOL(drm_unmap_mapping);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index eeb377a..d9eee26 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -999,6 +999,8 @@ struct drm_minor {
 	struct drm_mode_group mode_group;
 
 	struct address_space *dev_mapping;
+
+	struct list_head render_node_list;
 };
 
 /* mode specified on the command line */
@@ -1162,7 +1164,7 @@ struct drm_device {
 	unsigned int agp_buffer_token;
 	struct drm_minor *control;		/**< Control node for card */
 	struct drm_minor *primary;		/**< render type primary screen head */
-
+	struct list_head render_minor_list;
         struct drm_mode_config mode_config;	/**< Current mode config */
 
 	/** \name GEM information */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4aa793e..2b9d062 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -851,6 +851,7 @@ extern char *drm_get_dvi_i_select_name(int val);
 extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
+extern int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);
-- 
1.7.8.5

[PATCH 07/19] drm: initial multiple nodes ioctl work.

[Ilija Hadzic]

From: Dave Airlie <airlied@redhat.com>

just adds some unchecked ioctls to setup the nodes.

Signed-off-by: Dave Airlie <airlied@redhat.com>

v2: - original ioctl numbers are now taken, use next available
    - resolve some trivial conflicts due to bit-rot that
      occurred since the original patch was created

v3: - added planes to ABI to address a comment from Ville Syrjala

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_drv.c  |    4 ++-
 drivers/gpu/drm/drm_stub.c |   56 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm.h          |    3 ++
 include/drm/drmP.h         |    6 ++++
 include/drm/drm_mode.h     |   15 +++++++++++
 5 files changed, 83 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a4d7d44..dd04322 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -159,7 +159,9 @@ static struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED)
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_RENDER_NODE_CREATE, drm_render_node_create_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_RENDER_NODE_REMOVE, drm_render_node_remove_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED)
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 53033d3..5ef0e46 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -522,3 +522,59 @@ void drm_put_dev(struct drm_device *dev)
 	kfree(dev);
 }
 EXPORT_SYMBOL(drm_put_dev);
+
+int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file_priv)
+{
+	struct drm_render_node_create *args = data;
+	int ret;
+	struct drm_minor *new_minor;
+	int total_ids, i;
+	uint32_t __user *ids_ptr;
+	ret = drm_create_minor_render(dev, &new_minor);
+	if (ret)
+		goto out;
+
+	args->node_minor_id = new_minor->index;
+
+	if (args->num_crtc == 0 && args->num_encoder == 0 &&
+	    args->num_connector == 0 && args->num_plane == 0)
+		goto out;
+	if (args->num_crtc == 0 ||
+	    args->num_encoder == 0 ||
+	    args->num_connector == 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = drm_mode_group_init(dev, &new_minor->mode_group);
+	if (ret)
+		goto out;
+
+	ids_ptr = (uint32_t __user *)(unsigned long)args->id_list_ptr;
+	total_ids = args->num_crtc + args->num_encoder +
+		args->num_connector + args->num_plane;
+	for (i = 0; i < total_ids; i++) {
+		if (get_user(new_minor->mode_group.id_list[i], &ids_ptr[i])) {
+			ret = -EFAULT;
+			goto out_put;
+		}
+	}
+out_put:
+	drm_put_minor(&new_minor);
+out:
+	return ret;
+}
+
+int drm_render_node_remove_ioctl(struct drm_device *dev, void *data,
+				 struct drm_file *file_priv)
+{
+	struct drm_render_node_remove *args = data;
+	struct drm_minor *del_minor, *tmp;
+
+	list_for_each_entry_safe(del_minor, tmp, &dev->render_minor_list, render_node_list) {
+		if (del_minor->index == args->node_minor_id)
+			drm_put_minor(&del_minor);
+	}
+	return 0;
+}
diff --git a/include/drm/drm.h b/include/drm/drm.h
index 34a7b89..d2a893b 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -639,6 +639,9 @@ struct drm_get_cap {
 #define DRM_IOCTL_GEM_OPEN		DRM_IOWR(0x0b, struct drm_gem_open)
 #define DRM_IOCTL_GET_CAP		DRM_IOWR(0x0c, struct drm_get_cap)
 
+#define DRM_IOCTL_RENDER_NODE_CREATE    DRM_IOWR(0x0d, struct drm_render_node_create)
+#define DRM_IOCTL_RENDER_NODE_REMOVE    DRM_IOWR(0x0e, struct drm_render_node_remove)
+
 #define DRM_IOCTL_SET_UNIQUE		DRM_IOW( 0x10, struct drm_unique)
 #define DRM_IOCTL_AUTH_MAGIC		DRM_IOW( 0x11, struct drm_auth)
 #define DRM_IOCTL_BLOCK			DRM_IOWR(0x12, struct drm_block)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d9eee26..3bc5c71 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1458,6 +1458,12 @@ extern void drm_master_put(struct drm_master **master);
 
 extern void drm_put_dev(struct drm_device *dev);
 extern int drm_put_minor(struct drm_minor **minor);
+
+extern int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
+					struct drm_file *file_priv);
+extern int drm_render_node_remove_ioctl(struct drm_device *dev, void *data,
+					struct drm_file *file_priv);
+
 extern unsigned int drm_debug;
 
 extern unsigned int drm_vblank_offdelay;
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 2a2acda..c9b9fc1 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -441,4 +441,19 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+/* render node create and remove functions
+   if crtc/encoders/connectors/planes all == 0 then gpgpu node */
+struct drm_render_node_create {
+	__u32 node_minor_id;
+	__u32 num_crtc;
+	__u32 num_encoder;
+	__u32 num_connector;
+	__u32 num_plane;
+	__u64 id_list_ptr;
+};
+
+struct drm_render_node_remove {
+	__u32 node_minor_id;
+};
+
 #endif
-- 
1.7.8.5

Re: [PATCH 07/19] drm: initial multiple nodes ioctl work.

[Dave Airlie]

> +/* render node create and remove functions
> +   if crtc/encoders/connectors/planes all == 0 then gpgpu node */
> +struct drm_render_node_create {
> +       __u32 node_minor_id;
> +       __u32 num_crtc;
> +       __u32 num_encoder;
> +       __u32 num_connector;
> +       __u32 num_plane;
> +       __u64 id_list_ptr;
> +};

This struct is wrongly aligned, you need a 32-bit pad after num plane.

Dave.

[PATCH 08/19] drm: separate render node descriptor from minor

[Ilija Hadzic]

drm_minor structure had a list_head pointer that was
used only for render nodes. control and primary nodes
do not live in the list and had this list pointer unused.

Create a separate drm_render_node structure that is used
to build the render node list and that points to the
coresponding minor. This avoids having fields that are used
only in some cases and also to allows adding render node
specific fields that primary and control minor may not care
about.

Also fix a few minor things:
 * change the names of functions that create and delete
   render nodes to more logical ones.
 * make create_render_node actually populate minor_p
   pointer passed to it so that the ioctl has
   some chance of working

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |   45 +++++++++++++++++++++++++------------------
 drivers/gpu/drm/drm_vm.c   |    9 ++++---
 include/drm/drmP.h         |    9 ++++++-
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 5ef0e46..c6a0e71 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -269,7 +269,7 @@ int drm_fill_in_dev(struct drm_device *dev,
 	INIT_LIST_HEAD(&dev->vmalist);
 	INIT_LIST_HEAD(&dev->maplist);
 	INIT_LIST_HEAD(&dev->vblank_event_list);
-	INIT_LIST_HEAD(&dev->render_minor_list);
+	INIT_LIST_HEAD(&dev->render_node_list);
 
 	spin_lock_init(&dev->count_lock);
 	spin_lock_init(&dev->event_lock);
@@ -356,7 +356,6 @@ int drm_get_minor(struct drm_device *dev, struct drm_minor **minor, int type)
 	new_minor->dev = dev;
 	new_minor->index = minor_id;
 	INIT_LIST_HEAD(&new_minor->master_list);
-	INIT_LIST_HEAD(&new_minor->render_node_list);
 
 	idr_replace(&drm_minors_idr, new_minor, minor_id);
 
@@ -400,25 +399,36 @@ err_idr:
 	return ret;
 }
 
-int drm_create_minor_render(struct drm_device *dev, struct drm_minor **minor_p)
+int drm_create_render_node(struct drm_device *dev, struct drm_minor **minor_p)
 {
 	int ret;
 	struct drm_minor *minor;
+	struct drm_render_node *render_node;
+
+	render_node = kmalloc(sizeof(struct drm_render_node), GFP_KERNEL);
+	if (!render_node)
+		return -ENOMEM;
 
 	ret = drm_get_minor(dev, &minor, DRM_MINOR_RENDER);
-	if (ret)
+	if (ret) {
+		kfree(render_node);
 		return ret;
-
-	list_add_tail(&minor->render_node_list, &dev->render_minor_list);
+	}
+	render_node->minor = minor;
+	*minor_p = minor;
+	list_add_tail(&render_node->list, &dev->render_node_list);
 	return 0;
 }
 
-int drm_destroy_minor_render(struct drm_device *dev)
+int drm_destroy_all_render_nodes(struct drm_device *dev)
 {
-	struct drm_minor *minor, *tmp;
+	struct drm_render_node *render_node, *tmp;
 
-	list_for_each_entry_safe(minor, tmp, &dev->render_minor_list, render_node_list) {
-		drm_put_minor(&minor);
+	list_for_each_entry_safe(render_node, tmp,
+				 &dev->render_node_list, list) {
+		list_del(&render_node->list);
+		drm_put_minor(&render_node->minor);
+		kfree(render_node);
 	}
 	return 0;
 }
@@ -439,8 +449,6 @@ int drm_put_minor(struct drm_minor **minor_p)
 
 	DRM_DEBUG("release secondary minor %d\n", minor->index);
 
-	list_del(&minor->render_node_list);
-
 	if (minor->type == DRM_MINOR_LEGACY)
 		drm_proc_cleanup(minor, drm_proc_root);
 #if defined(CONFIG_DEBUG_FS)
@@ -505,8 +513,7 @@ void drm_put_dev(struct drm_device *dev)
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		drm_put_minor(&dev->control);
-
-		drm_destroy_minor_render(dev);
+		drm_destroy_all_render_nodes(dev);
 	}
 
 	if (driver->driver_features & DRIVER_GEM)
@@ -531,7 +538,7 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_minor *new_minor;
 	int total_ids, i;
 	uint32_t __user *ids_ptr;
-	ret = drm_create_minor_render(dev, &new_minor);
+	ret = drm_create_render_node(dev, &new_minor);
 	if (ret)
 		goto out;
 
@@ -570,11 +577,11 @@ int drm_render_node_remove_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *file_priv)
 {
 	struct drm_render_node_remove *args = data;
-	struct drm_minor *del_minor, *tmp;
+	struct drm_render_node *del_node, *tmp;
 
-	list_for_each_entry_safe(del_minor, tmp, &dev->render_minor_list, render_node_list) {
-		if (del_minor->index == args->node_minor_id)
-			drm_put_minor(&del_minor);
+	list_for_each_entry_safe(del_node, tmp, &dev->render_node_list, list) {
+		if (del_node->minor->index == args->node_minor_id)
+			drm_put_minor(&del_node->minor);
 	}
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index 7cf67dd..73146e3 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -691,14 +691,15 @@ EXPORT_SYMBOL(drm_mmap);
 void drm_unmap_mapping(struct drm_device *dev, loff_t const holebegin,
 		       loff_t const holelen)
 {
-	struct drm_minor *minor;
+	struct drm_render_node *render_node;
+
 	if (dev->primary->dev_mapping)
 		unmap_mapping_range(dev->primary->dev_mapping,
 				    holebegin, holelen, 1);
 
-	list_for_each_entry(minor, &dev->render_minor_list, render_node_list) {
-		if (minor->dev_mapping)
-			unmap_mapping_range(minor->dev_mapping,
+	list_for_each_entry(render_node, &dev->render_node_list, list) {
+		if (render_node->minor->dev_mapping)
+			unmap_mapping_range(render_node->minor->dev_mapping,
 					    holebegin, holelen, 1);
 	}
 }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3bc5c71..1131fd4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -999,8 +999,13 @@ struct drm_minor {
 	struct drm_mode_group mode_group;
 
 	struct address_space *dev_mapping;
+};
 
-	struct list_head render_node_list;
+
+/* render node descriptor */
+struct drm_render_node {
+	struct list_head list;
+	struct drm_minor *minor;
 };
 
 /* mode specified on the command line */
@@ -1164,7 +1169,7 @@ struct drm_device {
 	unsigned int agp_buffer_token;
 	struct drm_minor *control;		/**< Control node for card */
 	struct drm_minor *primary;		/**< render type primary screen head */
-	struct list_head render_minor_list;
+	struct list_head render_node_list;
         struct drm_mode_config mode_config;	/**< Current mode config */
 
 	/** \name GEM information */
-- 
1.7.8.5

[PATCH 09/19] drm: cleanup render node ioctls

[Ilija Hadzic]

Fix a few bugs in render node create and destroy ioctl
(mostly handling of error cases). Also separate some
common code into a new function for destroying
the render node.

v2: - support planes

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |   81 +++++++++++++++++++++++++++----------------
 1 files changed, 51 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index c6a0e71..d7ec39c 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -420,17 +420,30 @@ int drm_create_render_node(struct drm_device *dev, struct drm_minor **minor_p)
 	return 0;
 }
 
-int drm_destroy_all_render_nodes(struct drm_device *dev)
+int drm_destroy_render_node(struct drm_device *dev, int index)
 {
-	struct drm_render_node *render_node, *tmp;
+	struct drm_render_node *node, *tmp;
+
+	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
+		if (node->minor->index == index) {
+			list_del(&node->list);
+			drm_put_minor(&node->minor);
+			kfree(node);
+			return 0;
+		}
+	}
+	return -ENODEV;
+}
 
-	list_for_each_entry_safe(render_node, tmp,
-				 &dev->render_node_list, list) {
-		list_del(&render_node->list);
-		drm_put_minor(&render_node->minor);
-		kfree(render_node);
+void drm_destroy_all_render_nodes(struct drm_device *dev)
+{
+	struct drm_render_node *node, *tmp;
+
+	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
+		list_del(&node->list);
+		drm_put_minor(&node->minor);
+		kfree(node);
 	}
-	return 0;
 }
 
 /**
@@ -538,25 +551,30 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_minor *new_minor;
 	int total_ids, i;
 	uint32_t __user *ids_ptr;
-	ret = drm_create_render_node(dev, &new_minor);
-	if (ret)
-		goto out;
-
-	args->node_minor_id = new_minor->index;
 
+	/* trivial case: render node with no display resources */
 	if (args->num_crtc == 0 && args->num_encoder == 0 &&
-	    args->num_connector == 0 && args->num_plane == 0)
-		goto out;
+	    args->num_connector == 0 && args->num_plane == 0) {
+		ret = drm_create_render_node(dev, &new_minor);
+		if (!ret)
+			args->node_minor_id = new_minor->index;
+		return ret;
+	}
+
+	/* if we have display resources, then we need at least
+	 * one CRTC, one encoder and one connector */
 	if (args->num_crtc == 0 ||
 	    args->num_encoder == 0 ||
-	    args->num_connector == 0) {
-		ret = -EINVAL;
-		goto out;
-	}
+	    args->num_connector == 0)
+		return -EINVAL;
+
+	ret = drm_create_render_node(dev, &new_minor);
+	if (ret)
+		return ret;
 
 	ret = drm_mode_group_init(dev, &new_minor->mode_group);
 	if (ret)
-		goto out;
+		goto out_del;
 
 	ids_ptr = (uint32_t __user *)(unsigned long)args->id_list_ptr;
 	total_ids = args->num_crtc + args->num_encoder +
@@ -564,12 +582,18 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	for (i = 0; i < total_ids; i++) {
 		if (get_user(new_minor->mode_group.id_list[i], &ids_ptr[i])) {
 			ret = -EFAULT;
-			goto out_put;
+			goto out_del;
 		}
 	}
-out_put:
-	drm_put_minor(&new_minor);
-out:
+	new_minor->mode_group.num_crtcs = args->num_crtc;
+	new_minor->mode_group.num_encoders = args->num_encoder;
+	new_minor->mode_group.num_connectors = args->num_connector;
+	new_minor->mode_group.num_planes = args->num_plane;
+
+	args->node_minor_id = new_minor->index;
+	return 0;
+out_del:
+	drm_destroy_render_node(dev, new_minor->index);
 	return ret;
 }
 
@@ -577,11 +601,8 @@ int drm_render_node_remove_ioctl(struct drm_device *dev, void *data,
 				 struct drm_file *file_priv)
 {
 	struct drm_render_node_remove *args = data;
-	struct drm_render_node *del_node, *tmp;
+	int ret;
 
-	list_for_each_entry_safe(del_node, tmp, &dev->render_node_list, list) {
-		if (del_node->minor->index == args->node_minor_id)
-			drm_put_minor(&del_node->minor);
-	}
-	return 0;
+	ret = drm_destroy_render_node(dev, args->node_minor_id);
+	return ret;
 }
-- 
1.7.8.5

[PATCH 10/19] drm: only allow render node ioctls through control node

[Ilija Hadzic]

The render-node manipulation ioctls are supposed to be
issued through control node only. Add a check and return
-EPERM to user space if access is attempted through a
node other than a control node.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index d7ec39c..6629dd7 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -552,6 +552,10 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	int total_ids, i;
 	uint32_t __user *ids_ptr;
 
+	/* allow access through control node only */
+	if (file_priv->minor != dev->control)
+		return -EPERM;
+
 	/* trivial case: render node with no display resources */
 	if (args->num_crtc == 0 && args->num_encoder == 0 &&
 	    args->num_connector == 0 && args->num_plane == 0) {
@@ -603,6 +607,10 @@ int drm_render_node_remove_ioctl(struct drm_device *dev, void *data,
 	struct drm_render_node_remove *args = data;
 	int ret;
 
+	/* allow access through control node only */
+	if (file_priv->minor != dev->control)
+		return -EPERM;
+
 	ret = drm_destroy_render_node(dev, args->node_minor_id);
 	return ret;
 }
-- 
1.7.8.5

[PATCH 11/19] drm: do not remove a render node in use

[Ilija Hadzic]

Keep track of per-node open count and do not allow removal
of a render node in use.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_fops.c |    2 ++
 drivers/gpu/drm/drm_stub.c |    2 ++
 include/drm/drmP.h         |    1 +
 3 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 4498d76..686236f 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -136,6 +136,7 @@ int drm_open(struct inode *inode, struct file *filp)
 	retcode = drm_open_helper(filp, minor);
 	if (!retcode) {
 		atomic_inc(&dev->counts[_DRM_STAT_OPENS]);
+		minor->open_count++;
 		if (!dev->open_count++)
 			retcode = drm_setup(dev);
 	}
@@ -573,6 +574,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	 */
 
 	atomic_inc(&dev->counts[_DRM_STAT_CLOSES]);
+	file_priv->minor->open_count--;
 	if (!--dev->open_count) {
 		if (atomic_read(&dev->ioctl_count)) {
 			DRM_ERROR("Device busy: %d\n",
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 6629dd7..5ebc8bc 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -426,6 +426,8 @@ int drm_destroy_render_node(struct drm_device *dev, int index)
 
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		if (node->minor->index == index) {
+			if (node->minor->open_count)
+				return -EBUSY;
 			list_del(&node->list);
 			drm_put_minor(&node->minor);
 			kfree(node);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 1131fd4..981f005 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -983,6 +983,7 @@ struct drm_info_node {
 struct drm_minor {
 	int index;			/**< Minor device number */
 	int type;                       /**< Control or render */
+	int open_count;
 	dev_t device;			/**< Device number for mknod */
 	struct device kdev;		/**< Linux device */
 	struct drm_device *dev;
-- 
1.7.8.5

[PATCH 12/19] drm: allocate correct id_list size for a render node

[Ilija Hadzic]

When a new render node is created, the number of elements
of id_list allocated in drm_mode_group_init function should
not be the sum of all CRTCs, encoders, and connectors that
the device has, but the one specified by the ioctl that
created the node.

v2: - add planes

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |   18 ++++++++----------
 drivers/gpu/drm/drm_stub.c |    7 +++----
 include/drm/drm_crtc.h     |    2 +-
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4fc7c5f..78f2b96 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -933,15 +933,8 @@ void drm_mode_config_init(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_config_init);
 
-int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
+int drm_mode_group_init(struct drm_mode_group *group, int total_objects)
 {
-	uint32_t total_objects = 0;
-
-	total_objects += dev->mode_config.num_crtc;
-	total_objects += dev->mode_config.num_connector;
-	total_objects += dev->mode_config.num_encoder;
-	total_objects += dev->mode_config.num_plane;
-
 	group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL);
 	if (!group->id_list)
 		return -ENOMEM;
@@ -961,9 +954,14 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	struct drm_plane *plane;
-	int ret;
+	int ret, total_objects;
 
-	if ((ret = drm_mode_group_init(dev, group)))
+	total_objects = dev->mode_config.num_crtc;
+	total_objects += dev->mode_config.num_connector;
+	total_objects += dev->mode_config.num_encoder;
+	total_objects += dev->mode_config.num_plane;
+	ret = drm_mode_group_init(group, total_objects);
+	if (ret)
 		return ret;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 5ebc8bc..93746b7 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -577,14 +577,13 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	ret = drm_create_render_node(dev, &new_minor);
 	if (ret)
 		return ret;
-
-	ret = drm_mode_group_init(dev, &new_minor->mode_group);
+	total_ids = args->num_crtc + args->num_encoder +
+		args->num_connector + args->num_plane;
+	ret = drm_mode_group_init(&new_minor->mode_group, total_ids);
 	if (ret)
 		goto out_del;
 
 	ids_ptr = (uint32_t __user *)(unsigned long)args->id_list_ptr;
-	total_ids = args->num_crtc + args->num_encoder +
-		args->num_connector + args->num_plane;
 	for (i = 0; i < total_ids; i++) {
 		if (get_user(new_minor->mode_group.id_list[i], &ids_ptr[i])) {
 			ret = -EFAULT;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 2b9d062..939fed1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -851,7 +851,7 @@ extern char *drm_get_dvi_i_select_name(int val);
 extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
-extern int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group);
+extern int drm_mode_group_init(struct drm_mode_group *group, int total_objects);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);
-- 
1.7.8.5

[PATCH 13/19] drm: add drm_mode_group_fini function

[Ilija Hadzic]

This is the opposite function of drm_mode_group_init. It will
be needed to properly cleanup the render node after removing it.

v2: - add planes

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |   11 +++++++++++
 include/drm/drm_crtc.h     |    1 +
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 78f2b96..cce3d25 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -947,6 +947,17 @@ int drm_mode_group_init(struct drm_mode_group *group, int total_objects)
 }
 EXPORT_SYMBOL(drm_mode_group_init);
 
+void drm_mode_group_fini(struct drm_mode_group *group)
+{
+	kfree(group->id_list);
+	group->id_list = NULL;
+	group->num_crtcs = 0;
+	group->num_connectors = 0;
+	group->num_encoders = 0;
+	group->num_planes = 0;
+}
+EXPORT_SYMBOL(drm_mode_group_fini);
+
 int drm_mode_group_init_legacy_group(struct drm_device *dev,
 				     struct drm_mode_group *group)
 {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 939fed1..7c1227b 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -852,6 +852,7 @@ extern char *drm_get_tv_subconnector_name(int val);
 extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init(struct drm_mode_group *group, int total_objects);
+extern void drm_mode_group_fini(struct drm_mode_group *group);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);
-- 
1.7.8.5

[PATCH 14/19] drm: properly free id_list when a render node is removed

[Ilija Hadzic]

id_list is dynamically allocated when a render node is
created. Consequently, if must be freed when the render node
is removed, otherwise we have a memory leak.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 93746b7..d427316 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -426,10 +426,13 @@ int drm_destroy_render_node(struct drm_device *dev, int index)
 
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		if (node->minor->index == index) {
+			struct drm_mode_group *group;
 			if (node->minor->open_count)
 				return -EBUSY;
+			group = &node->minor->mode_group;
 			list_del(&node->list);
 			drm_put_minor(&node->minor);
+			drm_mode_group_fini(group);
 			kfree(node);
 			return 0;
 		}
@@ -442,8 +445,11 @@ void drm_destroy_all_render_nodes(struct drm_device *dev)
 	struct drm_render_node *node, *tmp;
 
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
+		struct drm_mode_group *group;
+		group = &node->minor->mode_group;
 		list_del(&node->list);
 		drm_put_minor(&node->minor);
+		drm_mode_group_fini(group);
 		kfree(node);
 	}
 }
-- 
1.7.8.5

[PATCH 15/19] drm: call drm_mode_group_fini on primary node

[Ilija Hadzic]

In drm_put_dev, the whole device is being destroyed, so id_list
of the primary (legacy) node should also be cleaned up.
This plugs a memory leak that has probably existed even without
the render-node work.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index d427316..20c72b0 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -540,6 +540,7 @@ void drm_put_dev(struct drm_device *dev)
 	if (driver->driver_features & DRIVER_GEM)
 		drm_gem_destroy(dev);
 
+	drm_mode_group_fini(&dev->primary->mode_group);
 	drm_put_minor(&dev->primary);
 
 	list_del(&dev->driver_item);
-- 
1.7.8.5

[PATCH 16/19] drm: more elaborate check for resource count

[Ilija Hadzic]

User space can send us all kinds of nonsense for num_crtc, num_encoder,
num_connector, or num_plane. So far, we have been checking only for
presence of at least one CRTC/encoder/connector (barring the trivial
case of a render node with no display resources, i.e., GPGPU node).

This patch makes the ioctl fail if user space requests more resources
than the physical GPU has. This is primarily to protect the kmalloc
in drm_mode_group_init from hogging a big chunk of memory if some
bozo sends us a request for some huge number of CRTCs, encoders,
or connectors.

v2: - fail when user space asks for more planes than available
      from physical device, but still allow zero planes
      (which is a legitimate case)

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 20c72b0..7aa54fb 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -574,9 +574,12 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 		return ret;
 	}
 
-	/* if we have display resources, then we need at least
-	 * one CRTC, one encoder and one connector */
-	if (args->num_crtc == 0 ||
+	/* sanity check for requested num_crtc/_encoder/_connector/_plane */
+	if (args->num_crtc > dev->mode_config.num_crtc ||
+	    args->num_encoder > dev->mode_config.num_encoder ||
+	    args->num_encoder > dev->mode_config.num_connector ||
+	    args->num_plane > dev->mode_config.num_plane ||
+	    args->num_crtc == 0 ||
 	    args->num_encoder == 0 ||
 	    args->num_connector == 0)
 		return -EINVAL;
-- 
1.7.8.5

[PATCH 17/19] drm: validate id list when creating a render node

[Ilija Hadzic]

Render node ioctl requires a list of DRM mode objects
in specific order: first all CRTCs, then all encoders,
followed by all connectors. Check that the IDs passed
from userland are in conformance with this requirement
and that they are consistent with specified num_crtc,
num_encoder and num_connector values.

Return -EINVAL to if the check fails. Otherwise, accept
the list and create the requested render node.

v2: - also check planes

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |   68 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 7aa54fb..340a7e4 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -399,6 +399,54 @@ err_idr:
 	return ret;
 }
 
+static int drm_get_id_from_user(struct drm_device *dev,
+				uint32_t *id_dst,
+				uint32_t __user *id_src,
+				uint32_t expected_type)
+{
+	struct drm_mode_object *drmmode_obj;
+	uint32_t id;
+
+	if (get_user(id, id_src))
+		return -EFAULT;
+	drmmode_obj = drm_mode_object_find(dev, id, expected_type);
+	if (!drmmode_obj)
+		return -EINVAL;
+	*id_dst = id;
+	return 0;
+}
+
+#define DRM_RN_NUM_EXP_TYPES 4
+static const uint32_t expected_type_list[DRM_RN_NUM_EXP_TYPES] = {
+	DRM_MODE_OBJECT_CRTC,
+	DRM_MODE_OBJECT_ENCODER,
+	DRM_MODE_OBJECT_CONNECTOR,
+	DRM_MODE_OBJECT_PLANE
+};
+
+static int drm_get_render_node_resources(struct drm_device *dev,
+					 uint32_t *id_list,
+					 uint32_t __user *ids_ptr,
+					 int *resource_count)
+
+{
+	int s, e, i, j;
+	int ret;
+
+	for (e = 0, j = 0; j < DRM_RN_NUM_EXP_TYPES; j++) {
+		s = e;
+		e += resource_count[j];
+		for (i = s; i < e; i++) {
+			ret = drm_get_id_from_user(dev, &id_list[i],
+						   &ids_ptr[i],
+						   expected_type_list[j]);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
 int drm_create_render_node(struct drm_device *dev, struct drm_minor **minor_p)
 {
 	int ret;
@@ -558,8 +606,10 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_render_node_create *args = data;
 	int ret;
 	struct drm_minor *new_minor;
-	int total_ids, i;
+	int total_ids;
+	int resource_count[DRM_RN_NUM_EXP_TYPES];
 	uint32_t __user *ids_ptr;
+	uint32_t *id_list;
 
 	/* allow access through control node only */
 	if (file_priv->minor != dev->control)
@@ -592,14 +642,16 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 	ret = drm_mode_group_init(&new_minor->mode_group, total_ids);
 	if (ret)
 		goto out_del;
-
+	resource_count[0] = args->num_crtc;
+	resource_count[1] = args->num_encoder;
+	resource_count[2] = args->num_connector;
+	resource_count[3] = args->num_plane;
 	ids_ptr = (uint32_t __user *)(unsigned long)args->id_list_ptr;
-	for (i = 0; i < total_ids; i++) {
-		if (get_user(new_minor->mode_group.id_list[i], &ids_ptr[i])) {
-			ret = -EFAULT;
-			goto out_del;
-		}
-	}
+	id_list = new_minor->mode_group.id_list;
+	ret = drm_get_render_node_resources(dev, id_list, ids_ptr,
+					    resource_count);
+	if (ret)
+		goto out_del;
 	new_minor->mode_group.num_crtcs = args->num_crtc;
 	new_minor->mode_group.num_encoders = args->num_encoder;
 	new_minor->mode_group.num_connectors = args->num_connector;
-- 
1.7.8.5

[PATCH 18/19] drm: keep track of which node holds which resource

[Ilija Hadzic]

Add fields to drm_crtc, drm_encoder, drm_connector, and drm_plane
that keep track of which render node owns it. Assign ownership
when resource is added and revoke it when resource is destroyed.
Do not allow creation of a node that tries to claim resources
that are already in use by another node.

v2: - track planes too

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_crtc.c |    4 ++
 drivers/gpu/drm/drm_stub.c |  111 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |    5 ++
 3 files changed, 120 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cce3d25..45a2925 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -375,6 +375,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 
 	crtc->dev = dev;
 	crtc->funcs = funcs;
+	crtc->render_node_owner = -1;
 
 	mutex_lock(&dev->mode_config.mutex);
 
@@ -483,6 +484,7 @@ int drm_connector_init(struct drm_device *dev,
 
 	connector->dev = dev;
 	connector->funcs = funcs;
+	connector->render_node_owner = -1;
 	connector->connector_type = connector_type;
 	connector->connector_type_id =
 		++drm_connector_enum_list[connector_type].count; /* TODO */
@@ -553,6 +555,7 @@ int drm_encoder_init(struct drm_device *dev,
 	if (ret)
 		goto out;
 
+	encoder->render_node_owner = -1;
 	encoder->dev = dev;
 	encoder->encoder_type = encoder_type;
 	encoder->funcs = funcs;
@@ -594,6 +597,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	plane->dev = dev;
 	plane->funcs = funcs;
+	plane->render_node_owner = -1;
 	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
 				      GFP_KERNEL);
 	if (!plane->format_types) {
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 340a7e4..13ff4c8 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -447,6 +447,95 @@ static int drm_get_render_node_resources(struct drm_device *dev,
 	return 0;
 }
 
+static int *drm_get_render_node_owner(struct drm_device *dev,
+				      uint32_t id, uint32_t type)
+{
+	struct drm_mode_object *obj;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	struct drm_plane *plane;
+
+	obj = drm_mode_object_find(dev, id, type);
+	if (!obj)
+		return NULL;
+	switch (type) {
+	case DRM_MODE_OBJECT_CRTC:
+		crtc = container_of(obj, struct drm_crtc, base);
+		return &crtc->render_node_owner;
+	case DRM_MODE_OBJECT_ENCODER:
+		encoder = container_of(obj, struct drm_encoder, base);
+		return &encoder->render_node_owner;
+	case DRM_MODE_OBJECT_CONNECTOR:
+		connector = container_of(obj, struct drm_connector, base);
+		return &connector->render_node_owner;
+	case DRM_MODE_OBJECT_PLANE:
+		plane = container_of(obj, struct drm_plane, base);
+		return &plane->render_node_owner;
+
+	default:
+		return NULL;
+	}
+}
+
+static void drm_release_render_node_resources(struct drm_device *dev,
+					      uint32_t *id_list,
+					      int *resource_count,
+					      int minor)
+{
+	int *render_node_owner;
+	int s, e, i, j;
+
+	for (e = 0, j = 0; j < DRM_RN_NUM_EXP_TYPES; j++) {
+		s = e;
+		e += resource_count[j];
+		for (i = s; i < e; i++) {
+			render_node_owner =
+				drm_get_render_node_owner(dev, id_list[i],
+					expected_type_list[j]);
+			if (render_node_owner &&
+			    *render_node_owner == minor)
+				*render_node_owner = -1;
+		}
+	}
+}
+
+static int drm_claim_render_node_resources(struct drm_device *dev,
+					   uint32_t *id_list,
+					   int *resource_count,
+					   int minor)
+{
+	int *render_node_owner;
+	int s, e, i, j;
+	int ret = 0;
+
+	for (e = 0, j = 0; j < DRM_RN_NUM_EXP_TYPES; j++) {
+		s = e;
+		e += resource_count[j];
+		for (i = s; i < e; i++) {
+			render_node_owner =
+				drm_get_render_node_owner(dev, id_list[i],
+					expected_type_list[j]);
+			if (!render_node_owner) {
+				/* list was validated, not supposed to fail */
+				WARN_ON(1);
+				ret = -EFAULT;
+				goto out_release;
+			}
+			if (*render_node_owner != -1) {
+				ret = -EBUSY;
+				goto out_release;
+			}
+			*render_node_owner = minor;
+		}
+	}
+	return ret;
+
+out_release:
+	drm_release_render_node_resources(dev, id_list, resource_count, minor);
+	return ret;
+}
+
 int drm_create_render_node(struct drm_device *dev, struct drm_minor **minor_p)
 {
 	int ret;
@@ -475,10 +564,19 @@ int drm_destroy_render_node(struct drm_device *dev, int index)
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		if (node->minor->index == index) {
 			struct drm_mode_group *group;
+			int resource_count[DRM_RN_NUM_EXP_TYPES];
+
 			if (node->minor->open_count)
 				return -EBUSY;
 			group = &node->minor->mode_group;
 			list_del(&node->list);
+			resource_count[0] = group->num_crtcs;
+			resource_count[1] = group->num_encoders;
+			resource_count[2] = group->num_connectors;
+			resource_count[3] = group->num_planes;
+			drm_release_render_node_resources(dev, group->id_list,
+							  resource_count,
+							  node->minor->index);
 			drm_put_minor(&node->minor);
 			drm_mode_group_fini(group);
 			kfree(node);
@@ -494,8 +592,17 @@ void drm_destroy_all_render_nodes(struct drm_device *dev)
 
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		struct drm_mode_group *group;
+		int resource_count[DRM_RN_NUM_EXP_TYPES];
+
 		group = &node->minor->mode_group;
 		list_del(&node->list);
+		resource_count[0] = group->num_crtcs;
+		resource_count[1] = group->num_encoders;
+		resource_count[2] = group->num_connectors;
+		resource_count[3] = group->num_planes;
+		drm_release_render_node_resources(dev, group->id_list,
+						  resource_count,
+						  node->minor->index);
 		drm_put_minor(&node->minor);
 		drm_mode_group_fini(group);
 		kfree(node);
@@ -652,6 +759,10 @@ int drm_render_node_create_ioctl(struct drm_device *dev, void *data,
 					    resource_count);
 	if (ret)
 		goto out_del;
+	ret = drm_claim_render_node_resources(dev, id_list, resource_count,
+					      new_minor->index);
+	if (ret)
+		goto out_del;
 	new_minor->mode_group.num_crtcs = args->num_crtc;
 	new_minor->mode_group.num_encoders = args->num_encoder;
 	new_minor->mode_group.num_connectors = args->num_connector;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 7c1227b..c38635d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -374,6 +374,7 @@ struct drm_crtc {
 	struct drm_framebuffer *fb;
 
 	bool enabled;
+	int render_node_owner;
 
 	/* Requested mode from modesetting. */
 	struct drm_display_mode mode;
@@ -479,6 +480,8 @@ struct drm_encoder {
 	uint32_t possible_crtcs;
 	uint32_t possible_clones;
 
+	int render_node_owner;
+
 	struct drm_crtc *crtc;
 	const struct drm_encoder_funcs *funcs;
 	void *helper_private;
@@ -556,6 +559,7 @@ struct drm_connector {
 	struct list_head modes; /* list of modes on this connector */
 
 	enum drm_connector_status status;
+	int render_node_owner;
 
 	/* these are modes added by probing with DDC or the BIOS */
 	struct list_head probed_modes;
@@ -641,6 +645,7 @@ struct drm_plane {
 	uint16_t *gamma_store;
 
 	bool enabled;
+	int render_node_owner;
 
 	const struct drm_plane_funcs *funcs;
 	void *helper_private;
-- 
1.7.8.5

[PATCH 19/19] drm: hold mutex in critical sections of render-node code

[Ilija Hadzic]

Critical sections are parts of the code where we claim or
release resources (we don't want two render-node create or
remove ioctl called in the context of different processes
to claim part of requested resources because of the race).
Another critical section is manipulating the render node
list. We can use dev->mode_config.mutex for both.

v2: - Use dev->mode_config.mutex instead of dev->struct_mutex
      because we are also racing against drm_mode_getresources()
      and drm_mode_getplane_res() functions.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_stub.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 13ff4c8..a5fd905 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -486,6 +486,7 @@ static void drm_release_render_node_resources(struct drm_device *dev,
 	int *render_node_owner;
 	int s, e, i, j;
 
+	/* no lock, assume we were called with mode_config mutex grabbed */
 	for (e = 0, j = 0; j < DRM_RN_NUM_EXP_TYPES; j++) {
 		s = e;
 		e += resource_count[j];
@@ -509,6 +510,7 @@ static int drm_claim_render_node_resources(struct drm_device *dev,
 	int s, e, i, j;
 	int ret = 0;
 
+	mutex_lock(&dev->mode_config.mutex);
 	for (e = 0, j = 0; j < DRM_RN_NUM_EXP_TYPES; j++) {
 		s = e;
 		e += resource_count[j];
@@ -529,10 +531,12 @@ static int drm_claim_render_node_resources(struct drm_device *dev,
 			*render_node_owner = minor;
 		}
 	}
+	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 
 out_release:
 	drm_release_render_node_resources(dev, id_list, resource_count, minor);
+	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
 
@@ -553,7 +557,9 @@ int drm_create_render_node(struct drm_device *dev, struct drm_minor **minor_p)
 	}
 	render_node->minor = minor;
 	*minor_p = minor;
+	mutex_lock(&dev->mode_config.mutex);
 	list_add_tail(&render_node->list, &dev->render_node_list);
+	mutex_unlock(&dev->mode_config.mutex);
 	return 0;
 }
 
@@ -561,13 +567,16 @@ int drm_destroy_render_node(struct drm_device *dev, int index)
 {
 	struct drm_render_node *node, *tmp;
 
+	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		if (node->minor->index == index) {
 			struct drm_mode_group *group;
 			int resource_count[DRM_RN_NUM_EXP_TYPES];
 
-			if (node->minor->open_count)
+			if (node->minor->open_count) {
+				mutex_unlock(&dev->mode_config.mutex);
 				return -EBUSY;
+			}
 			group = &node->minor->mode_group;
 			list_del(&node->list);
 			resource_count[0] = group->num_crtcs;
@@ -577,12 +586,14 @@ int drm_destroy_render_node(struct drm_device *dev, int index)
 			drm_release_render_node_resources(dev, group->id_list,
 							  resource_count,
 							  node->minor->index);
+			mutex_unlock(&dev->mode_config.mutex);
 			drm_put_minor(&node->minor);
 			drm_mode_group_fini(group);
 			kfree(node);
 			return 0;
 		}
 	}
+	mutex_unlock(&dev->mode_config.mutex);
 	return -ENODEV;
 }
 
@@ -590,6 +601,7 @@ void drm_destroy_all_render_nodes(struct drm_device *dev)
 {
 	struct drm_render_node *node, *tmp;
 
+	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(node, tmp, &dev->render_node_list, list) {
 		struct drm_mode_group *group;
 		int resource_count[DRM_RN_NUM_EXP_TYPES];
@@ -607,6 +619,7 @@ void drm_destroy_all_render_nodes(struct drm_device *dev)
 		drm_mode_group_fini(group);
 		kfree(node);
 	}
+	mutex_unlock(&dev->mode_config.mutex);
 }
 
 /**
-- 
1.7.8.5

Re: [RFC v2] Revive the work on render-nodes branch

[Ville Syrjälä]

On Thu, Apr 12, 2012 at 02:19:25PM -0400, Ilija Hadzic wrote:
> The following set of patches is the reword of the series
> sent two weeks ago [2] that will revive the drm-render-nodes [1]
> branch. Details of the original series are described in [2].
> 
> Patches in this series have been reworked (and a few prep patches
> have been added) to address the comment about the planes support
> (planes can now be included in the resource list for the render
> node).
> 
> Consequently, the ioctls have changed to include plane support
> so the libdrm side is also affected (patches for libdrm will be sent
> in the series following this one).

Didn't have time for a detailed look yet, but at least one thing 
missing from your patch set is handling of possible_crtcs and
possible_clones for getplane and getencoder ioctls. AFAICT the
bits in those are supposed to tell you the index of the object ID
in the getresources reply. So when queried via a "render node",
some form of remapping must be performed.

Also the name "render node" is still very confusing. How about just
callling it a partition or something. Or maybe someone could suggest
a more self explanatory name for this stuff.

-- 
Ville Syrjälä
Intel OTC

Re: [RFC v2] Revive the work on render-nodes branch

[Ilija Hadzic]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1333 bytes --]



On Thu, 12 Apr 2012, Ville [iso-8859-1] Syrjälä wrote:

> Didn't have time for a detailed look yet, but at least one thing
> missing from your patch set is handling of possible_crtcs and
> possible_clones for getplane and getencoder ioctls.

Yes I agree and I know. That is still work in progress. The ioctl for 
constructing the node should do more checks not only for possible_crtcs 
and possible_clones, but also whether a particular encoder can match the 
connector and not allow creation of the node with incompatible resources. 
I have all that on the TODO list, but I wanted to get the patches out 
early so that I can get some feedback (and hopefully motivate others to 
contribute).

There will definitely be v3 (and beyond) that will include stuff on my 
TODO list along with any feedback that I receive.

> Also the name "render node" is still very confusing. How about just
> callling it a partition or something. Or maybe someone could suggest
> a more self explanatory name for this stuff.
>

Dave has chosen the name two years ago when he did the initial demo work. 
I am just keeping this name for now (the advantage is that others know of 
this work by this name, so they know what it is about). However, if there 
is a concensus for a better name I'll be glad to adopt it.

-- Ilija

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

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

Re: [RFC v2] Revive the work on render-nodes branch

[Dave Airlie]

On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
> The following set of patches is the reword of the series
> sent two weeks ago [2] that will revive the drm-render-nodes [1]
> branch. Details of the original series are described in [2].

Thanks for looking at this,

So one thing about adding render nodes, is it gives us the chance to
maybe change the userspace API we expose to be more secure and have
less cruft in it.

Now I'm weary of a major API redesign as this could bog things now and
we'd never make forward progress, but I'd like to at least consider it
before adding new device nodes and locking us into the old APIs.

The areas suggested before:

1) GEM, drop flink/open ioctls - these make security hard, since once
you share a buffer any GEM opener can get access to it. Solutions to
this proposed before are flink_to and maybe using PRIME/dma-buf.

2) master/device ownership. We might be able to drop the first opener
is magically master, and require openers to ask for it somehow.

3) drop the old map ioctls from this interface, irq by busid, drop the AGP.

I'm not talking about changing ioctl operation, more about introducing
new ioctls and not allowing the old ones on the new nodes.

Dave.

Re: [RFC v2] Revive the work on render-nodes branch

[Daniel Vetter]

On Fri, Apr 20, 2012 at 11:20:45AM +0100, Dave Airlie wrote:
> On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
> <ihadzic@research.bell-labs.com> wrote:
> > The following set of patches is the reword of the series
> > sent two weeks ago [2] that will revive the drm-render-nodes [1]
> > branch. Details of the original series are described in [2].
> 
> Thanks for looking at this,
> 
> So one thing about adding render nodes, is it gives us the chance to
> maybe change the userspace API we expose to be more secure and have
> less cruft in it.
> 
> Now I'm weary of a major API redesign as this could bog things now and
> we'd never make forward progress, but I'd like to at least consider it
> before adding new device nodes and locking us into the old APIs.
> 
> The areas suggested before:
> 
> 1) GEM, drop flink/open ioctls - these make security hard, since once
> you share a buffer any GEM opener can get access to it. Solutions to
> this proposed before are flink_to and maybe using PRIME/dma-buf.
> 
> 2) master/device ownership. We might be able to drop the first opener
> is magically master, and require openers to ask for it somehow.
> 
> 3) drop the old map ioctls from this interface, irq by busid, drop the AGP.
> 
> I'm not talking about changing ioctl operation, more about introducing
> new ioctls and not allowing the old ones on the new nodes.

My current plan to clean up the ioctl interface mess in drm/i915 is to
start marking up any interfaces that userspace doesn't need any longer on
a per-device level (already started on this). We can't drop the old
support code, but this way we can at least stop supporting old interfaces
on newer cards.

The problem with that approach is that the drm middle-layer (hot topic
today, I guess) gets in the way of this - we have to set the driver
feature flags while registering the driver and can't change them when we
initialize for a specific device. E.g. for i915 kms we need the old
mapping interfaces and the agp support mess only on i945&g33 because we
shipped horrible userspace that only works on these platforms (XvMC
implementation fwiw).

My plan there is to remove the middle-layer smell by moving the drm
support subsystem initialization into the driver code. So imo we can solve
3) naturally by disabling old crap on new chips.

No strong opinions on 1)&2), but sounds reasonable.

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

Re: [RFC v2] Revive the work on render-nodes branch

[Ilija Hadzic]

On Fri, 20 Apr 2012, Dave Airlie wrote:

> On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
> <ihadzic@research.bell-labs.com> wrote:
>> The following set of patches is the reword of the series
>> sent two weeks ago [2] that will revive the drm-render-nodes [1]
>> branch. Details of the original series are described in [2].
>
> Thanks for looking at this,
>
> So one thing about adding render nodes, is it gives us the chance to
> maybe change the userspace API we expose to be more secure and have
> less cruft in it.
>
> Now I'm weary of a major API redesign as this could bog things now and
> we'd never make forward progress, but I'd like to at least consider it
> before adding new device nodes and locking us into the old APIs.
>
> The areas suggested before:
>
> 1) GEM, drop flink/open ioctls - these make security hard, since once
> you share a buffer any GEM opener can get access to it. Solutions to
> this proposed before are flink_to and maybe using PRIME/dma-buf.
>
> 2) master/device ownership. We might be able to drop the first opener
> is magically master, and require openers to ask for it somehow.
>
> 3) drop the old map ioctls from this interface, irq by busid, drop the AGP.
>
> I'm not talking about changing ioctl operation, more about introducing
> new ioctls and not allowing the old ones on the new nodes.
>
> Dave.
>

Thanks for the feedback, point taken. So there will be some "homework" to 
be done before you can take in the render nodes work. Would you be 
perceptive to consider taking in some of the prep. patches that I sent in 
my series? They will reduce the amount of baggage that needs to be carried 
with actual render-nodes work and make it easier to keep it alive while 
the above-proposed interface rework is being worked on.

For example, this one is just a simplification that makes 
things more readable:

http://lists.freedesktop.org/archives/dri-devel/2012-April/021327.html

Then these two will make drm_mode_getplane_res more consistent with what 
other getresources calls do (i.e., use drm_mode_group instead of 
drm_mode_config).

http://lists.freedesktop.org/archives/dri-devel/2012-April/021328.html
http://lists.freedesktop.org/archives/dri-devel/2012-April/021329.html

This one won't hurt either and will make things look a little bit more 
straightforward:

http://lists.freedesktop.org/archives/dri-devel/2012-April/021330.html

-- Ilija

Re: [RFC v2] Revive the work on render-nodes branch

[Kristian Høgsberg]

On Fri, Apr 20, 2012 at 6:20 AM, Dave Airlie <airlied@gmail.com> wrote:
> On Thu, Apr 12, 2012 at 7:19 PM, Ilija Hadzic
> <ihadzic@research.bell-labs.com> wrote:
>> The following set of patches is the reword of the series
>> sent two weeks ago [2] that will revive the drm-render-nodes [1]
>> branch. Details of the original series are described in [2].
>
> Thanks for looking at this,
>
> So one thing about adding render nodes, is it gives us the chance to
> maybe change the userspace API we expose to be more secure and have
> less cruft in it.
>
> Now I'm weary of a major API redesign as this could bog things now and
> we'd never make forward progress, but I'd like to at least consider it
> before adding new device nodes and locking us into the old APIs.
>
> The areas suggested before:
>
> 1) GEM, drop flink/open ioctls - these make security hard, since once
> you share a buffer any GEM opener can get access to it. Solutions to
> this proposed before are flink_to and maybe using PRIME/dma-buf.

Yup, and I retract my flink_to proposal.  Passing the buffer as an fd
has a few benefits over the flink_to idea: 1) we don't have to
establish an id/handle that represents the other process.  We could
use the process ID or the drm magic, but either way we'd need an
initial handshake to pass that id across before we can share buffer.
Not a deal breaker, just awkward.  2) Passing buffers as fds keeps the
buffer alive "in flight".  This allows the sender to send the buffer
and immediately destroy it, if it needs to.

Another thing I'd change for render nodes is to not require
authentication.  We can still protect access to render nodes to local
sessions using file system permissions (ie, you won't be able to ssh
in and start a GPGPU jobs).  Being able to use the drm device doesn't
give you access to the xserver frontbuffer like it did in the DRI1
days.  You can only render to your own buffers and send or receive
buffer handles over sockets.  If we restrict the ioctls to just GEM
and PRIME, I don't think authentication protects anything we couldn't
just use file system permission for instead.

> 2) master/device ownership. We might be able to drop the first opener
> is magically master, and require openers to ask for it somehow.

If we add render nodes that clients are expected to use, we can
restrict access to the legacy drm device node, such that only X can
open it.  That way it wouldn't matter too much whether we keep the
auto-master behavior or not.  I would like to see the root requirement
dropped from the drop and set master ioctls so that you could put X,
weston etc in a group that allows them to open /dev/dri/card0 and not
need root to perform vt switching duties.  Even today there's no need
for the root requirements, certainly drop master is harmless and
setmaster can just be restricted to drm fds that were previously
masters and only when there's no current master.

> 3) drop the old map ioctls from this interface, irq by busid, drop the AGP.

Yup, or another way to put it: what todays mesa needs, minus flink_to,
open, get_magic (this is the only libdrm call in the glx dri and
egl_dri2 loaders btw) , auth_magic, but with the PRIME ioctls added.

> I'm not talking about changing ioctl operation, more about introducing
> new ioctls and not allowing the old ones on the new nodes.

Yup, agree.

thanks,
Kristian