Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Wed, Jul 17, 2013 at 07:31:37PM -0700, Ben Widawsky wrote: On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote: Big-bada-boom; Just from looking at the code I think I see a bug. A bug which didn't exist in the original version of the code, and doesn't exist after the very next patch in the overall series. Indeed, that was the bug. Now I am terribly curious - why in the world (if that's indeed the bug) can I not seem to hit this locally on my machine? I'll send the patch for the fix now, but I'd really like to know what's different in our setup. I've tried, UXA, SNA, and the igt test suite... Just requires testing on the forgotten non-LLC generations. Or Baytrail. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Wed, 2013-07-17 at 12:19 -0700, Ben Widawsky wrote: Formerly: drm/i915: Create VMAs (part 1) In a previous patch, the notion of a VM was introduced. A VMA describes an area of part of the VM address space. A VMA is similar to the concept in the linux mm. However, instead of representing regular memory, a VMA is backed by a GEM BO. There may be many VMAs for a given object, one for each VM the object is to be used in. This may occur through flink, dma-buf, or a number of other transient states. Currently the code depends on only 1 VMA per object, for the global GTT (and aliasing PPGTT). The following patches will address this and make the rest of the infrastructure more suited v2: s/i915_obj/i915_gem_obj (Chris) v3: Only move an object to the now global unbound list if there are no more VMAs for the object which are bound into a VM (ie. the list is empty). v4: killed obj-gtt_space some reworks due to rebase v5: Free vma on error path (Imre) v6: Another missed vma free in i915_gem_object_bind_to_gtt error path (Imre) Fixed vma freeing in stolen preallocation (Imre) Signed-off-by: Ben Widawsky b...@bwidawsk.net Looks ok, so on patches 5-6: Reviewed-by: Imre Deak imre.d...@intel.com --- drivers/gpu/drm/i915/i915_drv.h| 48 +- drivers/gpu/drm/i915/i915_gem.c| 74 +++--- drivers/gpu/drm/i915/i915_gem_evict.c | 12 -- drivers/gpu/drm/i915/i915_gem_gtt.c| 5 ++- drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +-- 5 files changed, 120 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b3ba428..1a32412 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -533,6 +533,17 @@ struct i915_hw_ppgtt { int (*enable)(struct drm_device *dev); }; +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime + * will always be = an objects lifetime. So object refcounting should cover us. + */ +struct i915_vma { + struct drm_mm_node node; + struct drm_i915_gem_object *obj; + struct i915_address_space *vm; + + struct list_head vma_link; /* Link in the object's VMA list */ +}; + struct i915_ctx_hang_stats { /* This context had batch pending when hang was declared */ unsigned batch_pending; @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object { const struct drm_i915_gem_object_ops *ops; - /** Current space allocated to this object in the GTT, if any. */ - struct drm_mm_node gtt_space; + /** List of VMAs backed by this object */ + struct list_head vma_list; + /** Stolen memory for this object, instead of being backed by shmem. */ struct drm_mm_node *stolen; struct list_head global_list; @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object { #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) -/* Offset of the first PTE pointing to this object */ -static inline unsigned long -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +/* This is a temporary define to help transition us to real VMAs. If you see + * this, you're either reviewing code, or bisecting it. */ +static inline struct i915_vma * +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj) { - return o-gtt_space.start; + if (list_empty(obj-vma_list)) + return NULL; + return list_first_entry(obj-vma_list, struct i915_vma, vma_link); } /* Whether or not this object is currently mapped by the translation tables */ static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) { - return drm_mm_node_allocated(o-gtt_space); + struct i915_vma *vma = __i915_gem_obj_to_vma(o); + if (vma == NULL) + return false; + return drm_mm_node_allocated(vma-node); +} + +/* Offset of the first PTE pointing to this object */ +static inline unsigned long +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +{ + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.start; } /* The size used in the translation tables may be larger than the actual size of @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) static inline unsigned long i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o) { - return o-gtt_space.size; + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.size; } static inline void i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o, enum i915_cache_level color) { - o-gtt_space.color = color; + __i915_gem_obj_to_vma(o)-node.color = color; } /** @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
[Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
Formerly: drm/i915: Create VMAs (part 1) In a previous patch, the notion of a VM was introduced. A VMA describes an area of part of the VM address space. A VMA is similar to the concept in the linux mm. However, instead of representing regular memory, a VMA is backed by a GEM BO. There may be many VMAs for a given object, one for each VM the object is to be used in. This may occur through flink, dma-buf, or a number of other transient states. Currently the code depends on only 1 VMA per object, for the global GTT (and aliasing PPGTT). The following patches will address this and make the rest of the infrastructure more suited v2: s/i915_obj/i915_gem_obj (Chris) v3: Only move an object to the now global unbound list if there are no more VMAs for the object which are bound into a VM (ie. the list is empty). v4: killed obj-gtt_space some reworks due to rebase v5: Free vma on error path (Imre) v6: Another missed vma free in i915_gem_object_bind_to_gtt error path (Imre) Fixed vma freeing in stolen preallocation (Imre) Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h| 48 +- drivers/gpu/drm/i915/i915_gem.c| 74 +++--- drivers/gpu/drm/i915/i915_gem_evict.c | 12 -- drivers/gpu/drm/i915/i915_gem_gtt.c| 5 ++- drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +-- 5 files changed, 120 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b3ba428..1a32412 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -533,6 +533,17 @@ struct i915_hw_ppgtt { int (*enable)(struct drm_device *dev); }; +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime + * will always be = an objects lifetime. So object refcounting should cover us. + */ +struct i915_vma { + struct drm_mm_node node; + struct drm_i915_gem_object *obj; + struct i915_address_space *vm; + + struct list_head vma_link; /* Link in the object's VMA list */ +}; + struct i915_ctx_hang_stats { /* This context had batch pending when hang was declared */ unsigned batch_pending; @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object { const struct drm_i915_gem_object_ops *ops; - /** Current space allocated to this object in the GTT, if any. */ - struct drm_mm_node gtt_space; + /** List of VMAs backed by this object */ + struct list_head vma_list; + /** Stolen memory for this object, instead of being backed by shmem. */ struct drm_mm_node *stolen; struct list_head global_list; @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object { #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) -/* Offset of the first PTE pointing to this object */ -static inline unsigned long -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +/* This is a temporary define to help transition us to real VMAs. If you see + * this, you're either reviewing code, or bisecting it. */ +static inline struct i915_vma * +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj) { - return o-gtt_space.start; + if (list_empty(obj-vma_list)) + return NULL; + return list_first_entry(obj-vma_list, struct i915_vma, vma_link); } /* Whether or not this object is currently mapped by the translation tables */ static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) { - return drm_mm_node_allocated(o-gtt_space); + struct i915_vma *vma = __i915_gem_obj_to_vma(o); + if (vma == NULL) + return false; + return drm_mm_node_allocated(vma-node); +} + +/* Offset of the first PTE pointing to this object */ +static inline unsigned long +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +{ + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.start; } /* The size used in the translation tables may be larger than the actual size of @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) static inline unsigned long i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o) { - return o-gtt_space.size; + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.size; } static inline void i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o, enum i915_cache_level color) { - o-gtt_space.color = color; + __i915_gem_obj_to_vma(o)-node.color = color; } /** @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, size_t size); void i915_gem_free_object(struct drm_gem_object *obj); +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj, +struct
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote: Formerly: drm/i915: Create VMAs (part 1) In a previous patch, the notion of a VM was introduced. A VMA describes an area of part of the VM address space. A VMA is similar to the concept in the linux mm. However, instead of representing regular memory, a VMA is backed by a GEM BO. There may be many VMAs for a given object, one for each VM the object is to be used in. This may occur through flink, dma-buf, or a number of other transient states. Currently the code depends on only 1 VMA per object, for the global GTT (and aliasing PPGTT). The following patches will address this and make the rest of the infrastructure more suited v2: s/i915_obj/i915_gem_obj (Chris) v3: Only move an object to the now global unbound list if there are no more VMAs for the object which are bound into a VM (ie. the list is empty). v4: killed obj-gtt_space some reworks due to rebase v5: Free vma on error path (Imre) v6: Another missed vma free in i915_gem_object_bind_to_gtt error path (Imre) Fixed vma freeing in stolen preallocation (Imre) Signed-off-by: Ben Widawsky b...@bwidawsk.net Entire series merged to dinq, thanks a lot for the patches and review. On to the next step in this journey then! Cheers, Daniel --- drivers/gpu/drm/i915/i915_drv.h| 48 +- drivers/gpu/drm/i915/i915_gem.c| 74 +++--- drivers/gpu/drm/i915/i915_gem_evict.c | 12 -- drivers/gpu/drm/i915/i915_gem_gtt.c| 5 ++- drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +-- 5 files changed, 120 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b3ba428..1a32412 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -533,6 +533,17 @@ struct i915_hw_ppgtt { int (*enable)(struct drm_device *dev); }; +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime + * will always be = an objects lifetime. So object refcounting should cover us. + */ +struct i915_vma { + struct drm_mm_node node; + struct drm_i915_gem_object *obj; + struct i915_address_space *vm; + + struct list_head vma_link; /* Link in the object's VMA list */ +}; + struct i915_ctx_hang_stats { /* This context had batch pending when hang was declared */ unsigned batch_pending; @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object { const struct drm_i915_gem_object_ops *ops; - /** Current space allocated to this object in the GTT, if any. */ - struct drm_mm_node gtt_space; + /** List of VMAs backed by this object */ + struct list_head vma_list; + /** Stolen memory for this object, instead of being backed by shmem. */ struct drm_mm_node *stolen; struct list_head global_list; @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object { #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) -/* Offset of the first PTE pointing to this object */ -static inline unsigned long -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +/* This is a temporary define to help transition us to real VMAs. If you see + * this, you're either reviewing code, or bisecting it. */ +static inline struct i915_vma * +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj) { - return o-gtt_space.start; + if (list_empty(obj-vma_list)) + return NULL; + return list_first_entry(obj-vma_list, struct i915_vma, vma_link); } /* Whether or not this object is currently mapped by the translation tables */ static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) { - return drm_mm_node_allocated(o-gtt_space); + struct i915_vma *vma = __i915_gem_obj_to_vma(o); + if (vma == NULL) + return false; + return drm_mm_node_allocated(vma-node); +} + +/* Offset of the first PTE pointing to this object */ +static inline unsigned long +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o) +{ + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.start; } /* The size used in the translation tables may be larger than the actual size of @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o) static inline unsigned long i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o) { - return o-gtt_space.size; + BUG_ON(list_empty(o-vma_list)); + return __i915_gem_obj_to_vma(o)-node.size; } static inline void i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o, enum i915_cache_level color) { - o-gtt_space.color = color; + __i915_gem_obj_to_vma(o)-node.color = color; } /** @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, struct drm_i915_gem_object
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote: On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote: Formerly: drm/i915: Create VMAs (part 1) In a previous patch, the notion of a VM was introduced. A VMA describes an area of part of the VM address space. A VMA is similar to the concept in the linux mm. However, instead of representing regular memory, a VMA is backed by a GEM BO. There may be many VMAs for a given object, one for each VM the object is to be used in. This may occur through flink, dma-buf, or a number of other transient states. Currently the code depends on only 1 VMA per object, for the global GTT (and aliasing PPGTT). The following patches will address this and make the rest of the infrastructure more suited v2: s/i915_obj/i915_gem_obj (Chris) v3: Only move an object to the now global unbound list if there are no more VMAs for the object which are bound into a VM (ie. the list is empty). v4: killed obj-gtt_space some reworks due to rebase v5: Free vma on error path (Imre) v6: Another missed vma free in i915_gem_object_bind_to_gtt error path (Imre) Fixed vma freeing in stolen preallocation (Imre) Big-bada-boom; Just from looking at the code I think I see a bug. A bug which didn't exist in the original version of the code, and doesn't exist after the very next patch in the overall series. Now I am terribly curious - why in the world (if that's indeed the bug) can I not seem to hit this locally on my machine? I'll send the patch for the fix now, but I'd really like to know what's different in our setup. I've tried, UXA, SNA, and the igt test suite... set-cache-level needs to iterate over vma, and in particular should not dereference a non-existent one. Or if we decided that set-cache-level was a ggtt only property, just not explode if there is no global vma. -Chris The current state is that cache level is still per object, but this function itself will iterate over all VMAs. As I said, it happens in the very next patch in the series. In the original patch series cover letter, I made cache levels per VMA an optional TODO. For the time being, I don't see much benefit. -- Chris Wilson, Intel Open Source Technology Centre -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx