[Intel-gfx] [PATCH] drm/i915: Special handling for bonded requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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.

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Suggested-by: Xiaogang Li <xiaogang.li at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  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.
-Chris