8048 panic in drm_gem_unmap with Xorg 1.18

Reviewed by: Richard Lowe <richlowe@richlowe.net>
Reviewed by: Toomas Soome <tsoome@me.com>
Reviewed by: Aurlien Larcher <aurelien.larcher@gmail.com>

10 files changed, 196 insertions, 65 deletions

diff --git a/usr/src/common/libdrm/Makefile.drm b/usr/src/common/libdrm/Makefile.drm
index 43f85b7..6631c6d 100644
--- a/usr/src/common/libdrm/Makefile.drm
+++ b/usr/src/common/libdrm/Makefile.drm
@@ -39,5 +39,6 @@ CPPFLAGS +=	-I$(SRC)/uts/common \
 		-I$(SRC)/uts/common/drm \
 		-I$(LIBDRM_CMN_DIR)/include/drm
 
-# The libdrm code has lots of parentheses warnings.  Suppress.
+# The libdrm code has lots of warnings.  Just suppress.
 CERRWARN +=	-_gcc=-Wno-parentheses
+CERRWARN +=	-_gcc=-Wno-unused-function
diff --git a/usr/src/uts/common/io/drm/drm_gem.c b/usr/src/uts/common/io/drm/drm_gem.c
index ee88602..a1a4b9d 100644
--- a/usr/src/uts/common/io/drm/drm_gem.c
+++ b/usr/src/uts/common/io/drm/drm_gem.c
@@ -759,12 +759,16 @@ drm_gem_create_mmap_offset(struct drm_gem_object *obj)
 void
 drm_gem_mmap(struct drm_gem_object *obj, pfn_t pfn)
 {
+	ASSERT(obj->gtt_map_kaddr != NULL);
+	/* Does hat_devload() */
 	gfxp_load_kernel_space(pfn, obj->real_size, GFXP_MEMORY_WRITECOMBINED, obj->gtt_map_kaddr);
 }
 
 void
 drm_gem_release_mmap(struct drm_gem_object *obj)
 {
+	ASSERT(obj->gtt_map_kaddr != NULL);
+	/* Does hat_unload() */
 	gfxp_unload_kernel_space(obj->gtt_map_kaddr, obj->real_size);
 }
 
diff --git a/usr/src/uts/common/io/drm/drm_sunmod.c b/usr/src/uts/common/io/drm/drm_sunmod.c
index 0f4ef47..e4f5916 100644
--- a/usr/src/uts/common/io/drm/drm_sunmod.c
+++ b/usr/src/uts/common/io/drm/drm_sunmod.c
@@ -99,9 +99,17 @@ drm_devmap_map(devmap_cookie_t dhc, dev_t dev_id, uint_t flags,
 	mutex_enter(&dev->struct_mutex);
 	dhp = (devmap_handle_t *)dhc;
 	cp = (struct ddi_umem_cookie *)dhp->dh_cookie;
-	cp->cook_refcnt = 1;
+	ASSERT(cp->cook_refcnt == 0);
+	cp->cook_refcnt++;
 	mutex_exit(&dev->struct_mutex);
 
+	/*
+	 * This is the first reference via this devmap handle.
+	 * If needed, we could take a ref on some device-internal
+	 * object here, and release it in drm_devmap_unmap().
+	 */
+	DRM_DEBUG("first ref dev=%p cp=%p", dev, cp);
+
 	*new_priv = dev;
 	return (0);
 }
@@ -138,6 +146,7 @@ drm_devmap_unmap(devmap_cookie_t dhc, void *pvtp, offset_t off, size_t len,
 	devmap_handle_t	*ndhp;
 	struct ddi_umem_cookie	*cp;
 	struct ddi_umem_cookie	*ncp;
+	boolean_t last_ref = B_FALSE;
 
 	dhp = (devmap_handle_t *)dhc;
 	dev = (struct drm_device *)pvtp;
@@ -152,7 +161,6 @@ drm_devmap_unmap(devmap_cookie_t dhc, void *pvtp, offset_t off, size_t len,
 		*new_pvtp1 = dev;
 		ASSERT(ncp == cp);
 	}
-
 	if (new_dhp2 != NULL) {
 		ndhp = (devmap_handle_t *)new_dhp2;
 		ncp = (struct ddi_umem_cookie *)ndhp->dh_cookie;
@@ -161,16 +169,28 @@ drm_devmap_unmap(devmap_cookie_t dhc, void *pvtp, offset_t off, size_t len,
 		ASSERT(ncp == cp);
 	}
 
-	/* FIXME: dh_cookie should not be released here. */
-#if 0
+	ASSERT(cp->cook_refcnt > 0);
 	cp->cook_refcnt--;
 	if (cp->cook_refcnt == 0) {
-		gfxp_umem_cookie_destroy(dhp->dh_cookie);
-		dhp->dh_cookie = NULL;
+		last_ref = B_TRUE;
 	}
-#endif
 
 	mutex_exit(&dev->struct_mutex);
+
+	if (last_ref) {
+		/*
+		 * This is the last reference to this device
+		 * via this devmap handle, so drop whatever
+		 * reference was taken in drm_gem_map().
+		 *
+		 * Previous versions of this code released
+		 * dhp->dh_cookie in here, but that's handled
+		 * outside of devmap_map/_unmap.  Given that,
+		 * we probably don't need this callback at all,
+		 * though it's somewhat useful for debugging.
+		 */
+		DRM_DEBUG("last ref dev=%p cp=%p", dev, cp);
+	}
 }
 
 static struct devmap_callback_ctl drm_devmap_callbacks = {
@@ -194,14 +214,17 @@ __find_local_map(struct drm_device *dev, offset_t offset)
 }
 
 static int
-drm_gem_map(devmap_cookie_t dhp, dev_t dev, uint_t flags, offset_t off,
-		size_t len, void **pvtp)
+drm_gem_map(devmap_cookie_t dhc, dev_t dev_id, uint_t flags,
+    offset_t off, size_t len, void **pvtp)
 {
-	_NOTE(ARGUNUSED(dhp, flags, len))
+	_NOTE(ARGUNUSED(flags, len))
 
-	struct drm_device *drm_dev;
+	devmap_handle_t *dhp;
+	struct ddi_umem_cookie *cp;
+	struct drm_gem_object *obj;
+	struct drm_device *dev;
 	struct drm_minor *minor;
-	int minor_id = DRM_DEV2MINOR(dev);
+	int minor_id = DRM_DEV2MINOR(dev_id);
 	drm_local_map_t *map = NULL;
 
 	minor = idr_find(&drm_minors_idr, minor_id);
@@ -210,23 +233,48 @@ drm_gem_map(devmap_cookie_t dhp, dev_t dev, uint_t flags, offset_t off,
 	if (!minor->dev)
 		return (ENODEV);
 
-	drm_dev = minor->dev;
+	dev = minor->dev;
 
-	mutex_enter(&drm_dev->struct_mutex);
-	map = __find_local_map(drm_dev, off);
+	mutex_enter(&dev->struct_mutex);
+	map = __find_local_map(dev, off);
 	if (!map) {
-		mutex_exit(&drm_dev->struct_mutex);
+		mutex_exit(&dev->struct_mutex);
 		*pvtp = NULL;
 		return (DDI_EINVAL);
 	}
 
-	mutex_exit(&drm_dev->struct_mutex);
+	dhp = (devmap_handle_t *)dhc;
+	cp = (struct ddi_umem_cookie *)dhp->dh_cookie;
+	ASSERT(cp->cook_refcnt == 0);
+	cp->cook_refcnt++;
+
+	mutex_exit(&dev->struct_mutex);
 
-	*pvtp = map->handle;
+	/*
+	 * First reference via this devmap handle.  Take a ref. on the
+	 * gem object, and release it in drm_gem_unmap when last ref
+	 * from this devmap handle goes away.   This corresponds to
+	 * code in drm_gem_vm_open() in the Linux driver.
+	 */
+	obj = map->handle;
+	drm_gem_object_reference(obj);
+
+	DRM_DEBUG("first ref obj=%p cp=%p", obj, cp);
+
+	*pvtp = obj;
 
 	return (DDI_SUCCESS);
 }
 
+/*
+ * This is called by segdev_fault() to fault in pages for the given
+ * offset+len.  If the (GTT) device range has been configured with a
+ * fake offset for mmap, call the device's gem_fault handler to setup
+ * the GTT resources for this mapping.
+ *
+ * We should always call devmap_load(), as we're just interposing
+ * on these fault calls to (sometimes) setup GTT resources.
+ */
 static int
 drm_gem_map_access(devmap_cookie_t dhp, void *pvt, offset_t offset, size_t len,
 		uint_t type, uint_t rw)
@@ -236,20 +284,30 @@ drm_gem_map_access(devmap_cookie_t dhp, void *pvt, offset_t offset, size_t len,
 	struct gem_map_list *seg;
 
 	obj = (struct drm_gem_object *)pvt;
-	if (obj == NULL) {
-		dev = NULL;	/* avoid "used uninitialized" warning */
-		goto next;
-	}
 
-	dev = obj->dev;
-	if (dev->driver->gem_fault != NULL)
-		dev->driver->gem_fault(obj);
+	/*
+	 * Only call the driver fault handler after gtt_map_kaddr
+	 * has been set, i.e. by i915_gem_mmap_gtt(), or we'll
+	 * panic in trying to map pages at addr zero.
+	 */
+	if (obj != NULL && obj->gtt_map_kaddr != NULL) {
+		dev = obj->dev;
+		/* Could also check map->callback */
+		if (dev->driver->gem_fault != NULL)
+			dev->driver->gem_fault(obj);
+	}
 
-next:
+	/* Internal devmap_default_access(9F) */
 	if (devmap_load(dhp, offset, len, type, rw)) {
 		return (DDI_FAILURE);
 	}
+
+	/*
+	 * Save list of loaded translations for later use in
+	 * (i.e.) i915_gem_release_mmap()
+	 */
 	if (obj != NULL) {
+		dev = obj->dev;
 		seg = drm_alloc(sizeof (struct gem_map_list), DRM_MEM_MAPS);
 		if (seg != NULL) {
 			mutex_lock(&dev->page_fault_lock);
@@ -263,46 +321,111 @@ next:
 	return (DDI_SUCCESS);
 }
 
+static int
+drm_gem_dup(devmap_cookie_t dhc, void *pvt, devmap_cookie_t new_dhc,
+    void **new_pvtp)
+{
+	_NOTE(ARGUNUSED(new_dhc))
+
+	struct drm_gem_object *obj = pvt;
+	struct drm_device *dev = obj->dev;
+	devmap_handle_t *dhp;
+	struct ddi_umem_cookie *cp;
+
+	mutex_enter(&dev->struct_mutex);
+	dhp = (devmap_handle_t *)dhc;
+	cp = (struct ddi_umem_cookie *)dhp->dh_cookie;
+	cp->cook_refcnt++;
+	mutex_exit(&dev->struct_mutex);
+
+	*new_pvtp = obj;
+	return (0);
+}
+
 static void
-drm_gem_unmap(devmap_cookie_t dhp, void *pvtp, offset_t off, size_t len,
-		devmap_cookie_t new_dhp1, void **newpvtp1,
-		devmap_cookie_t new_dhp2, void **newpvtp2)
+drm_gem_unmap(devmap_cookie_t dhc, void *pvt, offset_t off, size_t len,
+		devmap_cookie_t new_dhp1, void **new_pvtp1,
+		devmap_cookie_t new_dhp2, void **new_pvtp2)
 {
+	devmap_handle_t *dhp = (devmap_handle_t *)dhc;
+	devmap_handle_t	*ndhp;
+	struct ddi_umem_cookie *cp;
+	struct ddi_umem_cookie *ncp;
 	struct drm_device *dev;
 	struct drm_gem_object *obj;
 	struct gem_map_list *entry, *temp;
+	boolean_t last_ref = B_FALSE;
 
-	_NOTE(ARGUNUSED(dhp, pvtp, off, len, new_dhp1, newpvtp1))
-	_NOTE(ARGUNUSED(new_dhp2, newpvtp2))
+	_NOTE(ARGUNUSED(off, len))
 
-	obj = (struct drm_gem_object *)pvtp;
+	obj = (struct drm_gem_object *)pvt;
 	if (obj == NULL)
 		return;
 
 	dev = obj->dev;
 
+	/*
+	 * Unload what drm_gem_map_access loaded.
+	 *
+	 * XXX: The devmap_unload() here is probably unnecessary,
+	 * as segdev_unmap() has done a hat_unload() for the
+	 * entire segment by the time we get here.
+	 */
 	mutex_lock(&dev->page_fault_lock);
-	if (list_empty(&obj->seg_list)) {
-		mutex_unlock(&dev->page_fault_lock);
-		return;
+	if (!list_empty(&obj->seg_list)) {
+		list_for_each_entry_safe(entry, temp, struct gem_map_list,
+		    &obj->seg_list, head) {
+			(void) devmap_unload(entry->dhp, entry->mapoffset,
+			    entry->maplen);
+			list_del(&entry->head);
+			drm_free(entry, sizeof (struct gem_map_list), DRM_MEM_MAPS);
+		}
 	}
+	mutex_unlock(&dev->page_fault_lock);
 
-	list_for_each_entry_safe(entry, temp, struct gem_map_list,
-	    &obj->seg_list, head) {
-		(void) devmap_unload(entry->dhp, entry->mapoffset,
-		    entry->maplen);
-		list_del(&entry->head);
-		drm_free(entry, sizeof (struct gem_map_list), DRM_MEM_MAPS);
+	/*
+	 * Manage dh_cookie ref counts
+	 */
+	mutex_enter(&dev->struct_mutex);
+	cp = (struct ddi_umem_cookie *)dhp->dh_cookie;
+	if (new_dhp1 != NULL) {
+		ndhp = (devmap_handle_t *)new_dhp1;
+		ncp = (struct ddi_umem_cookie *)ndhp->dh_cookie;
+		ncp->cook_refcnt++;
+		*new_pvtp1 = obj;
+		ASSERT(ncp == cp);
 	}
+	if (new_dhp2 != NULL) {
+		ndhp = (devmap_handle_t *)new_dhp2;
+		ncp = (struct ddi_umem_cookie *)ndhp->dh_cookie;
+		ncp->cook_refcnt++;
+		*new_pvtp2 = obj;
+		ASSERT(ncp == cp);
+	}
+	ASSERT(cp->cook_refcnt > 0);
+	cp->cook_refcnt--;
+	if (cp->cook_refcnt == 0) {
+		last_ref = B_TRUE;
+	}
+	mutex_exit(&dev->struct_mutex);
 
-	mutex_unlock(&dev->page_fault_lock);
+	if (last_ref) {
+		/*
+		 * This is the last reference to this GEM object
+		 * via this devmap handle, so drop the reference
+		 * taken in drm_gem_map().  This corresponds to
+		 * code in drm_gem_vm_close() in the Linux driver.
+		 */
+		DRM_DEBUG("last ref obj=%p cp=%p", obj, cp);
+		drm_gem_object_unreference(obj);
+	}
 }
 
 static struct devmap_callback_ctl drm_gem_map_ops = {
 	DEVMAP_OPS_REV,		/* devmap_ops version number */
 	drm_gem_map,		/* devmap_ops map routine */
 	drm_gem_map_access,	/* devmap_ops access routine */
-	NULL,			/* devmap_ops dup routine */
+	drm_gem_dup,		/* devmap_ops dup routine */
 	drm_gem_unmap,		/* devmap_ops unmap routine */
 };
 
@@ -411,17 +534,12 @@ static int
 __devmap_gem(struct drm_device *dev, devmap_cookie_t dhp,
     struct drm_local_map *map, size_t *maplen)
 {
-	struct devmap_callback_ctl *callbackops = NULL;
 	int ret;
 
-	if (map->callback == 1) {
-		callbackops = &drm_gem_map_ops;
-	}
-
 	if (!map->umem_cookie)
 		return (-EINVAL);
 
-	ret = gfxp_devmap_umem_setup(dhp, dev->devinfo, callbackops,
+	ret = gfxp_devmap_umem_setup(dhp, dev->devinfo, &drm_gem_map_ops,
 	    map->umem_cookie, 0, map->size, PROT_ALL,
 	    IOMEM_DATA_UC_WR_COMBINE | DEVMAP_ALLOW_REMAP, &gem_dev_attr);
 	if (ret != DDI_SUCCESS) {
diff --git a/usr/src/uts/intel/io/i915/i915_dma.c b/usr/src/uts/intel/io/i915/i915_dma.c
index 0556c12..c6d8be4 100644
--- a/usr/src/uts/intel/io/i915/i915_dma.c
+++ b/usr/src/uts/intel/io/i915/i915_dma.c
@@ -1510,7 +1510,6 @@ out_mtrrfree:
 out_rmmap:
 	drm_ioremapfree(dev_priv->regs);
 put_bridge:
-free_priv:
 	kfree(dev_priv, sizeof(struct drm_i915_private));
 	return ret;
 }
@@ -1573,7 +1572,7 @@ int i915_driver_unload(struct drm_device *dev)
 			i915_free_hws(dev);
 		i915_gem_lastclose(dev);
 		if (dev_priv->gtt.scratch_page)
-			teardown_scratch_page(dev);
+			i915_teardown_scratch_page(dev);
 		if (dev_priv->fbcon_obj != NULL) {
 			i915_gem_free_object(&dev_priv->fbcon_obj->base);
 			dev_priv->fbcon_obj = NULL;
diff --git a/usr/src/uts/intel/io/i915/i915_drv.c b/usr/src/uts/intel/io/i915/i915_drv.c
index 89ff8bb..464993e 100644
--- a/usr/src/uts/intel/io/i915/i915_drv.c
+++ b/usr/src/uts/intel/io/i915/i915_drv.c
@@ -1098,7 +1098,7 @@ i915_quiesce(dev_info_t *dip)
 		i915_gem_lastclose(dev);
 
 		if (dev_priv->gtt.scratch_page)
-			teardown_scratch_page(dev);
+			i915_teardown_scratch_page(dev);
 
 		if (MDB_TRACK_ENABLE) {
 			struct batch_info_list *r_list, *list_temp;
diff --git a/usr/src/uts/intel/io/i915/i915_drv.h b/usr/src/uts/intel/io/i915/i915_drv.h
index be7745d..21c43f9 100644
--- a/usr/src/uts/intel/io/i915/i915_drv.h
+++ b/usr/src/uts/intel/io/i915/i915_drv.h
@@ -1837,8 +1837,8 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
 int i915_gem_init_global_gtt(struct drm_device *dev);
 void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
 			       unsigned long mappable_end, unsigned long end);
-int setup_scratch_page(struct drm_device *dev);
-void teardown_scratch_page(struct drm_device *dev);
+int i915_setup_scratch_page(struct drm_device *dev);
+void i915_teardown_scratch_page(struct drm_device *dev);
 int i915_gem_gtt_init(struct drm_device *dev);
 void intel_rw_gtt(struct drm_device *dev, size_t size,
 		uint32_t gtt_offset, void *gttp, uint32_t type);
diff --git a/usr/src/uts/intel/io/i915/i915_gem.c b/usr/src/uts/intel/io/i915/i915_gem.c
index 4c89a79..04b2b4e 100644
--- a/usr/src/uts/intel/io/i915/i915_gem.c
+++ b/usr/src/uts/intel/io/i915/i915_gem.c
@@ -3625,7 +3625,7 @@ int i915_gem_init(struct drm_device *dev)
 		if (!dev_priv->fbcon_obj) {
 			DRM_ERROR("failed to allocate framebuffer");
 			mutex_unlock(&dev->struct_mutex);
-			teardown_scratch_page(dev);
+			i915_teardown_scratch_page(dev);
 			return (-ENOMEM);
 		}
 
@@ -3637,7 +3637,7 @@ int i915_gem_init(struct drm_device *dev)
 		if (ret) {
 			DRM_ERROR("failed to pin fb ret %d", ret);
 			mutex_unlock(&dev->struct_mutex);
-			teardown_scratch_page(dev);
+			i915_teardown_scratch_page(dev);
 			i915_gem_free_object(&dev_priv->fbcon_obj->base);
 			return ret;
 		}
diff --git a/usr/src/uts/intel/io/i915/i915_gem_gtt.c b/usr/src/uts/intel/io/i915/i915_gem_gtt.c
index ab81ea5..bb6c78c 100644
--- a/usr/src/uts/intel/io/i915/i915_gem_gtt.c
+++ b/usr/src/uts/intel/io/i915/i915_gem_gtt.c
@@ -698,7 +698,7 @@ int i915_gem_init_global_gtt(struct drm_device *dev)
 	}
 	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 
-	ret = setup_scratch_page(dev);
+	ret = i915_setup_scratch_page(dev);
 	if (ret)
 		return ret;
 
@@ -713,13 +713,20 @@ int i915_gem_init_global_gtt(struct drm_device *dev)
 	return 0;
 }
 
-int setup_scratch_page(struct drm_device *dev)
+/*
+ * Just to be safe against memory overwrite, let's allocate this with
+ * sizeof drm_i915_gem_object, even though we don't use the i915 part.
+ * This way, all i915 GEM objects are the same size.
+ */
+int
+i915_setup_scratch_page(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int gen;
 
 	/* setup scratch page */
-	dev_priv->gtt.scratch_page = kzalloc(sizeof(struct drm_gem_object), GFP_KERNEL);
+	dev_priv->gtt.scratch_page =
+	    kzalloc(sizeof(struct drm_i915_gem_object), GFP_KERNEL);
 	if (dev_priv->gtt.scratch_page == NULL) {
 		DRM_ERROR("setup_scratch_page: gem object init failed");
 		return (-ENOMEM);
@@ -731,7 +738,8 @@ int setup_scratch_page(struct drm_device *dev)
 		gen = INTEL_INFO(dev)->gen * 10;
 
 	if (drm_gem_object_init(dev, dev_priv->gtt.scratch_page, DRM_PAGE_SIZE, gen) != 0) {
-		kmem_free(dev_priv->gtt.scratch_page, sizeof (struct drm_gem_object));
+		kmem_free(dev_priv->gtt.scratch_page,
+		    sizeof (struct drm_i915_gem_object));
 		dev_priv->gtt.scratch_page = NULL;
 		DRM_ERROR("setup_scratch_page: gem object init failed");
 		return (-ENOMEM);
@@ -741,7 +749,8 @@ int setup_scratch_page(struct drm_device *dev)
 	return 0;
 }
 
-void teardown_scratch_page(struct drm_device *dev)
+void
+i915_teardown_scratch_page(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -749,7 +758,9 @@ void teardown_scratch_page(struct drm_device *dev)
 		return;
 
 	drm_gem_object_release(dev_priv->gtt.scratch_page);
-	kmem_free(dev_priv->gtt.scratch_page, sizeof (struct drm_gem_object));
+	kmem_free(dev_priv->gtt.scratch_page,
+	    sizeof (struct drm_i915_gem_object));
+	dev_priv->gtt.scratch_page = NULL;
 }
 
 static inline unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
diff --git a/usr/src/uts/intel/io/i915/intel_display.c b/usr/src/uts/intel/io/i915/intel_display.c
index f7d58e5..da0bf65 100644
--- a/usr/src/uts/intel/io/i915/intel_display.c
+++ b/usr/src/uts/intel/io/i915/intel_display.c
@@ -7619,7 +7619,6 @@ cleanup_pending:
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 
-cleanup:
 	spin_lock_irqsave(&dev->event_lock, flags);
 	intel_crtc->unpin_work = NULL;
 	spin_unlock_irqrestore(&dev->event_lock, flags);
diff --git a/usr/src/uts/intel/io/i915/intel_fb.c b/usr/src/uts/intel/io/i915/intel_fb.c
index 5eb89ca..48e7a97 100644
--- a/usr/src/uts/intel/io/i915/intel_fb.c
+++ b/usr/src/uts/intel/io/i915/intel_fb.c
@@ -92,7 +92,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 out_unpin:
 	i915_gem_object_unpin(obj);
-out_unref:
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 out: