[2/9] drm/i915: Pull waiting on an external dma-fence into its routine
diff mbox series

Message ID 20200508092933.738-2-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
As a means for a small code consolidation, but primarily to start
thinking more carefully about internal-vs-external linkage, pull the
pair of i915_sw_fence_await_dma_fence() calls into a common routine.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

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

> As a means for a small code consolidation, but primarily to start
> thinking more carefully about internal-vs-external linkage, pull the
> pair of i915_sw_fence_await_dma_fence() calls into a common routine.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index be2ce9065a29..94189c7d43cd 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1067,6 +1067,14 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
>  	return 0;
>  }
>  
> +static int
> +i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
> +{
> +	return i915_sw_fence_await_dma_fence(&rq->submit, fence,
> +					     fence->context ? I915_FENCE_TIMEOUT : 0,
> +					     I915_FENCE_GFP);
> +}
> +
>  int
>  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>  {
> @@ -1114,9 +1122,7 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>  		if (dma_fence_is_i915(fence))
>  			ret = i915_request_await_request(rq, to_request(fence));
>  		else
> -			ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
> -							    fence->context ? I915_FENCE_TIMEOUT : 0,
> -							    I915_FENCE_GFP);
> +			ret = i915_request_await_external(rq, fence);

For us (rq, rq), for external (rq, fence).

It looks neat for a reader. But then, how can external fence have
a context?

-Mika

>  		if (ret < 0)
>  			return ret;
>  
> @@ -1255,9 +1261,7 @@ i915_request_await_execution(struct i915_request *rq,
>  							     to_request(fence),
>  							     hook);
>  		else
> -			ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
> -							    I915_FENCE_TIMEOUT,
> -							    GFP_KERNEL);
> +			ret = i915_request_await_external(rq, fence);
>  		if (ret < 0)
>  			return ret;
>  	} while (--nchild);
> -- 
> 2.20.1
Chris Wilson May 8, 2020, 10:26 a.m. UTC | #2
Quoting Mika Kuoppala (2020-05-08 11:19:25)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > As a means for a small code consolidation, but primarily to start
> > thinking more carefully about internal-vs-external linkage, pull the
> > pair of i915_sw_fence_await_dma_fence() calls into a common routine.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index be2ce9065a29..94189c7d43cd 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1067,6 +1067,14 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
> >       return 0;
> >  }
> >  
> > +static int
> > +i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
> > +{
> > +     return i915_sw_fence_await_dma_fence(&rq->submit, fence,
> > +                                          fence->context ? I915_FENCE_TIMEOUT : 0,
> > +                                          I915_FENCE_GFP);
> > +}
> > +
> >  int
> >  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> >  {
> > @@ -1114,9 +1122,7 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> >               if (dma_fence_is_i915(fence))
> >                       ret = i915_request_await_request(rq, to_request(fence));
> >               else
> > -                     ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
> > -                                                         fence->context ? I915_FENCE_TIMEOUT : 0,
> > -                                                         I915_FENCE_GFP);
> > +                     ret = i915_request_await_external(rq, fence);
> 
> For us (rq, rq), for external (rq, fence).
> 
> It looks neat for a reader. But then, how can external fence have
> a context?

The 'context' here is the dma_fence timeline context. All dma_fences
have a context:seqno tuple which denotes their timeline and position
along the timeline.
-Chris
Chris Wilson May 8, 2020, 10:27 a.m. UTC | #3
Quoting Mika Kuoppala (2020-05-08 11:19:25)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > As a means for a small code consolidation, but primarily to start
> > thinking more carefully about internal-vs-external linkage, pull the
> > pair of i915_sw_fence_await_dma_fence() calls into a common routine.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index be2ce9065a29..94189c7d43cd 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1067,6 +1067,14 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
> >       return 0;
> >  }
> >  
> > +static int
> > +i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
> > +{
> > +     return i915_sw_fence_await_dma_fence(&rq->submit, fence,
> > +                                          fence->context ? I915_FENCE_TIMEOUT : 0,
> > +                                          I915_FENCE_GFP);
> > +}
> > +
> >  int
> >  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> >  {
> > @@ -1114,9 +1122,7 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
> >               if (dma_fence_is_i915(fence))
> >                       ret = i915_request_await_request(rq, to_request(fence));
> >               else
> > -                     ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
> > -                                                         fence->context ? I915_FENCE_TIMEOUT : 0,
> > -                                                         I915_FENCE_GFP);
> > +                     ret = i915_request_await_external(rq, fence);
> 
> For us (rq, rq), for external (rq, fence).
> 
> It looks neat for a reader. But then, how can external fence have
> a context?

How about s/fence/dma/?
-Chris
Mika Kuoppala May 8, 2020, 11:08 a.m. UTC | #4
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2020-05-08 11:19:25)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > As a means for a small code consolidation, but primarily to start
>> > thinking more carefully about internal-vs-external linkage, pull the
>> > pair of i915_sw_fence_await_dma_fence() calls into a common routine.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++------
>> >  1 file changed, 10 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> > index be2ce9065a29..94189c7d43cd 100644
>> > --- a/drivers/gpu/drm/i915/i915_request.c
>> > +++ b/drivers/gpu/drm/i915/i915_request.c
>> > @@ -1067,6 +1067,14 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
>> >       return 0;
>> >  }
>> >  
>> > +static int
>> > +i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
>> > +{
>> > +     return i915_sw_fence_await_dma_fence(&rq->submit, fence,
>> > +                                          fence->context ? I915_FENCE_TIMEOUT : 0,
>> > +                                          I915_FENCE_GFP);
>> > +}
>> > +
>> >  int
>> >  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>> >  {
>> > @@ -1114,9 +1122,7 @@ i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
>> >               if (dma_fence_is_i915(fence))
>> >                       ret = i915_request_await_request(rq, to_request(fence));
>> >               else
>> > -                     ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
>> > -                                                         fence->context ? I915_FENCE_TIMEOUT : 0,
>> > -                                                         I915_FENCE_GFP);
>> > +                     ret = i915_request_await_external(rq, fence);
>> 
>> For us (rq, rq), for external (rq, fence).
>> 
>> It looks neat for a reader. But then, how can external fence have
>> a context?
>
> How about s/fence/dma/?

It is fine like it is. I was just so confused that we piggyback our
context along the fence. But yeah this is the fence->context and zero
is a special. (thanks for explaining in irc)

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 be2ce9065a29..94189c7d43cd 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1067,6 +1067,14 @@  i915_request_await_request(struct i915_request *to, struct i915_request *from)
 	return 0;
 }
 
+static int
+i915_request_await_external(struct i915_request *rq, struct dma_fence *fence)
+{
+	return i915_sw_fence_await_dma_fence(&rq->submit, fence,
+					     fence->context ? I915_FENCE_TIMEOUT : 0,
+					     I915_FENCE_GFP);
+}
+
 int
 i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
 {
@@ -1114,9 +1122,7 @@  i915_request_await_dma_fence(struct i915_request *rq, struct dma_fence *fence)
 		if (dma_fence_is_i915(fence))
 			ret = i915_request_await_request(rq, to_request(fence));
 		else
-			ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
-							    fence->context ? I915_FENCE_TIMEOUT : 0,
-							    I915_FENCE_GFP);
+			ret = i915_request_await_external(rq, fence);
 		if (ret < 0)
 			return ret;
 
@@ -1255,9 +1261,7 @@  i915_request_await_execution(struct i915_request *rq,
 							     to_request(fence),
 							     hook);
 		else
-			ret = i915_sw_fence_await_dma_fence(&rq->submit, fence,
-							    I915_FENCE_TIMEOUT,
-							    GFP_KERNEL);
+			ret = i915_request_await_external(rq, fence);
 		if (ret < 0)
 			return ret;
 	} while (--nchild);