Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Chris Wilson 
On Fri, May 05, 2017 at 03:20:08PM +0100, Tvrtko Ursulin wrote:
> 
> On 05/05/2017 14:51, Chris Wilson wrote:
> >On Fri, May 05, 2017 at 02:37:41PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 05/05/2017 14:32, Chris Wilson wrote:
> >>>On Fri, May 05, 2017 at 02:19:07PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/05/2017 12:37, Chris Wilson wrote:
> >struct intel_engine_cs {
> >@@ -367,6 +373,7 @@ struct intel_engine_cs {
> >
> > /* Execlists */
> > struct tasklet_struct irq_tasklet;
> >+struct execlist_priolist default_priolist;
> > struct execlist_port {
> > struct drm_i915_gem_request *request_count;
> >#define EXECLIST_COUNT_BITS 2
> >
> >>>
> >>>Just a small bikeshed to consider. Having switched to
> >>>I915_PRIORITY_NORMAL, do we have a better name for default_priolist? I
> >>>still prefer default_priolist over normal_priolist. Go back to
> >>>I915_PRIORITY_DEFAULT?
> >>
> >>default_priolist is fine I think since it is dual purpose. Primary
> >>purpose to avoid allocations as you said.
> >>
> >>Although I am still a bit dejected how some userspace could decide
> >>one day to send everything at I915_PRIORITY_NORMAL - n, in order to
> >>use I915_PRIORITY_NORMAL as the high prio not requiring
> >>cap_sys_admin, and in doing so completely defeat the atomic kmalloc
> >>avoidance. :(
> >
> >Should we just bite the bullet and install a kmem_cache here?
> >It didn't solve the kmalloc error handling, but it does at least give us
> >a freelist. There is a reasonable argument that as soon as userspace
> >starts using non-default priorities, we may see many different levels
> >justifying allocating a whole slab upfront.
> 
> Actually my argument is pants since allocations only happen with
> "opening" a new priority level. So I think leave it as it is until
> there is a need.

Patch is ready for kmalloc showing up as a latency source.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Tvrtko Ursulin 


On 05/05/2017 15:04, Chris Wilson wrote:

On Fri, May 05, 2017 at 02:50:46PM +0100, Tvrtko Ursulin wrote:


On 03/05/2017 12:37, Chris Wilson wrote:

[snip]


+#include 
+
+static inline void __list_del_many(struct list_head *head,
+  struct list_head *first)
+{
+   head->next = first;
+   first->prev = head;
+}


Btw it is similar to __list_cut_position, but without the second
list you don't need and deleting the first entry which you do not
want.

So I am just thinking if consistent naming with the core function
would be beneficial like __list_cut_position_before/exclusive?


It's not a cut, as in we don't create two seperate lists. It is more of
splice with itself, though I did look at cut and wonder if it would
magically eliminate the unwanted paths but no..

Still favouring a list_del prefix. Once upon a time I just opencoded
this, it's far easily than trying to work out a good name :)


It's not __list_del_many either since it does not unlink the ones in the 
middle. ;) Doesn't matter, as you say, it is quite hard to find the 
ideal name.


Regards,

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

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Tvrtko Ursulin 


On 05/05/2017 14:51, Chris Wilson wrote:

On Fri, May 05, 2017 at 02:37:41PM +0100, Tvrtko Ursulin wrote:


On 05/05/2017 14:32, Chris Wilson wrote:

On Fri, May 05, 2017 at 02:19:07PM +0100, Tvrtko Ursulin wrote:


On 03/05/2017 12:37, Chris Wilson wrote:

struct intel_engine_cs {
@@ -367,6 +373,7 @@ struct intel_engine_cs {

/* Execlists */
struct tasklet_struct irq_tasklet;
+   struct execlist_priolist default_priolist;
struct execlist_port {
struct drm_i915_gem_request *request_count;
#define EXECLIST_COUNT_BITS 2



Just a small bikeshed to consider. Having switched to
I915_PRIORITY_NORMAL, do we have a better name for default_priolist? I
still prefer default_priolist over normal_priolist. Go back to
I915_PRIORITY_DEFAULT?


default_priolist is fine I think since it is dual purpose. Primary
purpose to avoid allocations as you said.

Although I am still a bit dejected how some userspace could decide
one day to send everything at I915_PRIORITY_NORMAL - n, in order to
use I915_PRIORITY_NORMAL as the high prio not requiring
cap_sys_admin, and in doing so completely defeat the atomic kmalloc
avoidance. :(


Should we just bite the bullet and install a kmem_cache here?
It didn't solve the kmalloc error handling, but it does at least give us
a freelist. There is a reasonable argument that as soon as userspace
starts using non-default priorities, we may see many different levels
justifying allocating a whole slab upfront.


Actually my argument is pants since allocations only happen with 
"opening" a new priority level. So I think leave it as it is until there 
is a need.


Regards,

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

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Chris Wilson 
On Fri, May 05, 2017 at 02:50:46PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/05/2017 12:37, Chris Wilson wrote:
> 
> [snip]
> 
> >+#include 
> >+
> >+static inline void __list_del_many(struct list_head *head,
> >+   struct list_head *first)
> >+{
> >+head->next = first;
> >+first->prev = head;
> >+}
> 
> Btw it is similar to __list_cut_position, but without the second
> list you don't need and deleting the first entry which you do not
> want.
> 
> So I am just thinking if consistent naming with the core function
> would be beneficial like __list_cut_position_before/exclusive?

It's not a cut, as in we don't create two seperate lists. It is more of
splice with itself, though I did look at cut and wonder if it would
magically eliminate the unwanted paths but no..

Still favouring a list_del prefix. Once upon a time I just opencoded
this, it's far easily than trying to work out a good name :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Chris Wilson 
On Fri, May 05, 2017 at 02:37:41PM +0100, Tvrtko Ursulin wrote:
> 
> On 05/05/2017 14:32, Chris Wilson wrote:
> >On Fri, May 05, 2017 at 02:19:07PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 03/05/2017 12:37, Chris Wilson wrote:
> >>>struct intel_engine_cs {
> >>>@@ -367,6 +373,7 @@ struct intel_engine_cs {
> >>>
> >>>   /* Execlists */
> >>>   struct tasklet_struct irq_tasklet;
> >>>+  struct execlist_priolist default_priolist;
> >>>   struct execlist_port {
> >>>   struct drm_i915_gem_request *request_count;
> >>>#define EXECLIST_COUNT_BITS 2
> >>>
> >
> >Just a small bikeshed to consider. Having switched to
> >I915_PRIORITY_NORMAL, do we have a better name for default_priolist? I
> >still prefer default_priolist over normal_priolist. Go back to
> >I915_PRIORITY_DEFAULT?
> 
> default_priolist is fine I think since it is dual purpose. Primary
> purpose to avoid allocations as you said.
> 
> Although I am still a bit dejected how some userspace could decide
> one day to send everything at I915_PRIORITY_NORMAL - n, in order to
> use I915_PRIORITY_NORMAL as the high prio not requiring
> cap_sys_admin, and in doing so completely defeat the atomic kmalloc
> avoidance. :(

Should we just bite the bullet and install a kmem_cache here?
It didn't solve the kmalloc error handling, but it does at least give us
a freelist. There is a reasonable argument that as soon as userspace
starts using non-default priorities, we may see many different levels
justifying allocating a whole slab upfront.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Tvrtko Ursulin 


On 03/05/2017 12:37, Chris Wilson wrote:

[snip]


+#include 
+
+static inline void __list_del_many(struct list_head *head,
+  struct list_head *first)
+{
+   head->next = first;
+   first->prev = head;
+}


Btw it is similar to __list_cut_position, but without the second list 
you don't need and deleting the first entry which you do not want.


So I am just thinking if consistent naming with the core function would 
be beneficial like __list_cut_position_before/exclusive?


Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Tvrtko Ursulin 


On 05/05/2017 14:32, Chris Wilson wrote:

On Fri, May 05, 2017 at 02:19:07PM +0100, Tvrtko Ursulin wrote:


On 03/05/2017 12:37, Chris Wilson wrote:

struct intel_engine_cs {
@@ -367,6 +373,7 @@ struct intel_engine_cs {

/* Execlists */
struct tasklet_struct irq_tasklet;
+   struct execlist_priolist default_priolist;
struct execlist_port {
struct drm_i915_gem_request *request_count;
#define EXECLIST_COUNT_BITS 2



Just a small bikeshed to consider. Having switched to
I915_PRIORITY_NORMAL, do we have a better name for default_priolist? I
still prefer default_priolist over normal_priolist. Go back to
I915_PRIORITY_DEFAULT?


default_priolist is fine I think since it is dual purpose. Primary 
purpose to avoid allocations as you said.


Although I am still a bit dejected how some userspace could decide one 
day to send everything at I915_PRIORITY_NORMAL - n, in order to use 
I915_PRIORITY_NORMAL as the high prio not requiring cap_sys_admin, and 
in doing so completely defeat the atomic kmalloc avoidance. :(


Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Chris Wilson 
On Fri, May 05, 2017 at 02:19:07PM +0100, Tvrtko Ursulin wrote:
> 
> On 03/05/2017 12:37, Chris Wilson wrote:
> > struct intel_engine_cs {
> >@@ -367,6 +373,7 @@ struct intel_engine_cs {
> >
> > /* Execlists */
> > struct tasklet_struct irq_tasklet;
> >+struct execlist_priolist default_priolist;
> > struct execlist_port {
> > struct drm_i915_gem_request *request_count;
> > #define EXECLIST_COUNT_BITS 2
> >

Just a small bikeshed to consider. Having switched to
I915_PRIORITY_NORMAL, do we have a better name for default_priolist? I
still prefer default_priolist over normal_priolist. Go back to
I915_PRIORITY_DEFAULT?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-05 Thread Tvrtko Ursulin 


On 03/05/2017 12:37, Chris Wilson wrote:

All the requests at the same priority are executed in FIFO order. They
do not need to be stored in the rbtree themselves, as they are a simple
list within a level. If we move the requests at one priority into a list,
we can then reduce the rbtree to the set of priorities. This should keep
the height of the rbtree small, as the number of active priorities can not
exceed the number of active requests and should be typically only a few.

Currently, we have ~2k possible different priority levels, that may
increase to allow even more fine grained selection. Allocating those in
advance seems a waste (and may be impossible), so we opt for allocating
upon first use, and freeing after its requests are depleted. To avoid
the possibility of an allocation failure causing us to lose a request,
we preallocate the default priority (0) and bump any request to that
priority if we fail to allocate it the appropriate plist. Having a
request (that is ready to run, so not leading to corruption) execute
out-of-order is better than leaking the request (and its dependency
tree) entirely.

There should be a benefit to reducing execlists_dequeue() to principally
using a simple list (and reducing the frequency of both rbtree iteration
and balancing on erase) but for typical workloads, request coalescing
should be small enough that we don't notice any change. The main gain is
from improving PI calls to schedule, and the explicit list within a
level should make request unwinding simpler (we just need to insert at
the head of the list rather than the tail and not have to make the
rbtree search more complicated).

v2: Avoid use-after-free when deleting a depleted priolist

Signed-off-by: Chris Wilson 
Cc: Michał Winiarski 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_debugfs.c|  12 ++-
 drivers/gpu/drm/i915/i915_gem_request.c|   4 +-
 drivers/gpu/drm/i915/i915_gem_request.h|   2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  53 ++
 drivers/gpu/drm/i915/i915_utils.h  |   9 ++
 drivers/gpu/drm/i915/intel_lrc.c   | 159 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.h|   7 ++
 7 files changed, 163 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 0b5d7142d8d9..a8c7788d986e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3314,7 +3314,6 @@ static int i915_engine_info(struct seq_file *m, void 
*unused)

if (i915.enable_execlists) {
u32 ptr, read, write;
-   struct rb_node *rb;
unsigned int idx;

seq_printf(m, "\tExeclist status: 0x%08x %08x\n",
@@ -3358,9 +3357,14 @@ static int i915_engine_info(struct seq_file *m, void 
*unused)
rcu_read_unlock();

spin_lock_irq(>timeline->lock);
-   for (rb = engine->execlist_first; rb; rb = rb_next(rb)) 
{
-   rq = rb_entry(rb, typeof(*rq), priotree.node);
-   print_request(m, rq, "\t\tQ ");
+   for (rb = engine->execlist_first; rb; rb = rb_next(rb)){
+   struct execlist_priolist *plist =
+   rb_entry(rb, typeof(*plist), node);
+
+   list_for_each_entry(rq,
+   >requests,
+   priotree.link)
+   print_request(m, rq, "\t\tQ ");
}
spin_unlock_irq(>timeline->lock);
} else if (INTEL_GEN(dev_priv) > 6) {
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c 
b/drivers/gpu/drm/i915/i915_gem_request.c
index 8d2a5c8e5005..ad2be26923fb 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -159,7 +159,7 @@ i915_priotree_fini(struct drm_i915_private *i915, struct 
i915_priotree *pt)
 {
struct i915_dependency *dep, *next;

-   GEM_BUG_ON(!RB_EMPTY_NODE(>node));
+   GEM_BUG_ON(!list_empty(>link));

/* Everyone we depended upon (the fences we wait to be signaled)
 * should retire before us and remove themselves from our list.
@@ -185,7 +185,7 @@ i915_priotree_init(struct i915_priotree *pt)
 {
INIT_LIST_HEAD(>signalers_list);
INIT_LIST_HEAD(>waiters_list);
-   RB_CLEAR_NODE(>node);
+   INIT_LIST_HEAD(>link);
pt->priority = INT_MIN;
 }

diff --git a/drivers/gpu/drm/i915/i915_gem_request.h 
b/drivers/gpu/drm/i915/i915_gem_request.h
index feb81463abc9..6c58f7d87746 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h

[Intel-gfx] [PATCH 6/9] drm/i915: Split execlist priority queue into rbtree + linked list

2017-05-03 Thread Chris Wilson 
All the requests at the same priority are executed in FIFO order. They
do not need to be stored in the rbtree themselves, as they are a simple
list within a level. If we move the requests at one priority into a list,
we can then reduce the rbtree to the set of priorities. This should keep
the height of the rbtree small, as the number of active priorities can not
exceed the number of active requests and should be typically only a few.

Currently, we have ~2k possible different priority levels, that may
increase to allow even more fine grained selection. Allocating those in
advance seems a waste (and may be impossible), so we opt for allocating
upon first use, and freeing after its requests are depleted. To avoid
the possibility of an allocation failure causing us to lose a request,
we preallocate the default priority (0) and bump any request to that
priority if we fail to allocate it the appropriate plist. Having a
request (that is ready to run, so not leading to corruption) execute
out-of-order is better than leaking the request (and its dependency
tree) entirely.

There should be a benefit to reducing execlists_dequeue() to principally
using a simple list (and reducing the frequency of both rbtree iteration
and balancing on erase) but for typical workloads, request coalescing
should be small enough that we don't notice any change. The main gain is
from improving PI calls to schedule, and the explicit list within a
level should make request unwinding simpler (we just need to insert at
the head of the list rather than the tail and not have to make the
rbtree search more complicated).

v2: Avoid use-after-free when deleting a depleted priolist

Signed-off-by: Chris Wilson 
Cc: Michał Winiarski 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_debugfs.c|  12 ++-
 drivers/gpu/drm/i915/i915_gem_request.c|   4 +-
 drivers/gpu/drm/i915/i915_gem_request.h|   2 +-
 drivers/gpu/drm/i915/i915_guc_submission.c |  53 ++
 drivers/gpu/drm/i915/i915_utils.h  |   9 ++
 drivers/gpu/drm/i915/intel_lrc.c   | 159 +++--
 drivers/gpu/drm/i915/intel_ringbuffer.h|   7 ++
 7 files changed, 163 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 0b5d7142d8d9..a8c7788d986e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3314,7 +3314,6 @@ static int i915_engine_info(struct seq_file *m, void 
*unused)
 
if (i915.enable_execlists) {
u32 ptr, read, write;
-   struct rb_node *rb;
unsigned int idx;
 
seq_printf(m, "\tExeclist status: 0x%08x %08x\n",
@@ -3358,9 +3357,14 @@ static int i915_engine_info(struct seq_file *m, void 
*unused)
rcu_read_unlock();
 
spin_lock_irq(>timeline->lock);
-   for (rb = engine->execlist_first; rb; rb = rb_next(rb)) 
{
-   rq = rb_entry(rb, typeof(*rq), priotree.node);
-   print_request(m, rq, "\t\tQ ");
+   for (rb = engine->execlist_first; rb; rb = rb_next(rb)){
+   struct execlist_priolist *plist =
+   rb_entry(rb, typeof(*plist), node);
+
+   list_for_each_entry(rq,
+   >requests,
+   priotree.link)
+   print_request(m, rq, "\t\tQ ");
}
spin_unlock_irq(>timeline->lock);
} else if (INTEL_GEN(dev_priv) > 6) {
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c 
b/drivers/gpu/drm/i915/i915_gem_request.c
index 8d2a5c8e5005..ad2be26923fb 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -159,7 +159,7 @@ i915_priotree_fini(struct drm_i915_private *i915, struct 
i915_priotree *pt)
 {
struct i915_dependency *dep, *next;
 
-   GEM_BUG_ON(!RB_EMPTY_NODE(>node));
+   GEM_BUG_ON(!list_empty(>link));
 
/* Everyone we depended upon (the fences we wait to be signaled)
 * should retire before us and remove themselves from our list.
@@ -185,7 +185,7 @@ i915_priotree_init(struct i915_priotree *pt)
 {
INIT_LIST_HEAD(>signalers_list);
INIT_LIST_HEAD(>waiters_list);
-   RB_CLEAR_NODE(>node);
+   INIT_LIST_HEAD(>link);
pt->priority = INT_MIN;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h 
b/drivers/gpu/drm/i915/i915_gem_request.h
index feb81463abc9..6c58f7d87746 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++