On 27/05/2020 10:20, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Our generic mechanism for specifying aligned request start,
I915_EXEC_FENCE_SUBMIT, has a bit too relaxed rules when it comes to the
actual use case of media scalability on Tigerlake.
While the submit fence can provide the aligned start, the actual media
pipeline expects that execution remains in lockstep from then onwards.
Otherwise the hardware cannot handle well one of the pair getting
preempted leading to GPU hangs and corrupted media playback.
This puts us in a difficult position where the only visible solution,
which does not involve adding new uapi, is to give more meaning to the
generic submit fence. At least when used between the video engines.
The special semantics this patch applies in that case are two fold. First
is that both of the aligned pairs will be marked as non-preemptable and
second is ensuring both are not sharing ELSP ports with any other context.
Non-preemptable property will ensure that once the start has been aligned
the requests remain executing until completion.
Single port ELSP property will ensure there are no possible inversions
between different independent pairs of aligned requests.
Nak.
Going forward we can think of introducing new uapi to request this
behaviour as a separate, more explicit flag, and at that point retire this
semi-generic special handling.
As CI will say, such behaviour will already need a new flag. Forcing
no-preemption should be a privileged flag, so I would expect some
acknowledge that this is a HW problem that we have to workaround.
I don't know how hw exactly works so from my side it's only empirical.
I agree new flag is needed but we also need a fix ASAP for TGL. And I
don't think we can add new uapi in a reasonable time frame here. We
would need the ctx set_parallel extension with a dont-preempt flag and
multi-batch execbuf. And a lot of work in media-driver which will not be
ready for TGL. I don't think a flag at the I915_EXEC_FENCE_SUBMIT level
is a solution.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Suggested-by: Xiaogang Li <xiaogang.li@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gt/intel_context.h | 2 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 46 +++++++++++++++++++++----
2 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
index 07be021882cc..576d11c0cdd9 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -212,7 +212,7 @@ intel_context_force_single_submission(const struct intel_context *ce)
static inline void
intel_context_set_single_submission(struct intel_context *ce)
{
- __set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ce->flags);
+ set_bit(CONTEXT_FORCE_SINGLE_SUBMISSION, &ce->flags);
}
static inline bool
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 3214a4ecc31a..88cf20fd92c8 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1734,8 +1734,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
static bool ctx_single_port_submission(const struct intel_context *ce)
{
- return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
- intel_context_force_single_submission(ce));
+ return intel_context_force_single_submission(ce);
}
static bool can_merge_ctx(const struct intel_context *prev,
@@ -4929,9 +4928,41 @@ static void execlists_park(struct intel_engine_cs *engine)
cancel_timer(&engine->execlists.preempt);
}
+static void
+mark_bonded_pair(struct i915_request *rq, struct i915_request *signal)
+{
+ /*
+ * Give (temporary) special meaning to a pair requests with requested
+ * aligned start along the video engines.
+ *
+ * They should be non-preemptable and have all ELSP ports to themselves
+ * to avoid any deadlocks caused by inversions.
This sentence needs expanding, because you are implying that this is an
issue we need to address in the code. If there is a deadlock resulting
from incorrect submission ordering, that is a bug in the code.
If we add no-preempt I think we have to have single elsp because there
is no ordering between unrelated pairs and then elsp inversion certainly
sounds like a deadlock.
ctx-A: vcs0 + vcs1
ctx-B: vcs0 + vcs1
There is no ordering between A and B but we'd have to pick same port for
both pairs of A and B respectively. I don't see that we can do that
since all four submissions are async.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
