* [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr
@ 2016-04-18 11:13 Chris Wilson
2016-04-18 11:14 ` [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc() Chris Wilson
` (5 more replies)
0 siblings, 6 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 11:13 UTC (permalink / raw)
To: intel-gfx
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>
Reviewed-by: 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 bcc3b6a016d8..9746b9841c13 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
@ 2016-04-18 11:14 ` Chris Wilson
2016-04-19 12:02 ` Chris Wilson
2016-04-18 11:14 ` [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt() Chris Wilson
` (4 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 11:14 UTC (permalink / raw)
To: intel-gfx
Cc: Tvrtko Ursulin, netdev, Yishai Hadas, linux-kernel,
Peter Zijlstra (Intel), dri-devel, linux-rdma, Daniel Vetter,
Dan Williams, Ingo Molnar, David Hildenbrand
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 9746b9841c13..0d5a376878d3 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
2016-04-18 11:14 ` [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc() Chris Wilson
@ 2016-04-18 11:14 ` Chris Wilson
2016-04-18 12:01 ` Tvrtko Ursulin
2016-04-18 13:37 ` Joonas Lahtinen
2016-04-18 11:14 ` [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA Chris Wilson
` (3 subsequent siblings)
5 siblings, 2 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 11:14 UTC (permalink / raw)
To: intel-gfx
In a couple of places, we have an i915_address_space that we know is
really an i915_ggtt that we want to use. Create an inline helper to
convert from the i915_address_space subclass into its container.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9f165feb54ae..b3af2e808b49 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -93,6 +93,13 @@
*
*/
+static inline struct i915_ggtt *
+i915_vm_to_ggtt(struct i915_address_space *vm)
+{
+ GEM_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);
@@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
enum i915_cache_level level, u32 unused)
{
struct drm_i915_private *dev_priv = to_i915(vm->dev);
- struct i915_ggtt *ggtt = &dev_priv->ggtt;
+ struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
unsigned first_entry = start >> PAGE_SHIFT;
gen8_pte_t __iomem *gtt_entries =
(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
@@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
enum i915_cache_level level, u32 flags)
{
struct drm_i915_private *dev_priv = to_i915(vm->dev);
- struct i915_ggtt *ggtt = &dev_priv->ggtt;
+ struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
unsigned first_entry = start >> PAGE_SHIFT;
gen6_pte_t __iomem *gtt_entries =
(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
@@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
bool use_scratch)
{
struct drm_i915_private *dev_priv = to_i915(vm->dev);
- struct i915_ggtt *ggtt = &dev_priv->ggtt;
+ struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
unsigned first_entry = start >> PAGE_SHIFT;
unsigned num_entries = length >> PAGE_SHIFT;
gen8_pte_t scratch_pte, __iomem *gtt_base =
@@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
bool use_scratch)
{
struct drm_i915_private *dev_priv = to_i915(vm->dev);
- struct i915_ggtt *ggtt = &dev_priv->ggtt;
+ struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
unsigned first_entry = start >> PAGE_SHIFT;
unsigned num_entries = length >> PAGE_SHIFT;
gen6_pte_t scratch_pte, __iomem *gtt_base =
--
2.8.0.rc3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
2016-04-18 11:14 ` [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc() Chris Wilson
2016-04-18 11:14 ` [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt() Chris Wilson
@ 2016-04-18 11:14 ` Chris Wilson
2016-04-18 11:56 ` Chris Wilson
2016-04-18 12:03 ` [PATCH v2 4/4] " Tvrtko Ursulin
2016-04-18 13:26 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Patchwork
` (2 subsequent siblings)
5 siblings, 2 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 11:14 UTC (permalink / raw)
To: intel-gfx
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.
v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
v5: Back to i915_vm_to_ggtt
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 ++
drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_gtt.h | 38 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 ++++++++++++++++++-----
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 6ce2c31b9a81..82d0af3f1692 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3378,6 +3378,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 b3af2e808b49..c894af7e0ea1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3634,3 +3634,26 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
return obj->base.size;
}
}
+
+void *i915_vma_iomap(struct i915_vma *vma)
+{
+ 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);
+
+ ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+ vma->node.start,
+ vma->node.size);
+ if (ptr == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ vma->iomap = ptr;
+ return ptr;
+}
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..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ 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) {
+ unsigned long count = vma->node.size >> PAGE_SHIFT;
+ if (vma->iomap && i915_vma_unbind(vma) == 0)
+ freed_pages += count;
+ }
+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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 11:14 ` [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA Chris Wilson
@ 2016-04-18 11:56 ` Chris Wilson
2016-04-18 12:08 ` Tvrtko Ursulin
2016-04-18 12:03 ` [PATCH v2 4/4] " Tvrtko Ursulin
1 sibling, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 11:56 UTC (permalink / raw)
To: intel-gfx
On Mon, Apr 18, 2016 at 12:14:02PM +0100, 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.
> v3: Move handling of ioremap space exhaustion to vmap_purge and also
> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
> v5: Back to i915_vm_to_ggtt
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 ++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 38 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 ++++++++++++++++++-----
> 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 6ce2c31b9a81..82d0af3f1692 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3378,6 +3378,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 b3af2e808b49..c894af7e0ea1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,26 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> return obj->base.size;
> }
> }
> +
> +void *i915_vma_iomap(struct i915_vma *vma)
I've been arguing with myself as to whether to make this
i915_vma_pin_iomap (and so acquire the vma->pin_count as well).
There will still be the requirement that the vma is already usuable, but
it will mean that the returned iomap cannot be reaped for the critical
section of its user.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()
2016-04-18 11:14 ` [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt() Chris Wilson
@ 2016-04-18 12:01 ` Tvrtko Ursulin
2016-04-18 13:37 ` Joonas Lahtinen
1 sibling, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-18 12:01 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 18/04/16 12:14, Chris Wilson wrote:
> In a couple of places, we have an i915_address_space that we know is
> really an i915_ggtt that we want to use. Create an inline helper to
> convert from the i915_address_space subclass into its container.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9f165feb54ae..b3af2e808b49 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,13 @@
> *
> */
>
> +static inline struct i915_ggtt *
> +i915_vm_to_ggtt(struct i915_address_space *vm)
> +{
> + GEM_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);
>
> @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> enum i915_cache_level level, u32 unused)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> gen8_pte_t __iomem *gtt_entries =
> (gen8_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
> enum i915_cache_level level, u32 flags)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> gen6_pte_t __iomem *gtt_entries =
> (gen6_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
> bool use_scratch)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> unsigned num_entries = length >> PAGE_SHIFT;
> gen8_pte_t scratch_pte, __iomem *gtt_base =
> @@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> bool use_scratch)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> unsigned num_entries = length >> PAGE_SHIFT;
> gen6_pte_t scratch_pte, __iomem *gtt_base =
>
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 11:14 ` [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA Chris Wilson
2016-04-18 11:56 ` Chris Wilson
@ 2016-04-18 12:03 ` Tvrtko Ursulin
1 sibling, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-18 12:03 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 18/04/16 12:14, 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.
> v3: Move handling of ioremap space exhaustion to vmap_purge and also
> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
> v5: Back to i915_vm_to_ggtt
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 ++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 38 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 ++++++++++++++++++-----
> drivers/gpu/drm/i915/intel_fbdev.c | 22 +++++++++---------
> 5 files changed, 97 insertions(+), 16 deletions(-)
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..82d0af3f1692 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3378,6 +3378,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 b3af2e808b49..c894af7e0ea1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,26 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> return obj->base.size;
> }
> }
> +
> +void *i915_vma_iomap(struct i915_vma *vma)
> +{
> + 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);
> +
> + ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> + vma->node.start,
> + vma->node.size);
> + if (ptr == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + vma->iomap = ptr;
> + return ptr;
> +}
> 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..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ 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) {
> + unsigned long count = vma->node.size >> PAGE_SHIFT;
> + if (vma->iomap && i915_vma_unbind(vma) == 0)
> + freed_pages += count;
> + }
>
> +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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 11:56 ` Chris Wilson
@ 2016-04-18 12:08 ` Tvrtko Ursulin
2016-04-18 14:54 ` [PATCH] " Chris Wilson
0 siblings, 1 reply; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-18 12:08 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Tvrtko Ursulin, Joonas Lahtinen
On 18/04/16 12:56, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:14:02PM +0100, 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.
>> v3: Move handling of ioremap space exhaustion to vmap_purge and also
>> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
>> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
>> v5: Back to i915_vm_to_ggtt
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 2 ++
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++++++++++++++++
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 38 ++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 ++++++++++++++++++-----
>> 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 6ce2c31b9a81..82d0af3f1692 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3378,6 +3378,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 b3af2e808b49..c894af7e0ea1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -3634,3 +3634,26 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>> return obj->base.size;
>> }
>> }
>> +
>> +void *i915_vma_iomap(struct i915_vma *vma)
>
> I've been arguing with myself as to whether to make this
> i915_vma_pin_iomap (and so acquire the vma->pin_count as well).
>
> There will still be the requirement that the vma is already usuable, but
> it will mean that the returned iomap cannot be reaped for the critical
> section of its user.
I don't have a strong feeling either way. vma_pin_iomap would make the
name (and usage model) more consistent with obj_pin_map and taking the
pin count could potentially make it a little bit safer for the users
where it would leak rather than crash. So maybe on the basis of those it
would be a tiny bit better.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
` (2 preceding siblings ...)
2016-04-18 11:14 ` [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA Chris Wilson
@ 2016-04-18 13:26 ` Patchwork
2016-04-18 15:29 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2) Patchwork
2016-04-19 11:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3) Patchwork
5 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2016-04-18 13:26 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr
URL : https://patchwork.freedesktop.org/series/5865/
State : warning
== Summary ==
Series 5865v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/1/mbox/
Test gem_exec_whisper:
Subgroup basic:
incomplete -> PASS (bdw-nuci7)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-b:
pass -> DMESG-WARN (snb-dellxps)
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:163 dwarn:0 dfail:0 fail:0 skip:39
byt-nuc total:202 pass:164 dwarn:0 dfail:0 fail:0 skip:38
hsw-gt2 total:203 pass:184 dwarn:0 dfail:0 fail:0 skip:19
ilk-hp8440p total:203 pass:135 dwarn:0 dfail:0 fail:0 skip:68
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:164 dwarn:1 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_1926/
78673b24b60c1a884c947ee5a45ad860ca618418 drm-intel-nightly: 2016y-04m-18d-12h-32m-33s UTC integration manifest
3888a6c drm/i915: Move ioremap_wc tracking onto VMA
1714d0a drm/i915: Introduce i915_vm_to_ggtt()
959fab4 io-mapping: Specify mapping size for io_mapping_map_wc()
ec6a39c 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()
2016-04-18 11:14 ` [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt() Chris Wilson
2016-04-18 12:01 ` Tvrtko Ursulin
@ 2016-04-18 13:37 ` Joonas Lahtinen
2016-04-18 14:02 ` Chris Wilson
1 sibling, 1 reply; 29+ messages in thread
From: Joonas Lahtinen @ 2016-04-18 13:37 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On ma, 2016-04-18 at 12:14 +0100, Chris Wilson wrote:
> In a couple of places, we have an i915_address_space that we know is
> really an i915_ggtt that we want to use. Create an inline helper to
> convert from the i915_address_space subclass into its container.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Comment below.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9f165feb54ae..b3af2e808b49 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -93,6 +93,13 @@
> *
> */
>
> +static inline struct i915_ggtt *
> +i915_vm_to_ggtt(struct i915_address_space *vm)
> +{
> + GEM_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);
>
> @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> enum i915_cache_level level, u32 unused)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
I do not see this as a very necessary change, because we are going to
have only one Global XYZ per device by definiton of "global".
Especially after the rename it's going to be;
struct i915_ggtt *ggtt = &i915->ggtt;
But regardless my R-b.
Regards, Joonas
> unsigned first_entry = start >> PAGE_SHIFT;
> gen8_pte_t __iomem *gtt_entries =
> (gen8_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2437,7 +2444,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
> enum i915_cache_level level, u32 flags)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> gen6_pte_t __iomem *gtt_entries =
> (gen6_pte_t __iomem *)ggtt->gsm + first_entry;
> @@ -2481,7 +2488,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
> bool use_scratch)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> unsigned num_entries = length >> PAGE_SHIFT;
> gen8_pte_t scratch_pte, __iomem *gtt_base =
> @@ -2513,7 +2520,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> bool use_scratch)
> {
> struct drm_i915_private *dev_priv = to_i915(vm->dev);
> - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> unsigned first_entry = start >> PAGE_SHIFT;
> unsigned num_entries = length >> PAGE_SHIFT;
> gen6_pte_t scratch_pte, __iomem *gtt_base =
--
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt()
2016-04-18 13:37 ` Joonas Lahtinen
@ 2016-04-18 14:02 ` Chris Wilson
0 siblings, 0 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 14:02 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Mon, Apr 18, 2016 at 04:37:12PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-04-18 at 12:14 +0100, Chris Wilson wrote:
> > In a couple of places, we have an i915_address_space that we know is
> > really an i915_ggtt that we want to use. Create an inline helper to
> > convert from the i915_address_space subclass into its container.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> Comment below.
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 9f165feb54ae..b3af2e808b49 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -93,6 +93,13 @@
> > *
> > */
> >
> > +static inline struct i915_ggtt *
> > +i915_vm_to_ggtt(struct i915_address_space *vm)
> > +{
> > + GEM_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);
> >
> > @@ -2359,7 +2366,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> > enum i915_cache_level level, u32 unused)
> > {
> > struct drm_i915_private *dev_priv = to_i915(vm->dev);
> > - struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > + struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
>
> I do not see this as a very necessary change, because we are going to
> have only one Global XYZ per device by definiton of "global".
I ultimately want to remove the dev_priv usage here. But it is also
useful for consistency with a couple of other ggtt PTE routines that do
not need the drm_i915_private pointer at all, that aren't yet upstream.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 12:08 ` Tvrtko Ursulin
@ 2016-04-18 14:54 ` Chris Wilson
2016-04-18 15:08 ` Tvrtko Ursulin
2016-04-18 15:20 ` kbuild test robot
0 siblings, 2 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 14:54 UTC (permalink / raw)
To: intel-gfx
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.
v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
v5: Back to i915_vm_to_ggtt
v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
sections and ensure the VMA cannot be reaped whilst mapped.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 ++
drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++
drivers/gpu/drm/i915/i915_gem_gtt.h | 58 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++----
drivers/gpu/drm/i915/intel_fbdev.c | 22 ++++++------
5 files changed, 120 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31b9a81..82d0af3f1692 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3378,6 +3378,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 b3af2e808b49..e185dff358e1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
return obj->base.size;
}
}
+
+void *i915_vma_pin_iomap(struct i915_vma *vma)
+{
+ void *ptr;
+
+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
+ 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);
+
+ ptr = vma->iomap;
+ if (ptr == NULL) {
+ ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+ vma->node.start,
+ vma->node.size);
+ if (ptr == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ vma->iomap = ptr;
+ }
+
+ vma->pin_count++;
+ return ptr;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..a01d78bc9f24 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,59 @@ size_t
i915_ggtt_view_size(struct drm_i915_gem_object *obj,
const struct i915_ggtt_view *view);
+/**
+ * i915_vma_pin_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.
+ * An extra pinning of the VMA is acquired for the return iomapping,
+ * the caller must call i915_vma_unpin_iomap to relinquish the pinning
+ * after the iomapping is no longer required.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void *i915_vma_pin_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_iounmap - unmaps the VMA
+ * @vma: VMA to unmap
+ *
+ * Unmaps the previously iomapped VMA using iounmap.
+ *
+ * Users of i915_vma_pin_iomap() should not manually unmap by calling this
+ * function but should call i915_vma_unpin_iomap() instead.
+ *
+ * Callers must hold the struct_mutex.
+ */
+static inline void i915_vma_iounmap(struct i915_vma *vma)
+{
+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
+ GEM_BUG_ON(vma->pin_count);
+
+ if (vma->iomap == NULL)
+ return;
+
+ io_mapping_unmap(vma->iomap);
+ vma->iomap = NULL;
+}
+
+/**
+ * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
+ * @vma: VMA to unpin
+ *
+ * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
+ *
+ * Callers must hold the struct_mutex. This function is only valid to be
+ * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
+ */
+static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
+{
+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
+ GEM_BUG_ON(vma->pin_count == 0);
+ GEM_BUG_ON(vma->iomap == NULL);
+ vma->pin_count--;
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ 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) {
+ unsigned long count = vma->node.size >> PAGE_SHIFT;
+ if (vma->iomap && i915_vma_unbind(vma) == 0)
+ freed_pages += count;
+ }
+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..93f54a10042f 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_pin_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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 14:54 ` [PATCH] " Chris Wilson
@ 2016-04-18 15:08 ` Tvrtko Ursulin
2016-04-18 15:22 ` Chris Wilson
2016-04-18 15:20 ` kbuild test robot
1 sibling, 1 reply; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-18 15:08 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 18/04/16 15:54, 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.
> v3: Move handling of ioremap space exhaustion to vmap_purge and also
> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
> v5: Back to i915_vm_to_ggtt
> v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
> sections and ensure the VMA cannot be reaped whilst mapped.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 ++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 58 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++----
> drivers/gpu/drm/i915/intel_fbdev.c | 22 ++++++------
> 5 files changed, 120 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..82d0af3f1692 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3378,6 +3378,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 b3af2e808b49..e185dff358e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> return obj->base.size;
> }
> }
> +
> +void *i915_vma_pin_iomap(struct i915_vma *vma)
> +{
> + void *ptr;
Just realized we should mark the pointers with __iomem here.
> +
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + 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);
> +
> + ptr = vma->iomap;
> + if (ptr == NULL) {
> + ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> + vma->node.start,
> + vma->node.size);
> + if (ptr == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + vma->iomap = ptr;
> + }
> +
> + vma->pin_count++;
> + return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..a01d78bc9f24 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,59 @@ size_t
> i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_pin_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.
> + * An extra pinning of the VMA is acquired for the return iomapping,
> + * the caller must call i915_vma_unpin_iomap to relinquish the pinning
> + * after the iomapping is no longer required.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_iounmap - unmaps the VMA
> + * @vma: VMA to unmap
> + *
> + * Unmaps the previously iomapped VMA using iounmap.
> + *
> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> + * function but should call i915_vma_unpin_iomap() instead.
> + *
> + * Callers must hold the struct_mutex.
> + */
> +static inline void i915_vma_iounmap(struct i915_vma *vma)
> +{
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + GEM_BUG_ON(vma->pin_count);
> +
> + if (vma->iomap == NULL)
> + return;
> +
> + io_mapping_unmap(vma->iomap);
> + vma->iomap = NULL;
> +}
It would be best to hide this near the unbind code. Alternatively maybe
put a louder warning in the kerneldoc? Just so it is a bit more
distinguishable from the GEM exported API for this feature.
> +
> +/**
> + * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
> + * @vma: VMA to unpin
> + *
> + * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
> + *
> + * Callers must hold the struct_mutex. This function is only valid to be
> + * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
> + */
> +static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
> +{
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + GEM_BUG_ON(vma->pin_count == 0);
> + GEM_BUG_ON(vma->iomap == NULL);
> + vma->pin_count--;
> +}
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ 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) {
> + unsigned long count = vma->node.size >> PAGE_SHIFT;
> + if (vma->iomap && i915_vma_unbind(vma) == 0)
> + freed_pages += count;
> + }
>
> +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..93f54a10042f 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_pin_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;
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 14:54 ` [PATCH] " Chris Wilson
2016-04-18 15:08 ` Tvrtko Ursulin
@ 2016-04-18 15:20 ` kbuild test robot
1 sibling, 0 replies; 29+ messages in thread
From: kbuild test robot @ 2016-04-18 15:20 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]
Hi Chris,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160418]
[cannot apply to v4.6-rc4]
[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/20160418-225730
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201616 (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_pin_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:27: error: implicit declaration of function 'i915_vm_to_ggtt' [-Werror=implicit-function-declaration]
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:51: error: invalid type argument of '->' (have 'int')
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
drivers/gpu/drm/i915/i915_gem_gtt.c:3644:9: error: too many arguments to function 'io_mapping_map_wc'
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
In file included from drivers/gpu/drm/i915/i915_drv.h:36:0,
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)
^
cc1: some warnings being treated as errors
vim +/i915_vm_to_ggtt +3644 drivers/gpu/drm/i915/i915_gem_gtt.c
3638
3639 GEM_BUG_ON(!vma->is_ggtt);
3640 GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
3641
3642 ptr = vma->iomap;
3643 if (ptr == NULL) {
> 3644 ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
3645 vma->node.start,
3646 vma->node.size);
3647 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: 25849 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 15:08 ` Tvrtko Ursulin
@ 2016-04-18 15:22 ` Chris Wilson
2016-04-18 15:27 ` Tvrtko Ursulin
0 siblings, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2016-04-18 15:22 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
> On 18/04/16 15:54, Chris Wilson wrote:
> >@@ -3378,6 +3378,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 b3af2e808b49..e185dff358e1 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> > return obj->base.size;
> > }
> > }
> >+
> >+void *i915_vma_pin_iomap(struct i915_vma *vma)
> >+{
> >+ void *ptr;
>
> Just realized we should mark the pointers with __iomem here.
The problem is that we need to lose the annotation at some point as we
mix access via ring->virtual_start. For correctness, we should report
__iomem here and discard it later.
> >+/**
> >+ * i915_vma_iounmap - unmaps the VMA
> >+ * @vma: VMA to unmap
> >+ *
> >+ * Unmaps the previously iomapped VMA using iounmap.
> >+ *
> >+ * Users of i915_vma_pin_iomap() should not manually unmap by calling this
> >+ * function but should call i915_vma_unpin_iomap() instead.
> >+ *
> >+ * Callers must hold the struct_mutex.
> >+ */
> >+static inline void i915_vma_iounmap(struct i915_vma *vma)
> >+{
> >+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
> >+ GEM_BUG_ON(vma->pin_count);
> >+
> >+ if (vma->iomap == NULL)
> >+ return;
> >+
> >+ io_mapping_unmap(vma->iomap);
> >+ vma->iomap = NULL;
> >+}
>
> It would be best to hide this near the unbind code. Alternatively
> maybe put a louder warning in the kerneldoc? Just so it is a bit
> more distinguishable from the GEM exported API for this feature.
I had the same thought, then decided to stick with the
i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 15:22 ` Chris Wilson
@ 2016-04-18 15:27 ` Tvrtko Ursulin
2016-04-19 11:06 ` Chris Wilson
0 siblings, 1 reply; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-18 15:27 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 18/04/16 16:22, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 04:08:55PM +0100, Tvrtko Ursulin wrote:
>> On 18/04/16 15:54, Chris Wilson wrote:
>>> @@ -3378,6 +3378,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 b3af2e808b49..e185dff358e1 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>> return obj->base.size;
>>> }
>>> }
>>> +
>>> +void *i915_vma_pin_iomap(struct i915_vma *vma)
>>> +{
>>> + void *ptr;
>>
>> Just realized we should mark the pointers with __iomem here.
>
> The problem is that we need to lose the annotation at some point as we
> mix access via ring->virtual_start. For correctness, we should report
> __iomem here and discard it later.
>
>>> +/**
>>> + * i915_vma_iounmap - unmaps the VMA
>>> + * @vma: VMA to unmap
>>> + *
>>> + * Unmaps the previously iomapped VMA using iounmap.
>>> + *
>>> + * Users of i915_vma_pin_iomap() should not manually unmap by calling this
>>> + * function but should call i915_vma_unpin_iomap() instead.
>>> + *
>>> + * Callers must hold the struct_mutex.
>>> + */
>>> +static inline void i915_vma_iounmap(struct i915_vma *vma)
>>> +{
>>> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
>>> + GEM_BUG_ON(vma->pin_count);
>>> +
>>> + if (vma->iomap == NULL)
>>> + return;
>>> +
>>> + io_mapping_unmap(vma->iomap);
>>> + vma->iomap = NULL;
>>> +}
>>
>> It would be best to hide this near the unbind code. Alternatively
>> maybe put a louder warning in the kerneldoc? Just so it is a bit
>> more distinguishable from the GEM exported API for this feature.
>
> I had the same thought, then decided to stick with the
> i915_vma_iounmap() call in i915_vma_unbind(). Maybe __i915_vma_iounmap?
__i915_vma_ionmap in i915_gem_gtt.h or in i915_gem.c ?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
` (3 preceding siblings ...)
2016-04-18 13:26 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Patchwork
@ 2016-04-18 15:29 ` Patchwork
2016-04-19 11:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3) Patchwork
5 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2016-04-18 15:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2)
URL : https://patchwork.freedesktop.org/series/5865/
State : warning
== Summary ==
Series 5865v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/2/mbox/
Test drv_module_reload_basic:
pass -> DMESG-WARN (skl-nuci5)
Test gem_busy:
Subgroup basic-blt:
pass -> SKIP (bsw-nuc-2)
Test gem_exec_whisper:
Subgroup basic:
incomplete -> PASS (bdw-nuci7)
Test kms_force_connector_basic:
Subgroup prune-stale-modes:
pass -> SKIP (snb-x220t)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-c:
pass -> DMESG-WARN (ivb-t430s)
Subgroup suspend-read-crc-pipe-c:
pass -> DMESG-WARN (skl-nuci5)
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:0 skip:40
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
hsw-gt2 total:203 pass:184 dwarn:0 dfail:0 fail:0 skip:19
ivb-t430s total:203 pass:174 dwarn:1 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:190 dwarn:2 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:164 dwarn:0 dfail:0 fail:1 skip:38
Results at /archive/results/CI_IGT_test/Patchwork_1929/
78673b24b60c1a884c947ee5a45ad860ca618418 drm-intel-nightly: 2016y-04m-18d-12h-32m-33s UTC integration manifest
d352727 drm/i915: Move ioremap_wc tracking onto VMA
0c66e48 drm/i915: Introduce i915_vm_to_ggtt()
e3f5de7 io-mapping: Specify mapping size for io_mapping_map_wc()
96da749 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-18 15:27 ` Tvrtko Ursulin
@ 2016-04-19 11:06 ` Chris Wilson
2016-04-19 12:19 ` Tvrtko Ursulin
2016-04-19 17:33 ` kbuild test robot
0 siblings, 2 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-19 11:06 UTC (permalink / raw)
To: intel-gfx, Tvrtko Ursulin
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.
v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
v5: Back to i915_vm_to_ggtt
v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
sections and ensure the VMA cannot be reaped whilst mapped.
v7: Move i915_vma_iounmap so that consumers of the API are not tempted,
and add iomem annotations
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++++
drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_gtt.h | 35 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++++++++++------
drivers/gpu/drm/i915/intel_fbdev.c | 22 +++++++++++---------
5 files changed, 106 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31b9a81..9ef47329e8ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
old_write_domain);
}
+static void __i915_vma_iounmap(struct i915_vma *vma)
+{
+ if (vma->iomap == NULL)
+ return;
+
+ io_mapping_unmap(vma->iomap);
+ vma->iomap = NULL;
+}
+
static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
{
struct drm_i915_gem_object *obj = vma->obj;
@@ -3378,6 +3387,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 b3af2e808b49..9d4293f247fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
return obj->base.size;
}
}
+
+void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
+{
+ void __iomem *ptr;
+
+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
+ 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);
+
+ ptr = vma->iomap;
+ if (ptr == NULL) {
+ ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+ vma->node.start,
+ vma->node.size);
+ if (ptr == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ vma->iomap = ptr;
+ }
+
+ vma->pin_count++;
+ return ptr;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8a8758..b0ae6632c01a 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 __iomem *iomap;
/** Flags and address space this VMA is bound to */
#define GLOBAL_BIND (1<<0)
@@ -559,4 +562,36 @@ size_t
i915_ggtt_view_size(struct drm_i915_gem_object *obj,
const struct i915_ggtt_view *view);
+/**
+ * i915_vma_pin_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.
+ * An extra pinning of the VMA is acquired for the return iomapping,
+ * the caller must call i915_vma_unpin_iomap to relinquish the pinning
+ * after the iomapping is no longer required.
+ *
+ * Callers must hold the struct_mutex.
+ *
+ * Returns a valid iomapped pointer or ERR_PTR.
+ */
+void __iomem *i915_vma_pin_iomap(struct i915_vma *vma);
+
+/**
+ * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
+ * @vma: VMA to unpin
+ *
+ * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
+ *
+ * Callers must hold the struct_mutex. This function is only valid to be
+ * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
+ */
+static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
+{
+ lockdep_assert_held(&vma->vm->dev->struct_mutex);
+ GEM_BUG_ON(vma->pin_count == 0);
+ GEM_BUG_ON(vma->iomap == NULL);
+ vma->pin_count--;
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d46388f25e04..6156ee96f429 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -387,17 +387,33 @@ 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) {
+ unsigned long count = vma->node.size >> PAGE_SHIFT;
+ if (vma->iomap && i915_vma_unbind(vma) == 0)
+ freed_pages += count;
+ }
+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..93f54a10042f 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_pin_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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3)
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
` (4 preceding siblings ...)
2016-04-18 15:29 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2) Patchwork
@ 2016-04-19 11:57 ` Patchwork
5 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2016-04-19 11:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3)
URL : https://patchwork.freedesktop.org/series/5865/
State : failure
== Summary ==
Series 5865v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5865/revisions/3/mbox/
Test gem_busy:
Subgroup basic-blt:
skip -> PASS (bsw-nuc-2)
Test kms_force_connector_basic:
Subgroup force-load-detect:
pass -> SKIP (ivb-t430s)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-b:
pass -> INCOMPLETE (snb-dellxps)
Subgroup read-crc-pipe-a-frame-sequence:
fail -> PASS (ivb-t430s)
bdw-nuci7 total:192 pass:180 dwarn:0 dfail:0 fail:0 skip:12
bdw-ultra total:127 pass:105 dwarn:0 dfail:0 fail:0 skip:22
bsw-nuc-2 total:191 pass:152 dwarn:0 dfail:0 fail:0 skip:39
byt-nuc total:191 pass:153 dwarn:0 dfail:0 fail:0 skip:38
hsw-brixbox total:192 pass:168 dwarn:0 dfail:0 fail:0 skip:24
hsw-gt2 total:192 pass:173 dwarn:0 dfail:0 fail:0 skip:19
ivb-t430s total:192 pass:163 dwarn:0 dfail:0 fail:0 skip:29
skl-i7k-2 total:192 pass:167 dwarn:0 dfail:0 fail:0 skip:25
skl-nuci5 total:192 pass:181 dwarn:0 dfail:0 fail:0 skip:11
snb-dellxps total:11 pass:7 dwarn:0 dfail:0 fail:0 skip:3
snb-x220t total:192 pass:154 dwarn:0 dfail:0 fail:1 skip:37
Results at /archive/results/CI_IGT_test/Patchwork_1939/
c438a31de88e7dc6bfe17ed0e094f9a1b08bcb94 drm-intel-nightly: 2016y-04m-19d-09h-36m-00s UTC integration manifest
c0bc8687 drm/i915: Move ioremap_wc tracking onto VMA
7b43c8e drm/i915: Introduce i915_vm_to_ggtt()
870b4c31 io-mapping: Specify mapping size for io_mapping_map_wc()
4c789fb 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()
2016-04-18 11:14 ` [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc() Chris Wilson
@ 2016-04-19 12:02 ` Chris Wilson
2016-04-19 12:30 ` Luis R. Rodriguez
0 siblings, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2016-04-19 12:02 UTC (permalink / raw)
To: intel-gfx
Cc: Tvrtko Ursulin, Daniel Vetter, Jani Nikula, David Airlie,
Yishai Hadas, Dan Williams, Ingo Molnar, Peter Zijlstra (Intel),
David Hildenbrand, Luis R. Rodriguez, dri-devel, netdev,
linux-rdma, linux-kernel
On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> 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.
Adding Luis Rodriguez as he has been active in the area of ioremap_*().
> 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: Luis R. Rodriguez <mcgrof@kernel.org>
> 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 9746b9841c13..0d5a376878d3 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;
> }
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-19 11:06 ` Chris Wilson
@ 2016-04-19 12:19 ` Tvrtko Ursulin
2016-04-19 17:33 ` kbuild test robot
1 sibling, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2016-04-19 12:19 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 19/04/16 12:06, 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.
> v3: Move handling of ioremap space exhaustion to vmap_purge and also
> allow vmallocs to recover old iomaps. Add Tvrtko's kerneldoc.
> v4: Fix a use-after-free in shrinker and rearrange i915_vma_iomap
> v5: Back to i915_vm_to_ggtt
> v6: Use i915_vma_pin_iomap and i915_vma_unpin_iomap to mark critical
> sections and ensure the VMA cannot be reaped whilst mapped.
> v7: Move i915_vma_iounmap so that consumers of the API are not tempted,
> and add iomem annotations
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v4
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 35 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 28 +++++++++++++++++++------
> drivers/gpu/drm/i915/intel_fbdev.c | 22 +++++++++++---------
> 5 files changed, 106 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
> old_write_domain);
> }
>
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> + if (vma->iomap == NULL)
> + return;
> +
> + io_mapping_unmap(vma->iomap);
> + vma->iomap = NULL;
> +}
> +
> static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
> {
> struct drm_i915_gem_object *obj = vma->obj;
> @@ -3378,6 +3387,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 b3af2e808b49..9d4293f247fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3634,3 +3634,29 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> return obj->base.size;
> }
> }
> +
> +void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
> +{
> + void __iomem *ptr;
> +
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + 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);
> +
> + ptr = vma->iomap;
> + if (ptr == NULL) {
> + ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> + vma->node.start,
> + vma->node.size);
> + if (ptr == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + vma->iomap = ptr;
> + }
> +
> + vma->pin_count++;
> + return ptr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..b0ae6632c01a 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 __iomem *iomap;
>
> /** Flags and address space this VMA is bound to */
> #define GLOBAL_BIND (1<<0)
> @@ -559,4 +562,36 @@ size_t
> i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view);
>
> +/**
> + * i915_vma_pin_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.
> + * An extra pinning of the VMA is acquired for the return iomapping,
> + * the caller must call i915_vma_unpin_iomap to relinquish the pinning
> + * after the iomapping is no longer required.
> + *
> + * Callers must hold the struct_mutex.
> + *
> + * Returns a valid iomapped pointer or ERR_PTR.
> + */
> +void __iomem *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +/**
> + * i915_vma_unpin_iomap - unpins the mapping returned from i915_vma_iomap
> + * @vma: VMA to unpin
> + *
> + * Unpins the previously iomapped VMA from i915_vma_pin_iomap().
> + *
> + * Callers must hold the struct_mutex. This function is only valid to be
> + * called on a VMA previously iomapped by the caller with i915_vma_pin_iomap().
> + */
> +static inline void i915_vma_unpin_iomap(struct i915_vma *vma)
> +{
> + lockdep_assert_held(&vma->vm->dev->struct_mutex);
> + GEM_BUG_ON(vma->pin_count == 0);
> + GEM_BUG_ON(vma->iomap == NULL);
> + vma->pin_count--;
> +}
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> index d46388f25e04..6156ee96f429 100644
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -387,17 +387,33 @@ 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) {
> + unsigned long count = vma->node.size >> PAGE_SHIFT;
> + if (vma->iomap && i915_vma_unbind(vma) == 0)
> + freed_pages += count;
> + }
>
> +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..93f54a10042f 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_pin_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;
>
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()
2016-04-19 12:02 ` Chris Wilson
@ 2016-04-19 12:30 ` Luis R. Rodriguez
2016-04-19 12:34 ` Chris Wilson
[not found] ` <1461069238-31539-1-git-send-email-chris@chris-wilson.co.uk>
0 siblings, 2 replies; 29+ messages in thread
From: Luis R. Rodriguez @ 2016-04-19 12:30 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Tvrtko Ursulin, Daniel Vetter,
Jani Nikula, David Airlie, Yishai Hadas, Dan Williams,
Ingo Molnar, Peter Zijlstra (Intel), David Hildenbrand,
Luis R. Rodriguez, dri-devel, netdev, linux-rdma, linux-kernel
On Tue, Apr 19, 2016 at 01:02:25PM +0100, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> > 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.
>
> Adding Luis Rodriguez as he has been active in the area of ioremap_*().
Can someone bounce me a copy of the original e-mails? Can you also use
mcgrof@kernel.org ?
Luis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()
2016-04-19 12:30 ` Luis R. Rodriguez
@ 2016-04-19 12:34 ` Chris Wilson
[not found] ` <1461069238-31539-1-git-send-email-chris@chris-wilson.co.uk>
1 sibling, 0 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-19 12:34 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: David Airlie, netdev, intel-gfx, linux-kernel, Ingo Molnar,
Peter Zijlstra (Intel), dri-devel, linux-rdma, Daniel Vetter,
Dan Williams, Yishai Hadas, David Hildenbrand
On Tue, Apr 19, 2016 at 02:30:28PM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 19, 2016 at 01:02:25PM +0100, Chris Wilson wrote:
> > On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> > > 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.
> >
> > Adding Luis Rodriguez as he has been active in the area of ioremap_*().
>
> Can someone bounce me a copy of the original e-mails? Can you also use
> mcgrof@kernel.org ?
Hopefully done. Please ping me if they don't arrive, or have a look at
https://cgit.freedesktop.org/~ickle/linux-2.6/log/?h=iomap
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-19 11:06 ` Chris Wilson
2016-04-19 12:19 ` Tvrtko Ursulin
@ 2016-04-19 17:33 ` kbuild test robot
1 sibling, 0 replies; 29+ messages in thread
From: kbuild test robot @ 2016-04-19 17:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]
Hi,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20160419]
[cannot apply to v4.6-rc4]
[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/20160419-190843
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (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_pin_iomap':
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:3: error: implicit declaration of function 'i915_vm_to_ggtt' [-Werror=implicit-function-declaration]
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:51: error: invalid type argument of '->' (have 'int')
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
>> drivers/gpu/drm/i915/i915_gem_gtt.c:3644:9: error: too many arguments to function 'io_mapping_map_wc'
ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
^
In file included from drivers/gpu/drm/i915/i915_drv.h:36:0,
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)
^
cc1: some warnings being treated as errors
vim +/i915_vm_to_ggtt +3644 drivers/gpu/drm/i915/i915_gem_gtt.c
3638
3639 GEM_BUG_ON(!vma->is_ggtt);
3640 GEM_BUG_ON((vma->bound & GLOBAL_BIND) == 0);
3641
3642 ptr = vma->iomap;
3643 if (ptr == NULL) {
> 3644 ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
3645 vma->node.start,
3646 vma->node.size);
3647 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: 36435 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA
[not found] ` <1461069238-31539-4-git-send-email-chris@chris-wilson.co.uk>
@ 2016-04-20 9:10 ` Luis R. Rodriguez
2016-04-20 9:38 ` Chris Wilson
2016-04-20 11:17 ` [Intel-gfx] " Daniel Vetter
0 siblings, 2 replies; 29+ messages in thread
From: Luis R. Rodriguez @ 2016-04-20 9:10 UTC (permalink / raw)
To: Chris Wilson
Cc: David Airlie, netdev, intel-gfx, linux-kernel, Ingo Molnar,
Peter Zijlstra (Intel), mcgrof, dri-devel, linux-rdma,
Daniel Vetter, Dan Williams, Yishai Hadas, David Hildenbrand
On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
> old_write_domain);
> }
>
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> + if (vma->iomap == NULL)
> + return;
> +
> + io_mapping_unmap(vma->iomap);
The NULL check could just be done by io_mapping_unmap() then you
can avoid this in other drivers too.
> + vma->iomap = NULL;
You added accounting here, by simple int and inc / dec'ing it.
I cannot confirm if it is correctly avoiding races, can you
confirm?
Also you added accounting for the custom vma pinning thing and do
GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
iounmap. That seems rather sloppy.
iomapping stuff has its own custom data structure, why not just use that data
structure instead of the struct i915_vma and generalize this ? Drivers can
be buggy and best if we avoid custom driver accounting and just do it in a neat
generic fashion.
Then other drivers could use this too.
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -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_pin_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;
some framebuffer drivers tend to use a generic start address of
iinfo->fix.smem_start and a length of info->fix.smem_len, this
driver sets the smem_start above, but its different than what
gets ioremap for a start address:
+ ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+ vma->node.start,
+ vma->node.size);
fix.smem_start is :
> + info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
The smem_len matches though. Can you clarify if its correct for
the io_mapping_map_wc() should not be using info->fix.smem_start
(which is dev->mode_config.fb_base + vma->node.start)?
Reason I ask is since I noticed a while ago a lot of drivers
were using info->fix.smem_start and info->fix.smem_len consistently
for their ioremap'd areas it might make sense instead to let the
internal framebuffer (register_framebuffer()) optionally manage the
ioremap_wc() for drivers, given that this is pretty generic stuff.
Luis
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-20 9:10 ` [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA Luis R. Rodriguez
@ 2016-04-20 9:38 ` Chris Wilson
2016-04-20 11:17 ` [Intel-gfx] " Daniel Vetter
1 sibling, 0 replies; 29+ messages in thread
From: Chris Wilson @ 2016-04-20 9:38 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Tvrtko Ursulin, netdev, intel-gfx, linux-kernel, Ingo Molnar,
Peter Zijlstra (Intel), dri-devel, linux-rdma, Daniel Vetter,
Dan Williams, Yishai Hadas, David Hildenbrand
On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6ce2c31b9a81..9ef47329e8ae 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
> > old_write_domain);
> > }
> >
> > +static void __i915_vma_iounmap(struct i915_vma *vma)
> > +{
> > + if (vma->iomap == NULL)
> > + return;
> > +
> > + io_mapping_unmap(vma->iomap);
>
> The NULL check could just be done by io_mapping_unmap() then you
> can avoid this in other drivers too.
>
> > + vma->iomap = NULL;
>
> You added accounting here, by simple int and inc / dec'ing it.
> I cannot confirm if it is correctly avoiding races, can you
> confirm?
Yes, the vma->pin_count is guarded by the struct_mutex atm. (The
struct_mutex is our own BKL :(
> Also you added accounting for the custom vma pinning thing and do
> GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
> do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
> iounmap. That seems rather sloppy.
It's placed next to the function where pin_count == 0 and only called
from it. Yes, I did think the same...
> iomapping stuff has its own custom data structure, why not just use that data
> structure instead of the struct i915_vma and generalize this ? Drivers can
> be buggy and best if we avoid custom driver accounting and just do it in a neat
> generic fashion.
Completely different tasks, as far as I am aware. The iomapping is about
providing CPU access to the IO region, dma-remapping about providing
device access to physical memory, and our own VMA is about how the
object sits in all the different views of both CPU and device address
spaces (of which there are many, and even the CPU accessible address
space is not the entirety of that particular address space).
> Then other drivers could use this too.
drivers/gpu/drm/ttm (you didn't hear me say that...)
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 79ac202f3870..93f54a10042f 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -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_pin_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;
>
> some framebuffer drivers tend to use a generic start address of
> iinfo->fix.smem_start and a length of info->fix.smem_len, this
> driver sets the smem_start above, but its different than what
> gets ioremap for a start address:
>
> + ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
> + vma->node.start,
> + vma->node.size);
>
> fix.smem_start is :
>
>
> > + info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
>
> The smem_len matches though. Can you clarify if its correct for
> the io_mapping_map_wc() should not be using info->fix.smem_start
> (which is dev->mode_config.fb_base + vma->node.start)?
dev->mode_config.fb_base is the base address of the mappable region. It
is an inconsistently in naming that just hasn't annoyed me enough to
fix.
> Reason I ask is since I noticed a while ago a lot of drivers
> were using info->fix.smem_start and info->fix.smem_len consistently
> for their ioremap'd areas it might make sense instead to let the
> internal framebuffer (register_framebuffer()) optionally manage the
> ioremap_wc() for drivers, given that this is pretty generic stuff.
Apart from drivers like ours we would end up with multiple mappings to
the same region. It was just a little grevience that I think was worth
fixing. It does highlight how buggy our code is currently though as we
never relinquish that mapping when the driver is unloaded.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-20 9:10 ` [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA Luis R. Rodriguez
2016-04-20 9:38 ` Chris Wilson
@ 2016-04-20 11:17 ` Daniel Vetter
[not found] ` <20160420111730.GL2510-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>
1 sibling, 1 reply; 29+ messages in thread
From: Daniel Vetter @ 2016-04-20 11:17 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: David Hildenbrand, netdev, intel-gfx, linux-kernel, dri-devel,
Peter Zijlstra (Intel), Daniel Vetter, Dan Williams, Yishai Hadas,
Ingo Molnar, linux-rdma
On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> Reason I ask is since I noticed a while ago a lot of drivers
> were using info->fix.smem_start and info->fix.smem_len consistently
> for their ioremap'd areas it might make sense instead to let the
> internal framebuffer (register_framebuffer()) optionally manage the
> ioremap_wc() for drivers, given that this is pretty generic stuff.
All that legacy fbdev stuff is just for legacy support, and I prefer to
have that as dumb as possible. There's been some discussion even around
lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
to have a simple drm driver for e.g. uefi.
But I definitely don't want a legacy horror show like fbdev to
automagically take care of device mappings for drivers.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA
[not found] ` <20160420111730.GL2510-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>
@ 2016-04-20 21:27 ` Luis R. Rodriguez
2016-04-21 7:27 ` Daniel Vetter
0 siblings, 1 reply; 29+ messages in thread
From: Luis R. Rodriguez @ 2016-04-20 21:27 UTC (permalink / raw)
To: Luis R. Rodriguez, Chris Wilson, David Airlie,
netdev-u79uwXL29TY76Z2rM5mHXA,
intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar,
Peter Zijlstra (Intel),
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Daniel Vetter, Dan Williams,
Yishai Hadas, David Hildenbrand
On Wed, Apr 20, 2016 at 01:17:30PM +0200, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> > Reason I ask is since I noticed a while ago a lot of drivers
> > were using info->fix.smem_start and info->fix.smem_len consistently
> > for their ioremap'd areas it might make sense instead to let the
> > internal framebuffer (register_framebuffer()) optionally manage the
> > ioremap_wc() for drivers, given that this is pretty generic stuff.
>
> All that legacy fbdev stuff is just for legacy support, and I prefer to
> have that as dumb as possible. There's been some discussion even around
> lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
> to have a simple drm driver for e.g. uefi.
>
> But I definitely don't want a legacy horror show like fbdev to
> automagically take care of device mappings for drivers.
Makes sense, it also still begs the question if more modern APIs
could manage the ioremap for you. Evidence shows people get
sloppy and if things were done internally with helpers it may
be easier to later make adjustments.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA
2016-04-20 21:27 ` Luis R. Rodriguez
@ 2016-04-21 7:27 ` Daniel Vetter
0 siblings, 0 replies; 29+ messages in thread
From: Daniel Vetter @ 2016-04-21 7:27 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: David Hildenbrand, David Airlie, netdev, intel-gfx, linux-kernel,
dri-devel, Peter Zijlstra (Intel), Daniel Vetter, Dan Williams,
Yishai Hadas, Ingo Molnar, linux-rdma
On Wed, Apr 20, 2016 at 11:27:27PM +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 20, 2016 at 01:17:30PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> > > Reason I ask is since I noticed a while ago a lot of drivers
> > > were using info->fix.smem_start and info->fix.smem_len consistently
> > > for their ioremap'd areas it might make sense instead to let the
> > > internal framebuffer (register_framebuffer()) optionally manage the
> > > ioremap_wc() for drivers, given that this is pretty generic stuff.
> >
> > All that legacy fbdev stuff is just for legacy support, and I prefer to
> > have that as dumb as possible. There's been some discussion even around
> > lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
> > to have a simple drm driver for e.g. uefi.
> >
> > But I definitely don't want a legacy horror show like fbdev to
> > automagically take care of device mappings for drivers.
>
> Makes sense, it also still begs the question if more modern APIs
> could manage the ioremap for you. Evidence shows people get
> sloppy and if things were done internally with helpers it may
> be easier to later make adjustments.
Real gpus generally have so much mmio space that you want to ioremap them
on demand. At least if you still care about 32bit support. And on-die gpus
on socs or similar tend to not have an mmio range to access the gfx
remapping range at all, but instead expect that to be done with gpu
pagetables.
So at least with gpus I don't see a real demand for this, and the existing
users are mostly old fbdev drivers that really no one should be touching
;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-04-21 7:27 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-18 11:13 [PATCH v2 1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Chris Wilson
2016-04-18 11:14 ` [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc() Chris Wilson
2016-04-19 12:02 ` Chris Wilson
2016-04-19 12:30 ` Luis R. Rodriguez
2016-04-19 12:34 ` Chris Wilson
[not found] ` <1461069238-31539-1-git-send-email-chris@chris-wilson.co.uk>
[not found] ` <1461069238-31539-4-git-send-email-chris@chris-wilson.co.uk>
2016-04-20 9:10 ` [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA Luis R. Rodriguez
2016-04-20 9:38 ` Chris Wilson
2016-04-20 11:17 ` [Intel-gfx] " Daniel Vetter
[not found] ` <20160420111730.GL2510-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>
2016-04-20 21:27 ` Luis R. Rodriguez
2016-04-21 7:27 ` Daniel Vetter
2016-04-18 11:14 ` [PATCH v2 3/4] drm/i915: Introduce i915_vm_to_ggtt() Chris Wilson
2016-04-18 12:01 ` Tvrtko Ursulin
2016-04-18 13:37 ` Joonas Lahtinen
2016-04-18 14:02 ` Chris Wilson
2016-04-18 11:14 ` [PATCH v2 4/4] drm/i915: Move ioremap_wc tracking onto VMA Chris Wilson
2016-04-18 11:56 ` Chris Wilson
2016-04-18 12:08 ` Tvrtko Ursulin
2016-04-18 14:54 ` [PATCH] " Chris Wilson
2016-04-18 15:08 ` Tvrtko Ursulin
2016-04-18 15:22 ` Chris Wilson
2016-04-18 15:27 ` Tvrtko Ursulin
2016-04-19 11:06 ` Chris Wilson
2016-04-19 12:19 ` Tvrtko Ursulin
2016-04-19 17:33 ` kbuild test robot
2016-04-18 15:20 ` kbuild test robot
2016-04-18 12:03 ` [PATCH v2 4/4] " Tvrtko Ursulin
2016-04-18 13:26 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr Patchwork
2016-04-18 15:29 ` ✗ Fi.CI.BAT: warning for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev2) Patchwork
2016-04-19 11:57 ` ✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915/overlay: Replace i915_gem_obj_ggtt_offset() with the known flip_addr (rev3) Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox