* [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
@ 2015-03-16 12:11 Joonas Lahtinen
2015-03-16 13:26 ` Tvrtko Ursulin
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2015-03-16 12:11 UTC (permalink / raw)
To: intel-gfx
GGTT views are only applicable when dealing with GGTT. Change the code to
reject ggtt_view where it should not be used and require it when it should
be.
v2:
- Dropped _ppgtt_ infixes, allow both types to be passed
- Disregard other but normal views when no view is specified
- More checks that valid parameters are passed
- More readable error checking
v3:
- Prefer WARN_ONCE over BUG_ON when there is code path for failure
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 102 +++++++++------------
drivers/gpu/drm/i915/i915_gem.c | 175 ++++++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_gem_gtt.c | 71 +++++++++++----
3 files changed, 229 insertions(+), 119 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a76c6ee..996f6a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2623,20 +2623,16 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
#define PIN_GLOBAL 0x4
#define PIN_OFFSET_BIAS 0x8
#define PIN_OFFSET_MASK (~4095)
-int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- uint32_t alignment,
- uint64_t flags,
- const struct i915_ggtt_view *view);
-static inline
-int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- uint32_t alignment,
- uint64_t flags)
-{
- return i915_gem_object_pin_view(obj, vm, alignment, flags,
- &i915_ggtt_view_normal);
-}
+int __must_check
+i915_gem_object_pin(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ uint32_t alignment,
+ uint64_t flags);
+int __must_check
+i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view,
+ uint32_t alignment,
+ uint64_t flags);
int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
u32 flags);
@@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
void i915_gem_restore_fences(struct drm_device *dev);
-unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
- struct i915_address_space *vm,
- enum i915_ggtt_view_type view);
-static inline
-unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
- struct i915_address_space *vm)
+static inline bool i915_is_ggtt(struct i915_address_space *vm);
+
+unsigned long
+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
+ enum i915_ggtt_view_type view);
+unsigned long
+i915_gem_obj_offset(struct drm_i915_gem_object *o,
+ struct i915_address_space *vm);
+static inline unsigned long
+i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
{
- return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
+ return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
}
+
bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
-bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
- struct i915_address_space *vm,
- enum i915_ggtt_view_type view);
-static inline
+bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
+ enum i915_ggtt_view_type view);
bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
- struct i915_address_space *vm)
-{
- return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
-}
+ struct i915_address_space *vm);
unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
struct i915_address_space *vm);
-struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- const struct i915_ggtt_view *view);
-static inline
-struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm)
-{
- return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
-}
-
struct i915_vma *
-i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- const struct i915_ggtt_view *view);
+i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm);
+struct i915_vma *
+i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view);
-static inline
struct i915_vma *
i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm)
-{
- return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
- &i915_ggtt_view_normal);
-}
+ struct i915_address_space *vm);
+struct i915_vma *
+i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view);
-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
-static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
- struct i915_vma *vma;
- list_for_each_entry(vma, &obj->vma_list, vma_link)
- if (vma->pin_count > 0)
- return true;
- return false;
+static inline struct i915_vma *
+i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
+{
+ return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
}
+bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
/* Some GGTT VM helpers */
#define i915_obj_to_ggtt(obj) \
@@ -2876,13 +2860,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
{
- return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
-}
-
-static inline unsigned long
-i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
-{
- return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
+ return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
}
static inline unsigned long
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0fe313d..e2876bf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3518,9 +3518,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
static struct i915_vma *
i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
struct i915_address_space *vm,
+ const struct i915_ggtt_view *ggtt_view,
unsigned alignment,
- uint64_t flags,
- const struct i915_ggtt_view *view)
+ uint64_t flags)
{
struct drm_device *dev = obj->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3532,6 +3532,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
struct i915_vma *vma;
int ret;
+ if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
+ return ERR_PTR(-EINVAL);
+
fence_size = i915_gem_get_gtt_size(dev,
obj->base.size,
obj->tiling_mode);
@@ -3570,7 +3573,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
i915_gem_object_pin_pages(obj);
- vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
+ vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
+ i915_gem_obj_lookup_or_create_vma(obj, vm);
+
if (IS_ERR(vma))
goto err_unpin;
@@ -4167,12 +4172,12 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
return false;
}
-int
-i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- uint32_t alignment,
- uint64_t flags,
- const struct i915_ggtt_view *view)
+static int
+i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_ggtt_view *ggtt_view,
+ uint32_t alignment,
+ uint64_t flags)
{
struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
struct i915_vma *vma;
@@ -4188,17 +4193,29 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
return -EINVAL;
- vma = i915_gem_obj_to_vma_view(obj, vm, view);
+ if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
+ return -EINVAL;
+
+ vma = ggtt_view ? i915_gem_obj_to_ggtt_view(obj, ggtt_view) :
+ i915_gem_obj_to_vma(obj, vm);
+
+ if (IS_ERR(vma))
+ return PTR_ERR(vma);
+
if (vma) {
if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
return -EBUSY;
if (i915_vma_misplaced(vma, alignment, flags)) {
+ unsigned long offset;
+ offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
+ i915_gem_obj_offset(obj, vm);
WARN(vma->pin_count,
- "bo is already pinned with incorrect alignment:"
+ "bo is already pinned in %s with incorrect alignment:"
" offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
" obj->map_and_fenceable=%d\n",
- i915_gem_obj_offset_view(obj, vm, view->type),
+ ggtt_view ? "ggtt" : "ppgtt",
+ offset,
alignment,
!!(flags & PIN_MAPPABLE),
obj->map_and_fenceable);
@@ -4212,8 +4229,8 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
bound = vma ? vma->bound : 0;
if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
- vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
- flags, view);
+ vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
+ flags);
if (IS_ERR(vma))
return PTR_ERR(vma);
}
@@ -4254,6 +4271,30 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
return 0;
}
+int
+i915_gem_object_pin(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ uint32_t alignment,
+ uint64_t flags)
+{
+ return i915_gem_object_do_pin(obj, vm,
+ i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL,
+ alignment, flags);
+}
+
+int
+i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view,
+ uint32_t alignment,
+ uint64_t flags)
+{
+ if (WARN_ONCE(!view, "no view specified"))
+ return -EINVAL;
+
+ return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
+ alignment, flags);
+}
+
void
i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
{
@@ -4559,15 +4600,32 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
intel_runtime_pm_put(dev_priv);
}
-struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- const struct i915_ggtt_view *view)
+struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm)
{
struct i915_vma *vma;
- list_for_each_entry(vma, &obj->vma_list, vma_link)
- if (vma->vm == vm && vma->ggtt_view.type == view->type)
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
+ continue;
+ if (vma->vm == vm)
return vma;
+ }
+ return NULL;
+}
+struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
+ const struct i915_ggtt_view *view)
+{
+ struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
+ struct i915_vma *vma;
+
+ if (WARN_ONCE(!view, "no view specified"))
+ return ERR_PTR(-EINVAL);
+
+ list_for_each_entry(vma, &obj->vma_list, vma_link)
+ if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
+ return vma;
return NULL;
}
@@ -5176,9 +5234,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
}
/* All the new VM stuff */
-unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
- struct i915_address_space *vm,
- enum i915_ggtt_view_type view)
+unsigned long
+i915_gem_obj_offset(struct drm_i915_gem_object *o,
+ struct i915_address_space *vm)
{
struct drm_i915_private *dev_priv = o->base.dev->dev_private;
struct i915_vma *vma;
@@ -5186,23 +5244,58 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
list_for_each_entry(vma, &o->vma_list, vma_link) {
- if (vma->vm == vm && vma->ggtt_view.type == view)
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
+ continue;
+ if (vma->vm == vm)
return vma->node.start;
-
}
+
WARN(1, "%s vma for this object not found.\n",
i915_is_ggtt(vm) ? "global" : "ppgtt");
return -1;
}
-bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
- struct i915_address_space *vm,
- enum i915_ggtt_view_type view)
+unsigned long
+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
+ enum i915_ggtt_view_type view)
{
+ struct drm_i915_private *dev_priv = o->base.dev->dev_private;
+ struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
struct i915_vma *vma;
list_for_each_entry(vma, &o->vma_list, vma_link)
- if (vma->vm == vm &&
+ if (vma->vm == ggtt && vma->ggtt_view.type == view)
+ return vma->node.start;
+
+ WARN(1, "global vma for this object not found.\n");
+ return -1;
+}
+
+bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
+ struct i915_address_space *vm)
+{
+ struct i915_vma *vma;
+
+ list_for_each_entry(vma, &o->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
+ continue;
+ if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
+ return true;
+ }
+
+ return false;
+}
+
+bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
+ enum i915_ggtt_view_type view)
+{
+ struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
+ struct i915_vma *vma;
+
+ list_for_each_entry(vma, &o->vma_list, vma_link)
+ if (vma->vm == ggtt &&
vma->ggtt_view.type == view &&
drm_mm_node_allocated(&vma->node))
return true;
@@ -5231,10 +5324,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
BUG_ON(list_empty(&o->vma_list));
- list_for_each_entry(vma, &o->vma_list, vma_link)
+ list_for_each_entry(vma, &o->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
+ continue;
if (vma->vm == vm)
return vma->node.size;
-
+ }
return 0;
}
@@ -5334,15 +5430,16 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
return NOTIFY_DONE;
}
-struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
+bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
{
- struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
struct i915_vma *vma;
-
- list_for_each_entry(vma, &obj->vma_list, vma_link)
- if (vma->vm == ggtt &&
- vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
- return vma;
-
- return NULL;
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ if (i915_is_ggtt(vma->vm) &&
+ vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
+ continue;
+ if (vma->pin_count > 0)
+ return true;
+ }
+ return false;
}
+
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2034f7c..f1b9ea6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -67,8 +67,9 @@
* i915_ggtt_view_type and struct i915_ggtt_view.
*
* A new flavour of core GEM functions which work with GGTT bound objects were
- * added with the _view suffix. They take the struct i915_ggtt_view parameter
- * encapsulating all metadata required to implement a view.
+ * added with the _ggtt_ infix, and sometimes with _view postfix to avoid
+ * renaming in large amounts of code. They take the struct i915_ggtt_view
+ * parameter encapsulating all metadata required to implement a view.
*
* As a helper for callers which are only interested in the normal view,
* globally const i915_ggtt_view_normal singleton instance exists. All old core
@@ -1726,11 +1727,15 @@ static void ggtt_bind_vma(struct i915_vma *vma,
struct drm_device *dev = vma->vm->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_object *obj = vma->obj;
+ struct sg_table *pages = obj->pages;
/* Currently applicable only to VLV */
if (obj->gt_ro)
flags |= PTE_READ_ONLY;
+ if (i915_is_ggtt(vma->vm))
+ pages = vma->ggtt_view.pages;
+
/* If there is no aliasing PPGTT, or the caller needs a global mapping,
* or we have a global mapping already but the cacheability flags have
* changed, set the global PTEs.
@@ -1745,7 +1750,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
if (!(vma->bound & GLOBAL_BIND) ||
(cache_level != obj->cache_level)) {
- vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
+ vma->vm->insert_entries(vma->vm, pages,
vma->node.start,
cache_level, flags);
vma->bound |= GLOBAL_BIND;
@@ -1756,8 +1761,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
(!(vma->bound & LOCAL_BIND) ||
(cache_level != obj->cache_level))) {
struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
- appgtt->base.insert_entries(&appgtt->base,
- vma->ggtt_view.pages,
+ appgtt->base.insert_entries(&appgtt->base, pages,
vma->node.start,
cache_level, flags);
vma->bound |= LOCAL_BIND;
@@ -2331,23 +2335,28 @@ int i915_gem_gtt_init(struct drm_device *dev)
return 0;
}
-static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
- const struct i915_ggtt_view *view)
+static struct i915_vma *
+__i915_gem_vma_create(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm,
+ const struct i915_ggtt_view *ggtt_view)
{
struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
if (vma == NULL)
return ERR_PTR(-ENOMEM);
+ if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
+ return ERR_PTR(-EINVAL);
+
INIT_LIST_HEAD(&vma->vma_link);
INIT_LIST_HEAD(&vma->mm_list);
INIT_LIST_HEAD(&vma->exec_list);
vma->vm = vm;
vma->obj = obj;
- vma->ggtt_view = *view;
if (INTEL_INFO(vm->dev)->gen >= 6) {
if (i915_is_ggtt(vm)) {
+ vma->ggtt_view = *ggtt_view;
+
vma->unbind_vma = ggtt_unbind_vma;
vma->bind_vma = ggtt_bind_vma;
} else {
@@ -2356,6 +2365,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
}
} else {
BUG_ON(!i915_is_ggtt(vm));
+ vma->ggtt_view = *ggtt_view;
vma->unbind_vma = i915_ggtt_unbind_vma;
vma->bind_vma = i915_ggtt_bind_vma;
}
@@ -2368,21 +2378,44 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
}
struct i915_vma *
-i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
- struct i915_address_space *vm,
+i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
+ struct i915_address_space *vm)
+{
+ struct i915_vma *vma;
+
+ vma = i915_gem_obj_to_vma(obj, vm);
+ if (!vma)
+ vma = __i915_gem_vma_create(obj, vm,
+ i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL);
+
+ return vma;
+}
+
+struct i915_vma *
+i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
const struct i915_ggtt_view *view)
{
+ struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
struct i915_vma *vma;
- vma = i915_gem_obj_to_vma_view(obj, vm, view);
+ if (WARN_ON(!view))
+ return ERR_PTR(-EINVAL);
+
+ vma = i915_gem_obj_to_ggtt_view(obj, view);
+
+ if (IS_ERR(vma))
+ return vma;
+
if (!vma)
- vma = __i915_gem_vma_create(obj, vm, view);
+ vma = __i915_gem_vma_create(obj, ggtt, view);
return vma;
+
}
+
static inline
-int i915_get_vma_pages(struct i915_vma *vma)
+int i915_get_ggtt_vma_pages(struct i915_vma *vma)
{
if (vma->ggtt_view.pages)
return 0;
@@ -2394,7 +2427,7 @@ int i915_get_vma_pages(struct i915_vma *vma)
vma->ggtt_view.type);
if (!vma->ggtt_view.pages) {
- DRM_ERROR("Failed to get pages for VMA view type %u!\n",
+ DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
vma->ggtt_view.type);
return -EINVAL;
}
@@ -2415,10 +2448,12 @@ int i915_get_vma_pages(struct i915_vma *vma)
int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
u32 flags)
{
- int ret = i915_get_vma_pages(vma);
+ if (i915_is_ggtt(vma->vm)) {
+ int ret = i915_get_ggtt_vma_pages(vma);
- if (ret)
- return ret;
+ if (ret)
+ return ret;
+ }
vma->bind_vma(vma, cache_level, flags);
--
1.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 12:11 [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Joonas Lahtinen
@ 2015-03-16 13:26 ` Tvrtko Ursulin
2015-03-16 14:48 ` Joonas Lahtinen
2015-03-16 14:52 ` shuang.he
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2015-03-16 13:26 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
Hi,
On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
[snip]
> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view);
Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
vma in the name I think.
> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> {
> struct i915_vma *vma;
> - list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
Since there are 4-5 instances of this check it may make sense to add a
helper like i915_is_normal_ggtt_view(vma), but it is not that important
for me.
The rest looks good to me.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 13:26 ` Tvrtko Ursulin
@ 2015-03-16 14:48 ` Joonas Lahtinen
2015-03-16 14:50 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Joonas Lahtinen @ 2015-03-16 14:48 UTC (permalink / raw)
To: Tvrtko Ursulin, Daniel Vetter; +Cc: intel-gfx
Hi,
Regression testing completed without problems for BYT, HSW and BDW
already.
On ma, 2015-03-16 at 13:26 +0000, Tvrtko Ursulin wrote:
> Hi,
>
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> > GGTT views are only applicable when dealing with GGTT. Change the code to
> > reject ggtt_view where it should not be used and require it when it should
> > be.
> >
> > v2:
> > - Dropped _ppgtt_ infixes, allow both types to be passed
> > - Disregard other but normal views when no view is specified
> > - More checks that valid parameters are passed
> > - More readable error checking
> >
> > v3:
> > - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> [snip]
>
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
>
> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
> vma in the name I think.
>
The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
would cause a lot of changes all around code to change), so I decided to
stay with the same convention. In that sense it would add more
confusion, compared to the current *_view function being the same as
without _view, but with explicitly specified view.
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > {
> > struct i915_vma *vma;
> > - list_for_each_entry(vma, &obj->vma_list, vma_link)
> > - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> > + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
>
> Since there are 4-5 instances of this check it may make sense to add a
> helper like i915_is_normal_ggtt_view(vma), but it is not that important
> for me.
>
This will be done in following patch that makes the view struct (minus
implementation parts like the pages sg_table) define the view.
> The rest looks good to me.
>
Sound like you could R-B this then?
Best Regards,
Joonas
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 14:48 ` Joonas Lahtinen
@ 2015-03-16 14:50 ` Tvrtko Ursulin
0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2015-03-16 14:50 UTC (permalink / raw)
To: Joonas Lahtinen, Daniel Vetter; +Cc: intel-gfx
On 03/16/2015 02:48 PM, Joonas Lahtinen wrote:
> Hi,
>
> Regression testing completed without problems for BYT, HSW and BDW
> already.
>
> On ma, 2015-03-16 at 13:26 +0000, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
>>> GGTT views are only applicable when dealing with GGTT. Change the code to
>>> reject ggtt_view where it should not be used and require it when it should
>>> be.
>>>
>>> v2:
>>> - Dropped _ppgtt_ infixes, allow both types to be passed
>>> - Disregard other but normal views when no view is specified
>>> - More checks that valid parameters are passed
>>> - More readable error checking
>>>
>>> v3:
>>> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>>
>> [snip]
>>
>>> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> + struct i915_address_space *vm);
>>> +struct i915_vma *
>>> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
>>> + const struct i915_ggtt_view *view);
>>
>> Would i915_gem_obj_to_ggtt_vma be a better name? At least should have
>> vma in the name I think.
>>
>
> The i915_gem_obj_to_ggtt functions doesn't mention _vma either (and
> would cause a lot of changes all around code to change), so I decided to
> stay with the same convention. In that sense it would add more
> confusion, compared to the current *_view function being the same as
> without _view, but with explicitly specified view.
Yes, I overlooked that.
>>> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>>> + struct i915_address_space *vm)
>>> {
>>> struct i915_vma *vma;
>>> - list_for_each_entry(vma, &obj->vma_list, vma_link)
>>> - if (vma->vm == vm && vma->ggtt_view.type == view->type)
>>> + list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> + if (i915_is_ggtt(vma->vm) &&
>>> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
>>
>> Since there are 4-5 instances of this check it may make sense to add a
>> helper like i915_is_normal_ggtt_view(vma), but it is not that important
>> for me.
>>
>
> This will be done in following patch that makes the view struct (minus
> implementation parts like the pages sg_table) define the view.
Cool.
>> The rest looks good to me.
>>
>
> Sound like you could R-B this then?
Yes,
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 12:11 [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Joonas Lahtinen
2015-03-16 13:26 ` Tvrtko Ursulin
@ 2015-03-16 14:52 ` shuang.he
2015-03-16 17:50 ` Daniel Vetter
2015-03-17 14:19 ` Tvrtko Ursulin
3 siblings, 0 replies; 10+ messages in thread
From: shuang.he @ 2015-03-16 14:52 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, joonas.lahtinen
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5957
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 275/275 275/275
ILK 303/303 303/303
SNB -1 279/279 278/279
IVB 343/343 343/343
BYT 287/287 287/287
HSW -1 361/361 360/361
BDW 308/308 308/308
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*SNB igt_gem_persistent_relocs_forked-interruptible-thrash-inactive PASS(2) DMESG_WARN(1)PASS(1)
HSW igt_gem_storedw_loop_bsd DMESG_WARN(1)PASS(1) DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 12:11 [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Joonas Lahtinen
2015-03-16 13:26 ` Tvrtko Ursulin
2015-03-16 14:52 ` shuang.he
@ 2015-03-16 17:50 ` Daniel Vetter
2015-03-17 5:31 ` Joonas Lahtinen
2015-03-17 14:19 ` Tvrtko Ursulin
3 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2015-03-16 17:50 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 102 +++++++++------------
> drivers/gpu/drm/i915/i915_gem.c | 175 ++++++++++++++++++++++++++++--------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 71 +++++++++++----
> 3 files changed, 229 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a76c6ee..996f6a1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2623,20 +2623,16 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
> #define PIN_GLOBAL 0x4
> #define PIN_OFFSET_BIAS 0x8
> #define PIN_OFFSET_MASK (~4095)
> -int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - uint32_t alignment,
> - uint64_t flags,
> - const struct i915_ggtt_view *view);
> -static inline
> -int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - uint32_t alignment,
> - uint64_t flags)
> -{
> - return i915_gem_object_pin_view(obj, vm, alignment, flags,
> - &i915_ggtt_view_normal);
> -}
> +int __must_check
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + uint32_t alignment,
> + uint64_t flags);
> +int __must_check
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view,
> + uint32_t alignment,
> + uint64_t flags);
>
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> u32 flags);
> @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>
> void i915_gem_restore_fences(struct drm_device *dev);
>
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view);
> -static inline
> -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm)
> +static inline bool i915_is_ggtt(struct i915_address_space *vm);
This forward decl seems unneeded leftover from earlier patch iterations.
I've removed it and merged the patch.
Thanks, Daniel
> +
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view);
> +unsigned long
> +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm);
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> {
> - return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> + return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
> }
> +
> bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view);
> -static inline
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view);
> bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm)
> -{
> - return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> -}
> + struct i915_address_space *vm);
>
> unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> struct i915_address_space *vm);
> -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view);
> -static inline
> -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm)
> -{
> - return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> -}
> -
> struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view);
> +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view);
>
> -static inline
> struct i915_vma *
> i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm)
> -{
> - return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> - &i915_ggtt_view_normal);
> -}
> + struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view);
>
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> -static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> - struct i915_vma *vma;
> - list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (vma->pin_count > 0)
> - return true;
> - return false;
> +static inline struct i915_vma *
> +i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +{
> + return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
> }
> +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
>
> /* Some GGTT VM helpers */
> #define i915_obj_to_ggtt(obj) \
> @@ -2876,13 +2860,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>
> static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> {
> - return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
> -}
> -
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> -{
> - return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> + return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
> }
>
> static inline unsigned long
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0fe313d..e2876bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3518,9 +3518,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
> static struct i915_vma *
> i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view,
> unsigned alignment,
> - uint64_t flags,
> - const struct i915_ggtt_view *view)
> + uint64_t flags)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3532,6 +3532,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> struct i915_vma *vma;
> int ret;
>
> + if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> + return ERR_PTR(-EINVAL);
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3570,7 +3573,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>
> i915_gem_object_pin_pages(obj);
>
> - vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
> + vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> + i915_gem_obj_lookup_or_create_vma(obj, vm);
> +
> if (IS_ERR(vma))
> goto err_unpin;
>
> @@ -4167,12 +4172,12 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
> return false;
> }
>
> -int
> -i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - uint32_t alignment,
> - uint64_t flags,
> - const struct i915_ggtt_view *view)
> +static int
> +i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view,
> + uint32_t alignment,
> + uint64_t flags)
> {
> struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> struct i915_vma *vma;
> @@ -4188,17 +4193,29 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> return -EINVAL;
>
> - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> + if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> + return -EINVAL;
> +
> + vma = ggtt_view ? i915_gem_obj_to_ggtt_view(obj, ggtt_view) :
> + i915_gem_obj_to_vma(obj, vm);
> +
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
> +
> if (vma) {
> if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> return -EBUSY;
>
> if (i915_vma_misplaced(vma, alignment, flags)) {
> + unsigned long offset;
> + offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> + i915_gem_obj_offset(obj, vm);
> WARN(vma->pin_count,
> - "bo is already pinned with incorrect alignment:"
> + "bo is already pinned in %s with incorrect alignment:"
> " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
> " obj->map_and_fenceable=%d\n",
> - i915_gem_obj_offset_view(obj, vm, view->type),
> + ggtt_view ? "ggtt" : "ppgtt",
> + offset,
> alignment,
> !!(flags & PIN_MAPPABLE),
> obj->map_and_fenceable);
> @@ -4212,8 +4229,8 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
>
> bound = vma ? vma->bound : 0;
> if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> - vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
> - flags, view);
> + vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
> + flags);
> if (IS_ERR(vma))
> return PTR_ERR(vma);
> }
> @@ -4254,6 +4271,30 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> return 0;
> }
>
> +int
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + uint32_t alignment,
> + uint64_t flags)
> +{
> + return i915_gem_object_do_pin(obj, vm,
> + i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL,
> + alignment, flags);
> +}
> +
> +int
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view,
> + uint32_t alignment,
> + uint64_t flags)
> +{
> + if (WARN_ONCE(!view, "no view specified"))
> + return -EINVAL;
> +
> + return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
> + alignment, flags);
> +}
> +
> void
> i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> {
> @@ -4559,15 +4600,32 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> intel_runtime_pm_put(dev_priv);
> }
>
> -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view)
> +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> {
> struct i915_vma *vma;
> - list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> + continue;
> + if (vma->vm == vm)
> return vma;
> + }
> + return NULL;
> +}
>
> +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view)
> +{
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> + struct i915_vma *vma;
> +
> + if (WARN_ONCE(!view, "no view specified"))
> + return ERR_PTR(-EINVAL);
> +
> + list_for_each_entry(vma, &obj->vma_list, vma_link)
> + if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
> + return vma;
> return NULL;
> }
>
> @@ -5176,9 +5234,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
> }
>
> /* All the new VM stuff */
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view)
> +unsigned long
> +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm)
> {
> struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> struct i915_vma *vma;
> @@ -5186,23 +5244,58 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
>
> list_for_each_entry(vma, &o->vma_list, vma_link) {
> - if (vma->vm == vm && vma->ggtt_view.type == view)
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> + continue;
> + if (vma->vm == vm)
> return vma->node.start;
> -
> }
> +
> WARN(1, "%s vma for this object not found.\n",
> i915_is_ggtt(vm) ? "global" : "ppgtt");
> return -1;
> }
>
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view)
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view)
> {
> + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> struct i915_vma *vma;
>
> list_for_each_entry(vma, &o->vma_list, vma_link)
> - if (vma->vm == vm &&
> + if (vma->vm == ggtt && vma->ggtt_view.type == view)
> + return vma->node.start;
> +
> + WARN(1, "global vma for this object not found.\n");
> + return -1;
> +}
> +
> +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma;
> +
> + list_for_each_entry(vma, &o->vma_list, vma_link) {
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> + continue;
> + if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
> + return true;
> + }
> +
> + return false;
> +}
> +
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view)
> +{
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> + struct i915_vma *vma;
> +
> + list_for_each_entry(vma, &o->vma_list, vma_link)
> + if (vma->vm == ggtt &&
> vma->ggtt_view.type == view &&
> drm_mm_node_allocated(&vma->node))
> return true;
> @@ -5231,10 +5324,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>
> BUG_ON(list_empty(&o->vma_list));
>
> - list_for_each_entry(vma, &o->vma_list, vma_link)
> + list_for_each_entry(vma, &o->vma_list, vma_link) {
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> + continue;
> if (vma->vm == vm)
> return vma->node.size;
> -
> + }
> return 0;
> }
>
> @@ -5334,15 +5430,16 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
> return NOTIFY_DONE;
> }
>
> -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
> {
> - struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> struct i915_vma *vma;
> -
> - list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (vma->vm == ggtt &&
> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> - return vma;
> -
> - return NULL;
> + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> + if (i915_is_ggtt(vma->vm) &&
> + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> + continue;
> + if (vma->pin_count > 0)
> + return true;
> + }
> + return false;
> }
> +
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 2034f7c..f1b9ea6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -67,8 +67,9 @@
> * i915_ggtt_view_type and struct i915_ggtt_view.
> *
> * A new flavour of core GEM functions which work with GGTT bound objects were
> - * added with the _view suffix. They take the struct i915_ggtt_view parameter
> - * encapsulating all metadata required to implement a view.
> + * added with the _ggtt_ infix, and sometimes with _view postfix to avoid
> + * renaming in large amounts of code. They take the struct i915_ggtt_view
> + * parameter encapsulating all metadata required to implement a view.
> *
> * As a helper for callers which are only interested in the normal view,
> * globally const i915_ggtt_view_normal singleton instance exists. All old core
> @@ -1726,11 +1727,15 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> struct drm_device *dev = vma->vm->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *obj = vma->obj;
> + struct sg_table *pages = obj->pages;
>
> /* Currently applicable only to VLV */
> if (obj->gt_ro)
> flags |= PTE_READ_ONLY;
>
> + if (i915_is_ggtt(vma->vm))
> + pages = vma->ggtt_view.pages;
> +
> /* If there is no aliasing PPGTT, or the caller needs a global mapping,
> * or we have a global mapping already but the cacheability flags have
> * changed, set the global PTEs.
> @@ -1745,7 +1750,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> if (!(vma->bound & GLOBAL_BIND) ||
> (cache_level != obj->cache_level)) {
> - vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
> + vma->vm->insert_entries(vma->vm, pages,
> vma->node.start,
> cache_level, flags);
> vma->bound |= GLOBAL_BIND;
> @@ -1756,8 +1761,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> (!(vma->bound & LOCAL_BIND) ||
> (cache_level != obj->cache_level))) {
> struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> - appgtt->base.insert_entries(&appgtt->base,
> - vma->ggtt_view.pages,
> + appgtt->base.insert_entries(&appgtt->base, pages,
> vma->node.start,
> cache_level, flags);
> vma->bound |= LOCAL_BIND;
> @@ -2331,23 +2335,28 @@ int i915_gem_gtt_init(struct drm_device *dev)
> return 0;
> }
>
> -static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view)
> +static struct i915_vma *
> +__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view)
> {
> struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> if (vma == NULL)
> return ERR_PTR(-ENOMEM);
>
> + if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> + return ERR_PTR(-EINVAL);
> +
> INIT_LIST_HEAD(&vma->vma_link);
> INIT_LIST_HEAD(&vma->mm_list);
> INIT_LIST_HEAD(&vma->exec_list);
> vma->vm = vm;
> vma->obj = obj;
> - vma->ggtt_view = *view;
>
> if (INTEL_INFO(vm->dev)->gen >= 6) {
> if (i915_is_ggtt(vm)) {
> + vma->ggtt_view = *ggtt_view;
> +
> vma->unbind_vma = ggtt_unbind_vma;
> vma->bind_vma = ggtt_bind_vma;
> } else {
> @@ -2356,6 +2365,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> }
> } else {
> BUG_ON(!i915_is_ggtt(vm));
> + vma->ggtt_view = *ggtt_view;
> vma->unbind_vma = i915_ggtt_unbind_vma;
> vma->bind_vma = i915_ggtt_bind_vma;
> }
> @@ -2368,21 +2378,44 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> }
>
> struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> +i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma;
> +
> + vma = i915_gem_obj_to_vma(obj, vm);
> + if (!vma)
> + vma = __i915_gem_vma_create(obj, vm,
> + i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL);
> +
> + return vma;
> +}
> +
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view)
> {
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> struct i915_vma *vma;
>
> - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> + if (WARN_ON(!view))
> + return ERR_PTR(-EINVAL);
> +
> + vma = i915_gem_obj_to_ggtt_view(obj, view);
> +
> + if (IS_ERR(vma))
> + return vma;
> +
> if (!vma)
> - vma = __i915_gem_vma_create(obj, vm, view);
> + vma = __i915_gem_vma_create(obj, ggtt, view);
>
> return vma;
> +
> }
>
> +
> static inline
> -int i915_get_vma_pages(struct i915_vma *vma)
> +int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> {
> if (vma->ggtt_view.pages)
> return 0;
> @@ -2394,7 +2427,7 @@ int i915_get_vma_pages(struct i915_vma *vma)
> vma->ggtt_view.type);
>
> if (!vma->ggtt_view.pages) {
> - DRM_ERROR("Failed to get pages for VMA view type %u!\n",
> + DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
> vma->ggtt_view.type);
> return -EINVAL;
> }
> @@ -2415,10 +2448,12 @@ int i915_get_vma_pages(struct i915_vma *vma)
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> u32 flags)
> {
> - int ret = i915_get_vma_pages(vma);
> + if (i915_is_ggtt(vma->vm)) {
> + int ret = i915_get_ggtt_vma_pages(vma);
>
> - if (ret)
> - return ret;
> + if (ret)
> + return ret;
> + }
>
> vma->bind_vma(vma, cache_level, flags);
>
> --
> 1.9.3
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 17:50 ` Daniel Vetter
@ 2015-03-17 5:31 ` Joonas Lahtinen
2015-03-17 5:34 ` Lahtinen, Joonas
0 siblings, 1 reply; 10+ messages in thread
From: Joonas Lahtinen @ 2015-03-17 5:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote:
> On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> [snip]
> > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > u32 flags);
> > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> >
> > void i915_gem_restore_fences(struct drm_device *dev);
> >
> > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm,
> > - enum i915_ggtt_view_type view);
> > -static inline
> > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm)
> > +static inline bool i915_is_ggtt(struct i915_address_space *vm);
>
> This forward decl seems unneeded leftover from earlier patch iterations.
> I've removed it and merged the patch.
>
> Thanks, Daniel
>
> > +
> > +unsigned long
> > +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type view);
> > +unsigned long
> > +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm);
> > +static inline unsigned long
> > +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> > {
> > - return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> > + return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
> > }
> > +
> > bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> > -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm,
> > - enum i915_ggtt_view_type view);
> > -static inline
> > +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type view);
> > bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> > -}
> > + struct i915_address_space *vm);
> >
> > unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> > struct i915_address_space *vm);
> > -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view);
> > -static inline
> > -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> > -}
> > -
> > struct i915_vma *
> > -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view);
> > +i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
> >
> > -static inline
> > struct i915_vma *
> > i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm)
> > -{
> > - return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> > - &i915_ggtt_view_normal);
> > -}
> > + struct i915_address_space *vm);
> > +struct i915_vma *
> > +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view);
> >
> > -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> > -static inline bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj) {
> > - struct i915_vma *vma;
> > - list_for_each_entry(vma, &obj->vma_list, vma_link)
> > - if (vma->pin_count > 0)
> > - return true;
> > - return false;
> > +static inline struct i915_vma *
> > +i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> > +{
> > + return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
> > }
> > +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
> >
> > /* Some GGTT VM helpers */
> > #define i915_obj_to_ggtt(obj) \
> > @@ -2876,13 +2860,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
> >
> > static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> > {
> > - return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
> > -}
> > -
> > -static inline unsigned long
> > -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> > -{
> > - return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> > + return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
> > }
> >
> > static inline unsigned long
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0fe313d..e2876bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3518,9 +3518,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
> > static struct i915_vma *
> > i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > struct i915_address_space *vm,
> > + const struct i915_ggtt_view *ggtt_view,
> > unsigned alignment,
> > - uint64_t flags,
> > - const struct i915_ggtt_view *view)
> > + uint64_t flags)
> > {
> > struct drm_device *dev = obj->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3532,6 +3532,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > struct i915_vma *vma;
> > int ret;
> >
> > + if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > + return ERR_PTR(-EINVAL);
> > +
> > fence_size = i915_gem_get_gtt_size(dev,
> > obj->base.size,
> > obj->tiling_mode);
> > @@ -3570,7 +3573,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >
> > i915_gem_object_pin_pages(obj);
> >
> > - vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
> > + vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> > + i915_gem_obj_lookup_or_create_vma(obj, vm);
> > +
> > if (IS_ERR(vma))
> > goto err_unpin;
> >
> > @@ -4167,12 +4172,12 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
> > return false;
> > }
> >
> > -int
> > -i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - uint32_t alignment,
> > - uint64_t flags,
> > - const struct i915_ggtt_view *view)
> > +static int
> > +i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > + const struct i915_ggtt_view *ggtt_view,
> > + uint32_t alignment,
> > + uint64_t flags)
> > {
> > struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > struct i915_vma *vma;
> > @@ -4188,17 +4193,29 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> > if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> > return -EINVAL;
> >
> > - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> > + if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > + return -EINVAL;
> > +
> > + vma = ggtt_view ? i915_gem_obj_to_ggtt_view(obj, ggtt_view) :
> > + i915_gem_obj_to_vma(obj, vm);
> > +
> > + if (IS_ERR(vma))
> > + return PTR_ERR(vma);
> > +
> > if (vma) {
> > if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> > return -EBUSY;
> >
> > if (i915_vma_misplaced(vma, alignment, flags)) {
> > + unsigned long offset;
> > + offset = ggtt_view ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> > + i915_gem_obj_offset(obj, vm);
> > WARN(vma->pin_count,
> > - "bo is already pinned with incorrect alignment:"
> > + "bo is already pinned in %s with incorrect alignment:"
> > " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
> > " obj->map_and_fenceable=%d\n",
> > - i915_gem_obj_offset_view(obj, vm, view->type),
> > + ggtt_view ? "ggtt" : "ppgtt",
> > + offset,
> > alignment,
> > !!(flags & PIN_MAPPABLE),
> > obj->map_and_fenceable);
> > @@ -4212,8 +4229,8 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >
> > bound = vma ? vma->bound : 0;
> > if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> > - vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
> > - flags, view);
> > + vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
> > + flags);
> > if (IS_ERR(vma))
> > return PTR_ERR(vma);
> > }
> > @@ -4254,6 +4271,30 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> > return 0;
> > }
> >
> > +int
> > +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > + uint32_t alignment,
> > + uint64_t flags)
> > +{
> > + return i915_gem_object_do_pin(obj, vm,
> > + i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL,
> > + alignment, flags);
> > +}
> > +
> > +int
> > +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view,
> > + uint32_t alignment,
> > + uint64_t flags)
> > +{
> > + if (WARN_ONCE(!view, "no view specified"))
> > + return -EINVAL;
> > +
> > + return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
> > + alignment, flags);
> > +}
> > +
> > void
> > i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> > {
> > @@ -4559,15 +4600,32 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > intel_runtime_pm_put(dev_priv);
> > }
> >
> > -struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view)
> > +struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > {
> > struct i915_vma *vma;
> > - list_for_each_entry(vma, &obj->vma_list, vma_link)
> > - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> > + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > + continue;
> > + if (vma->vm == vm)
> > return vma;
> > + }
> > + return NULL;
> > +}
> >
> > +struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
> > + const struct i915_ggtt_view *view)
> > +{
> > + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> > + struct i915_vma *vma;
> > +
> > + if (WARN_ONCE(!view, "no view specified"))
> > + return ERR_PTR(-EINVAL);
> > +
> > + list_for_each_entry(vma, &obj->vma_list, vma_link)
> > + if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
> > + return vma;
> > return NULL;
> > }
> >
> > @@ -5176,9 +5234,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
> > }
> >
> > /* All the new VM stuff */
> > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm,
> > - enum i915_ggtt_view_type view)
> > +unsigned long
> > +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm)
> > {
> > struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > struct i915_vma *vma;
> > @@ -5186,23 +5244,58 @@ unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
> >
> > list_for_each_entry(vma, &o->vma_list, vma_link) {
> > - if (vma->vm == vm && vma->ggtt_view.type == view)
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > + continue;
> > + if (vma->vm == vm)
> > return vma->node.start;
> > -
> > }
> > +
> > WARN(1, "%s vma for this object not found.\n",
> > i915_is_ggtt(vm) ? "global" : "ppgtt");
> > return -1;
> > }
> >
> > -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> > - struct i915_address_space *vm,
> > - enum i915_ggtt_view_type view)
> > +unsigned long
> > +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type view)
> > {
> > + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> > + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> > struct i915_vma *vma;
> >
> > list_for_each_entry(vma, &o->vma_list, vma_link)
> > - if (vma->vm == vm &&
> > + if (vma->vm == ggtt && vma->ggtt_view.type == view)
> > + return vma->node.start;
> > +
> > + WARN(1, "global vma for this object not found.\n");
> > + return -1;
> > +}
> > +
> > +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> > + struct i915_address_space *vm)
> > +{
> > + struct i915_vma *vma;
> > +
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > + continue;
> > + if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> > + enum i915_ggtt_view_type view)
> > +{
> > + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> > + struct i915_vma *vma;
> > +
> > + list_for_each_entry(vma, &o->vma_list, vma_link)
> > + if (vma->vm == ggtt &&
> > vma->ggtt_view.type == view &&
> > drm_mm_node_allocated(&vma->node))
> > return true;
> > @@ -5231,10 +5324,13 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> >
> > BUG_ON(list_empty(&o->vma_list));
> >
> > - list_for_each_entry(vma, &o->vma_list, vma_link)
> > + list_for_each_entry(vma, &o->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > + continue;
> > if (vma->vm == vm)
> > return vma->node.size;
> > -
> > + }
> > return 0;
> > }
> >
> > @@ -5334,15 +5430,16 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
> > return NOTIFY_DONE;
> > }
> >
> > -struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> > +bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
> > {
> > - struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> > struct i915_vma *vma;
> > -
> > - list_for_each_entry(vma, &obj->vma_list, vma_link)
> > - if (vma->vm == ggtt &&
> > - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> > - return vma;
> > -
> > - return NULL;
> > + list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > + if (i915_is_ggtt(vma->vm) &&
> > + vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
> > + continue;
> > + if (vma->pin_count > 0)
> > + return true;
> > + }
> > + return false;
> > }
> > +
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 2034f7c..f1b9ea6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -67,8 +67,9 @@
> > * i915_ggtt_view_type and struct i915_ggtt_view.
> > *
> > * A new flavour of core GEM functions which work with GGTT bound objects were
> > - * added with the _view suffix. They take the struct i915_ggtt_view parameter
> > - * encapsulating all metadata required to implement a view.
> > + * added with the _ggtt_ infix, and sometimes with _view postfix to avoid
> > + * renaming in large amounts of code. They take the struct i915_ggtt_view
> > + * parameter encapsulating all metadata required to implement a view.
> > *
> > * As a helper for callers which are only interested in the normal view,
> > * globally const i915_ggtt_view_normal singleton instance exists. All old core
> > @@ -1726,11 +1727,15 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> > struct drm_device *dev = vma->vm->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj = vma->obj;
> > + struct sg_table *pages = obj->pages;
> >
> > /* Currently applicable only to VLV */
> > if (obj->gt_ro)
> > flags |= PTE_READ_ONLY;
> >
> > + if (i915_is_ggtt(vma->vm))
> > + pages = vma->ggtt_view.pages;
> > +
> > /* If there is no aliasing PPGTT, or the caller needs a global mapping,
> > * or we have a global mapping already but the cacheability flags have
> > * changed, set the global PTEs.
> > @@ -1745,7 +1750,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> > if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> > if (!(vma->bound & GLOBAL_BIND) ||
> > (cache_level != obj->cache_level)) {
> > - vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
> > + vma->vm->insert_entries(vma->vm, pages,
> > vma->node.start,
> > cache_level, flags);
> > vma->bound |= GLOBAL_BIND;
> > @@ -1756,8 +1761,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> > (!(vma->bound & LOCAL_BIND) ||
> > (cache_level != obj->cache_level))) {
> > struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> > - appgtt->base.insert_entries(&appgtt->base,
> > - vma->ggtt_view.pages,
> > + appgtt->base.insert_entries(&appgtt->base, pages,
> > vma->node.start,
> > cache_level, flags);
> > vma->bound |= LOCAL_BIND;
> > @@ -2331,23 +2335,28 @@ int i915_gem_gtt_init(struct drm_device *dev)
> > return 0;
> > }
> >
> > -static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > - const struct i915_ggtt_view *view)
> > +static struct i915_vma *
> > +__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm,
> > + const struct i915_ggtt_view *ggtt_view)
> > {
> > struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> > if (vma == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > + if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> > + return ERR_PTR(-EINVAL);
> > +
> > INIT_LIST_HEAD(&vma->vma_link);
> > INIT_LIST_HEAD(&vma->mm_list);
> > INIT_LIST_HEAD(&vma->exec_list);
> > vma->vm = vm;
> > vma->obj = obj;
> > - vma->ggtt_view = *view;
> >
> > if (INTEL_INFO(vm->dev)->gen >= 6) {
> > if (i915_is_ggtt(vm)) {
> > + vma->ggtt_view = *ggtt_view;
> > +
> > vma->unbind_vma = ggtt_unbind_vma;
> > vma->bind_vma = ggtt_bind_vma;
> > } else {
> > @@ -2356,6 +2365,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > }
> > } else {
> > BUG_ON(!i915_is_ggtt(vm));
> > + vma->ggtt_view = *ggtt_view;
> > vma->unbind_vma = i915_ggtt_unbind_vma;
> > vma->bind_vma = i915_ggtt_bind_vma;
> > }
> > @@ -2368,21 +2378,44 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> > }
> >
> > struct i915_vma *
> > -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> > - struct i915_address_space *vm,
> > +i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> > + struct i915_address_space *vm)
> > +{
> > + struct i915_vma *vma;
> > +
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > + if (!vma)
> > + vma = __i915_gem_vma_create(obj, vm,
> > + i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL);
> > +
> > + return vma;
> > +}
> > +
> > +struct i915_vma *
> > +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> > const struct i915_ggtt_view *view)
> > {
> > + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> > struct i915_vma *vma;
> >
> > - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> > + if (WARN_ON(!view))
> > + return ERR_PTR(-EINVAL);
> > +
> > + vma = i915_gem_obj_to_ggtt_view(obj, view);
> > +
> > + if (IS_ERR(vma))
> > + return vma;
> > +
> > if (!vma)
> > - vma = __i915_gem_vma_create(obj, vm, view);
> > + vma = __i915_gem_vma_create(obj, ggtt, view);
> >
> > return vma;
> > +
> > }
> >
> > +
> > static inline
> > -int i915_get_vma_pages(struct i915_vma *vma)
> > +int i915_get_ggtt_vma_pages(struct i915_vma *vma)
> > {
> > if (vma->ggtt_view.pages)
> > return 0;
> > @@ -2394,7 +2427,7 @@ int i915_get_vma_pages(struct i915_vma *vma)
> > vma->ggtt_view.type);
> >
> > if (!vma->ggtt_view.pages) {
> > - DRM_ERROR("Failed to get pages for VMA view type %u!\n",
> > + DRM_ERROR("Failed to get pages for GGTT view type %u!\n",
> > vma->ggtt_view.type);
> > return -EINVAL;
> > }
> > @@ -2415,10 +2448,12 @@ int i915_get_vma_pages(struct i915_vma *vma)
> > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > u32 flags)
> > {
> > - int ret = i915_get_vma_pages(vma);
> > + if (i915_is_ggtt(vma->vm)) {
> > + int ret = i915_get_ggtt_vma_pages(vma);
> >
> > - if (ret)
> > - return ret;
> > + if (ret)
> > + return ret;
> > + }
> >
> > vma->bind_vma(vma, cache_level, flags);
> >
> > --
> > 1.9.3
> >
> >
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-17 5:31 ` Joonas Lahtinen
@ 2015-03-17 5:34 ` Lahtinen, Joonas
0 siblings, 0 replies; 10+ messages in thread
From: Lahtinen, Joonas @ 2015-03-17 5:34 UTC (permalink / raw)
To: daniel; +Cc: intel-gfx
On ti, 2015-03-17 at 07:31 +0200, Joonas Lahtinen wrote:
> On ma, 2015-03-16 at 18:50 +0100, Daniel Vetter wrote:
> > On Mon, Mar 16, 2015 at 02:11:13PM +0200, Joonas Lahtinen wrote:
> > [snip]
> > > int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > > u32 flags);
> > > @@ -2800,60 +2796,48 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> > >
> > > void i915_gem_restore_fences(struct drm_device *dev);
> > >
> > > -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> > > - struct i915_address_space *vm,
> > > - enum i915_ggtt_view_type view);
> > > -static inline
> > > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> > > - struct i915_address_space *vm)
> > > +static inline bool i915_is_ggtt(struct i915_address_space *vm);
Indeed it is. Thanks!
Regards, Joonas
PS. Sorry for spam, the enter key is so close to the space bar.
> >
> > This forward decl seems unneeded leftover from earlier patch iterations.
> > I've removed it and merged the patch.
> >
> > Thanks, Daniel
> >
> > [snip]
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-16 12:11 [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Joonas Lahtinen
` (2 preceding siblings ...)
2015-03-16 17:50 ` Daniel Vetter
@ 2015-03-17 14:19 ` Tvrtko Ursulin
2015-03-18 8:22 ` Daniel Vetter
3 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2015-03-17 14:19 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx
Hi,
On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> v2:
> - Dropped _ppgtt_ infixes, allow both types to be passed
> - Disregard other but normal views when no view is specified
> - More checks that valid parameters are passed
> - More readable error checking
>
> v3:
> - Prefer WARN_ONCE over BUG_ON when there is code path for failure
[snip]
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view)
> {
> + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
Unused variable slipped through, oops! :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
2015-03-17 14:19 ` Tvrtko Ursulin
@ 2015-03-18 8:22 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2015-03-18 8:22 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Tue, Mar 17, 2015 at 02:19:18PM +0000, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 03/16/2015 12:11 PM, Joonas Lahtinen wrote:
> >GGTT views are only applicable when dealing with GGTT. Change the code to
> >reject ggtt_view where it should not be used and require it when it should
> >be.
> >
> >v2:
> >- Dropped _ppgtt_ infixes, allow both types to be passed
> >- Disregard other but normal views when no view is specified
> >- More checks that valid parameters are passed
> >- More readable error checking
> >
> >v3:
> >- Prefer WARN_ONCE over BUG_ON when there is code path for failure
>
> [snip]
>
> >+unsigned long
> >+i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> >+ enum i915_ggtt_view_type view)
> > {
> >+ struct drm_i915_private *dev_priv = o->base.dev->dev_private;
>
> Unused variable slipped through, oops! :)
Indeed, fixed up.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-03-18 8:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-16 12:11 [PATCH v3] drm/i915: Do not use ggtt_view with (aliasing) PPGTT Joonas Lahtinen
2015-03-16 13:26 ` Tvrtko Ursulin
2015-03-16 14:48 ` Joonas Lahtinen
2015-03-16 14:50 ` Tvrtko Ursulin
2015-03-16 14:52 ` shuang.he
2015-03-16 17:50 ` Daniel Vetter
2015-03-17 5:31 ` Joonas Lahtinen
2015-03-17 5:34 ` Lahtinen, Joonas
2015-03-17 14:19 ` Tvrtko Ursulin
2015-03-18 8:22 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.