Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs

2013-07-18 Thread Chris Wilson 
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

2013-07-18 Thread Imre Deak 
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

2013-07-17 Thread Ben Widawsky 
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

2013-07-17 Thread Daniel Vetter 
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

2013-07-17 Thread Ben Widawsky 
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