Re: [PATCH v5 7/7] drm/i915: refactor duplicate object vmap functions (reworked some more)

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 29/02/16 11:13, 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.
>
> With this change, we have only one vmap() in the whole driver :)

The above line does not apply to this patch after the split any more. :)

>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Dai <yu.dai@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         | 22 +++++++++++++++-----
>   drivers/gpu/drm/i915/i915_gem.c         | 36 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 37 ++++-----------------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++-----
>   4 files changed, 62 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 918b0bb..7facdb5 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
> +		 * avoids issues of exhausting kernel vmappable address
> +		 * space on 32-bit kernels.
> +		 */
> +		vunmap(obj->vmapping);
> +		obj->vmapping = NULL;
> +	}
> +}
> +
> +void *__must_check i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj);
> +static inline void i915_gem_object_unpin_vmap(struct drm_i915_gem_object *obj)
> +{
> +	i915_gem_object_unpin_pages(obj);
>   }
>
>   void *__must_check i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c126211..79d3d5b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2235,6 +2235,8 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	ops->put_pages(obj);
>   	obj->pages = NULL;
>
> +	BUG_ON(obj->vmapping);
> +

Do we know for certain kernel will crash and not just maybe leak a 
vmapping? I think not so would be better to just WARN.

Maybe even WARN_ON and return -EBUSY higher up, as the function already 
does for obj->pages_pin_count?

>   	i915_gem_object_invalidate(obj);
>
>   	return 0;
> @@ -2452,6 +2454,40 @@ void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
>   	return addr;
>   }
>
> +/**
> + * i915_gem_object_pin_vmap - pin a GEM object and map it into kernel space
> + * @obj: the GEM object to be mapped
> + *
> + * Combines the functions of get_pages(), pin_pages() and vmap_range() on
> + * the whole object.  The caller should release the mapping by calling
> + * i915_gem_object_unpin_vmap() when it is no longer required.
> + *
> + * Returns the address at which the object has been mapped, or an ERR_PTR
> + * on failure.
> + */
> +void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
> +{
> +	int ret;
> +
> +	ret = i915_gem_object_get_pages(obj);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	i915_gem_object_pin_pages(obj);
> +
> +	if (obj->vmapping == NULL) {
> +		obj->vmapping = i915_gem_object_vmap_range(obj, 0,
> +					obj->base.size >> PAGE_SHIFT);
> +
> +		if (obj->vmapping == NULL) {
> +			i915_gem_object_unpin_pages(obj);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return obj->vmapping;
> +}
> +
>   void i915_vma_move_to_active(struct i915_vma *vma,
>   			     struct drm_i915_gem_request *req)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 68e21d1..adc7b5e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -108,41 +108,17 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>   {
>   	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>   	struct drm_device *dev = obj->base.dev;
> +	void *addr;
>   	int ret;
>
>   	ret = i915_mutex_lock_interruptible(dev);
>   	if (ret)
>   		return ERR_PTR(ret);
>
> -	if (obj->dma_buf_vmapping) {
> -		obj->vmapping_count++;
> -		goto out_unlock;
> -	}
> -
> -	ret = i915_gem_object_get_pages(obj);
> -	if (ret)
> -		goto err;
> -
> -	i915_gem_object_pin_pages(obj);
> -
> -	ret = -ENOMEM;
> -
> -	obj->dma_buf_vmapping = i915_gem_object_vmap_range(obj, 0,
> -						dma_buf->size >> PAGE_SHIFT);
> -
> -	if (!obj->dma_buf_vmapping)
> -		goto err_unpin;
> -
> -	obj->vmapping_count = 1;
> -out_unlock:
> +	addr = i915_gem_object_pin_vmap(obj);
>   	mutex_unlock(&dev->struct_mutex);
> -	return obj->dma_buf_vmapping;
>
> -err_unpin:
> -	i915_gem_object_unpin_pages(obj);
> -err:
> -	mutex_unlock(&dev->struct_mutex);
> -	return ERR_PTR(ret);
> +	return addr;
>   }
>
>   static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> @@ -151,12 +127,7 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>   	struct drm_device *dev = obj->base.dev;
>
>   	mutex_lock(&dev->struct_mutex);
> -	if (--obj->vmapping_count == 0) {
> -		vunmap(obj->dma_buf_vmapping);
> -		obj->dma_buf_vmapping = NULL;
> -
> -		i915_gem_object_unpin_pages(obj);
> -	}
> +	i915_gem_object_unpin_vmap(obj);
>   	mutex_unlock(&dev->struct_mutex);
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 15e2d29..47f186e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2056,7 +2056,7 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
>   void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>   {
>   	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
> -		vunmap(ringbuf->virtual_start);
> +		i915_gem_object_unpin_vmap(ringbuf->obj);
>   	else
>   		iounmap(ringbuf->virtual_start);
>   	ringbuf->virtual_start = NULL;
> @@ -2080,10 +2080,10 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   		if (ret)
>   			goto unpin;
>
> -		ringbuf->virtual_start = i915_gem_object_vmap_range(obj, 0,
> -						ringbuf->size >> PAGE_SHIFT);
> -		if (ringbuf->virtual_start == NULL) {
> -			ret = -ENOMEM;
> +		ringbuf->virtual_start = i915_gem_object_pin_vmap(obj);
> +		if (IS_ERR(ringbuf->virtual_start)) {
> +			ret = PTR_ERR(ringbuf->virtual_start);
> +			ringbuf->virtual_start = NULL;
>   			goto unpin;
>   		}
>   	} else {
>

Looks OK to me. Even the BUG_ON is not critical since it can't happen 
when using the API although I don't like to see them. Either way:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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