[Intel-gfx] [PATCH 1/6] drm/i915: Allow late allocation of request for i915_add_request()

[Daniel Vetter]

On Thu, Jul 12, 2012 at 04:13:34PM +0100, Chris Wilson wrote:
> Request preallocation was added to i915_add_request() in order to
> support the overlay. However, not all uses care and can quite happily
> ignore the failure to allocate the request as they will simply repeat
> the request in the future.
> 
> By pushing the allocation down into i915_add_request(), we can then
> remove some rather ugly error handling in the callers.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |    6 ++---
>  drivers/gpu/drm/i915/i915_gem.c            |   39 ++++++++++------------------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    7 +----
>  3 files changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 627fe35..51e234e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1358,9 +1358,9 @@ void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
>  int __must_check i915_gem_idle(struct drm_device *dev);
> -int __must_check i915_add_request(struct intel_ring_buffer *ring,
> -				  struct drm_file *file,
> -				  struct drm_i915_gem_request *request);
> +int i915_add_request(struct intel_ring_buffer *ring,
> +		     struct drm_file *file,
> +		     struct drm_i915_gem_request *request);
>  int __must_check i915_wait_seqno(struct intel_ring_buffer *ring,
>  				 uint32_t seqno);
>  int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 53c3946..875b745 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1597,7 +1597,6 @@ i915_add_request(struct intel_ring_buffer *ring,
>  		ring->gpu_caches_dirty = false;
>  	}
>  
> -	BUG_ON(request == NULL);
>  	seqno = i915_gem_next_request_seqno(ring);
>  
>  	/* Record the position of the start of the request so that
> @@ -1609,7 +1608,13 @@ i915_add_request(struct intel_ring_buffer *ring,
>  
>  	ret = ring->add_request(ring, &seqno);
>  	if (ret)
> -	    return ret;
> +		return ret;
> +
> +	if (request == NULL) {
> +		request = kmalloc(sizeof(*request), GFP_KERNEL);
> +		if (request == NULL)
> +			return -ENOMEM;
> +	}

Hm, shouldn't we try to allocate the request struct before we emit the
request? Otherwise looks good.
-Daniel
>  
>  	trace_i915_gem_request_add(ring, seqno);
>  
> @@ -1859,14 +1864,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	 */
>  	idle = true;
>  	for_each_ring(ring, dev_priv, i) {
> -		if (ring->gpu_caches_dirty) {
> -			struct drm_i915_gem_request *request;
> -
> -			request = kzalloc(sizeof(*request), GFP_KERNEL);
> -			if (request == NULL ||
> -			    i915_add_request(ring, NULL, request))
> -			    kfree(request);
> -		}
> +		if (ring->gpu_caches_dirty)
> +			i915_add_request(ring, NULL, NULL);
>  
>  		idle &= list_empty(&ring->request_list);
>  	}
> @@ -1913,25 +1912,13 @@ i915_gem_check_wedge(struct drm_i915_private *dev_priv,
>  static int
>  i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
>  
> -	if (seqno == ring->outstanding_lazy_request) {
> -		struct drm_i915_gem_request *request;
> -
> -		request = kzalloc(sizeof(*request), GFP_KERNEL);
> -		if (request == NULL)
> -			return -ENOMEM;
> -
> -		ret = i915_add_request(ring, NULL, request);
> -		if (ret) {
> -			kfree(request);
> -			return ret;
> -		}
> -
> -		BUG_ON(seqno != request->seqno);
> -	}
> +	ret = 0;
> +	if (seqno == ring->outstanding_lazy_request)
> +		ret = i915_add_request(ring, NULL, NULL);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 88e2e11..0c26692 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -967,16 +967,11 @@ i915_gem_execbuffer_retire_commands(struct drm_device *dev,
>  				    struct drm_file *file,
>  				    struct intel_ring_buffer *ring)
>  {
> -	struct drm_i915_gem_request *request;
> -
>  	/* Unconditionally force add_request to emit a full flush. */
>  	ring->gpu_caches_dirty = true;
>  
>  	/* Add a breadcrumb for the completion of the batch buffer */
> -	request = kzalloc(sizeof(*request), GFP_KERNEL);
> -	if (request == NULL || i915_add_request(ring, file, request)) {
> -		kfree(request);
> -	}
> +	(void)i915_add_request(ring, file, NULL);
>  }
>  
>  static int
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48