drm/i915: Special handling for bonded requests
diff mbox series

Message ID 20200527085322.25861-1-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • drm/i915: Special handling for bonded requests
Related show

Commit Message

Tvrtko Ursulin May 27, 2020, 8:53 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@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.

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.

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

Comments

Chris Wilson May 27, 2020, 9:20 a.m. UTC | #1
Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Suggested-by: Xiaogang Li <xiaogang.li@intel.com>
> Cc: Chris Wilson <chris@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
Tvrtko Ursulin May 27, 2020, 9:36 a.m. UTC | #2
On 27/05/2020 10:20, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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.

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@intel.com>
>> Suggested-by: Xiaogang Li <xiaogang.li@intel.com>
>> Cc: Chris Wilson <chris@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.

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
Chris Wilson May 27, 2020, 10:05 a.m. UTC | #3
Quoting Tvrtko Ursulin (2020-05-27 10:36:31)
> 
> On 27/05/2020 10:20, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@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.
> 
> 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.

I'm going to say the starting point is to remove the
I915_ENGINE_HAS_PREEMPTION flag from vcs*. That way the tests that
require preemption will skip. Sadly that's the level of our current API.

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

You don't have a deadlock between unrelated pairs :)

> 
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Chris Wilson May 28, 2020, 9:57 a.m. UTC | #4
Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
> +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.
> +        *
> +        * Gen11+
> +        */
> +       if (INTEL_GEN(rq->i915) < 11 ||
> +           rq->engine->class != VIDEO_DECODE_CLASS ||
> +           rq->engine->class != signal->engine->class)
> +               return;
> +
> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &rq->fence.flags);
> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &signal->fence.flags);
> +
> +       intel_context_set_single_submission(rq->context);
> +       intel_context_set_single_submission(signal->context);

The thought that just popped into my head:

This can be after signal is already submitted into ELSP[1].
-Chris
Tvrtko Ursulin May 28, 2020, 10:20 a.m. UTC | #5
On 28/05/2020 10:57, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
>> +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.
>> +        *
>> +        * Gen11+
>> +        */
>> +       if (INTEL_GEN(rq->i915) < 11 ||
>> +           rq->engine->class != VIDEO_DECODE_CLASS ||
>> +           rq->engine->class != signal->engine->class)
>> +               return;
>> +
>> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &rq->fence.flags);
>> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &signal->fence.flags);
>> +
>> +       intel_context_set_single_submission(rq->context);
>> +       intel_context_set_single_submission(signal->context);
> 
> The thought that just popped into my head:
> 
> This can be after signal is already submitted into ELSP[1].

Yep I knew that but thought it would still work.

Master in vcs0 port1, slave in vcs1 port0 or queued.

If queued that means at worst case another bonded pair is running on 
same engines, so they should be able to complete.

If slave submitted to vcs1 port0 then it will stay there until whatever 
is in vcs0 port0 finishes and lets master in.

Do you see a possibility for things to go bad?

Regards,

Tvrtko
Chris Wilson May 28, 2020, 8:23 p.m. UTC | #6
Quoting Tvrtko Ursulin (2020-05-28 11:20:07)
> 
> On 28/05/2020 10:57, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
> >> +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.
> >> +        *
> >> +        * Gen11+
> >> +        */
> >> +       if (INTEL_GEN(rq->i915) < 11 ||
> >> +           rq->engine->class != VIDEO_DECODE_CLASS ||
> >> +           rq->engine->class != signal->engine->class)
> >> +               return;
> >> +
> >> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &rq->fence.flags);
> >> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &signal->fence.flags);
> >> +
> >> +       intel_context_set_single_submission(rq->context);
> >> +       intel_context_set_single_submission(signal->context);
> > 
> > The thought that just popped into my head:
> > 
> > This can be after signal is already submitted into ELSP[1].
> 
> Yep I knew that but thought it would still work.
> 
> Master in vcs0 port1, slave in vcs1 port0 or queued.
> 
> If queued that means at worst case another bonded pair is running on 
> same engines, so they should be able to complete.
> 
> If slave submitted to vcs1 port0 then it will stay there until whatever 
> is in vcs0 port0 finishes and lets master in.
> 
> Do you see a possibility for things to go bad?

Because the master is already submitted in port1, the bond can go into
port0. Then a second bond turns up for the master in port0, and we're
back at square one.

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 37855ae8f8b3..698608e11df3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2502,6 +2502,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
        lockdep_unpin_lock(&tl->mutex, rq->cookie);
 
        trace_i915_request_add(rq);
+       set_bit(I915_FENCE_FLAG_SENTINEL, &rq->fence.flags);
 
        prev = __i915_request_commit(rq);

Will do the trick.

(Plus fixing up the rules for assert_pending_valid).
-Chris
Tvrtko Ursulin May 29, 2020, 7:25 a.m. UTC | #7
On 28/05/2020 21:23, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-28 11:20:07)
>>
>> On 28/05/2020 10:57, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-05-27 09:53:22)
>>>> +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.
>>>> +        *
>>>> +        * Gen11+
>>>> +        */
>>>> +       if (INTEL_GEN(rq->i915) < 11 ||
>>>> +           rq->engine->class != VIDEO_DECODE_CLASS ||
>>>> +           rq->engine->class != signal->engine->class)
>>>> +               return;
>>>> +
>>>> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &rq->fence.flags);
>>>> +       set_bit(I915_FENCE_FLAG_NOPREEMPT, &signal->fence.flags);
>>>> +
>>>> +       intel_context_set_single_submission(rq->context);
>>>> +       intel_context_set_single_submission(signal->context);
>>>
>>> The thought that just popped into my head:
>>>
>>> This can be after signal is already submitted into ELSP[1].
>>
>> Yep I knew that but thought it would still work.
>>
>> Master in vcs0 port1, slave in vcs1 port0 or queued.
>>
>> If queued that means at worst case another bonded pair is running on
>> same engines, so they should be able to complete.
>>
>> If slave submitted to vcs1 port0 then it will stay there until whatever
>> is in vcs0 port0 finishes and lets master in.
>>
>> Do you see a possibility for things to go bad?
> 
> Because the master is already submitted in port1, the bond can go into
> port0. Then a second bond turns up for the master in port0, and we're
> back at square one.
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 37855ae8f8b3..698608e11df3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2502,6 +2502,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>          lockdep_unpin_lock(&tl->mutex, rq->cookie);
>   
>          trace_i915_request_add(rq);
> +       set_bit(I915_FENCE_FLAG_SENTINEL, &rq->fence.flags);
>   
>          prev = __i915_request_commit(rq);
> 
> Will do the trick.
> 
> (Plus fixing up the rules for assert_pending_valid).

Hmm yes, my logic was flawed by missing to see the async disconnect 
between master and slave submission on both ends. That's why Xiaogang 
was saying slaves must not have no-preempt set... But sentinel on 
everything? Or just everything vcs and gen11+?

So if we indeed had slave preemptible the deadlock would have been 
avoided I think, but can the media pipeline handle that is the question.

Another question is that it sounds it could be possible to work around 
this in userspace, combined with this patch (original thread), if a 
fence was used to block master until slave is submitted.

  split_fence = sw_fence_create()
  execbuf(master, in_fence = split_fence) = master_fence
  execbuf(slave, submit_fence = master_fence)
  sw_fence_advance(split_fence)

That would make sure the single port and no-preempt properties are 
applied before either master or slave can enter elsp.

Sounds tempting to try, thoughts?

Regards,

Tvrtko

Patch
diff mbox series

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.
+	 *
+	 * Gen11+
+	 */
+	if (INTEL_GEN(rq->i915) < 11 ||
+	    rq->engine->class != VIDEO_DECODE_CLASS ||
+	    rq->engine->class != signal->engine->class)
+		return;
+
+	set_bit(I915_FENCE_FLAG_NOPREEMPT, &rq->fence.flags);
+	set_bit(I915_FENCE_FLAG_NOPREEMPT, &signal->fence.flags);
+
+	intel_context_set_single_submission(rq->context);
+	intel_context_set_single_submission(signal->context);
+}
+
+static void
+execlists_bond_execute(struct i915_request *rq, struct dma_fence *signal)
+{
+	mark_bonded_pair(rq, to_request(signal));
+}
+
 void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
 {
 	engine->submit_request = execlists_submit_request;
+	if (engine->class == VIDEO_DECODE_CLASS)
+		engine->bond_execute = execlists_bond_execute;
 	engine->schedule = i915_schedule;
 	engine->execlists.tasklet.func = execlists_submission_tasklet;
 
@@ -5602,15 +5633,18 @@  virtual_find_bond(struct virtual_engine *ve,
 }
 
 static void
-virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
+virtual_bond_execute(struct i915_request *rq, struct dma_fence *sigfence)
 {
 	struct virtual_engine *ve = to_virtual_engine(rq->engine);
+	struct i915_request *signal = to_request(sigfence);
 	intel_engine_mask_t allowed, exec;
 	struct ve_bond *bond;
 
-	allowed = ~to_request(signal)->engine->mask;
+	mark_bonded_pair(rq, signal);
+
+	allowed = ~signal->engine->mask;
 
-	bond = virtual_find_bond(ve, to_request(signal)->engine);
+	bond = virtual_find_bond(ve, signal->engine);
 	if (bond)
 		allowed &= bond->sibling_mask;
 
@@ -5620,7 +5654,7 @@  virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
 		;
 
 	/* Prevent the master from being re-run on the bonded engines */
-	to_request(signal)->execution_mask &= ~allowed;
+	signal->execution_mask &= ~allowed;
 }
 
 struct intel_context *