* [PATCH 2/3] drm/i915/execlists: Suppress preempting self
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
@ 2019-01-23 12:36 ` Chris Wilson
2019-01-23 14:08 ` Tvrtko Ursulin
2019-01-23 17:35 ` [PATCH v2] " Chris Wilson
2019-01-23 12:36 ` [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption Chris Wilson
` (9 subsequent siblings)
10 siblings, 2 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 12:36 UTC (permalink / raw)
To: intel-gfx
In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.
To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++++++++---
2 files changed, 76 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..fb5d953430e5 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
return engine;
}
+static bool inflight(const struct i915_request *rq,
+ const struct intel_engine_cs *engine)
+{
+ const struct i915_request *active;
+
+ if (!rq->global_seqno)
+ return false;
+
+ active = port_request(engine->execlists.port);
+ return active->hw_context == rq->hw_context;
+}
+
static void __i915_schedule(struct i915_request *rq,
const struct i915_sched_attr *attr)
{
@@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
INIT_LIST_HEAD(&dep->dfs_link);
engine = sched_lock_engine(node, engine);
+ lockdep_assert_held(&engine->timeline.lock);
/* Recheck after acquiring the engine->timeline.lock */
if (prio <= node->attr.priority || node_signaled(node))
@@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
if (prio <= engine->execlists.queue_priority)
continue;
+ engine->execlists.queue_priority = prio;
+
/*
* If we are already the currently executing context, don't
* bother evaluating if we should preempt ourselves.
*/
- if (node_to_request(node)->global_seqno &&
- i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
- node_to_request(node)->global_seqno))
+ if (inflight(node_to_request(node), engine))
continue;
/* Defer (tasklet) submission until after all of our updates. */
- engine->execlists.queue_priority = prio;
tasklet_hi_schedule(&engine->execlists.tasklet);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8aa8a4862543..d9d744f6ab2c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
}
static inline bool need_preempt(const struct intel_engine_cs *engine,
- const struct i915_request *last,
- int prio)
+ const struct i915_request *rq,
+ int q_prio)
{
- return (intel_engine_has_preemption(engine) &&
- __execlists_need_preempt(prio, rq_prio(last)) &&
- !i915_request_completed(last));
+ const struct intel_context *ctx = rq->hw_context;
+ const int last_prio = rq_prio(rq);
+ struct rb_node *rb;
+ int idx;
+
+ if (!intel_engine_has_preemption(engine))
+ return false;
+
+ if (i915_request_completed(rq))
+ return false;
+
+ if (!__execlists_need_preempt(q_prio, last_prio))
+ return false;
+
+ /*
+ * The queue_priority is a mere hint that we may need to preempt.
+ * If that hint is stale or we may be trying to preempt ourselves,
+ * ignore the request.
+ */
+
+ list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
+ GEM_BUG_ON(rq->hw_context == ctx);
+ if (rq_prio(rq) > last_prio)
+ return true;
+ }
+
+ rb = rb_first_cached(&engine->execlists.queue);
+ if (!rb)
+ return false;
+
+ priolist_for_each_request(rq, to_priolist(rb), idx)
+ return rq->hw_context != ctx && rq_prio(rq) > last_prio;
+
+ return false;
+}
+
+__maybe_unused static inline bool
+assert_priority_queue(const struct intel_engine_execlists *execlists,
+ const struct i915_request *prev,
+ const struct i915_request *next)
+{
+ if (!prev)
+ return true;
+
+ /*
+ * Without preemption, the prev may refer to the still active element
+ * which we refuse to let go.
+ *
+ * Even with premption, there are times when we think it is better not
+ * to preempt and leave an ostensibly lower priority request in flight.
+ */
+ if (port_request(execlists->port) == prev)
+ return true;
+
+ return rq_prio(prev) >= rq_prio(next);
}
/*
@@ -626,8 +678,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
int i;
priolist_for_each_request_consume(rq, rn, p, i) {
- GEM_BUG_ON(last &&
- need_preempt(engine, last, rq_prio(rq)));
+ GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
/*
* Can we combine this request with the current port?
@@ -872,6 +923,8 @@ static void process_csb(struct intel_engine_cs *engine)
const u32 * const buf = execlists->csb_status;
u8 head, tail;
+ lockdep_assert_held(&engine->timeline.lock);
+
/*
* Note that csb_write, csb_status may be either in HWSP or mmio.
* When reading from the csb_write mmio register, we have to be
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self
2019-01-23 12:36 ` [PATCH 2/3] drm/i915/execlists: Suppress preempting self Chris Wilson
@ 2019-01-23 14:08 ` Tvrtko Ursulin
2019-01-23 14:22 ` Chris Wilson
2019-01-23 17:35 ` [PATCH v2] " Chris Wilson
1 sibling, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2019-01-23 14:08 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 23/01/2019 12:36, Chris Wilson wrote:
> In order to avoid preempting ourselves, we currently refuse to schedule
> the tasklet if we reschedule an inflight context. However, this glosses
> over a few issues such as what happens after a CS completion event and
> we then preempt the newly executing context with itself, or if something
> else causes a tasklet_schedule triggering the same evaluation to
> preempt the active context with itself.
>
> To avoid the extra complications, after deciding that we have
> potentially queued a request with higher priority than the currently
> executing request, inspect the head of the queue to see if it is indeed
> higher priority from another context.
>
> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
> drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++++++++---
> 2 files changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 340faea6c08a..fb5d953430e5 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> return engine;
> }
>
> +static bool inflight(const struct i915_request *rq,
> + const struct intel_engine_cs *engine)
> +{
> + const struct i915_request *active;
> +
> + if (!rq->global_seqno)
> + return false;
> +
> + active = port_request(engine->execlists.port);
> + return active->hw_context == rq->hw_context;
> +}
> +
> static void __i915_schedule(struct i915_request *rq,
> const struct i915_sched_attr *attr)
> {
> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
> INIT_LIST_HEAD(&dep->dfs_link);
>
> engine = sched_lock_engine(node, engine);
> + lockdep_assert_held(&engine->timeline.lock);
>
> /* Recheck after acquiring the engine->timeline.lock */
> if (prio <= node->attr.priority || node_signaled(node))
> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
> if (prio <= engine->execlists.queue_priority)
> continue;
>
> + engine->execlists.queue_priority = prio;
> +
> /*
> * If we are already the currently executing context, don't
> * bother evaluating if we should preempt ourselves.
> */
> - if (node_to_request(node)->global_seqno &&
> - i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> - node_to_request(node)->global_seqno))
> + if (inflight(node_to_request(node), engine))
> continue;
>
> /* Defer (tasklet) submission until after all of our updates. */
> - engine->execlists.queue_priority = prio;
> tasklet_hi_schedule(&engine->execlists.tasklet);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8aa8a4862543..d9d744f6ab2c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
> }
>
> static inline bool need_preempt(const struct intel_engine_cs *engine,
> - const struct i915_request *last,
> - int prio)
> + const struct i915_request *rq,
> + int q_prio)
> {
> - return (intel_engine_has_preemption(engine) &&
> - __execlists_need_preempt(prio, rq_prio(last)) &&
> - !i915_request_completed(last));
> + const struct intel_context *ctx = rq->hw_context;
> + const int last_prio = rq_prio(rq);
> + struct rb_node *rb;
> + int idx;
> +
> + if (!intel_engine_has_preemption(engine))
> + return false;
> +
> + if (i915_request_completed(rq))
> + return false;
> +
> + if (!__execlists_need_preempt(q_prio, last_prio))
> + return false;
> +
> + /*
> + * The queue_priority is a mere hint that we may need to preempt.
> + * If that hint is stale or we may be trying to preempt ourselves,
> + * ignore the request.
> + */
> +
> + list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> + GEM_BUG_ON(rq->hw_context == ctx);
Why would there be no more requests belonging to the same context on the
engine timeline after the first one? Did you mean "if (rq->hw_context ==
ctx) continue;" ?
> + if (rq_prio(rq) > last_prio)
> + return true;
> + }
> +
> + rb = rb_first_cached(&engine->execlists.queue);
> + if (!rb)
> + return false;
> +
> + priolist_for_each_request(rq, to_priolist(rb), idx)
> + return rq->hw_context != ctx && rq_prio(rq) > last_prio;
This isn't equivalent to top of the queue priority
(engine->execlists.queue_priority)? Apart from the different ctx check.
So I guess it is easier than storing new engine->execlists.queue_top_ctx
and wondering about the validity of that pointer.
Regards,
Tvrtko
> +
> + return false;
> +}
> +
> +__maybe_unused static inline bool
> +assert_priority_queue(const struct intel_engine_execlists *execlists,
> + const struct i915_request *prev,
> + const struct i915_request *next)
> +{
> + if (!prev)
> + return true;
> +
> + /*
> + * Without preemption, the prev may refer to the still active element
> + * which we refuse to let go.
> + *
> + * Even with premption, there are times when we think it is better not
> + * to preempt and leave an ostensibly lower priority request in flight.
> + */
> + if (port_request(execlists->port) == prev)
> + return true;
> +
> + return rq_prio(prev) >= rq_prio(next);
> }
>
> /*
> @@ -626,8 +678,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> int i;
>
> priolist_for_each_request_consume(rq, rn, p, i) {
> - GEM_BUG_ON(last &&
> - need_preempt(engine, last, rq_prio(rq)));
> + GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
>
> /*
> * Can we combine this request with the current port?
> @@ -872,6 +923,8 @@ static void process_csb(struct intel_engine_cs *engine)
> const u32 * const buf = execlists->csb_status;
> u8 head, tail;
>
> + lockdep_assert_held(&engine->timeline.lock);
> +
> /*
> * Note that csb_write, csb_status may be either in HWSP or mmio.
> * When reading from the csb_write mmio register, we have to be
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self
2019-01-23 14:08 ` Tvrtko Ursulin
@ 2019-01-23 14:22 ` Chris Wilson
2019-01-23 16:40 ` Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 14:22 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
>
> On 23/01/2019 12:36, Chris Wilson wrote:
> > In order to avoid preempting ourselves, we currently refuse to schedule
> > the tasklet if we reschedule an inflight context. However, this glosses
> > over a few issues such as what happens after a CS completion event and
> > we then preempt the newly executing context with itself, or if something
> > else causes a tasklet_schedule triggering the same evaluation to
> > preempt the active context with itself.
> >
> > To avoid the extra complications, after deciding that we have
> > potentially queued a request with higher priority than the currently
> > executing request, inspect the head of the queue to see if it is indeed
> > higher priority from another context.
> >
> > References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
> > drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++++++++---
> > 2 files changed, 76 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> > index 340faea6c08a..fb5d953430e5 100644
> > --- a/drivers/gpu/drm/i915/i915_scheduler.c
> > +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> > @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> > return engine;
> > }
> >
> > +static bool inflight(const struct i915_request *rq,
> > + const struct intel_engine_cs *engine)
> > +{
> > + const struct i915_request *active;
> > +
> > + if (!rq->global_seqno)
> > + return false;
> > +
> > + active = port_request(engine->execlists.port);
> > + return active->hw_context == rq->hw_context;
> > +}
> > +
> > static void __i915_schedule(struct i915_request *rq,
> > const struct i915_sched_attr *attr)
> > {
> > @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
> > INIT_LIST_HEAD(&dep->dfs_link);
> >
> > engine = sched_lock_engine(node, engine);
> > + lockdep_assert_held(&engine->timeline.lock);
> >
> > /* Recheck after acquiring the engine->timeline.lock */
> > if (prio <= node->attr.priority || node_signaled(node))
> > @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
> > if (prio <= engine->execlists.queue_priority)
> > continue;
> >
> > + engine->execlists.queue_priority = prio;
> > +
> > /*
> > * If we are already the currently executing context, don't
> > * bother evaluating if we should preempt ourselves.
> > */
> > - if (node_to_request(node)->global_seqno &&
> > - i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> > - node_to_request(node)->global_seqno))
> > + if (inflight(node_to_request(node), engine))
> > continue;
> >
> > /* Defer (tasklet) submission until after all of our updates. */
> > - engine->execlists.queue_priority = prio;
> > tasklet_hi_schedule(&engine->execlists.tasklet);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 8aa8a4862543..d9d744f6ab2c 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
> > }
> >
> > static inline bool need_preempt(const struct intel_engine_cs *engine,
> > - const struct i915_request *last,
> > - int prio)
> > + const struct i915_request *rq,
> > + int q_prio)
> > {
> > - return (intel_engine_has_preemption(engine) &&
> > - __execlists_need_preempt(prio, rq_prio(last)) &&
> > - !i915_request_completed(last));
> > + const struct intel_context *ctx = rq->hw_context;
> > + const int last_prio = rq_prio(rq);
> > + struct rb_node *rb;
> > + int idx;
> > +
> > + if (!intel_engine_has_preemption(engine))
> > + return false;
> > +
> > + if (i915_request_completed(rq))
> > + return false;
> > +
> > + if (!__execlists_need_preempt(q_prio, last_prio))
> > + return false;
> > +
> > + /*
> > + * The queue_priority is a mere hint that we may need to preempt.
> > + * If that hint is stale or we may be trying to preempt ourselves,
> > + * ignore the request.
> > + */
> > +
> > + list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> > + GEM_BUG_ON(rq->hw_context == ctx);
>
> Why would there be no more requests belonging to the same context on the
> engine timeline after the first one? Did you mean "if (rq->hw_context ==
> ctx) continue;" ?
We enter the function with rq == execlist->port, i.e. the last request
submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
and all the request here belong to that context. It is illegal for
ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.
>
> > + if (rq_prio(rq) > last_prio)
> > + return true;
> > + }
> > +
> > + rb = rb_first_cached(&engine->execlists.queue);
> > + if (!rb)
> > + return false;
> > +
> > + priolist_for_each_request(rq, to_priolist(rb), idx)
> > + return rq->hw_context != ctx && rq_prio(rq) > last_prio;
>
> This isn't equivalent to top of the queue priority
> (engine->execlists.queue_priority)? Apart from the different ctx check.
> So I guess it is easier than storing new engine->execlists.queue_top_ctx
> and wondering about the validity of that pointer.
The problem being that queue_priority may not always match the top of
the execlists->queue. Right, the first attempt I tried was to store the
queue_context matching the queue_priority, but due to the suppression of
inflight preemptions, it doesn't match for long, and is not as accurate
as one would like across CS events.
priolist_for_each_request() isn't too horrible for finding the first
pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
though. If we didn't care about checking hw_context, we can compute the
prio from (p->priority+1)<<SHIFT - ffs(p->used).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self
2019-01-23 14:22 ` Chris Wilson
@ 2019-01-23 16:40 ` Tvrtko Ursulin
2019-01-23 17:09 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2019-01-23 16:40 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 23/01/2019 14:22, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
>>
>> On 23/01/2019 12:36, Chris Wilson wrote:
>>> In order to avoid preempting ourselves, we currently refuse to schedule
>>> the tasklet if we reschedule an inflight context. However, this glosses
>>> over a few issues such as what happens after a CS completion event and
>>> we then preempt the newly executing context with itself, or if something
>>> else causes a tasklet_schedule triggering the same evaluation to
>>> preempt the active context with itself.
>>>
>>> To avoid the extra complications, after deciding that we have
>>> potentially queued a request with higher priority than the currently
>>> executing request, inspect the head of the queue to see if it is indeed
>>> higher priority from another context.
>>>
>>> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
>>> drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++++++++---
>>> 2 files changed, 76 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
>>> index 340faea6c08a..fb5d953430e5 100644
>>> --- a/drivers/gpu/drm/i915/i915_scheduler.c
>>> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
>>> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
>>> return engine;
>>> }
>>>
>>> +static bool inflight(const struct i915_request *rq,
>>> + const struct intel_engine_cs *engine)
>>> +{
>>> + const struct i915_request *active;
>>> +
>>> + if (!rq->global_seqno)
>>> + return false;
>>> +
>>> + active = port_request(engine->execlists.port);
>>> + return active->hw_context == rq->hw_context;
>>> +}
>>> +
>>> static void __i915_schedule(struct i915_request *rq,
>>> const struct i915_sched_attr *attr)
>>> {
>>> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
>>> INIT_LIST_HEAD(&dep->dfs_link);
>>>
>>> engine = sched_lock_engine(node, engine);
>>> + lockdep_assert_held(&engine->timeline.lock);
>>>
>>> /* Recheck after acquiring the engine->timeline.lock */
>>> if (prio <= node->attr.priority || node_signaled(node))
>>> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
>>> if (prio <= engine->execlists.queue_priority)
>>> continue;
>>>
>>> + engine->execlists.queue_priority = prio;
>>> +
>>> /*
>>> * If we are already the currently executing context, don't
>>> * bother evaluating if we should preempt ourselves.
>>> */
>>> - if (node_to_request(node)->global_seqno &&
>>> - i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
>>> - node_to_request(node)->global_seqno))
>>> + if (inflight(node_to_request(node), engine))
>>> continue;
>>>
>>> /* Defer (tasklet) submission until after all of our updates. */
>>> - engine->execlists.queue_priority = prio;
>>> tasklet_hi_schedule(&engine->execlists.tasklet);
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 8aa8a4862543..d9d744f6ab2c 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
>>> }
>>>
>>> static inline bool need_preempt(const struct intel_engine_cs *engine,
>>> - const struct i915_request *last,
>>> - int prio)
>>> + const struct i915_request *rq,
>>> + int q_prio)
>>> {
>>> - return (intel_engine_has_preemption(engine) &&
>>> - __execlists_need_preempt(prio, rq_prio(last)) &&
>>> - !i915_request_completed(last));
>>> + const struct intel_context *ctx = rq->hw_context;
>>> + const int last_prio = rq_prio(rq);
>>> + struct rb_node *rb;
>>> + int idx;
>>> +
>>> + if (!intel_engine_has_preemption(engine))
>>> + return false;
>>> +
>>> + if (i915_request_completed(rq))
>>> + return false;
>>> +
>>> + if (!__execlists_need_preempt(q_prio, last_prio))
>>> + return false;
>>> +
>>> + /*
>>> + * The queue_priority is a mere hint that we may need to preempt.
>>> + * If that hint is stale or we may be trying to preempt ourselves,
>>> + * ignore the request.
>>> + */
>>> +
>>> + list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
>>> + GEM_BUG_ON(rq->hw_context == ctx);
>>
>> Why would there be no more requests belonging to the same context on the
>> engine timeline after the first one? Did you mean "if (rq->hw_context ==
>> ctx) continue;" ?
>
> We enter the function with rq == execlist->port, i.e. the last request
> submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
> and all the request here belong to that context. It is illegal for
> ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.
Yes, you are right yet again. I missed the fact it is guaranteed this is
called with port0. I wonder if function signature should change to make
this obvious so someone doesn't get the idea to call it with any old
request.
>
>>
>>> + if (rq_prio(rq) > last_prio)
>>> + return true;
>>> + }
>>> +
>>> + rb = rb_first_cached(&engine->execlists.queue);
>>> + if (!rb)
>>> + return false;
>>> +
>>> + priolist_for_each_request(rq, to_priolist(rb), idx)
>>> + return rq->hw_context != ctx && rq_prio(rq) > last_prio;
>>
>> This isn't equivalent to top of the queue priority
>> (engine->execlists.queue_priority)? Apart from the different ctx check.
>> So I guess it is easier than storing new engine->execlists.queue_top_ctx
>> and wondering about the validity of that pointer.
>
> The problem being that queue_priority may not always match the top of
> the execlists->queue. Right, the first attempt I tried was to store the
> queue_context matching the queue_priority, but due to the suppression of
> inflight preemptions, it doesn't match for long, and is not as accurate
> as one would like across CS events.
>
> priolist_for_each_request() isn't too horrible for finding the first
> pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
> though. If we didn't care about checking hw_context, we can compute the
> prio from (p->priority+1)<<SHIFT - ffs(p->used).
And this check is definitely needed to avoid some issue? I'll need to
have another try of understanding the commit and code paths fully
tomorrow. I mean, only because it would be good to have something more
elegant that full blown tree lookup.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] drm/i915/execlists: Suppress preempting self
2019-01-23 16:40 ` Tvrtko Ursulin
@ 2019-01-23 17:09 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 17:09 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-23 16:40:44)
>
> On 23/01/2019 14:22, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-23 14:08:56)
> >>
> >> On 23/01/2019 12:36, Chris Wilson wrote:
> >>> In order to avoid preempting ourselves, we currently refuse to schedule
> >>> the tasklet if we reschedule an inflight context. However, this glosses
> >>> over a few issues such as what happens after a CS completion event and
> >>> we then preempt the newly executing context with itself, or if something
> >>> else causes a tasklet_schedule triggering the same evaluation to
> >>> preempt the active context with itself.
> >>>
> >>> To avoid the extra complications, after deciding that we have
> >>> potentially queued a request with higher priority than the currently
> >>> executing request, inspect the head of the queue to see if it is indeed
> >>> higher priority from another context.
> >>>
> >>> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++++--
> >>> drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++++++++---
> >>> 2 files changed, 76 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> >>> index 340faea6c08a..fb5d953430e5 100644
> >>> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> >>> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> >>> @@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> >>> return engine;
> >>> }
> >>>
> >>> +static bool inflight(const struct i915_request *rq,
> >>> + const struct intel_engine_cs *engine)
> >>> +{
> >>> + const struct i915_request *active;
> >>> +
> >>> + if (!rq->global_seqno)
> >>> + return false;
> >>> +
> >>> + active = port_request(engine->execlists.port);
> >>> + return active->hw_context == rq->hw_context;
> >>> +}
> >>> +
> >>> static void __i915_schedule(struct i915_request *rq,
> >>> const struct i915_sched_attr *attr)
> >>> {
> >>> @@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
> >>> INIT_LIST_HEAD(&dep->dfs_link);
> >>>
> >>> engine = sched_lock_engine(node, engine);
> >>> + lockdep_assert_held(&engine->timeline.lock);
> >>>
> >>> /* Recheck after acquiring the engine->timeline.lock */
> >>> if (prio <= node->attr.priority || node_signaled(node))
> >>> @@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
> >>> if (prio <= engine->execlists.queue_priority)
> >>> continue;
> >>>
> >>> + engine->execlists.queue_priority = prio;
> >>> +
> >>> /*
> >>> * If we are already the currently executing context, don't
> >>> * bother evaluating if we should preempt ourselves.
> >>> */
> >>> - if (node_to_request(node)->global_seqno &&
> >>> - i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> >>> - node_to_request(node)->global_seqno))
> >>> + if (inflight(node_to_request(node), engine))
> >>> continue;
> >>>
> >>> /* Defer (tasklet) submission until after all of our updates. */
> >>> - engine->execlists.queue_priority = prio;
> >>> tasklet_hi_schedule(&engine->execlists.tasklet);
> >>> }
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>> index 8aa8a4862543..d9d744f6ab2c 100644
> >>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>> @@ -182,12 +182,64 @@ static inline int rq_prio(const struct i915_request *rq)
> >>> }
> >>>
> >>> static inline bool need_preempt(const struct intel_engine_cs *engine,
> >>> - const struct i915_request *last,
> >>> - int prio)
> >>> + const struct i915_request *rq,
> >>> + int q_prio)
> >>> {
> >>> - return (intel_engine_has_preemption(engine) &&
> >>> - __execlists_need_preempt(prio, rq_prio(last)) &&
> >>> - !i915_request_completed(last));
> >>> + const struct intel_context *ctx = rq->hw_context;
> >>> + const int last_prio = rq_prio(rq);
> >>> + struct rb_node *rb;
> >>> + int idx;
> >>> +
> >>> + if (!intel_engine_has_preemption(engine))
> >>> + return false;
> >>> +
> >>> + if (i915_request_completed(rq))
> >>> + return false;
> >>> +
> >>> + if (!__execlists_need_preempt(q_prio, last_prio))
> >>> + return false;
> >>> +
> >>> + /*
> >>> + * The queue_priority is a mere hint that we may need to preempt.
> >>> + * If that hint is stale or we may be trying to preempt ourselves,
> >>> + * ignore the request.
> >>> + */
> >>> +
> >>> + list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
> >>> + GEM_BUG_ON(rq->hw_context == ctx);
> >>
> >> Why would there be no more requests belonging to the same context on the
> >> engine timeline after the first one? Did you mean "if (rq->hw_context ==
> >> ctx) continue;" ?
> >
> > We enter the function with rq == execlist->port, i.e. the last request
> > submitted to ELSP[0]. In this loop, we iterate from the start of ELSP[1]
> > and all the request here belong to that context. It is illegal for
> > ELSP[0].lrca == ELSP[1].lrca, i.e. the context must be different.
>
> Yes, you are right yet again. I missed the fact it is guaranteed this is
> called with port0. I wonder if function signature should change to make
> this obvious so someone doesn't get the idea to call it with any old
> request.
I did miss something obvious though. Due to PI, the first request on
ELSP[1] is also the highest priority (we make sure to promote all
inflight requests along the same context).
> >>> + if (rq_prio(rq) > last_prio)
> >>> + return true;
> >>> + }
> >>> +
> >>> + rb = rb_first_cached(&engine->execlists.queue);
> >>> + if (!rb)
> >>> + return false;
> >>> +
> >>> + priolist_for_each_request(rq, to_priolist(rb), idx)
> >>> + return rq->hw_context != ctx && rq_prio(rq) > last_prio;
> >>
> >> This isn't equivalent to top of the queue priority
> >> (engine->execlists.queue_priority)? Apart from the different ctx check.
> >> So I guess it is easier than storing new engine->execlists.queue_top_ctx
> >> and wondering about the validity of that pointer.
> >
> > The problem being that queue_priority may not always match the top of
> > the execlists->queue. Right, the first attempt I tried was to store the
> > queue_context matching the queue_priority, but due to the suppression of
> > inflight preemptions, it doesn't match for long, and is not as accurate
> > as one would like across CS events.
> >
> > priolist_for_each_request() isn't too horrible for finding the first
> > pointer. I noted that we teach it to do: for(idx = __ffs(p->used); ...)
> > though. If we didn't care about checking hw_context, we can compute the
> > prio from (p->priority+1)<<SHIFT - ffs(p->used).
>
> And this check is definitely needed to avoid some issue? I'll need to
> have another try of understanding the commit and code paths fully
> tomorrow. I mean, only because it would be good to have something more
> elegant that full blown tree lookup.
Hmm. No, the hw_context check should be redundant. If it were the same
context as ELSP[0], we would have applied PI to last_prio already, so
rq_prio(rq) can never be greater. All we need to realise is that we
cannot trust queue_priority alone.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] drm/i915/execlists: Suppress preempting self
2019-01-23 12:36 ` [PATCH 2/3] drm/i915/execlists: Suppress preempting self Chris Wilson
2019-01-23 14:08 ` Tvrtko Ursulin
@ 2019-01-23 17:35 ` Chris Wilson
1 sibling, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 17:35 UTC (permalink / raw)
To: intel-gfx
In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.
To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.
v2: We can simplify a bunch of tests based on the knowledge that PI will
ensure that earlier requests along the same context will have the highest
priority.
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_scheduler.c | 20 ++++--
drivers/gpu/drm/i915/intel_lrc.c | 91 ++++++++++++++++++++++++---
2 files changed, 100 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..fb5d953430e5 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
return engine;
}
+static bool inflight(const struct i915_request *rq,
+ const struct intel_engine_cs *engine)
+{
+ const struct i915_request *active;
+
+ if (!rq->global_seqno)
+ return false;
+
+ active = port_request(engine->execlists.port);
+ return active->hw_context == rq->hw_context;
+}
+
static void __i915_schedule(struct i915_request *rq,
const struct i915_sched_attr *attr)
{
@@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
INIT_LIST_HEAD(&dep->dfs_link);
engine = sched_lock_engine(node, engine);
+ lockdep_assert_held(&engine->timeline.lock);
/* Recheck after acquiring the engine->timeline.lock */
if (prio <= node->attr.priority || node_signaled(node))
@@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
if (prio <= engine->execlists.queue_priority)
continue;
+ engine->execlists.queue_priority = prio;
+
/*
* If we are already the currently executing context, don't
* bother evaluating if we should preempt ourselves.
*/
- if (node_to_request(node)->global_seqno &&
- i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
- node_to_request(node)->global_seqno))
+ if (inflight(node_to_request(node), engine))
continue;
/* Defer (tasklet) submission until after all of our updates. */
- engine->execlists.queue_priority = prio;
tasklet_hi_schedule(&engine->execlists.tasklet);
}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8aa8a4862543..11d7fa810d9a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -181,13 +181,89 @@ static inline int rq_prio(const struct i915_request *rq)
return rq->sched.attr.priority;
}
+static int queue_priority(const struct intel_engine_execlists *execlists)
+{
+ struct i915_priolist *p;
+ struct rb_node *rb;
+
+ rb = rb_first_cached(&execlists->queue);
+ if (!rb)
+ return INT_MIN;
+
+ /*
+ * As the priolist[] are inverted, with the highest priority in [0],
+ * we have to flip the index value to become priority.
+ */
+ p = to_priolist(rb);
+ return ((p->priority + 1) << I915_USER_PRIORITY_SHIFT) - ffs(p->used);
+}
+
static inline bool need_preempt(const struct intel_engine_cs *engine,
- const struct i915_request *last,
- int prio)
+ const struct i915_request *rq,
+ int q_prio)
{
- return (intel_engine_has_preemption(engine) &&
- __execlists_need_preempt(prio, rq_prio(last)) &&
- !i915_request_completed(last));
+ const struct intel_context *ctx = rq->hw_context;
+ const int last_prio = rq_prio(rq);
+
+ if (!intel_engine_has_preemption(engine))
+ return false;
+
+ if (i915_request_completed(rq))
+ return false;
+
+ /*
+ * Check if the current queue_priority merits a preemption attempt.
+ *
+ * However, the queue_priority is a mere hint that we may need to
+ * preempt. If that hint is stale or we may be trying to preempt
+ * ourselves, ignore the request.
+ */
+ if (!__execlists_need_preempt(q_prio, last_prio))
+ return false;
+
+ /*
+ * Check against the first request in ELSP[1], it will, thanks to the
+ * power of PI, be the highest priority of that context.
+ */
+ if (!list_is_last(&rq->link, &engine->timeline.requests)) {
+ rq = list_next_entry(rq, link);
+ GEM_BUG_ON(rq->hw_context == ctx);
+ if (rq_prio(rq) > last_prio)
+ return true;
+ }
+
+ /*
+ * If the inflight context did not trigger the preemption, then maybe
+ * it was the set of queued requests? Pick the highest priority in
+ * the queue (the first active priolist) and see if it deserves to be
+ * running instead of ELSP[0].
+ *
+ * The highest priority request in the queue can not be either
+ * ELSP[0] or ELSP[1] as, thanks again to PI, if it was the same
+ * context, it's priority would not exceed ELSP[0] aka last_prio.
+ */
+ return queue_priority(&engine->execlists) > last_prio;
+}
+
+__maybe_unused static inline bool
+assert_priority_queue(const struct intel_engine_execlists *execlists,
+ const struct i915_request *prev,
+ const struct i915_request *next)
+{
+ if (!prev)
+ return true;
+
+ /*
+ * Without preemption, the prev may refer to the still active element
+ * which we refuse to let go.
+ *
+ * Even with premption, there are times when we think it is better not
+ * to preempt and leave an ostensibly lower priority request in flight.
+ */
+ if (port_request(execlists->port) == prev)
+ return true;
+
+ return rq_prio(prev) >= rq_prio(next);
}
/*
@@ -626,8 +702,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
int i;
priolist_for_each_request_consume(rq, rn, p, i) {
- GEM_BUG_ON(last &&
- need_preempt(engine, last, rq_prio(rq)));
+ GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
/*
* Can we combine this request with the current port?
@@ -872,6 +947,8 @@ static void process_csb(struct intel_engine_cs *engine)
const u32 * const buf = execlists->csb_status;
u8 head, tail;
+ lockdep_assert_held(&engine->timeline.lock);
+
/*
* Note that csb_write, csb_status may be either in HWSP or mmio.
* When reading from the csb_write mmio register, we have to be
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
2019-01-23 12:36 ` [PATCH 2/3] drm/i915/execlists: Suppress preempting self Chris Wilson
@ 2019-01-23 12:36 ` Chris Wilson
2019-01-23 13:47 ` Chris Wilson
2019-01-23 15:13 ` [PATCH v2] " Chris Wilson
2019-01-23 13:22 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption Patchwork
` (8 subsequent siblings)
10 siblings, 2 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 12:36 UTC (permalink / raw)
To: intel-gfx
On unwinding the active request we give it a small (limited to internal
priority levels) boost to prevent it from being gazumped a second time.
However, this means that it can be promoted to above the request that
triggered the preemption request, causing a preempt-to-idle cycle for no
change. We can avoid this if we take the boost into account when
checking if the preemption request is valid.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d9d744f6ab2c..74726f647e47 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -163,6 +163,8 @@
#define WA_TAIL_DWORDS 2
#define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
+#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
+
static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
struct intel_engine_cs *engine,
struct intel_context *ce);
@@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
return rq->sched.attr.priority;
}
+static inline int active_prio(const struct i915_request *rq)
+{
+ int prio = rq_prio(rq);
+
+ /*
+ * On unwinding the active request, we give it a priority bump
+ * equivalent to a freshly submitted request. This protects it from
+ * being gazumped again, but it would be preferrable if we didn't
+ * let it be gazumped in the first place!
+ *
+ * See __unwind_incomplete_requests()
+ */
+ if (i915_request_started(rq))
+ prio |= ACTIVE_PRIORITY;
+
+ return prio;
+}
+
static inline bool need_preempt(const struct intel_engine_cs *engine,
const struct i915_request *rq,
int q_prio)
{
const struct intel_context *ctx = rq->hw_context;
- const int last_prio = rq_prio(rq);
struct rb_node *rb;
+ int last_prio;
int idx;
if (!intel_engine_has_preemption(engine))
@@ -196,6 +216,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
if (i915_request_completed(rq))
return false;
+ last_prio = active_prio(rq);
if (!__execlists_need_preempt(q_prio, last_prio))
return false;
@@ -320,7 +341,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
{
struct i915_request *rq, *rn, *active = NULL;
struct list_head *uninitialized_var(pl);
- int prio = I915_PRIORITY_INVALID | I915_PRIORITY_NEWCLIENT;
+ int prio = I915_PRIORITY_INVALID | ACTIVE_PRIORITY;
lockdep_assert_held(&engine->timeline.lock);
@@ -352,8 +373,8 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
* stream, so give it the equivalent small priority bump to prevent
* it being gazumped a second time by another peer.
*/
- if (!(prio & I915_PRIORITY_NEWCLIENT)) {
- prio |= I915_PRIORITY_NEWCLIENT;
+ if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
+ prio |= ACTIVE_PRIORITY;
active->sched.attr.priority = prio;
list_move_tail(&active->sched.link,
i915_sched_lookup_priolist(engine, prio));
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption
2019-01-23 12:36 ` [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption Chris Wilson
@ 2019-01-23 13:47 ` Chris Wilson
2019-01-23 14:14 ` Chris Wilson
2019-01-23 15:13 ` [PATCH v2] " Chris Wilson
1 sibling, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 13:47 UTC (permalink / raw)
To: intel-gfx
Quoting Chris Wilson (2019-01-23 12:36:02)
> On unwinding the active request we give it a small (limited to internal
> priority levels) boost to prevent it from being gazumped a second time.
> However, this means that it can be promoted to above the request that
> triggered the preemption request, causing a preempt-to-idle cycle for no
> change. We can avoid this if we take the boost into account when
> checking if the preemption request is valid.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d9d744f6ab2c..74726f647e47 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -163,6 +163,8 @@
> #define WA_TAIL_DWORDS 2
> #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
>
> +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> +
> static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine,
> struct intel_context *ce);
> @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
> return rq->sched.attr.priority;
> }
>
> +static inline int active_prio(const struct i915_request *rq)
> +{
> + int prio = rq_prio(rq);
> +
> + /*
> + * On unwinding the active request, we give it a priority bump
> + * equivalent to a freshly submitted request. This protects it from
> + * being gazumped again, but it would be preferrable if we didn't
> + * let it be gazumped in the first place!
> + *
> + * See __unwind_incomplete_requests()
> + */
> + if (i915_request_started(rq))
> + prio |= ACTIVE_PRIORITY;
Hmm, actually we are put to the tail of that priolist so we don't get
rerun ahead of the preemptee if we end up at the same priority.
ACTIVE_PRIORITY - 1 would seem to be the right compromise.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption
2019-01-23 13:47 ` Chris Wilson
@ 2019-01-23 14:14 ` Chris Wilson
2019-01-23 14:44 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 14:14 UTC (permalink / raw)
To: intel-gfx
Quoting Chris Wilson (2019-01-23 13:47:29)
> Quoting Chris Wilson (2019-01-23 12:36:02)
> > On unwinding the active request we give it a small (limited to internal
> > priority levels) boost to prevent it from being gazumped a second time.
> > However, this means that it can be promoted to above the request that
> > triggered the preemption request, causing a preempt-to-idle cycle for no
> > change. We can avoid this if we take the boost into account when
> > checking if the preemption request is valid.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> > 1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index d9d744f6ab2c..74726f647e47 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -163,6 +163,8 @@
> > #define WA_TAIL_DWORDS 2
> > #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> >
> > +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> > +
> > static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> > struct intel_engine_cs *engine,
> > struct intel_context *ce);
> > @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
> > return rq->sched.attr.priority;
> > }
> >
> > +static inline int active_prio(const struct i915_request *rq)
> > +{
> > + int prio = rq_prio(rq);
> > +
> > + /*
> > + * On unwinding the active request, we give it a priority bump
> > + * equivalent to a freshly submitted request. This protects it from
> > + * being gazumped again, but it would be preferrable if we didn't
> > + * let it be gazumped in the first place!
> > + *
> > + * See __unwind_incomplete_requests()
> > + */
> > + if (i915_request_started(rq))
> > + prio |= ACTIVE_PRIORITY;
>
> Hmm, actually we are put to the tail of that priolist so we don't get
> rerun ahead of the preemptee if we end up at the same priority.
> ACTIVE_PRIORITY - 1 would seem to be the right compromise.
gem_sync/switch-default says ACTIVE_PRIORITY though. Hmm.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption
2019-01-23 14:14 ` Chris Wilson
@ 2019-01-23 14:44 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 14:44 UTC (permalink / raw)
To: intel-gfx
Quoting Chris Wilson (2019-01-23 14:14:05)
> Quoting Chris Wilson (2019-01-23 13:47:29)
> > Quoting Chris Wilson (2019-01-23 12:36:02)
> > > On unwinding the active request we give it a small (limited to internal
> > > priority levels) boost to prevent it from being gazumped a second time.
> > > However, this means that it can be promoted to above the request that
> > > triggered the preemption request, causing a preempt-to-idle cycle for no
> > > change. We can avoid this if we take the boost into account when
> > > checking if the preemption request is valid.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> > > 1 file changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > > index d9d744f6ab2c..74726f647e47 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -163,6 +163,8 @@
> > > #define WA_TAIL_DWORDS 2
> > > #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> > >
> > > +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> > > +
> > > static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> > > struct intel_engine_cs *engine,
> > > struct intel_context *ce);
> > > @@ -181,13 +183,31 @@ static inline int rq_prio(const struct i915_request *rq)
> > > return rq->sched.attr.priority;
> > > }
> > >
> > > +static inline int active_prio(const struct i915_request *rq)
> > > +{
> > > + int prio = rq_prio(rq);
> > > +
> > > + /*
> > > + * On unwinding the active request, we give it a priority bump
> > > + * equivalent to a freshly submitted request. This protects it from
> > > + * being gazumped again, but it would be preferrable if we didn't
> > > + * let it be gazumped in the first place!
> > > + *
> > > + * See __unwind_incomplete_requests()
> > > + */
> > > + if (i915_request_started(rq))
> > > + prio |= ACTIVE_PRIORITY;
> >
> > Hmm, actually we are put to the tail of that priolist so we don't get
> > rerun ahead of the preemptee if we end up at the same priority.
> > ACTIVE_PRIORITY - 1 would seem to be the right compromise.
>
> gem_sync/switch-default says ACTIVE_PRIORITY though. Hmm.
The answer is don't be lazy.
- if (i915_request_started(rq))
+ if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY &&
+ i915_request_started(rq)) {
prio |= ACTIVE_PRIORITY;
+ prio--;
+ }
That doesn't break switch-default while providing a more accurate
estimate of prio after preemption.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] drm/i915/execlists: Suppress redundant preemption
2019-01-23 12:36 ` [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption Chris Wilson
2019-01-23 13:47 ` Chris Wilson
@ 2019-01-23 15:13 ` Chris Wilson
1 sibling, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2019-01-23 15:13 UTC (permalink / raw)
To: intel-gfx
On unwinding the active request we give it a small (limited to internal
priority levels) boost to prevent it from being gazumped a second time.
However, this means that it can be promoted to above the request that
triggered the preemption request, causing a preempt-to-idle cycle for no
change. We can avoid this if we take the boost into account when
checking if the preemption request is valid.
v2: After preemption the active request will be after the preemptee if
they end up with equal priority.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d9d744f6ab2c..65997ed055b2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -163,6 +163,8 @@
#define WA_TAIL_DWORDS 2
#define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
+#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
+
static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
struct intel_engine_cs *engine,
struct intel_context *ce);
@@ -181,13 +183,41 @@ static inline int rq_prio(const struct i915_request *rq)
return rq->sched.attr.priority;
}
+static inline int active_prio(const struct i915_request *rq)
+{
+ int prio = rq_prio(rq);
+
+ /*
+ * On unwinding the active request, we give it a priority bump
+ * equivalent to a freshly submitted request. This protects it from
+ * being gazumped again, but it would be preferable if we didn't
+ * let it be gazumped in the first place!
+ *
+ * See __unwind_incomplete_requests()
+ */
+ if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY &&
+ i915_request_started(rq)) {
+ /*
+ * After preemption, we insert the active request at the
+ * end of the new priority level. This means that we will be
+ * _lower_ priority than the preemptee all things equal (and
+ * so the preemption is valid), so adjust our comparison
+ * accordingly.
+ */
+ prio |= ACTIVE_PRIORITY;
+ prio--;
+ }
+
+ return prio;
+}
+
static inline bool need_preempt(const struct intel_engine_cs *engine,
const struct i915_request *rq,
int q_prio)
{
const struct intel_context *ctx = rq->hw_context;
- const int last_prio = rq_prio(rq);
struct rb_node *rb;
+ int last_prio;
int idx;
if (!intel_engine_has_preemption(engine))
@@ -196,6 +226,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
if (i915_request_completed(rq))
return false;
+ last_prio = active_prio(rq);
if (!__execlists_need_preempt(q_prio, last_prio))
return false;
@@ -320,7 +351,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
{
struct i915_request *rq, *rn, *active = NULL;
struct list_head *uninitialized_var(pl);
- int prio = I915_PRIORITY_INVALID | I915_PRIORITY_NEWCLIENT;
+ int prio = I915_PRIORITY_INVALID | ACTIVE_PRIORITY;
lockdep_assert_held(&engine->timeline.lock);
@@ -352,8 +383,8 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
* stream, so give it the equivalent small priority bump to prevent
* it being gazumped a second time by another peer.
*/
- if (!(prio & I915_PRIORITY_NEWCLIENT)) {
- prio |= I915_PRIORITY_NEWCLIENT;
+ if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
+ prio |= ACTIVE_PRIORITY;
active->sched.attr.priority = prio;
list_move_tail(&active->sched.link,
i915_sched_lookup_priolist(engine, prio));
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
2019-01-23 12:36 ` [PATCH 2/3] drm/i915/execlists: Suppress preempting self Chris Wilson
2019-01-23 12:36 ` [PATCH 3/3] drm/i915/execlists: Suppress redundant preemption Chris Wilson
@ 2019-01-23 13:22 ` Patchwork
2019-01-23 13:22 ` ✗ Fi.CI.SPARSE: " Patchwork
` (7 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 13:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL : https://patchwork.freedesktop.org/series/55638/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
6817512a013e drm/i915/execlists: Mark up priority boost on preemption
7085cc180979 drm/i915/execlists: Suppress preempting self
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18:
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")'
#18:
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
-:138: WARNING:TYPO_SPELLING: 'premption' may be misspelled - perhaps 'preemption'?
#138: FILE: drivers/gpu/drm/i915/intel_lrc.c:236:
+ * Even with premption, there are times when we think it is better not
total: 1 errors, 2 warnings, 0 checks, 131 lines checked
97e0a1959b72 drm/i915/execlists: Suppress redundant preemption
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (2 preceding siblings ...)
2019-01-23 13:22 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption Patchwork
@ 2019-01-23 13:22 ` Patchwork
2019-01-23 13:34 ` [PATCH 1/3] " Tvrtko Ursulin
` (6 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 13:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL : https://patchwork.freedesktop.org/series/55638/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/execlists: Mark up priority boost on preemption
+drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)
Commit: drm/i915/execlists: Suppress preempting self
-drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)
Commit: drm/i915/execlists: Suppress redundant preemption
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (3 preceding siblings ...)
2019-01-23 13:22 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-01-23 13:34 ` Tvrtko Ursulin
2019-01-23 13:40 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] " Patchwork
` (5 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2019-01-23 13:34 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 23/01/2019 12:36, Chris Wilson wrote:
> Record the priority boost we giving to the preempted client or else we
> may end up in a situation where the priority queue no longer matches the
> request priority order and so we can end up in an infinite loop of
> preempting the same pair of requests.
>
> Fixes: e9eaf82d97a2 ("drm/i915: Priority boost for waiting clients")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 436e59724900..8aa8a4862543 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -302,6 +302,7 @@ static void __unwind_incomplete_requests(struct intel_engine_cs *engine)
> */
> if (!(prio & I915_PRIORITY_NEWCLIENT)) {
> prio |= I915_PRIORITY_NEWCLIENT;
> + active->sched.attr.priority = prio;
> list_move_tail(&active->sched.link,
> i915_sched_lookup_priolist(engine, prio));
> }
> @@ -625,6 +626,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> int i;
>
> priolist_for_each_request_consume(rq, rn, p, i) {
> + GEM_BUG_ON(last &&
> + need_preempt(engine, last, rq_prio(rq)));
> +
> /*
> * Can we combine this request with the current port?
> * It has to be the same context/ringbuffer and not
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (4 preceding siblings ...)
2019-01-23 13:34 ` [PATCH 1/3] " Tvrtko Ursulin
@ 2019-01-23 13:40 ` Patchwork
2019-01-23 15:34 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2) Patchwork
` (4 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 13:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption
URL : https://patchwork.freedesktop.org/series/55638/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_5470 -> Patchwork_12015
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_12015 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_12015, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://patchwork.freedesktop.org/api/1.0/series/55638/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12015:
### IGT changes ###
#### Possible regressions ####
* igt@pm_rpm@module-reload:
- fi-skl-6770hq: PASS -> FAIL
Known issues
------------
Here are the changes found in Patchwork_12015 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_execlists:
- fi-apl-guc: PASS -> DMESG-WARN [fdo#108622]
* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: PASS -> DMESG-FAIL [fdo#103841]
* igt@kms_flip@basic-flip-vs-dpms:
- fi-skl-6700hq: PASS -> DMESG-WARN [fdo#105998]
* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: PASS -> FAIL [fdo#103167]
* igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
- fi-byt-clapper: PASS -> FAIL [fdo#107362]
#### Possible fixes ####
* igt@i915_selftest@live_hangcheck:
- fi-bwr-2160: DMESG-FAIL [fdo#108735] -> PASS
* igt@kms_frontbuffer_tracking@basic:
- {fi-icl-u3}: FAIL [fdo#103167] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
- fi-hsw-4770: {SKIP} [fdo#109271] -> PASS +3
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
Participating hosts (45 -> 40)
------------------------------
Missing (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-pnv-d510
Build changes
-------------
* Linux: CI_DRM_5470 -> Patchwork_12015
CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12015: 97e0a1959b722739a349f620c1e83e12359923e6 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
97e0a1959b72 drm/i915/execlists: Suppress redundant preemption
7085cc180979 drm/i915/execlists: Suppress preempting self
6817512a013e drm/i915/execlists: Mark up priority boost on preemption
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12015/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (5 preceding siblings ...)
2019-01-23 13:40 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] " Patchwork
@ 2019-01-23 15:34 ` Patchwork
2019-01-23 15:35 ` ✗ Fi.CI.SPARSE: " Patchwork
` (3 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 15:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL : https://patchwork.freedesktop.org/series/55638/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
8de30aea3cc3 drm/i915/execlists: Mark up priority boost on preemption
49059512f804 drm/i915/execlists: Suppress preempting self
-:18: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#18:
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")'
#18:
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
-:138: WARNING:TYPO_SPELLING: 'premption' may be misspelled - perhaps 'preemption'?
#138: FILE: drivers/gpu/drm/i915/intel_lrc.c:236:
+ * Even with premption, there are times when we think it is better not
total: 1 errors, 2 warnings, 0 checks, 131 lines checked
81300ac71039 drm/i915/execlists: Suppress redundant preemption
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (6 preceding siblings ...)
2019-01-23 15:34 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2) Patchwork
@ 2019-01-23 15:35 ` Patchwork
2019-01-23 15:53 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 15:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL : https://patchwork.freedesktop.org/series/55638/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/execlists: Mark up priority boost on preemption
+drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)
Commit: drm/i915/execlists: Suppress preempting self
-drivers/gpu/drm/i915/intel_ringbuffer.h:602:23: warning: expression using sizeof(void)
Commit: drm/i915/execlists: Suppress redundant preemption
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (7 preceding siblings ...)
2019-01-23 15:35 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-01-23 15:53 ` Patchwork
2019-01-23 17:46 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3) Patchwork
2019-01-23 18:15 ` ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2) Patchwork
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 15:53 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL : https://patchwork.freedesktop.org/series/55638/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5470 -> Patchwork_12017
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/55638/revisions/2/mbox/
Known issues
------------
Here are the changes found in Patchwork_12017 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: PASS -> FAIL [fdo#103167]
* igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362]
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
- fi-byt-clapper: PASS -> FAIL [fdo#107362]
#### Possible fixes ####
* igt@i915_selftest@live_hangcheck:
- fi-bwr-2160: DMESG-FAIL [fdo#108735] -> PASS
* igt@kms_frontbuffer_tracking@basic:
- {fi-icl-u3}: FAIL [fdo#103167] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
- fi-hsw-4770: {SKIP} [fdo#109271] -> PASS +3
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
Participating hosts (45 -> 41)
------------------------------
Missing (4): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan
Build changes
-------------
* Linux: CI_DRM_5470 -> Patchwork_12017
CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12017: 81300ac710397ca473f7cebe94a2100e454340c8 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
81300ac71039 drm/i915/execlists: Suppress redundant preemption
49059512f804 drm/i915/execlists: Suppress preempting self
8de30aea3cc3 drm/i915/execlists: Mark up priority boost on preemption
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3)
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (8 preceding siblings ...)
2019-01-23 15:53 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-01-23 17:46 ` Patchwork
2019-01-23 18:15 ` ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2) Patchwork
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 17:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3)
URL : https://patchwork.freedesktop.org/series/55638/
State : failure
== Summary ==
Applying: drm/i915/execlists: Mark up priority boost on preemption
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/intel_lrc.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/execlists: Suppress preempting self
Applying: drm/i915/execlists: Suppress redundant preemption
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/intel_lrc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_lrc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_lrc.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0003 drm/i915/execlists: Suppress redundant preemption
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
2019-01-23 12:36 [PATCH 1/3] drm/i915/execlists: Mark up priority boost on preemption Chris Wilson
` (9 preceding siblings ...)
2019-01-23 17:46 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev3) Patchwork
@ 2019-01-23 18:15 ` Patchwork
10 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-01-23 18:15 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/execlists: Mark up priority boost on preemption (rev2)
URL : https://patchwork.freedesktop.org/series/55638/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5470_full -> Patchwork_12017_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_12017_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
- shard-kbl: NOTRUN -> DMESG-WARN [fdo#107956]
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
- shard-apl: PASS -> FAIL [fdo#106510] / [fdo#108145]
* igt@kms_cursor_crc@cursor-128x128-random:
- shard-apl: PASS -> FAIL [fdo#103232] +5
* igt@kms_cursor_crc@cursor-128x42-random:
- shard-glk: PASS -> FAIL [fdo#103232] +4
* igt@kms_cursor_crc@cursor-256x256-suspend:
- shard-apl: PASS -> FAIL [fdo#103191] / [fdo#103232]
* igt@kms_flip@flip-vs-panning:
- shard-snb: PASS -> INCOMPLETE [fdo#105411]
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-indfb-pgflip-blt:
- shard-snb: NOTRUN -> INCOMPLETE [fdo#105411] / [fdo#107469]
* igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
- shard-glk: PASS -> FAIL [fdo#108948]
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
- shard-kbl: NOTRUN -> FAIL [fdo#108145]
* igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
- shard-apl: PASS -> FAIL [fdo#103166] +2
* igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
- shard-glk: PASS -> FAIL [fdo#103166]
* igt@kms_setmode@basic:
- shard-kbl: PASS -> FAIL [fdo#99912]
#### Possible fixes ####
* igt@gem_softpin@noreloc-s3:
- shard-apl: INCOMPLETE [fdo#103927] -> PASS
* igt@kms_cursor_crc@cursor-128x42-onscreen:
- shard-glk: FAIL [fdo#103232] -> PASS +2
* igt@kms_cursor_crc@cursor-256x85-sliding:
- shard-apl: FAIL [fdo#103232] -> PASS +2
* igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
- shard-glk: FAIL [fdo#104873] -> PASS
* igt@kms_plane@pixel-format-pipe-b-planes:
- shard-apl: FAIL [fdo#103166] -> PASS
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
- shard-glk: FAIL [fdo#108145] -> PASS +1
* igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
- shard-glk: FAIL [fdo#103166] -> PASS +2
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-apl: DMESG-FAIL [fdo#108950] -> PASS
* igt@kms_setmode@basic:
- shard-hsw: FAIL [fdo#99912] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
[fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
[fdo#108950]: https://bugs.freedesktop.org/show_bug.cgi?id=108950
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (7 -> 5)
------------------------------
Missing (2): shard-skl shard-iclb
Build changes
-------------
* Linux: CI_DRM_5470 -> Patchwork_12017
CI_DRM_5470: bfac8844379563353df6f77b0cde827ed74653eb @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4785: 70749c70926f12043d3408b160606e1e6238ed3a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12017: 81300ac710397ca473f7cebe94a2100e454340c8 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread