[1/9] drm/i915: Ignore submit-fences on the same timeline
diff mbox series

Message ID 20200508092933.738-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [1/9] drm/i915: Ignore submit-fences on the same timeline
Related show

Commit Message

Chris Wilson May 8, 2020, 9:29 a.m. UTC
While we ordinarily do not skip submit-fences due to the accompanying
hook that we want to callback on execution, a submit-fence on the same
timeline is meaningless.

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

Comments

Mika Kuoppala May 8, 2020, 9:57 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> While we ordinarily do not skip submit-fences due to the accompanying
> hook that we want to callback on execution, a submit-fence on the same
> timeline is meaningless.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_request.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 589739bfee25..be2ce9065a29 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1242,6 +1242,9 @@ i915_request_await_execution(struct i915_request *rq,
>  			continue;
>  		}
>  
> +		if (fence->context == rq->fence.context)
> +			continue;
> +
>  		/*
>  		 * We don't squash repeated fence dependencies here as we
>  		 * want to run our callback in all cases.

The comment in here makes me nervous. Is this skipping on same context
other than squashing?

-Mika

> -- 
> 2.20.1
Chris Wilson May 8, 2020, 10:09 a.m. UTC | #2
Quoting Mika Kuoppala (2020-05-08 10:57:37)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > While we ordinarily do not skip submit-fences due to the accompanying
> > hook that we want to callback on execution, a submit-fence on the same
> > timeline is meaningless.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 589739bfee25..be2ce9065a29 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1242,6 +1242,9 @@ i915_request_await_execution(struct i915_request *rq,
> >                       continue;
> >               }
> >  
> > +             if (fence->context == rq->fence.context)
> > +                     continue;
> > +
> >               /*
> >                * We don't squash repeated fence dependencies here as we
> >                * want to run our callback in all cases.
> 
> The comment in here makes me nervous. Is this skipping on same context
> other than squashing?

The hooks we have only apply between timelines, so skipping isn't an
issue. Suppressing the wait ensures that

syncobj-future-submit-past:
	I915_EXEC_FENCE_WAIT |
	I915_EXEC_FENCE_WAIT_SUBMIT |
	I915_EXEC_FENCE_SIGNAL

is a no-op. That is if you declare that request should wait for itself
to be submitted before it is submitted, we correctly conclude that is
degenerate and a no-op. We can generalise that to realise that waiting for
any fence along the same timeline to be submitted before we are
submitted is guaranteed by the timeline itself, and so all are no-ops.
-Chris
Mika Kuoppala May 8, 2020, 11:11 a.m. UTC | #3
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2020-05-08 10:57:37)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > While we ordinarily do not skip submit-fences due to the accompanying
>> > hook that we want to callback on execution, a submit-fence on the same
>> > timeline is meaningless.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_request.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> > index 589739bfee25..be2ce9065a29 100644
>> > --- a/drivers/gpu/drm/i915/i915_request.c
>> > +++ b/drivers/gpu/drm/i915/i915_request.c
>> > @@ -1242,6 +1242,9 @@ i915_request_await_execution(struct i915_request *rq,
>> >                       continue;
>> >               }
>> >  
>> > +             if (fence->context == rq->fence.context)
>> > +                     continue;
>> > +
>> >               /*
>> >                * We don't squash repeated fence dependencies here as we
>> >                * want to run our callback in all cases.
>> 
>> The comment in here makes me nervous. Is this skipping on same context
>> other than squashing?
>
> The hooks we have only apply between timelines, so skipping isn't an
> issue. Suppressing the wait ensures that
>
> syncobj-future-submit-past:
> 	I915_EXEC_FENCE_WAIT |
> 	I915_EXEC_FENCE_WAIT_SUBMIT |
> 	I915_EXEC_FENCE_SIGNAL
>
> is a no-op. That is if you declare that request should wait for itself
> to be submitted before it is submitted, we correctly conclude that is
> degenerate and a no-op. We can generalise that to realise that waiting for
> any fence along the same timeline to be submitted before we are
> submitted is guaranteed by the timeline itself, and so all are no-ops.

The last sentence nails it.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 589739bfee25..be2ce9065a29 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1242,6 +1242,9 @@  i915_request_await_execution(struct i915_request *rq,
 			continue;
 		}
 
+		if (fence->context == rq->fence.context)
+			continue;
+
 		/*
 		 * We don't squash repeated fence dependencies here as we
 		 * want to run our callback in all cases.