[PATCH 1/3] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[PATCH 1/3] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[Chris Wilson]

When setting up the overlay page, we pin it into the GGTT (when using
virtual addresses) and store the offset as overlay->flip_addr. Rather
than doing a lookup of the GGTT address everytime, we can use the known
address instead.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_overlay.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 6694e9230cd5..e487ff18b42f 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,7 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 i915_gem_obj_ggtt_offset(overlay->reg_bo));
+					 overlay->flip_addr);
 
 	return regs;
 }
@@ -1493,7 +1493,7 @@ intel_overlay_map_regs_atomic(struct intel_overlay *overlay)
 			overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_atomic_wc(ggtt->mappable,
-						i915_gem_obj_ggtt_offset(overlay->reg_bo));
+						overlay->flip_addr);
 
 	return regs;
 }
@@ -1523,10 +1523,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_handle->vaddr;
-	else
-		error->base = i915_gem_obj_ggtt_offset(overlay->reg_bo);
+	error->base = overlay->flip_addr;
 
 	regs = intel_overlay_map_regs_atomic(overlay);
 	if (!regs)
-- 
2.8.0.rc3

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

[PATCH 2/3] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

The ioremap() hidden behind the io_mapping_map_wc() convenience helper
can be used for remapping multiple pages. Extend the helper so that
future callers can use it for larger ranges.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Yishai Hadas <yishaih@mellanox.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_overlay.c    |  3 ++-
 drivers/net/ethernet/mellanox/mlx4/pd.c |  4 +++-
 include/linux/io-mapping.h              | 10 +++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e487ff18b42f..5bba2cf62a26 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,8 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 overlay->flip_addr);
+					 overlay->flip_addr,
+					 PAGE_SIZE);
 
 	return regs;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/pd.c b/drivers/net/ethernet/mellanox/mlx4/pd.c
index b3cc3ab63799..6fc156a3918d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/pd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/pd.c
@@ -205,7 +205,9 @@ int mlx4_bf_alloc(struct mlx4_dev *dev, struct mlx4_bf *bf, int node)
 			goto free_uar;
 		}
 
-		uar->bf_map = io_mapping_map_wc(priv->bf_mapping, uar->index << PAGE_SHIFT);
+		uar->bf_map = io_mapping_map_wc(priv->bf_mapping,
+						uar->index << PAGE_SHIFT,
+						PAGE_SIZE);
 		if (!uar->bf_map) {
 			err = -ENOMEM;
 			goto unamp_uar;
diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index e399029b68c5..645ad06b5d52 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -100,14 +100,16 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 }
 
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	resource_size_t phys_addr;
 
 	BUG_ON(offset >= mapping->size);
 	phys_addr = mapping->base + offset;
 
-	return ioremap_wc(phys_addr, PAGE_SIZE);
+	return ioremap_wc(phys_addr, size);
 }
 
 static inline void
@@ -155,7 +157,9 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 
 /* Non-atomic map/unmap */
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	return ((char __force __iomem *) mapping) + offset;
 }
-- 
2.8.0.rc3

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

[PATCH 2/3] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

The ioremap() hidden behind the io_mapping_map_wc() convenience helper
can be used for remapping multiple pages. Extend the helper so that
future callers can use it for larger ranges.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Yishai Hadas <yishaih@mellanox.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_overlay.c    |  3 ++-
 drivers/net/ethernet/mellanox/mlx4/pd.c |  4 +++-
 include/linux/io-mapping.h              | 10 +++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e487ff18b42f..5bba2cf62a26 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,8 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 overlay->flip_addr);
+					 overlay->flip_addr,
+					 PAGE_SIZE);
 
 	return regs;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/pd.c b/drivers/net/ethernet/mellanox/mlx4/pd.c
index b3cc3ab63799..6fc156a3918d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/pd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/pd.c
@@ -205,7 +205,9 @@ int mlx4_bf_alloc(struct mlx4_dev *dev, struct mlx4_bf *bf, int node)
 			goto free_uar;
 		}
 
-		uar->bf_map = io_mapping_map_wc(priv->bf_mapping, uar->index << PAGE_SHIFT);
+		uar->bf_map = io_mapping_map_wc(priv->bf_mapping,
+						uar->index << PAGE_SHIFT,
+						PAGE_SIZE);
 		if (!uar->bf_map) {
 			err = -ENOMEM;
 			goto unamp_uar;
diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index e399029b68c5..645ad06b5d52 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -100,14 +100,16 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 }
 
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	resource_size_t phys_addr;
 
 	BUG_ON(offset >= mapping->size);
 	phys_addr = mapping->base + offset;
 
-	return ioremap_wc(phys_addr, PAGE_SIZE);
+	return ioremap_wc(phys_addr, size);
 }
 
 static inline void
@@ -155,7 +157,9 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 
 /* Non-atomic map/unmap */
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	return ((char __force __iomem *) mapping) + offset;
 }
-- 
2.8.0.rc3

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

[PATCH 2/3] io-mapping: Specify mapping size for io_mapping_map_wc()

[Chris Wilson]

The ioremap() hidden behind the io_mapping_map_wc() convenience helper
can be used for remapping multiple pages. Extend the helper so that
future callers can use it for larger ranges.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Yishai Hadas <yishaih@mellanox.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: netdev@vger.kernel.org
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_overlay.c    |  3 ++-
 drivers/net/ethernet/mellanox/mlx4/pd.c |  4 +++-
 include/linux/io-mapping.h              | 10 +++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e487ff18b42f..5bba2cf62a26 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -198,7 +198,8 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
 		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
 	else
 		regs = io_mapping_map_wc(ggtt->mappable,
-					 overlay->flip_addr);
+					 overlay->flip_addr,
+					 PAGE_SIZE);
 
 	return regs;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/pd.c b/drivers/net/ethernet/mellanox/mlx4/pd.c
index b3cc3ab63799..6fc156a3918d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/pd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/pd.c
@@ -205,7 +205,9 @@ int mlx4_bf_alloc(struct mlx4_dev *dev, struct mlx4_bf *bf, int node)
 			goto free_uar;
 		}
 
-		uar->bf_map = io_mapping_map_wc(priv->bf_mapping, uar->index << PAGE_SHIFT);
+		uar->bf_map = io_mapping_map_wc(priv->bf_mapping,
+						uar->index << PAGE_SHIFT,
+						PAGE_SIZE);
 		if (!uar->bf_map) {
 			err = -ENOMEM;
 			goto unamp_uar;
diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index e399029b68c5..645ad06b5d52 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -100,14 +100,16 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 }
 
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	resource_size_t phys_addr;
 
 	BUG_ON(offset >= mapping->size);
 	phys_addr = mapping->base + offset;
 
-	return ioremap_wc(phys_addr, PAGE_SIZE);
+	return ioremap_wc(phys_addr, size);
 }
 
 static inline void
@@ -155,7 +157,9 @@ io_mapping_unmap_atomic(void __iomem *vaddr)
 
 /* Non-atomic map/unmap */
 static inline void __iomem *
-io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
+io_mapping_map_wc(struct io_mapping *mapping,
+		  unsigned long offset,
+		  unsigned long size)
 {
 	return ((char __force __iomem *) mapping) + offset;
 }
-- 
2.8.0.rc3

[PATCH 3/3] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

By tracking the iomapping on the VMA itself, we can share that area
between multiple users. Also by only revoking the iomapping upon
unbinding from the mappable portion of the GGTT, we can keep that iomap
across multiple invocations (e.g. execlists context pinning).

Note that by moving the iounnmap tracking to the VMA, we actually end up
fixing a leak of the iomapping in intel_fbdev.

v1.5: Rebase prompted by Tvrtko
v2: Drop dev_priv parameter, we can recover the i915_ggtt from the vma.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c     |  2 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++++++++++++
 drivers/gpu/drm/i915/intel_fbdev.c  | 20 +++++++++---------
 4 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b37ffea8b458..6a485630595e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3393,6 +3393,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
 		ret = i915_gem_object_put_fence(obj);
 		if (ret)
 			return ret;
+
+		i915_vma_iounmap(vma);
 	}
 
 	trace_i915_vma_unbind(vma);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c5cb04907525..e759f8cafd9e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -93,6 +93,12 @@
  *
  */
 
+static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm)
+{
+	BUG_ON(!i915_is_ggtt(vm));
+	return container_of(vm, struct i915_ggtt, base);
+}
+
 static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma);
 
@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+void *i915_vma_iomap(struct i915_vma *vma)
+{
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	BUG_ON(!vma->is_ggtt);
+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	if (vma->iomap == NULL) {
+		struct i915_ggtt *ggtt = to_ggtt(vma->vm);
+		void *ptr;
+
+		ptr = io_mapping_map_wc(ggtt->mappable,
+					vma->node.start,
+					vma->node.size);
+		if (ptr == NULL) {
+			int ret;
+
+			/* Too many areas already allocated? */
+			ret = i915_gem_evict_vm(vma->vm, true);
+			if (ret)
+				return ERR_PTR(ret);
+
+			ptr = io_mapping_map_wc(ggtt->mappable,
+						vma->node.start,
+						vma->node.size);
+			if (ptr == NULL)
+				return ERR_PTR(-ENOMEM);
+		}
+
+		vma->iomap = ptr;
+	}
+
+	return vma->iomap;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..4b8ffc1c3d33 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,14 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+void *i915_vma_iomap(struct i915_vma *vma);
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..a01bfc33e3d7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,10 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +215,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +244,22 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	info->screen_base = i915_vma_iomap(vma);
+	if (IS_ERR(info->screen_base)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(info->screen_base);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

Re: [PATCH 3/3] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin]

On 13/04/16 10:52, Chris Wilson wrote:
> By tracking the iomapping on the VMA itself, we can share that area
> between multiple users. Also by only revoking the iomapping upon
> unbinding from the mappable portion of the GGTT, we can keep that iomap
> across multiple invocations (e.g. execlists context pinning).
>
> Note that by moving the iounnmap tracking to the VMA, we actually end up
> fixing a leak of the iomapping in intel_fbdev.
>
> v1.5: Rebase prompted by Tvrtko
> v2: Drop dev_priv parameter, we can recover the i915_ggtt from the vma.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     |  2 ++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++++++++++++
>   drivers/gpu/drm/i915/intel_fbdev.c  | 20 +++++++++---------
>   4 files changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b37ffea8b458..6a485630595e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3393,6 +3393,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>   		ret = i915_gem_object_put_fence(obj);
>   		if (ret)
>   			return ret;
> +
> +		i915_vma_iounmap(vma);
>   	}
>
>   	trace_i915_vma_unbind(vma);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c5cb04907525..e759f8cafd9e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,12 @@
>    *
>    */
>
> +static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm)
> +{
> +	BUG_ON(!i915_is_ggtt(vm));
> +	return container_of(vm, struct i915_ggtt, base);
> +}
> +

As long as it remains hidden in here, otherwise is a bit heavy and rude 
(BUG_ON), or weak as API (if converted to GEM_BUG_ON and return pointer 
undocumented). And if it stays here BUG_ON is redundant. Hm.. leaning 
towards the direct container_of below to bypass all these considerations.

>   static int
>   i915_get_ggtt_vma_pages(struct i915_vma *vma);
>
> @@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		return obj->base.size;
>   	}
>   }
> +
> +void *i915_vma_iomap(struct i915_vma *vma)

Kerneldoc! :)

> +{
> +	if (WARN_ON(!vma->obj->map_and_fenceable))
> +		return ERR_PTR(-ENODEV);
> +
> +	BUG_ON(!vma->is_ggtt);
> +	BUG_ON((vma->bound & GLOBAL_BIND) == 0);

Maybe aggregate the two into a single WARN_ON and return -EINVAL ? Since 
we easily can sounds better.

> +
> +	if (vma->iomap == NULL) {
> +		struct i915_ggtt *ggtt = to_ggtt(vma->vm);
> +		void *ptr;
> +
> +		ptr = io_mapping_map_wc(ggtt->mappable,
> +					vma->node.start,
> +					vma->node.size);
> +		if (ptr == NULL) {
> +			int ret;
> +
> +			/* Too many areas already allocated? */
> +			ret = i915_gem_evict_vm(vma->vm, true);

I don't know much about the iomapping bussiness. Can we exhaust the 
address space similar to vmap and would then interfere with our own (or 
other?) call sites doing plain io_mapping_map* calls?

> +			if (ret)
> +				return ERR_PTR(ret);
> +
> +			ptr = io_mapping_map_wc(ggtt->mappable,
> +						vma->node.start,
> +						vma->node.size);
> +			if (ptr == NULL)
> +				return ERR_PTR(-ENOMEM);
> +		}
> +
> +		vma->iomap = ptr;
> +	}
> +
> +	return vma->iomap;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..4b8ffc1c3d33 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -34,6 +34,8 @@
>   #ifndef __I915_GEM_GTT_H__
>   #define __I915_GEM_GTT_H__
>
> +#include <linux/io-mapping.h>
> +
>   struct drm_i915_file_private;
>
>   typedef uint32_t gen6_pte_t;
> @@ -175,6 +177,7 @@ struct i915_vma {
>   	struct drm_mm_node node;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_address_space *vm;
> +	void *iomap;
>
>   	/** Flags and address space this VMA is bound to */
>   #define GLOBAL_BIND	(1<<0)
> @@ -559,4 +562,14 @@ size_t
>   i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>   		    const struct i915_ggtt_view *view);
>
> +void *i915_vma_iomap(struct i915_vma *vma);
> +static inline void i915_vma_iounmap(struct i915_vma *vma)
> +{
> +	if (vma->iomap == NULL)
> +		return;
> +
> +	io_mapping_unmap(vma->iomap);
> +	vma->iomap = NULL;
> +}
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..a01bfc33e3d7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -186,9 +186,10 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>   	struct fb_info *info;
>   	struct drm_framebuffer *fb;
> +	struct i915_vma *vma;
>   	struct drm_i915_gem_object *obj;
> -	int size, ret;
>   	bool prealloc = false;
> +	int ret;
>
>   	if (intel_fb &&
>   	    (sizes->fb_width > intel_fb->base.width ||
> @@ -214,7 +215,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	}
>
>   	obj = intel_fb->obj;
> -	size = obj->base.size;
>
>   	mutex_lock(&dev->struct_mutex);
>
> @@ -244,22 +244,22 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>   	info->fbops = &intelfb_ops;
>
> +	vma = i915_gem_obj_to_ggtt(obj);
> +
>   	/* setup aperture base/size for vesafb takeover */
>   	info->apertures->ranges[0].base = dev->mode_config.fb_base;
>   	info->apertures->ranges[0].size = ggtt->mappable_end;
>
> -	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> -	info->fix.smem_len = size;
> +	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> +	info->fix.smem_len = vma->node.size;
>
> -	info->screen_base =
> -		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -			   size);
> -	if (!info->screen_base) {
> +	info->screen_base = i915_vma_iomap(vma);

We are slowly establishing that ERR_PTR should not be stored in 
structure members but I suppose here we can allow it since the structure 
is freed if it fails.

> +	if (IS_ERR(info->screen_base)) {
>   		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> -		ret = -ENOSPC;
> +		ret = PTR_ERR(info->screen_base);
>   		goto out_destroy_fbi;
>   	}
> -	info->screen_size = size;
> +	info->screen_size = vma->node.size;
>
>   	/* This driver doesn't need a VT switch to restore the mode on resume */
>   	info->skip_vt_switch = true;
>

Regards,

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

Re: [PATCH 3/3] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

On Wed, Apr 13, 2016 at 01:30:39PM +0100, Tvrtko Ursulin wrote:
> As long as it remains hidden in here, otherwise is a bit heavy and
> rude (BUG_ON), or weak as API (if converted to GEM_BUG_ON and return
> pointer undocumented). And if it stays here BUG_ON is redundant.
> Hm.. leaning towards the direct container_of below to bypass all
> these considerations.

Reasonable. I do value the shorthand to_ggtt() though. Ok, the shorthand
can wait until we have more use cases.

> >  static int
> >  i915_get_ggtt_vma_pages(struct i915_vma *vma);
> >
> >@@ -3626,3 +3632,39 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> >  		return obj->base.size;
> >  	}
> >  }
> >+
> >+void *i915_vma_iomap(struct i915_vma *vma)
> 
> Kerneldoc! :)

Sorry. Should have double checked with your patch.

> >+{
> >+	if (WARN_ON(!vma->obj->map_and_fenceable))
> >+		return ERR_PTR(-ENODEV);
> >+
> >+	BUG_ON(!vma->is_ggtt);
> >+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> 
> Maybe aggregate the two into a single WARN_ON and return -EINVAL ?
> Since we easily can sounds better.
> 
> >+
> >+	if (vma->iomap == NULL) {
> >+		struct i915_ggtt *ggtt = to_ggtt(vma->vm);
> >+		void *ptr;
> >+
> >+		ptr = io_mapping_map_wc(ggtt->mappable,
> >+					vma->node.start,
> >+					vma->node.size);
> >+		if (ptr == NULL) {
> >+			int ret;
> >+
> >+			/* Too many areas already allocated? */
> >+			ret = i915_gem_evict_vm(vma->vm, true);
> 
> I don't know much about the iomapping bussiness. Can we exhaust the
> address space similar to vmap and would then interfere with our own
> (or other?) call sites doing plain io_mapping_map* calls?

It's vmap underneath. My thinking here was that if we couldn't allocate
a new ioremap_wc, the obvious candidates were to reap everything inside
the GGTT i.e. inactive ioremap_wc.

But the question is, do we want to reap vma->iomaps upon vmap_purge.
Yes, we probably should - simplest will be to do a similar forced GGTT
eviction from vmap_purge and refine from there.

> >+	vma = i915_gem_obj_to_ggtt(obj);
> >+
> >  	/* setup aperture base/size for vesafb takeover */
> >  	info->apertures->ranges[0].base = dev->mode_config.fb_base;
> >  	info->apertures->ranges[0].size = ggtt->mappable_end;
> >
> >-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
> >-	info->fix.smem_len = size;
> >+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> >+	info->fix.smem_len = vma->node.size;
> >
> >-	info->screen_base =
> >-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> >-			   size);
> >-	if (!info->screen_base) {
> >+	info->screen_base = i915_vma_iomap(vma);
> 
> We are slowly establishing that ERR_PTR should not be stored in
> structure members but I suppose here we can allow it since the
> structure is freed if it fails.

I know, I tell everyone else off for doing it like this as well.
Practice what you preach!
-Chris

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

[PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

By tracking the iomapping on the VMA itself, we can share that area
between multiple users. Also by only revoking the iomapping upon
unbinding from the mappable portion of the GGTT, we can keep that iomap
across multiple invocations (e.g. execlists context pinning).

Note that by moving the iounnmap tracking to the VMA, we actually end up
fixing a leak of the iomapping in intel_fbdev.

v1.5: Rebase prompted by Tvrtko
v2: Drop dev_priv parameter, we can recover the i915_ggtt from the vma.
v3: Move handling of ioremap space exhaustion to vmap_purge and also
allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c          |  2 ++
 drivers/gpu/drm/i915/i915_gem_gtt.c      | 25 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h      | 38 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++++++++++++++++-----
 drivers/gpu/drm/i915/intel_fbdev.c       | 22 +++++++++---------
 5 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b37ffea8b458..6a485630595e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3393,6 +3393,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
 		ret = i915_gem_object_put_fence(obj);
 		if (ret)
 			return ret;
+
+		i915_vma_iounmap(vma);
 	}
 
 	trace_i915_vma_unbind(vma);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c5cb04907525..53e55aead512 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3626,3 +3626,28 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+void *i915_vma_iomap(struct i915_vma *vma)
+{
+	if (WARN_ON(!vma->obj->map_and_fenceable))
+		return ERR_PTR(-ENODEV);
+
+	BUG_ON(!vma->is_ggtt);
+	BUG_ON((vma->bound & GLOBAL_BIND) == 0);
+
+	if (vma->iomap == NULL) {
+		struct i915_ggtt *ggtt =
+			container_of(vma->vm, struct i915_ggtt, base);
+		void *ptr;
+
+		ptr = io_mapping_map_wc(ggtt->mappable,
+					vma->node.start,
+					vma->node.size);
+		if (ptr == NULL)
+			return ERR_PTR(-ENOMEM);
+
+		vma->iomap = ptr;
+	}
+
+	return vma->iomap;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..d95190ddf2d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -34,6 +34,8 @@
 #ifndef __I915_GEM_GTT_H__
 #define __I915_GEM_GTT_H__
 
+#include <linux/io-mapping.h>
+
 struct drm_i915_file_private;
 
 typedef uint32_t gen6_pte_t;
@@ -175,6 +177,7 @@ struct i915_vma {
 	struct drm_mm_node node;
 	struct drm_i915_gem_object *obj;
 	struct i915_address_space *vm;
+	void *iomap;
 
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
@@ -559,4 +562,39 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+/**
+ * i915_vma_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
+ * @vma: VMA to iomap
+ *
+ * The passed in VMA has to be pinned in the global GTT mappable region. Or in
+ * other words callers are responsible for managing the VMA pinned lifetime and
+ * ensuring it covers the use of the returned mapping.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void *i915_vma_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_iounmap - unmaps the mapping returned from i915_vma_iomap
+ * @dev_priv: i915 private pointer
+ * @vma: VMA to unmap
+ *
+ * Unmaps the previously iomapped VMA using iounmap.
+ *
+ * Users of i915_vma_iomap should not manually unmap by calling this function
+ * if they want to take advantage of the mapping getting cached in the VMA.
+ *
+ * Callers must hold the struct_mutex.
+ */
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+	if (vma->iomap == NULL)
+		return;
+
+	io_mapping_unmap(vma->iomap);
+	vma->iomap = NULL;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..908c083a39f1 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,31 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	struct drm_i915_private *dev_priv =
 		container_of(nb, struct drm_i915_private, mm.vmap_notifier);
 	struct shrinker_lock_uninterruptible slu;
-	unsigned long freed_pages;
+	struct i915_vma *vma, *next;
+	unsigned long freed_pages = 0;
+	int ret;
 
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink(dev_priv, -1UL,
-				      I915_SHRINK_BOUND |
-				      I915_SHRINK_UNBOUND |
-				      I915_SHRINK_ACTIVE |
-				      I915_SHRINK_VMAPS);
+	/* Force everything onto the inactive lists */
+	ret = i915_gpu_idle(dev_priv->dev);
+	if (ret)
+		goto out;
 
+	freed_pages += i915_gem_shrink(dev_priv, -1UL,
+				       I915_SHRINK_BOUND |
+				       I915_SHRINK_UNBOUND |
+				       I915_SHRINK_ACTIVE |
+				       I915_SHRINK_VMAPS);
+
+	/* We also want to clear any cached iomaps as they wrap vmap */
+	list_for_each_entry_safe(vma, next,
+				 &dev_priv->ggtt.base.inactive_list, vm_link)
+		if (vma->iomap && i915_vma_unbind(vma) == 0)
+			freed_pages += vma->node.size >> PAGE_SHIFT;
+
+out:
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
 	*(unsigned long *)ptr += freed_pages;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202f3870..3f3c97a30418 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -186,9 +186,11 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
+	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
-	int size, ret;
 	bool prealloc = false;
+	void *vaddr;
+	int ret;
 
 	if (intel_fb &&
 	    (sizes->fb_width > intel_fb->base.width ||
@@ -214,7 +216,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 
 	obj = intel_fb->obj;
-	size = obj->base.size;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &intelfb_ops;
 
+	vma = i915_gem_obj_to_ggtt(obj);
+
 	/* setup aperture base/size for vesafb takeover */
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = ggtt->mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
-	info->fix.smem_len = size;
+	info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
+	info->fix.smem_len = vma->node.size;
 
-	info->screen_base =
-		ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
-	if (!info->screen_base) {
+	vaddr = i915_vma_iomap(vma);
+	if (IS_ERR(vaddr)) {
 		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
-		ret = -ENOSPC;
+		ret = PTR_ERR(vaddr);
 		goto out_destroy_fbi;
 	}
-	info->screen_size = size;
+	info->screen_base = vaddr;
+	info->screen_size = vma->node.size;
 
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
-- 
2.8.0.rc3

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1696 bytes --]

Hi Chris,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160413]
[cannot apply to v4.6-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Move-ioremap_wc-tracking-onto-VMA/20160413-225200
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201615 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_gem_gtt.c: In function 'i915_vma_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3643:9: error: too many arguments to function 'io_mapping_map_wc'
      ptr = io_mapping_map_wc(ggtt->mappable,
            ^
   In file included from drivers/gpu/drm/i915/i915_gem_gtt.h:37:0,
                    from drivers/gpu/drm/i915/i915_drv.h:42,
                    from drivers/gpu/drm/i915/i915_gem_gtt.c:30:
   include/linux/io-mapping.h:158:1: note: declared here
    io_mapping_map_wc(struct io_mapping *mapping, unsigned long offset)
    ^

vim +/io_mapping_map_wc +3643 drivers/gpu/drm/i915/i915_gem_gtt.c

  3637	
  3638		if (vma->iomap == NULL) {
  3639			struct i915_ggtt *ggtt =
  3640				container_of(vma->vm, struct i915_ggtt, base);
  3641			void *ptr;
  3642	
> 3643			ptr = io_mapping_map_wc(ggtt->mappable,
  3644						vma->node.start,
  3645						vma->node.size);
  3646			if (ptr == NULL)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 32481 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> +	/* We also want to clear any cached iomaps as they wrap vmap */
> +	list_for_each_entry_safe(vma, next,
> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> +			freed_pages += vma->node.size >> PAGE_SHIFT;

Use after free. I need to store the page count in a local before calling
unbind.
-Chris

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin]

On 13/04/16 16:12, Chris Wilson wrote:
> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>> +	list_for_each_entry_safe(vma, next,
>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>
> Use after free. I need to store the page count in a local before calling
> unbind.

Waiting for respin. :)

Also, shouldn't the patch which adds the size argument to 
io_mapping_map_wc be in this series?

Regards,

Tvrtko

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> 
> On 13/04/16 16:12, Chris Wilson wrote:
> >On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> >>+	/* We also want to clear any cached iomaps as they wrap vmap */
> >>+	list_for_each_entry_safe(vma, next,
> >>+				 &dev_priv->ggtt.base.inactive_list, vm_link)
> >>+		if (vma->iomap && i915_vma_unbind(vma) == 0)
> >>+			freed_pages += vma->node.size >> PAGE_SHIFT;
> >
> >Use after free. I need to store the page count in a local before calling
> >unbind.
> 
> Waiting for respin. :)
> 
> Also, shouldn't the patch which adds the size argument to
> io_mapping_map_wc be in this series?

It was, this was just an update to patch 2. The delta here is just
unsigned long count = vma->node.size >> PAGE_SHIFT;
if (vma->iomap && i915_vma_unbind(vma) == 0)
	freed_pages += count;
-Chris

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin]

On 15/04/16 11:00, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
>>
>> On 13/04/16 16:12, Chris Wilson wrote:
>>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>>>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>>>> +	list_for_each_entry_safe(vma, next,
>>>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>>>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>>>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>>>
>>> Use after free. I need to store the page count in a local before calling
>>> unbind.
>>
>> Waiting for respin. :)
>>
>> Also, shouldn't the patch which adds the size argument to
>> io_mapping_map_wc be in this series?
>
> It was, this was just an update to patch 2. The delta here is just
> unsigned long count = vma->node.size >> PAGE_SHIFT;
> if (vma->iomap && i915_vma_unbind(vma) == 0)
> 	freed_pages += count;

My bad, I got lost in the threads.. :(

Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs now 
that is in?

Maybe also add a quick return at the top, as a micro-opt:

	if (vma->iomap)
		reutrn vma->iomap;

Followed by WARNs, GEM_BUG_ONs and rest?

Shrinker fix mandatory and with or without my above comments r-b from me.

Regards,

Tvrtko

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Chris Wilson]

On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
> On 15/04/16 11:00, Chris Wilson wrote:
> >On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 13/04/16 16:12, Chris Wilson wrote:
> >>>On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> >>>>+	/* We also want to clear any cached iomaps as they wrap vmap */
> >>>>+	list_for_each_entry_safe(vma, next,
> >>>>+				 &dev_priv->ggtt.base.inactive_list, vm_link)
> >>>>+		if (vma->iomap && i915_vma_unbind(vma) == 0)
> >>>>+			freed_pages += vma->node.size >> PAGE_SHIFT;
> >>>
> >>>Use after free. I need to store the page count in a local before calling
> >>>unbind.
> >>
> >>Waiting for respin. :)
> >>
> >>Also, shouldn't the patch which adds the size argument to
> >>io_mapping_map_wc be in this series?
> >
> >It was, this was just an update to patch 2. The delta here is just
> >unsigned long count = vma->node.size >> PAGE_SHIFT;
> >if (vma->iomap && i915_vma_unbind(vma) == 0)
> >	freed_pages += count;
> 
> My bad, I got lost in the threads.. :(
> 
> Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
> now that is in?
> 
> Maybe also add a quick return at the top, as a micro-opt:
> 
> 	if (vma->iomap)
> 		reutrn vma->iomap;
> 
> Followed by WARNs, GEM_BUG_ONs and rest?

Like so?

void *i915_vma_iomap(struct i915_vma *vma)
{
        struct i915_ggtt *ggtt;
        void *ptr;

        if (vma->iomap)
                return vma->iomap;

        if (WARN_ON(!vma->obj->map_and_fenceable))
                return ERR_PTR(-ENODEV);

        GEM_BUG_ON(!vma->is_ggtt);
        GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);

        ggtt = container_of(vma->vm, struct i915_ggtt, base);
        ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
        if (ptr == NULL)
                return ERR_PTR(-ENOMEM);

        vma->iomap = ptr;
        return ptr;
}
-Chris

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Tvrtko Ursulin]

On 15/04/16 11:38, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
>> On 15/04/16 11:00, Chris Wilson wrote:
>>> On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 13/04/16 16:12, Chris Wilson wrote:
>>>>> On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
>>>>>> +	/* We also want to clear any cached iomaps as they wrap vmap */
>>>>>> +	list_for_each_entry_safe(vma, next,
>>>>>> +				 &dev_priv->ggtt.base.inactive_list, vm_link)
>>>>>> +		if (vma->iomap && i915_vma_unbind(vma) == 0)
>>>>>> +			freed_pages += vma->node.size >> PAGE_SHIFT;
>>>>>
>>>>> Use after free. I need to store the page count in a local before calling
>>>>> unbind.
>>>>
>>>> Waiting for respin. :)
>>>>
>>>> Also, shouldn't the patch which adds the size argument to
>>>> io_mapping_map_wc be in this series?
>>>
>>> It was, this was just an update to patch 2. The delta here is just
>>> unsigned long count = vma->node.size >> PAGE_SHIFT;
>>> if (vma->iomap && i915_vma_unbind(vma) == 0)
>>> 	freed_pages += count;
>>
>> My bad, I got lost in the threads.. :(
>>
>> Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
>> now that is in?
>>
>> Maybe also add a quick return at the top, as a micro-opt:
>>
>> 	if (vma->iomap)
>> 		reutrn vma->iomap;
>>
>> Followed by WARNs, GEM_BUG_ONs and rest?
>
> Like so?
>
> void *i915_vma_iomap(struct i915_vma *vma)
> {
>          struct i915_ggtt *ggtt;
>          void *ptr;
>
>          if (vma->iomap)
>                  return vma->iomap;
>
>          if (WARN_ON(!vma->obj->map_and_fenceable))
>                  return ERR_PTR(-ENODEV);
>
>          GEM_BUG_ON(!vma->is_ggtt);
>          GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
>
>          ggtt = container_of(vma->vm, struct i915_ggtt, base);
>          ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
>          if (ptr == NULL)
>                  return ERR_PTR(-ENOMEM);
>
>          vma->iomap = ptr;
>          return ptr;
> }

Yes, that's what I had in mind. You can put my r-b in the repost.

Regards,

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

Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA

[Joonas Lahtinen]

On pe, 2016-04-15 at 11:38 +0100, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 11:19:30AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 15/04/16 11:00, Chris Wilson wrote:
> > > 
> > > On Fri, Apr 15, 2016 at 10:40:42AM +0100, Tvrtko Ursulin wrote:
> > > > 
> > > > 
> > > > On 13/04/16 16:12, Chris Wilson wrote:
> > > > > 
> > > > > On Wed, Apr 13, 2016 at 03:47:58PM +0100, Chris Wilson wrote:
> > > > > > 
> > > > > > +	/* We also want to clear any cached iomaps as they wrap vmap */
> > > > > > +	list_for_each_entry_safe(vma, next,
> > > > > > +				 &dev_priv->ggtt.base.inactive_list, vm_link)
> > > > > > +		if (vma->iomap && i915_vma_unbind(vma) == 0)
> > > > > > +			freed_pages += vma->node.size >> PAGE_SHIFT;
> > > > > Use after free. I need to store the page count in a local before calling
> > > > > unbind.
> > > > Waiting for respin. :)
> > > > 
> > > > Also, shouldn't the patch which adds the size argument to
> > > > io_mapping_map_wc be in this series?
> > > It was, this was just an update to patch 2. The delta here is just
> > > unsigned long count = vma->node.size >> PAGE_SHIFT;
> > > if (vma->iomap && i915_vma_unbind(vma) == 0)
> > > 	freed_pages += count;
> > My bad, I got lost in the threads.. :(
> > 
> > Could I ask for those two BUG_ONs to be replaced with GEM_BUG_ONs
> > now that is in?
> > 
> > Maybe also add a quick return at the top, as a micro-opt:
> > 
> > 	if (vma->iomap)
> > 		reutrn vma->iomap;
> > 
> > Followed by WARNs, GEM_BUG_ONs and rest?
> Like so?
> 
> void *i915_vma_iomap(struct i915_vma *vma)
> {
>         struct i915_ggtt *ggtt;
>         void *ptr;
> 
>         if (vma->iomap)
>                 return vma->iomap;
> 
>         if (WARN_ON(!vma->obj->map_and_fenceable))
>                 return ERR_PTR(-ENODEV);
> 
>         GEM_BUG_ON(!vma->is_ggtt);
>         GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
> 
>         ggtt = container_of(vma->vm, struct i915_ggtt, base);

We have:

static inline struct i915_hw_ppgtt *
i915_vm_to_ppgtt(struct i915_address_space *vm)

So rather make a function just like it.


>         ptr = io_mapping_map_wc(ggtt->mappable vma->node.start, vma->node.size);
>         if (ptr == NULL)
>                 return ERR_PTR(-ENOMEM);
> 
>         vma->iomap = ptr;
>         return ptr;
> }
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

[Tvrtko Ursulin]

On 13/04/16 10:52, Chris Wilson wrote:
> When setting up the overlay page, we pin it into the GGTT (when using
> virtual addresses) and store the offset as overlay->flip_addr. Rather
> than doing a lookup of the GGTT address everytime, we can use the known
> address instead.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_overlay.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 6694e9230cd5..e487ff18b42f 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -198,7 +198,7 @@ intel_overlay_map_regs(struct intel_overlay *overlay)
>   		regs = (struct overlay_registers __iomem *)overlay->reg_bo->phys_handle->vaddr;
>   	else
>   		regs = io_mapping_map_wc(ggtt->mappable,
> -					 i915_gem_obj_ggtt_offset(overlay->reg_bo));
> +					 overlay->flip_addr);
>
>   	return regs;
>   }
> @@ -1493,7 +1493,7 @@ intel_overlay_map_regs_atomic(struct intel_overlay *overlay)
>   			overlay->reg_bo->phys_handle->vaddr;
>   	else
>   		regs = io_mapping_map_atomic_wc(ggtt->mappable,
> -						i915_gem_obj_ggtt_offset(overlay->reg_bo));
> +						overlay->flip_addr);
>
>   	return regs;
>   }
> @@ -1523,10 +1523,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_handle->vaddr;
> -	else
> -		error->base = i915_gem_obj_ggtt_offset(overlay->reg_bo);
> +	error->base = overlay->flip_addr;
>
>   	regs = intel_overlay_map_regs_atomic(overlay);
>   	if (!regs)
>

I don't know this code, so I explored a bit and the patch looks correct.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)
URL   : https://patchwork.freedesktop.org/series/5649/
State : success

== Summary ==

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

Test drv_hangman:
        Subgroup error-state-basic:
                incomplete -> PASS       (bdw-nuci7)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:162  dwarn:0   dfail:0   fail:1   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:165  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1891/

e26bcbcb66f5fe06d824fd6c2930ea933eaee62d drm-intel-nightly: 2016y-04m-14d-06h-19m-12s UTC integration manifest
3c2d525 drm/i915: Move ioremap_wc tracking onto VMA
4ec9a06 io-mapping: Specify mapping size for io_mapping_map_wc()
0d30c94 drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr

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