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

2015-12-16 Thread Chris Wilson 
On Wed, Dec 16, 2015 at 05:16:19PM +, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 14/12/15 11:36, 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.
> >
> >All told, less code, simpler and faster, and more extensible.
> 
> I get the general idea but it seems unfinished. For example there is
> no handling for last_write and last_fence. So as for bisectability
> requirement it does not seem appropriate.

Pardon. It is there, working and complete. If it wasn't for rebasing, it
would have ~10 months of testing behind it. Plus comparable
implementations in userspace.

last_fence is no a no-op (we just need to track the request, we don't
need to anything special when it completes, I provided a stub callback
for the rare case rather than have a conditional function call),
last_write just needs to update the status on the framebuffer object.
 
> I also wanted to suggest splitting out the req->list to link
> renaming but see you've already done it in your branch.
> 
> I915_BO_ACTIVE_SHIFT is undefined up to and including this patch.

The curse of rebasing.

> Variable naming conventions for requests is still a mess but whatever.

I know, I am slowly fixing it up.
 
> And I don't like drm_i915_gem_request_active, wouldn't
> drm_i915_gem_active_request be better?

Yeah, active_request is better. Still lacks something.
-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 02/11] drm/i915: Refactor activity tracking for requests

2015-12-16 Thread Tvrtko Ursulin 


Hi,

On 14/12/15 11:36, 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.

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


I get the general idea but it seems unfinished. For example there is no 
handling for last_write and last_fence. So as for bisectability 
requirement it does not seem appropriate.


I also wanted to suggest splitting out the req->list to link renaming 
but see you've already done it in your branch.


I915_BO_ACTIVE_SHIFT is undefined up to and including this patch.

Variable naming conventions for requests is still a mess but whatever.

And I don't like drm_i915_gem_request_active, wouldn't 
drm_i915_gem_active_request be better?


Regards,

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

[Intel-gfx] [PATCH 02/11] drm/i915: Refactor activity tracking for requests

2015-12-14 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.

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

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/Makefile   |   1 -
 drivers/gpu/drm/i915/i915_debugfs.c |   4 +-
 drivers/gpu/drm/i915/i915_drv.h |  10 ---
 drivers/gpu/drm/i915/i915_gem.c | 149 
 drivers/gpu/drm/i915/i915_gem_debug.c   |  70 ---
 drivers/gpu/drm/i915/i915_gem_fence.c   |  10 +--
 drivers/gpu/drm/i915/i915_gem_request.c |  39 ++---
 drivers/gpu/drm/i915/i915_gem_request.h |  26 +-
 drivers/gpu/drm/i915/i915_gpu_error.c   |   4 +-
 drivers/gpu/drm/i915/intel_lrc.c|   1 -
 drivers/gpu/drm/i915/intel_ringbuffer.c |   7 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h |  12 ---
 12 files changed, 93 insertions(+), 240 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 3df88be4752e..14794370db44 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -21,7 +21,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_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index dafc58b94c9f..68310ba73d23 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -695,13 +695,13 @@ static int i915_gem_request_info(struct seq_file *m, void 
*data)
int count;
 
count = 0;
-   list_for_each_entry(req, >request_list, list)
+   list_for_each_entry(req, >request_list, link)
count++;
if (count == 0)
continue;
 
seq_printf(m, "%s requests: %d\n", ring->name, count);
-   list_for_each_entry(req, >request_list, list) {
+   list_for_each_entry(req, >request_list, link) {
struct task_struct *task;
 
rcu_read_lock();
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43de7c2c6b3e..dbcb7659ba2b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -444,8 +444,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;
@@ -2014,7 +2012,6 @@ struct drm_i915_gem_object {
struct drm_mm_node *stolen;
struct list_head global_list;
 
-   struct list_head ring_list[I915_NUM_RINGS];
/** Used in execbuf to temporarily hold a ref */
struct list_head obj_exec_link;
 
@@ -3079,13 +3076,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 */
 int i915_debugfs_init(struct drm_minor *minor);
 void i915_debugfs_cleanup(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b412b9291d91..a7c39d5fbcca 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -38,14 +38,8 @@
 #include 
 #include 
 
-#define RQ_BUG_ON(expr)
-
 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 ring);
 
 static bool cpu_cache_is_coherent(struct drm_device *dev,
  enum i915_cache_level level)
@@ -119,7 +113,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
if (ret)
return ret;
 
-   WARN_ON(i915_verify_lists(dev));