Attachment 97503 Details for Bug 77351 – Fix dynamic allocation of physical handles

[patch] Fix dynamic allocation of physical handles

0001-drm-i915-Fix-dynamic-allocation-of-physical-handles.patch (text/plain), 15.97 KB, created by Chris Wilson on 2014-04-17 10:14:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2014-04-17 10:14:13 UTC

Size: 15.97 KB

patch

obsolete

>From 70d4ed6db838685ba5b496de965ff9ce8340eb4e Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Thu, 17 Apr 2014 10:34:30 +0100
>Subject: [PATCH] drm/i915: Fix dynamic allocation of physical handles
>
>A single object may be referenced by multiple registers fundamentally
>breaking the static allotment of ids in the current design. When the
>object is used the second time, the physical address of the first
>assignment is relinquished and a second one granted. However, the
>hardware is still reading (and possibly writing) to the old physical
>address now returned to the system. Eventually hilarity will ensue, but
>in the short term, it just means that cursors are broken when using more
>than one pipe.
>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77351
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/i915_dma.c      |   1 -
> drivers/gpu/drm/i915/i915_drv.h      |  24 +--
> drivers/gpu/drm/i915/i915_gem.c      | 310 +++++++++++++----------------------
> drivers/gpu/drm/i915/intel_display.c |  11 +-
> drivers/gpu/drm/i915/intel_overlay.c |  12 +-
> 5 files changed, 126 insertions(+), 232 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>index d67ca8051e07..12571aac7516 100644
>--- a/drivers/gpu/drm/i915/i915_dma.c
>+++ b/drivers/gpu/drm/i915/i915_dma.c
>@@ -1854,7 +1854,6 @@ int i915_driver_unload(struct drm_device *dev)
> 		flush_workqueue(dev_priv->wq);
> 
> 		mutex_lock(&dev->struct_mutex);
>-		i915_gem_free_all_phys_object(dev);
> 		i915_gem_cleanup_ringbuffer(dev);
> 		i915_gem_context_fini(dev);
> 		WARN_ON(dev_priv->mm.aliasing_ppgtt);
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 0e85bf2c3326..a2375a0ef27c 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -246,18 +246,6 @@ struct intel_ddi_plls {
> #define WATCH_LISTS	0
> #define WATCH_GTT	0
> 
>-#define I915_GEM_PHYS_CURSOR_0 1
>-#define I915_GEM_PHYS_CURSOR_1 2
>-#define I915_GEM_PHYS_OVERLAY_REGS 3
>-#define I915_MAX_PHYS_OBJECT (I915_GEM_PHYS_OVERLAY_REGS)
>-
>-struct drm_i915_gem_phys_object {
>-	int id;
>-	struct page **page_list;
>-	drm_dma_handle_t *handle;
>-	struct drm_i915_gem_object *cur_obj;
>-};
>-
> struct opregion_header;
> struct opregion_acpi;
> struct opregion_swsci;
>@@ -1036,9 +1024,6 @@ struct i915_gem_mm {
> 	/** Bit 6 swizzling required for Y tiling */
> 	uint32_t bit_6_swizzle_y;
> 
>-	/* storage for physical objects */
>-	struct drm_i915_gem_phys_object *phys_objs[I915_MAX_PHYS_OBJECT];
>-
> 	/* accounting, useful for userland debugging */
> 	spinlock_t object_stat_lock;
> 	size_t object_memory;
>@@ -1647,7 +1632,7 @@ struct drm_i915_gem_object {
> 	struct drm_file *pin_filp;
> 
> 	/** for phy allocated objects */
>-	struct drm_i915_gem_phys_object *phys_obj;
>+	drm_dma_handle_t *phys_handle;
> 
> 	union {
> 		struct i915_gem_userptr {
>@@ -2250,13 +2235,8 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> 				     u32 alignment,
> 				     struct intel_ring_buffer *pipelined);
> void i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj);
>-int i915_gem_attach_phys_object(struct drm_device *dev,
>-				struct drm_i915_gem_object *obj,
>-				int id,
>+int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> 				int align);
>-void i915_gem_detach_phys_object(struct drm_device *dev,
>-				 struct drm_i915_gem_object *obj);
>-void i915_gem_free_all_phys_object(struct drm_device *dev);
> int i915_gem_open(struct drm_device *dev, struct drm_file *file);
> void i915_gem_release(struct drm_device *dev, struct drm_file *file);
> 
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index 764467ebade5..29aa543f5022 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -46,11 +46,6 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
> static void
> i915_gem_object_retire(struct drm_i915_gem_object *obj);
> 
>-static int i915_gem_phys_pwrite(struct drm_device *dev,
>-				struct drm_i915_gem_object *obj,
>-				struct drm_i915_gem_pwrite *args,
>-				struct drm_file *file);
>-
> static void i915_gem_write_fence(struct drm_device *dev, int reg,
> 				 struct drm_i915_gem_object *obj);
> static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
>@@ -212,6 +207,119 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
> 	return 0;
> }
> 
>+static void i915_gem_object_detach_phys(struct drm_i915_gem_object *obj)
>+{
>+	drm_dma_handle_t *phys = obj->phys_handle;
>+	struct address_space *mapping = file_inode(obj->base.filp)->i_mapping;
>+	int i;
>+
>+	if (!phys)
>+		return;
>+
>+	if (obj->madv == I915_MADV_WILLNEED) {
>+		char *vaddr = phys->vaddr;
>+		for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
>+			struct page *page = shmem_read_mapping_page(mapping, i);
>+			if (!IS_ERR(page)) {
>+				char *dst = kmap_atomic(page);
>+				memcpy(dst, vaddr, PAGE_SIZE);
>+				kunmap_atomic(dst);
>+
>+				drm_clflush_pages(&page, 1);
>+
>+				set_page_dirty(page);
>+				mark_page_accessed(page);
>+				page_cache_release(page);
>+			}
>+			vaddr += PAGE_SIZE;
>+		}
>+		i915_gem_chipset_flush(obj->base.dev);
>+	}
>+
>+#ifdef CONFIG_X86
>+	set_memory_wb((unsigned long)phys->vaddr, phys->size / PAGE_SIZE);
>+#endif
>+	drm_pci_free(obj->base.dev, phys);
>+	obj->phys_handle = NULL;
>+}
>+
>+int
>+i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>+			    int align)
>+{
>+	struct address_space *mapping = file_inode(obj->base.filp)->i_mapping;
>+	drm_dma_handle_t *phys;
>+	char *vaddr;
>+	int i;
>+
>+	if (obj->phys_handle) {
>+		if (((unsigned long)obj->phys_handle->vaddr & (align -1)) == 0)
>+			return 0;
>+
>+		i915_gem_object_detach_phys(obj);
>+	}
>+
>+	if (obj->madv != I915_MADV_WILLNEED)
>+		return -EFAULT;
>+
>+	/* create a new object */
>+	phys = drm_pci_alloc(obj->base.dev, obj->base.size, align);
>+	if (!phys)
>+		return -ENOMEM;
>+
>+	vaddr = phys->vaddr;
>+#ifdef CONFIG_X86
>+	set_memory_wc((unsigned long)vaddr, phys->size / PAGE_SIZE);
>+#endif
>+	for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
>+		struct page *page;
>+		char *src;
>+
>+		page = shmem_read_mapping_page(mapping, i);
>+		if (IS_ERR(page))
>+			return PTR_ERR(page);
>+
>+		src = kmap_atomic(page);
>+		memcpy(vaddr, src, PAGE_SIZE);
>+		kunmap_atomic(src);
>+
>+		mark_page_accessed(page);
>+		page_cache_release(page);
>+
>+		vaddr += PAGE_SIZE;
>+	}
>+
>+	obj->phys_handle = phys;
>+	return 0;
>+}
>+
>+static int
>+i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>+		     struct drm_i915_gem_pwrite *args,
>+		     struct drm_file *file_priv)
>+{
>+	struct drm_device *dev = obj->base.dev;
>+	void *vaddr = obj->phys_handle->vaddr + args->offset;
>+	char __user *user_data = to_user_ptr(args->data_ptr);
>+
>+	if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
>+		unsigned long unwritten;
>+
>+		/* The physical object once assigned is fixed for the lifetime
>+		 * of the obj, so we can safely drop the lock and continue
>+		 * to access vaddr.
>+		 */
>+		mutex_unlock(&dev->struct_mutex);
>+		unwritten = copy_from_user(vaddr, user_data, args->size);
>+		mutex_lock(&dev->struct_mutex);
>+		if (unwritten)
>+			return -EFAULT;
>+	}
>+
>+	i915_gem_chipset_flush(dev);
>+	return 0;
>+}
>+
> void *i915_gem_object_alloc(struct drm_device *dev)
> {
> 	struct drm_i915_private *dev_priv = dev->dev_private;
>@@ -1071,8 +1179,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> 	 * pread/pwrite currently are reading and writing from the CPU
> 	 * perspective, requiring manual detiling by the client.
> 	 */
>-	if (obj->phys_obj) {
>-		ret = i915_gem_phys_pwrite(dev, obj, args, file);
>+	if (obj->phys_handle) {
>+		ret = i915_gem_phys_pwrite(obj, args, file);
> 		goto out;
> 	}
> 
>@@ -4376,9 +4484,6 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> 
> 	trace_i915_gem_object_destroy(obj);
> 
>-	if (obj->phys_obj)
>-		i915_gem_detach_phys_object(dev, obj);
>-
> 	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> 		int ret;
> 
>@@ -4408,6 +4513,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> 	i915_gem_object_put_pages(obj);
> 	i915_gem_object_free_mmap_offset(obj);
> 	i915_gem_object_release_stolen(obj);
>+	i915_gem_object_detach_phys(obj);
> 
> 	BUG_ON(obj->pages);
> 
>@@ -4875,190 +4981,6 @@ i915_gem_load(struct drm_device *dev)
> 	register_shrinker(&dev_priv->mm.shrinker);
> }
> 
>-/*
>- * Create a physically contiguous memory object for this object
>- * e.g. for cursor + overlay regs
>- */
>-static int i915_gem_init_phys_object(struct drm_device *dev,
>-				     int id, int size, int align)
>-{
>-	struct drm_i915_private *dev_priv = dev->dev_private;
>-	struct drm_i915_gem_phys_object *phys_obj;
>-	int ret;
>-
>-	if (dev_priv->mm.phys_objs[id - 1] || !size)
>-		return 0;
>-
>-	phys_obj = kzalloc(sizeof(*phys_obj), GFP_KERNEL);
>-	if (!phys_obj)
>-		return -ENOMEM;
>-
>-	phys_obj->id = id;
>-
>-	phys_obj->handle = drm_pci_alloc(dev, size, align);
>-	if (!phys_obj->handle) {
>-		ret = -ENOMEM;
>-		goto kfree_obj;
>-	}
>-#ifdef CONFIG_X86
>-	set_memory_wc((unsigned long)phys_obj->handle->vaddr, phys_obj->handle->size / PAGE_SIZE);
>-#endif
>-
>-	dev_priv->mm.phys_objs[id - 1] = phys_obj;
>-
>-	return 0;
>-kfree_obj:
>-	kfree(phys_obj);
>-	return ret;
>-}
>-
>-static void i915_gem_free_phys_object(struct drm_device *dev, int id)
>-{
>-	struct drm_i915_private *dev_priv = dev->dev_private;
>-	struct drm_i915_gem_phys_object *phys_obj;
>-
>-	if (!dev_priv->mm.phys_objs[id - 1])
>-		return;
>-
>-	phys_obj = dev_priv->mm.phys_objs[id - 1];
>-	if (phys_obj->cur_obj) {
>-		i915_gem_detach_phys_object(dev, phys_obj->cur_obj);
>-	}
>-
>-#ifdef CONFIG_X86
>-	set_memory_wb((unsigned long)phys_obj->handle->vaddr, phys_obj->handle->size / PAGE_SIZE);
>-#endif
>-	drm_pci_free(dev, phys_obj->handle);
>-	kfree(phys_obj);
>-	dev_priv->mm.phys_objs[id - 1] = NULL;
>-}
>-
>-void i915_gem_free_all_phys_object(struct drm_device *dev)
>-{
>-	int i;
>-
>-	for (i = I915_GEM_PHYS_CURSOR_0; i <= I915_MAX_PHYS_OBJECT; i++)
>-		i915_gem_free_phys_object(dev, i);
>-}
>-
>-void i915_gem_detach_phys_object(struct drm_device *dev,
>-				 struct drm_i915_gem_object *obj)
>-{
>-	struct address_space *mapping = file_inode(obj->base.filp)->i_mapping;
>-	char *vaddr;
>-	int i;
>-	int page_count;
>-
>-	if (!obj->phys_obj)
>-		return;
>-	vaddr = obj->phys_obj->handle->vaddr;
>-
>-	page_count = obj->base.size / PAGE_SIZE;
>-	for (i = 0; i < page_count; i++) {
>-		struct page *page = shmem_read_mapping_page(mapping, i);
>-		if (!IS_ERR(page)) {
>-			char *dst = kmap_atomic(page);
>-			memcpy(dst, vaddr + i*PAGE_SIZE, PAGE_SIZE);
>-			kunmap_atomic(dst);
>-
>-			drm_clflush_pages(&page, 1);
>-
>-			set_page_dirty(page);
>-			mark_page_accessed(page);
>-			page_cache_release(page);
>-		}
>-	}
>-	i915_gem_chipset_flush(dev);
>-
>-	obj->phys_obj->cur_obj = NULL;
>-	obj->phys_obj = NULL;
>-}
>-
>-int
>-i915_gem_attach_phys_object(struct drm_device *dev,
>-			    struct drm_i915_gem_object *obj,
>-			    int id,
>-			    int align)
>-{
>-	struct address_space *mapping = file_inode(obj->base.filp)->i_mapping;
>-	struct drm_i915_private *dev_priv = dev->dev_private;
>-	int ret = 0;
>-	int page_count;
>-	int i;
>-
>-	if (id > I915_MAX_PHYS_OBJECT)
>-		return -EINVAL;
>-
>-	if (obj->phys_obj) {
>-		if (obj->phys_obj->id == id)
>-			return 0;
>-		i915_gem_detach_phys_object(dev, obj);
>-	}
>-
>-	/* create a new object */
>-	if (!dev_priv->mm.phys_objs[id - 1]) {
>-		ret = i915_gem_init_phys_object(dev, id,
>-						obj->base.size, align);
>-		if (ret) {
>-			DRM_ERROR("failed to init phys object %d size: %zu\n",
>-				  id, obj->base.size);
>-			return ret;
>-		}
>-	}
>-
>-	/* bind to the object */
>-	obj->phys_obj = dev_priv->mm.phys_objs[id - 1];
>-	obj->phys_obj->cur_obj = obj;
>-
>-	page_count = obj->base.size / PAGE_SIZE;
>-
>-	for (i = 0; i < page_count; i++) {
>-		struct page *page;
>-		char *dst, *src;
>-
>-		page = shmem_read_mapping_page(mapping, i);
>-		if (IS_ERR(page))
>-			return PTR_ERR(page);
>-
>-		src = kmap_atomic(page);
>-		dst = obj->phys_obj->handle->vaddr + (i * PAGE_SIZE);
>-		memcpy(dst, src, PAGE_SIZE);
>-		kunmap_atomic(src);
>-
>-		mark_page_accessed(page);
>-		page_cache_release(page);
>-	}
>-
>-	return 0;
>-}
>-
>-static int
>-i915_gem_phys_pwrite(struct drm_device *dev,
>-		     struct drm_i915_gem_object *obj,
>-		     struct drm_i915_gem_pwrite *args,
>-		     struct drm_file *file_priv)
>-{
>-	void *vaddr = obj->phys_obj->handle->vaddr + args->offset;
>-	char __user *user_data = to_user_ptr(args->data_ptr);
>-
>-	if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
>-		unsigned long unwritten;
>-
>-		/* The physical object once assigned is fixed for the lifetime
>-		 * of the obj, so we can safely drop the lock and continue
>-		 * to access vaddr.
>-		 */
>-		mutex_unlock(&dev->struct_mutex);
>-		unwritten = copy_from_user(vaddr, user_data, args->size);
>-		mutex_lock(&dev->struct_mutex);
>-		if (unwritten)
>-			return -EFAULT;
>-	}
>-
>-	i915_gem_chipset_flush(dev);
>-	return 0;
>-}
>-
> void i915_gem_release(struct drm_device *dev, struct drm_file *file)
> {
> 	struct drm_i915_file_private *file_priv = file->driver_priv;
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 3d017f5b656a..61735fde3f12 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -7777,14 +7777,12 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> 		addr = i915_gem_obj_ggtt_offset(obj);
> 	} else {
> 		int align = IS_I830(dev) ? 16 * 1024 : 256;
>-		ret = i915_gem_attach_phys_object(dev, obj,
>-						  (intel_crtc->pipe == 0) ? I915_GEM_PHYS_CURSOR_0 : I915_GEM_PHYS_CURSOR_1,
>-						  align);
>+		ret = i915_gem_object_attach_phys(obj, align);
> 		if (ret) {
> 			DRM_DEBUG_KMS("failed to attach phys object\n");
> 			goto fail_locked;
> 		}
>-		addr = obj->phys_obj->handle->busaddr;
>+		addr = obj->phys_handle->busaddr;
> 	}
> 
> 	if (IS_GEN2(dev))
>@@ -7792,10 +7790,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> 
>  finish:
> 	if (intel_crtc->cursor_bo) {
>-		if (INTEL_INFO(dev)->cursor_needs_physical) {
>-			if (intel_crtc->cursor_bo != obj)
>-				i915_gem_detach_phys_object(dev, intel_crtc->cursor_bo);
>-		} else
>+		if (!INTEL_INFO(dev)->cursor_needs_physical)
> 			i915_gem_object_unpin_from_display_plane(intel_crtc->cursor_bo);
> 		drm_gem_object_unreference(&intel_crtc->cursor_bo->base);
> 	}
>diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
>index 4232230a78bd..5423eb0c1335 100644
>--- a/drivers/gpu/drm/i915/intel_overlay.c
>+++ b/drivers/gpu/drm/i915/intel_overlay.c
>@@ -194,7 +194,7 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
> 	struct overlay_registers __iomem *regs;
> 
> 	if (OVERLAY_NEEDS_PHYSICAL(overlay->dev))
>-		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_obj->handle->vaddr;
>+		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
> 	else
> 		regs = io_mapping_map_wc(dev_priv->gtt.mappable,
> 					 i915_gem_obj_ggtt_offset(overlay->reg_bo));
>@@ -1347,14 +1347,12 @@ void intel_setup_overlay(struct drm_device *dev)
> 	overlay->reg_bo = reg_bo;
> 
> 	if (OVERLAY_NEEDS_PHYSICAL(dev)) {
>-		ret = i915_gem_attach_phys_object(dev, reg_bo,
>-						  I915_GEM_PHYS_OVERLAY_REGS,
>-						  PAGE_SIZE);
>+		ret = i915_gem_object_attach_phys(reg_bo, PAGE_SIZE);
> 		if (ret) {
> 			DRM_ERROR("failed to attach phys overlay regs\n");
> 			goto out_free_bo;
> 		}
>-		overlay->flip_addr = reg_bo->phys_obj->handle->busaddr;
>+		overlay->flip_addr = reg_bo->phys_handle->busaddr;
> 	} else {
> 		ret = i915_gem_obj_ggtt_pin(reg_bo, PAGE_SIZE, PIN_MAPPABLE);
> 		if (ret) {
>@@ -1436,7 +1434,7 @@ intel_overlay_map_regs_atomic(struct intel_overlay *overlay)
> 		/* Cast to make sparse happy, but it's wc memory anyway, so
> 		 * equivalent to the wc io mapping on X86. */
> 		regs = (struct overlay_registers __iomem *)
>-			overlay->reg_bo->phys_obj->handle->vaddr;
>+			overlay->reg_bo->phys_handle->vaddr;
> 	else
> 		regs = io_mapping_map_atomic_wc(dev_priv->gtt.mappable,
> 						i915_gem_obj_ggtt_offset(overlay->reg_bo));
>@@ -1470,7 +1468,7 @@ intel_overlay_capture_error_state(struct drm_device *dev)
> 	error->dovsta = I915_READ(DOVSTA);
> 	error->isr = I915_READ(ISR);
> 	if (OVERLAY_NEEDS_PHYSICAL(overlay->dev))
>-		error->base = (__force long)overlay->reg_bo->phys_obj->handle->vaddr;
>+		error->base = (__force long)overlay->reg_bo->phys_handle->vaddr;
> 	else
> 		error->base = i915_gem_obj_ggtt_offset(overlay->reg_bo);
> 
>-- 
>1.9.2
>

Actions: View | Splinter Review

Attachments on bug 77351: 97243 | 97244 | 97245 | 97352 | 97353 | 97503 | 97557

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.