diff mbox series

[2/3] drm/i915/gem: Treat submit-fence as weak dependency for new clients

Message ID 20200507082124.1673-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/i915: Mark concurrent submissions with a weak-dependency | expand

Commit Message

Chris Wilson May 7, 2020, 8:21 a.m. UTC 
The submit-fence adds a weak dependency to the requests, and for the
purpose of our FQ_CODEL hinting we do not want to treat as a
restriction. This is primarily because clients may treat submit-fences
as a bidirectional bonding between a pair of co-ordinating requests.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Tvrtko Ursulin May 7, 2020, 2:59 p.m. UTC | #1 
On 07/05/2020 09:21, Chris Wilson wrote:
> The submit-fence adds a weak dependency to the requests, and for the
> purpose of our FQ_CODEL hinting we do not want to treat as a
> restriction. This is primarily because clients may treat submit-fences
> as a bidirectional bonding between a pair of co-ordinating requests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 966523a8503f..e8bf0cf02fd7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
>   			break;
>   }
>   
> +static bool new_client(struct i915_request *rq)
> +{
> +	struct i915_dependency *p;
> +
> +	list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> +		if (!(p->flags & I915_DEPENDENCY_WEAK))
> +			return false;
> +
> +	return true;
> +}
> +
>   static void eb_request_add(struct i915_execbuffer *eb)
>   {
>   	struct i915_request *rq = eb->request;
> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>   		 * Allow interactive/synchronous clients to jump ahead of
>   		 * the bulk clients. (FQ_CODEL)
>   		 */
> -		if (list_empty(&rq->sched.signalers_list))
> +		if (new_client(rq))
>   			attr.priority |= I915_PRIORITY_WAIT;
>   	} else {
>   		/* Serialise with context_close via the add_to_timeline */
> 

Did absence of this have any functional effect? I hope not, but anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
Chris Wilson May 7, 2020, 3:05 p.m. UTC | #2 
Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
> 
> On 07/05/2020 09:21, Chris Wilson wrote:
> > The submit-fence adds a weak dependency to the requests, and for the
> > purpose of our FQ_CODEL hinting we do not want to treat as a
> > restriction. This is primarily because clients may treat submit-fences
> > as a bidirectional bonding between a pair of co-ordinating requests.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 966523a8503f..e8bf0cf02fd7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
> >                       break;
> >   }
> >   
> > +static bool new_client(struct i915_request *rq)
> > +{
> > +     struct i915_dependency *p;
> > +
> > +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> > +             if (!(p->flags & I915_DEPENDENCY_WEAK))
> > +                     return false;
> > +
> > +     return true;
> > +}
> > +
> >   static void eb_request_add(struct i915_execbuffer *eb)
> >   {
> >       struct i915_request *rq = eb->request;
> > @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
> >                * Allow interactive/synchronous clients to jump ahead of
> >                * the bulk clients. (FQ_CODEL)
> >                */
> > -             if (list_empty(&rq->sched.signalers_list))
> > +             if (new_client(rq))
> >                       attr.priority |= I915_PRIORITY_WAIT;
> >       } else {
> >               /* Serialise with context_close via the add_to_timeline */
> > 
> 
> Did absence of this have any functional effect? I hope not, but anyway:

Bah, I have a new test case where this WAIT bumping is still upsetting us.

I don't think I have any choice but to rip it out if we have timeslicing
enabled.

Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
under if (!intel_engine_has_timeslicing(rq->engine)) ?
-Chris
Tvrtko Ursulin May 7, 2020, 3:10 p.m. UTC | #3 
On 07/05/2020 16:05, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
>>
>> On 07/05/2020 09:21, Chris Wilson wrote:
>>> The submit-fence adds a weak dependency to the requests, and for the
>>> purpose of our FQ_CODEL hinting we do not want to treat as a
>>> restriction. This is primarily because clients may treat submit-fences
>>> as a bidirectional bonding between a pair of co-ordinating requests.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> index 966523a8503f..e8bf0cf02fd7 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>>> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
>>>                        break;
>>>    }
>>>    
>>> +static bool new_client(struct i915_request *rq)
>>> +{
>>> +     struct i915_dependency *p;
>>> +
>>> +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
>>> +             if (!(p->flags & I915_DEPENDENCY_WEAK))
>>> +                     return false;
>>> +
>>> +     return true;
>>> +}
>>> +
>>>    static void eb_request_add(struct i915_execbuffer *eb)
>>>    {
>>>        struct i915_request *rq = eb->request;
>>> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
>>>                 * Allow interactive/synchronous clients to jump ahead of
>>>                 * the bulk clients. (FQ_CODEL)
>>>                 */
>>> -             if (list_empty(&rq->sched.signalers_list))
>>> +             if (new_client(rq))
>>>                        attr.priority |= I915_PRIORITY_WAIT;
>>>        } else {
>>>                /* Serialise with context_close via the add_to_timeline */
>>>
>>
>> Did absence of this have any functional effect? I hope not, but anyway:
> 
> Bah, I have a new test case where this WAIT bumping is still upsetting us.
> 
> I don't think I have any choice but to rip it out if we have timeslicing
> enabled.
> 
> Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
> under if (!intel_engine_has_timeslicing(rq->engine)) ?

Doesn't feel worthwhile to keep it for just BDW right?

Regards,

Tvrtko
Chris Wilson May 7, 2020, 3:17 p.m. UTC | #4 
Quoting Tvrtko Ursulin (2020-05-07 16:10:37)
> 
> On 07/05/2020 16:05, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-05-07 15:59:56)
> >>
> >> On 07/05/2020 09:21, Chris Wilson wrote:
> >>> The submit-fence adds a weak dependency to the requests, and for the
> >>> purpose of our FQ_CODEL hinting we do not want to treat as a
> >>> restriction. This is primarily because clients may treat submit-fences
> >>> as a bidirectional bonding between a pair of co-ordinating requests.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ++++++++++++-
> >>>    1 file changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> index 966523a8503f..e8bf0cf02fd7 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >>> @@ -2565,6 +2565,17 @@ static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
> >>>                        break;
> >>>    }
> >>>    
> >>> +static bool new_client(struct i915_request *rq)
> >>> +{
> >>> +     struct i915_dependency *p;
> >>> +
> >>> +     list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
> >>> +             if (!(p->flags & I915_DEPENDENCY_WEAK))
> >>> +                     return false;
> >>> +
> >>> +     return true;
> >>> +}
> >>> +
> >>>    static void eb_request_add(struct i915_execbuffer *eb)
> >>>    {
> >>>        struct i915_request *rq = eb->request;
> >>> @@ -2604,7 +2615,7 @@ static void eb_request_add(struct i915_execbuffer *eb)
> >>>                 * Allow interactive/synchronous clients to jump ahead of
> >>>                 * the bulk clients. (FQ_CODEL)
> >>>                 */
> >>> -             if (list_empty(&rq->sched.signalers_list))
> >>> +             if (new_client(rq))
> >>>                        attr.priority |= I915_PRIORITY_WAIT;
> >>>        } else {
> >>>                /* Serialise with context_close via the add_to_timeline */
> >>>
> >>
> >> Did absence of this have any functional effect? I hope not, but anyway:
> > 
> > Bah, I have a new test case where this WAIT bumping is still upsetting us.
> > 
> > I don't think I have any choice but to rip it out if we have timeslicing
> > enabled.
> > 
> > Would you prefer a complete remission of I915_PRIORITY_WAIT or keep it
> > under if (!intel_engine_has_timeslicing(rq->engine)) ?
> 
> Doesn't feel worthwhile to keep it for just BDW right?

No. There's ivb waiting in the rings (as I haven't figured out
preemption for it yet), but similarly just doesn't feel worth the
complications. :(
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 966523a8503f..e8bf0cf02fd7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2565,6 +2565,17 @@  static void retire_requests(struct intel_timeline *tl, struct i915_request *end)
 			break;
 }
 
+static bool new_client(struct i915_request *rq)
+{
+	struct i915_dependency *p;
+
+	list_for_each_entry(p, &rq->sched.signalers_list, signal_link)
+		if (!(p->flags & I915_DEPENDENCY_WEAK))
+			return false;
+
+	return true;
+}
+
 static void eb_request_add(struct i915_execbuffer *eb)
 {
 	struct i915_request *rq = eb->request;
@@ -2604,7 +2615,7 @@  static void eb_request_add(struct i915_execbuffer *eb)
 		 * Allow interactive/synchronous clients to jump ahead of
 		 * the bulk clients. (FQ_CODEL)
 		 */
-		if (list_empty(&rq->sched.signalers_list))
+		if (new_client(rq))
 			attr.priority |= I915_PRIORITY_WAIT;
 	} else {
 		/* Serialise with context_close via the add_to_timeline */