Re: [Intel-gfx] [PATCH 06/22] drm/i915: Pass intel_context to i915_request_create()
Quoting Chris Wilson (2019-04-03 08:54:41)
> Quoting Tvrtko Ursulin (2019-04-02 14:17:30)
> >
> > On 25/03/2019 09:03, Chris Wilson wrote:
> > > @@ -727,17 +695,19 @@ i915_request_alloc(struct intel_engine_cs *engine,
> > > struct i915_gem_context *ctx)
> > > if (ret)
> > > goto err_unwind;
> > >
> > > - ret = engine->request_alloc(rq);
> > > + ret = rq->engine->request_alloc(rq);
> > > if (ret)
> > > goto err_unwind;
> > >
> > > + rq->infix = rq->ring->emit; /* end of header; start of user payload
> > > */
> > > +
> > > /* Keep a second pin for the dual retirement along engine and ring
> > > */
> > > __intel_context_pin(ce);
> > > -
> > > - rq->infix = rq->ring->emit; /* end of header; start of user payload
> > > */
> > > + atomic_inc(>gt.active_requests);
> >
> > Oh I hate this..
> >
> > >
> > > /* Check that we didn't interrupt ourselves with a new request */
> > > GEM_BUG_ON(rq->timeline->seqno != rq->fence.seqno);
> > > + lockdep_assert_held(>mutex);
> > > return rq;
>
> [snip]
>
> > > + /*
> > > + * Pinning the contexts may generate requests in order to acquire
> > > + * GGTT space, so do this first before we reserve a seqno for
> > > + * ourselves.
> > > + */
> > > + ce = intel_context_pin(ctx, engine);
> > > + if (IS_ERR(ce))
> > > + return ERR_CAST(ce);
> > > +
> > > + i915_gem_unpark(i915);
> > > +
> > > + rq = i915_request_create(ce);
> > > +
> > > + i915_gem_park(i915);
> >
> > ... because it is so anti-self-documenting.
> >
> > Maybe we could have something like:
> >
> > __i915_gem_unpark(...)
> > {
> > GEM_BUG_ON(!atomic_read(active_requests));
> > atomic_inc(active_requests);
> > }
> >
> > Similar to __intel_context_pin, just so it is more obvious what the code
> > is doing.
>
> But not here though. Since this is the path that has to wake up the
> device itself, pin the context (eventually run retirement, handle
> allocation fallback) and not presume the caller already has (or will).
>
> Inside i915_require_create we do have the assert that the device is
> awake, which should be has GT wakeref instead. (Note, not GEM wakeref at
> this point, that's the troubling bit.)
>
> Post snip: Saw the connected argument.
The problem with that is no, we are not putting a __i915_gem_unpark()
into i915_request_create. At this point, we care about the gt.wakeref.
Maybe I should take another shot at splitting the GEM wakeref from GT.
i915_gem_runtime_pm_get()
i915_gt_unpark()
i915_gt_park()
i915_gem_runtime_pm_put()
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/22] drm/i915: Pass intel_context to i915_request_create()
Quoting Tvrtko Ursulin (2019-04-02 14:17:30)
>
> On 25/03/2019 09:03, Chris Wilson wrote:
> > @@ -727,17 +695,19 @@ i915_request_alloc(struct intel_engine_cs *engine,
> > struct i915_gem_context *ctx)
> > if (ret)
> > goto err_unwind;
> >
> > - ret = engine->request_alloc(rq);
> > + ret = rq->engine->request_alloc(rq);
> > if (ret)
> > goto err_unwind;
> >
> > + rq->infix = rq->ring->emit; /* end of header; start of user payload */
> > +
> > /* Keep a second pin for the dual retirement along engine and ring */
> > __intel_context_pin(ce);
> > -
> > - rq->infix = rq->ring->emit; /* end of header; start of user payload */
> > + atomic_inc(>gt.active_requests);
>
> Oh I hate this..
>
> >
> > /* Check that we didn't interrupt ourselves with a new request */
> > GEM_BUG_ON(rq->timeline->seqno != rq->fence.seqno);
> > + lockdep_assert_held(>mutex);
> > return rq;
[snip]
> > + /*
> > + * Pinning the contexts may generate requests in order to acquire
> > + * GGTT space, so do this first before we reserve a seqno for
> > + * ourselves.
> > + */
> > + ce = intel_context_pin(ctx, engine);
> > + if (IS_ERR(ce))
> > + return ERR_CAST(ce);
> > +
> > + i915_gem_unpark(i915);
> > +
> > + rq = i915_request_create(ce);
> > +
> > + i915_gem_park(i915);
>
> ... because it is so anti-self-documenting.
>
> Maybe we could have something like:
>
> __i915_gem_unpark(...)
> {
> GEM_BUG_ON(!atomic_read(active_requests));
> atomic_inc(active_requests);
> }
>
> Similar to __intel_context_pin, just so it is more obvious what the code
> is doing.
But not here though. Since this is the path that has to wake up the
device itself, pin the context (eventually run retirement, handle
allocation fallback) and not presume the caller already has (or will).
Inside i915_require_create we do have the assert that the device is
awake, which should be has GT wakeref instead. (Note, not GEM wakeref at
this point, that's the troubling bit.)
Post snip: Saw the connected argument.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/22] drm/i915: Pass intel_context to i915_request_create()
On 25/03/2019 09:03, Chris Wilson wrote:
Start acquiring the logical intel_context and using that as our primary
means for request allocation. This is the initial step to allow us to
avoid requiring struct_mutex for request allocation along the
perma-pinned kernel context, but it also provides a foundation for
breaking up the complex request allocation to handle different scenarios
inside execbuf.
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_gem_context.c | 20 +--
drivers/gpu/drm/i915/i915_perf.c | 22 ++--
drivers/gpu/drm/i915/i915_request.c | 118 ++
drivers/gpu/drm/i915/i915_request.h | 3 +
drivers/gpu/drm/i915/i915_reset.c | 8 +-
drivers/gpu/drm/i915/intel_overlay.c | 28 +++--
drivers/gpu/drm/i915/selftests/i915_active.c | 2 +-
.../drm/i915/selftests/i915_gem_coherency.c | 2 +-
.../gpu/drm/i915/selftests/i915_gem_object.c | 9 +-
drivers/gpu/drm/i915/selftests/i915_request.c | 9 +-
.../gpu/drm/i915/selftests/i915_timeline.c| 4 +-
11 files changed, 135 insertions(+), 90 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 25f267a03d3d..6a452345ffdb 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -88,6 +88,7 @@
#include
#include
#include "i915_drv.h"
+#include "i915_gem_pm.h"
#include "i915_globals.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
@@ -863,7 +864,6 @@ static int context_barrier_task(struct i915_gem_context
*ctx,
struct drm_i915_private *i915 = ctx->i915;
struct context_barrier_task *cb;
struct intel_context *ce, *next;
- intel_wakeref_t wakeref;
int err = 0;
lockdep_assert_held(>drm.struct_mutex);
@@ -876,7 +876,7 @@ static int context_barrier_task(struct i915_gem_context
*ctx,
i915_active_init(i915, >base, cb_retire);
i915_active_acquire(>base);
- wakeref = intel_runtime_pm_get(i915);
+ i915_gem_unpark(i915);
rbtree_postorder_for_each_entry_safe(ce, next, >hw_contexts, node)
{
struct intel_engine_cs *engine = ce->engine;
struct i915_request *rq;
@@ -890,7 +890,7 @@ static int context_barrier_task(struct i915_gem_context
*ctx,
break;
}
- rq = i915_request_alloc(engine, ctx);
+ rq = i915_request_create(ce);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
break;
@@ -906,7 +906,7 @@ static int context_barrier_task(struct i915_gem_context
*ctx,
if (err)
break;
}
- intel_runtime_pm_put(i915, wakeref);
+ i915_gem_park(i915);
cb->task = err ? NULL : task; /* caller needs to unwind instead */
cb->data = data;
@@ -930,11 +930,12 @@ int i915_gem_switch_to_kernel_context(struct
drm_i915_private *i915,
if (i915_terminally_wedged(i915))
return 0;
+ i915_gem_unpark(i915);
for_each_engine_masked(engine, i915, mask, mask) {
struct intel_ring *ring;
struct i915_request *rq;
- rq = i915_request_alloc(engine, i915->kernel_context);
+ rq = i915_request_create(engine->kernel_context);
if (IS_ERR(rq))
return PTR_ERR(rq);
@@ -960,6 +961,7 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915,
i915_request_add(rq);
}
+ i915_gem_park(i915);
return 0;
}
@@ -1158,7 +1160,6 @@ gen8_modify_rpcs(struct intel_context *ce, struct
intel_sseu sseu)
{
struct drm_i915_private *i915 = ce->engine->i915;
struct i915_request *rq, *prev;
- intel_wakeref_t wakeref;
int ret;
lockdep_assert_held(>pin_mutex);
@@ -1173,9 +1174,9 @@ gen8_modify_rpcs(struct intel_context *ce, struct
intel_sseu sseu)
return 0;
/* Submitting requests etc needs the hw awake. */
- wakeref = intel_runtime_pm_get(i915);
+ i915_gem_unpark(i915);
- rq = i915_request_alloc(ce->engine, i915->kernel_context);
+ rq = i915_request_create(ce->engine->kernel_context);
if (IS_ERR(rq)) {
ret = PTR_ERR(rq);
goto out_put;
@@ -1213,8 +1214,7 @@ gen8_modify_rpcs(struct intel_context *ce, struct
intel_sseu sseu)
out_add:
i915_request_add(rq);
out_put:
- intel_runtime_pm_put(i915, wakeref);
-
+ i915_gem_park(i915);
return ret;
}
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 85c5cb779297..fe7267da52e5 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -196,6 +196,7 @@
#include
#include "i915_drv.h"
+#include "i915_gem_pm.h"
#include "i915_oa_hsw.h"
