Re: [Intel-gfx] [PATCH 36/73] drm/i915: Refactor activity tracking for requests

2016-08-01 Thread Joonas Lahtinen
On ma, 2016-08-01 at 10:10 +0100, Chris Wilson wrote:
> With the introduction of requests, we amplified the number of atomic
> refcounted objects we use and update every execbuffer; from none to
> several references, and a set of references that need to be changed. We
> also introduced interesting side-effects in the order of retiring
> requests and objects.
> 
> Instead of independently tracking the last request for an object, track
> the active objects for each request. The object will reside in the
> buffer list of its most recent active request and so we reduce the kref
> interchange to a list_move. Now retirements are entirely driven by the
> request, dramatically simplifying activity tracking on the object
> themselves, and removing the ambiguity between retiring objects and
> retiring requests.
> 
> Furthermore with the consolidation of managing the activity tracking
> centrally, we can look forward to using RCU to enable lockless lookup of
> the current active requests for an object. In the future, we will be
> able to query the status or wait upon rendering to an object without
> even touching the struct_mutex BKL.
> 
> All told, less code, simpler and faster, and more extensible.
> 
> v2: Add a typedef for the function pointer for convenience later.
> v3: Make the noop retirement callback explicit. Allow passing NULL to
> the init_gem_active() which is expanded to a common noop function.

s/init_gem_active/init_request_active/

Assuming you kept the changes to those,

Reviewed-by: Joonas Lahtinen 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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


[Intel-gfx] [PATCH 36/73] drm/i915: Refactor activity tracking for requests

2016-08-01 Thread Chris Wilson
With the introduction of requests, we amplified the number of atomic
refcounted objects we use and update every execbuffer; from none to
several references, and a set of references that need to be changed. We
also introduced interesting side-effects in the order of retiring
requests and objects.

Instead of independently tracking the last request for an object, track
the active objects for each request. The object will reside in the
buffer list of its most recent active request and so we reduce the kref
interchange to a list_move. Now retirements are entirely driven by the
request, dramatically simplifying activity tracking on the object
themselves, and removing the ambiguity between retiring objects and
retiring requests.

Furthermore with the consolidation of managing the activity tracking
centrally, we can look forward to using RCU to enable lockless lookup of
the current active requests for an object. In the future, we will be
able to query the status or wait upon rendering to an object without
even touching the struct_mutex BKL.

All told, less code, simpler and faster, and more extensible.

v2: Add a typedef for the function pointer for convenience later.
v3: Make the noop retirement callback explicit. Allow passing NULL to
the init_gem_active() which is expanded to a common noop function.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/Makefile   |   1 -
 drivers/gpu/drm/i915/i915_drv.h |  10 ---
 drivers/gpu/drm/i915/i915_gem.c | 129 ++--
 drivers/gpu/drm/i915/i915_gem_debug.c   |  70 -
 drivers/gpu/drm/i915/i915_gem_fence.c   |  11 +--
 drivers/gpu/drm/i915/i915_gem_request.c |  50 +++--
 drivers/gpu/drm/i915/i915_gem_request.h |  97 +---
 drivers/gpu/drm/i915/intel_engine_cs.c  |   1 -
 drivers/gpu/drm/i915/intel_ringbuffer.h |  12 ---
 9 files changed, 138 insertions(+), 243 deletions(-)
 delete mode 100644 drivers/gpu/drm/i915/i915_gem_debug.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 6092f0ea24df..dda724f04445 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -25,7 +25,6 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
 i915-y += i915_cmd_parser.o \
  i915_gem_batch_pool.o \
  i915_gem_context.o \
- i915_gem_debug.o \
  i915_gem_dmabuf.o \
  i915_gem_evict.o \
  i915_gem_execbuffer.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 17a206f19fcb..6b57d8ff7218 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -432,8 +432,6 @@ void intel_link_compute_m_n(int bpp, int nlanes,
 #define DRIVER_MINOR   6
 #define DRIVER_PATCHLEVEL  0
 
-#define WATCH_LISTS0
-
 struct opregion_header;
 struct opregion_acpi;
 struct opregion_swsci;
@@ -2153,7 +2151,6 @@ struct drm_i915_gem_object {
struct drm_mm_node *stolen;
struct list_head global_list;
 
-   struct list_head engine_list[I915_NUM_ENGINES];
/** Used in execbuf to temporarily hold a ref */
struct list_head obj_exec_link;
 
@@ -3463,13 +3460,6 @@ static inline bool 
i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec
obj->tiling_mode != I915_TILING_NONE;
 }
 
-/* i915_gem_debug.c */
-#if WATCH_LISTS
-int i915_verify_lists(struct drm_device *dev);
-#else
-#define i915_verify_lists(dev) 0
-#endif
-
 /* i915_debugfs.c */
 #ifdef CONFIG_DEBUG_FS
 int i915_debugfs_register(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 320a8e40d21b..c47c9f95eafa 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -43,10 +43,6 @@
 
 static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object 
*obj);
 static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object 
*obj);
-static void
-i915_gem_object_retire__write(struct drm_i915_gem_object *obj);
-static void
-i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int engine);
 
 static bool cpu_cache_is_coherent(struct drm_device *dev,
  enum i915_cache_level level)
@@ -141,7 +137,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
if (ret)
return ret;
 
-   WARN_ON(i915_verify_lists(dev));
return 0;
 }
 
@@ -1339,23 +1334,6 @@ put_rpm:
return ret;
 }
 
-static void
-i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
-  struct drm_i915_gem_request *req)
-{
-   int idx = req->engine->id;
-
-   if (i915_gem_active_peek(&obj->last_read[idx],
-&obj->base.dev->struct_mutex) == req)
-   i915_gem_object_retire__read(obj, idx);
-   else if (i915_gem_active_peek(&obj->last_write,
- &obj->base.dev->struct_mut