On 19/04/2018 12:10, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-04-19 11:40:06)
On 19/04/2018 08:17, Chris Wilson wrote:
To ease the frequent and ugly pointer dance of
&request->gem_context->engine[request->engine->id] during request
submission, store that pointer as request->hw_context. One major
advantage that we will exploit later is that this decouples the logical
context state from the engine itself.
I can see merit in making all the places which work on engines or
requests more logically and elegantly grab the engine specific context.
Authors of current unmerged work will probably be not so thrilled though.
@@ -353,60 +346,56 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
struct intel_vgpu_submission *s = &vgpu->submission;
struct i915_gem_context *shadow_ctx = s->shadow_ctx;
struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
- int ring_id = workload->ring_id;
- struct intel_engine_cs *engine = dev_priv->engine[ring_id];
- struct intel_ring *ring;
+ struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
+ struct intel_context *ce;
int ret;
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- if (workload->shadowed)
+ if (workload->req)
return 0;
GVT team will need to look at this hunk/function.
...
At which point I skip over all GVT and continue further down. :)
That code doesn't merit your attention (or ours really until it is
improved, it really is staging/ quality).
Still, best to ask GVT team to r-b their parts.
[snip]
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0777226e65a6..e6725690b5b6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -164,7 +164,8 @@
#define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
- struct intel_engine_cs *engine);
+ struct intel_engine_cs *engine,
+ struct intel_context *ce);
Feels a bit redundant to pass in everything but OK.
You do remember which patch this was split out of ;)
{
return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
- i915_gem_context_force_single_submission(ctx));
+ i915_gem_context_force_single_submission(ctx->gem_context));
}
But the whole function change is again not needed I think.
-static bool can_merge_ctx(const struct i915_gem_context *prev,
- const struct i915_gem_context *next)
+static bool can_merge_ctx(const struct intel_context *prev,
+ const struct intel_context *next)
This one neither.
Consider what it means. The rule is that we are not allowed to submit
the same lrc descriptor to subsequent ports; that corresponds to
the hw context, not gem_context. The payoff, aside from the better
semantics now, is in subsequent patches.
Yes all fine, just saying it did not have to be in _this_ series but
could have came later.
+static struct intel_context *
+execlists_context_pin(struct intel_engine_cs *engine,
+ struct i915_gem_context *ctx)
{
- struct intel_context *ce = &ctx->engine[engine->id];
+ struct intel_context *ce = to_intel_context(ctx, engine);
lockdep_assert_held(&ctx->i915->drm.struct_mutex);
- GEM_BUG_ON(ce->pin_count == 0);
-
- if (--ce->pin_count)
- return;
- intel_ring_unpin(ce->ring);
+ if (likely(ce->pin_count++))
+ return ce;
+ GEM_BUG_ON(!ce->pin_count); /* no overflow please! */
- ce->state->obj->pin_global--;
- i915_gem_object_unpin_map(ce->state->obj);
- i915_vma_unpin(ce->state);
+ ce->ops = &execlists_context_ops;
If ops are set on pin it would be good to unset them on destroy.
No point on destroy, since it's being destroyed.
Hm, or even set them not on pin but on creation.
engine->context_pin() is our intel_context factory, pin is the creation
function.
Definitely a bit asymmetrical, the key bit was having ce->unpin() so that
we didn't try to release the intel_context via a stale engine reference.
What I have been considering is passing intel_context to
i915_request_alloc() instead and trying to avoid that asymmetry by
managing the creation in the caller and separating it from pin().
Not a huge deal but it is a
bit unbalanced, and also if ce->ops is also a guard to distinguish
possible from impossible engines then it is a bit more strange.
Not impossible engines in this regard, unused contexts.
Both then, since impossible engines will have unused contexts. :)
@@ -1323,10 +1345,6 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
intel_engine_setup_common(engine);
- err = intel_engine_init_common(engine);
- if (err)
- goto err;
-
ring = intel_engine_create_ring(engine, 32 * PAGE_SIZE);
if (IS_ERR(ring)) {
err = PTR_ERR(ring);
@@ -1341,8 +1359,14 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
GEM_BUG_ON(engine->buffer);
engine->buffer = ring;
+ err = intel_engine_init_common(engine);
+ if (err)
+ goto err_unpin;
+
This ordering change doesn't look like it has to be done in this patch.
It does. Do you want to take another look ;)
The problem here is the legacy ringbuffer submission uses a common
engine->buffer, and the kernel contexts are pinned in init_common; so we
have to create that buffer first.
So it looks like an pre-existing bug. intel_ring_context_pin returns
engine->buffer which hasn't been created yet. Oh well.. can you inject a
patch which solves only that before this one?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
