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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx