[7/9] drm/i915: add fences to the request struct

Commit Message

David Woodhouse Oct. 9, 2015, 1:29 p.m. UTC

On Fri, 2015-09-04 at 09:59 -0700, Jesse Barnes wrote:
> 
> @@ -2286,6 +2287,10 @@ struct drm_i915_gem_request {
>         /** Execlists no. of times this request has been sent to the ELSP */
>         int elsp_submitted;
>  
> +       /* core fence obj for this request, may be exported */
> +       struct fence fence;

As discussed, this doesn't work as-is. The final fence_put() will
attempt to free(&req->fence). Unless you have a .release method in your
fence ops, which you don't.

I suppose we could tie up a .release method with the existing release
method for the drm_i915_gem_request.

As things stand, though, bad things are happening. This makes it go
away and at least lets me get on with testing.

Comments

Jesse Barnes Oct. 9, 2015, 4:11 p.m. UTC | #1

On 10/09/2015 06:29 AM, David Woodhouse wrote:
> On Fri, 2015-09-04 at 09:59 -0700, Jesse Barnes wrote:
>>
>> @@ -2286,6 +2287,10 @@ struct drm_i915_gem_request {
>>         /** Execlists no. of times this request has been sent to the ELSP */
>>         int elsp_submitted;
>>  
>> +       /* core fence obj for this request, may be exported */
>> +       struct fence fence;
> 
> As discussed, this doesn't work as-is. The final fence_put() will
> attempt to free(&req->fence). Unless you have a .release method in your
> fence ops, which you don't.
> 
> I suppose we could tie up a .release method with the existing release
> method for the drm_i915_gem_request.
> 
> As things stand, though, bad things are happening. This makes it go
> away and at least lets me get on with testing.
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8ef19e2..2d0c93c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2297,7 +2298,7 @@ struct drm_i915_gem_request {
>  	int elsp_submitted;
>  
>  	/* core fence obj for this request, may be exported */
> -	struct fence fence;
> +	struct fence *fence;
>  
>  	wait_queue_t wait;
>  };
> diff --git a/drivers/gpu/drm/i915/i915_sync.c b/drivers/gpu/drm/i915/i915_sync.c
> index 085f1f9..6ffe273 100644
> --- a/drivers/gpu/drm/i915/i915_sync.c
> +++ b/drivers/gpu/drm/i915/i915_sync.c
> @@ -58,7 +58,12 @@ struct i915_sync_timeline {
>   *   allow non-RCS fences (need ring/context association)
>   */
>  
> -#define to_i915_request(x) container_of(x, struct drm_i915_gem_request, fence)
> +struct foo {
> +	struct fence fence;
> +	struct drm_i915_gem_request *req;
> +};
> +
> +#define to_i915_request(x) (((struct foo *)(x))->req)
>  
>  static const char *i915_fence_get_driver_name(struct fence *fence)
>  {
> @@ -81,10 +86,10 @@ static int i915_fence_ring_check(wait_queue_t *wait, unsigned mode, int flags,
>  	if (!i915_gem_request_completed(req, false))
>  		return 0;
>  
> -	fence_signal_locked(&req->fence);
> +	fence_signal_locked(req->fence);
>  
>  	__remove_wait_queue(&ring->irq_queue, wait);
> -	fence_put(&req->fence);
> +	fence_put(req->fence);
>  	ring->irq_put(ring);
>  
>  	return 0;
> @@ -200,6 +205,15 @@ struct fence *i915_fence_create_ring(struct intel_engine_cs *ring,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	request->fence = kmalloc(sizeof(struct foo), GFP_KERNEL);
> +	if (!request->fence) {
> +		ret = -ENOMEM;
> +		goto err_cancel;
> +	}
> +	/* I have no clue how this is *supposed* to work and no real interest
> +	   in finding out. Just stop hurting me please. */
> +	((struct foo *)request->fence)->req = request;
> +
>  	if (i915.enable_execlists) {
>  		ringbuf = ctx->engine[ring->id].ringbuf;
>  	} else
> @@ -270,10 +284,10 @@ struct fence *i915_fence_create_ring(struct intel_engine_cs *ring,
>  			   round_jiffies_up_relative(HZ));
>  	intel_mark_busy(dev_priv->dev);
>  
> -	fence_init(&request->fence, &i915_fence_ring_ops, &fence_lock,
> +	fence_init(request->fence, &i915_fence_ring_ops, &fence_lock,
>  		   ctx->user_handle, request->seqno);
>  
> -	return &request->fence;
> +	return request->fence;
>  
>  err_cancel:
>  	i915_gem_request_cancel(request);
> @@ -306,10 +320,10 @@ static struct fence *i915_fence_create_display(struct intel_context *ctx)
>  
>  	req = ring->outstanding_lazy_request;
>  
> -	fence_init(&req->fence, &i915_fence_ops, &fence_lock,
> +	fence_init(req->fence, &i915_fence_ops, &fence_lock,
>  		   ctx->user_handle, req->seqno);
>  
> -	return &req->fence;
> +	return req->fence;
>  }
>  #endif

Yeah this is definitely better than what I had (untested code and all
that).  But the actual signaling and such still needs work.  I had a
question for Maarten on that actually; today it doesn't look like the
fence would enabling signaling at the right point, so I had to add
something.  But I'll look and see what the latest is here from John H; I
know his Android code worked, so it would probably be best to just use that.

Jesse

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8ef19e2..2d0c93c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2297,7 +2298,7 @@  struct drm_i915_gem_request {
 	int elsp_submitted;
 
 	/* core fence obj for this request, may be exported */
-	struct fence fence;
+	struct fence *fence;
 
 	wait_queue_t wait;
 };
diff --git a/drivers/gpu/drm/i915/i915_sync.c b/drivers/gpu/drm/i915/i915_sync.c
index 085f1f9..6ffe273 100644
--- a/drivers/gpu/drm/i915/i915_sync.c
+++ b/drivers/gpu/drm/i915/i915_sync.c
@@ -58,7 +58,12 @@  struct i915_sync_timeline {
  *   allow non-RCS fences (need ring/context association)
  */
 
-#define to_i915_request(x) container_of(x, struct drm_i915_gem_request, fence)
+struct foo {
+	struct fence fence;
+	struct drm_i915_gem_request *req;
+};
+
+#define to_i915_request(x) (((struct foo *)(x))->req)
 
 static const char *i915_fence_get_driver_name(struct fence *fence)
 {
@@ -81,10 +86,10 @@  static int i915_fence_ring_check(wait_queue_t *wait, unsigned mode, int flags,
 	if (!i915_gem_request_completed(req, false))
 		return 0;
 
-	fence_signal_locked(&req->fence);
+	fence_signal_locked(req->fence);
 
 	__remove_wait_queue(&ring->irq_queue, wait);
-	fence_put(&req->fence);
+	fence_put(req->fence);
 	ring->irq_put(ring);
 
 	return 0;
@@ -200,6 +205,15 @@  struct fence *i915_fence_create_ring(struct intel_engine_cs *ring,
 	if (ret)
 		return ERR_PTR(ret);
 
+	request->fence = kmalloc(sizeof(struct foo), GFP_KERNEL);
+	if (!request->fence) {
+		ret = -ENOMEM;
+		goto err_cancel;
+	}
+	/* I have no clue how this is *supposed* to work and no real interest
+	   in finding out. Just stop hurting me please. */
+	((struct foo *)request->fence)->req = request;
+
 	if (i915.enable_execlists) {
 		ringbuf = ctx->engine[ring->id].ringbuf;
 	} else
@@ -270,10 +284,10 @@  struct fence *i915_fence_create_ring(struct intel_engine_cs *ring,
 			   round_jiffies_up_relative(HZ));
 	intel_mark_busy(dev_priv->dev);
 
-	fence_init(&request->fence, &i915_fence_ring_ops, &fence_lock,
+	fence_init(request->fence, &i915_fence_ring_ops, &fence_lock,
 		   ctx->user_handle, request->seqno);
 
-	return &request->fence;
+	return request->fence;
 
 err_cancel:
 	i915_gem_request_cancel(request);
@@ -306,10 +320,10 @@  static struct fence *i915_fence_create_display(struct intel_context *ctx)
 
 	req = ring->outstanding_lazy_request;
 
-	fence_init(&req->fence, &i915_fence_ops, &fence_lock,
+	fence_init(req->fence, &i915_fence_ops, &fence_lock,
 		   ctx->user_handle, req->seqno);
 
-	return &req->fence;
+	return req->fence;
 }
 #endif