[Intel-gfx] [PATCH v7 6/7] drm/i915: refactor duplicate object vmap functions (the final rework?)

[Chris Wilson]

On Tue, Mar 01, 2016 at 04:33:58PM +0000, Dave Gordon wrote:
> This is essentially Chris Wilson's patch of a similar name, reworked on
> top of Alex Dai's recent patch:
> | drm/i915: Add i915_gem_object_vmap to map GEM object to virtual space
> 
> Chris' original commentary said:
> | We now have two implementations for vmapping a whole object, one for
> | dma-buf and one for the ringbuffer. If we couple the vmapping into
> | the obj->pages lifetime, then we can reuse an obj->vmapping for both
> | and at the same time couple it into the shrinker.
> |
> | v2: Mark the failable kmalloc() as __GFP_NOWARN (vsyrjala)
> | v3: Call unpin_vmap from the right dmabuf unmapper
> 
> v4: reimplements the same functionality, but now as wrappers round the
>     recently-introduced i915_gem_object_vmap_range() from Alex's patch
>     mentioned above.
> 
> v5: separated from two minor but unrelated changes [Tvrtko Ursulin];
>     this is the third and most substantial portion.
> 
>     Decided not to hold onto vmappings after the pin count goes to zero.
>     This may reduce the benefit of Chris' scheme a bit, but does avoid
>     any increased risk of exhausting kernel vmap space on 32-bit kernels
>     [Tvrtko Ursulin]. Potentially, the vunmap() could be deferred until
>     the put_pages() stage if a suitable notifier were written, but we're
>     not doing that here. Nonetheless, the simplification of both dmabuf
>     and ringbuffer code makes it worthwhile in its own right.
> 
> v6: change BUG_ON() to WARN_ON(). [Tvrtko Ursulin]
> 
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Alex Dai <yu.dai at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 22 ++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem.c         | 39 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 36 ++++--------------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  9 ++++----
>  4 files changed, 65 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ae191..f1ad3b3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2172,10 +2172,7 @@ struct drm_i915_gem_object {
>  		struct scatterlist *sg;
>  		int last;
>  	} get_page;
> -
> -	/* prime dma-buf support */
> -	void *dma_buf_vmapping;
> -	int vmapping_count;
> +	void *vmapping;
>  
>  	/** Breadcrumb of last rendering to the buffer.
>  	 * There can only be one writer, but we allow for multiple readers.
> @@ -2980,7 +2977,22 @@ static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
>  static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>  {
>  	BUG_ON(obj->pages_pin_count == 0);
> -	obj->pages_pin_count--;
> +	if (--obj->pages_pin_count == 0 && obj->vmapping) {
> +		/*
> +		 * Releasing the vmapping here may yield less benefit than
> +		 * if we kept it until put_pages(), but on the other hand

Yields no benefit. Makes the patch pointless.
Plus there is also pressure to enable WC vmaps.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre