[Intel-gfx] [PATCH 1/2] drm/i915: Add i915_gem_object_vmap to map GEM object to virtual space
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Feb 18 10:49:22 UTC 2016
Hi,
On 17/02/16 23:18, yu.dai at intel.com wrote:
> From: Alex Dai <yu.dai at intel.com>
>
> There are several places inside driver where a GEM object is mapped to
> kernel virtual space. The mapping is either done for the whole object
> or certain page range of it.
>
> This patch introduces a function i915_gem_object_vmap to do such job.
>
> Signed-off-by: Alex Dai <yu.dai at intel.com>
> Cc: Dave Gordon <david.s.gordon at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 28 +--------------------
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/gpu/drm/i915/i915_gem.c | 44 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 15 ++---------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++----------------
> 5 files changed, 52 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 814d894..915e8c1 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -863,37 +863,11 @@ find_reg(const struct drm_i915_reg_descriptor *table,
> static u32 *vmap_batch(struct drm_i915_gem_object *obj,
> unsigned start, unsigned len)
> {
> - int i;
> - void *addr = NULL;
> - struct sg_page_iter sg_iter;
> int first_page = start >> PAGE_SHIFT;
> int last_page = (len + start + 4095) >> PAGE_SHIFT;
> int npages = last_page - first_page;
> - struct page **pages;
> -
> - pages = drm_malloc_ab(npages, sizeof(*pages));
> - if (pages == NULL) {
> - DRM_DEBUG_DRIVER("Failed to get space for pages\n");
> - goto finish;
> - }
> -
> - i = 0;
> - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, first_page) {
> - pages[i++] = sg_page_iter_page(&sg_iter);
> - if (i == npages)
> - break;
> - }
> -
> - addr = vmap(pages, i, 0, PAGE_KERNEL);
> - if (addr == NULL) {
> - DRM_DEBUG_DRIVER("Failed to vmap pages\n");
> - goto finish;
> - }
>
> -finish:
> - if (pages)
> - drm_free_large(pages);
> - return (u32*)addr;
> + return (u32*)i915_gem_object_vmap(obj, first_page, npages);
> }
>
> /* Returns a vmap'd pointer to dest_obj, which the caller must unmap */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6644c2e..5b00a6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2899,6 +2899,9 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
> struct drm_device *dev, const void *data, size_t size);
> void i915_gem_free_object(struct drm_gem_object *obj);
> void i915_gem_vma_destroy(struct i915_vma *vma);
> +void *i915_gem_object_vmap(struct drm_i915_gem_object *obj,
> + unsigned int first,
> + unsigned int npages);
>
> /* Flags used by pin/bind&friends. */
> #define PIN_MAPPABLE (1<<0)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f68f346..a6f465b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5356,3 +5356,47 @@ fail:
> drm_gem_object_unreference(&obj->base);
> return ERR_PTR(ret);
> }
> +
> +/**
> + * i915_gem_object_vmap - map a GEM obj into kernel virtual space
> + * @obj: the GEM obj to be mapped
> + * @first: index of the first page where mapping starts
> + * @npages: how many pages to be mapped, starting from first page
> + *
> + * Map a given page range of GEM obj into kernel virtual space. The caller must
> + * make sure the associated pages are gathered and pinned before calling this
> + * function. vunmap should be called after use.
> + *
> + * NULL will be returned if fails.
> + */
> +void *i915_gem_object_vmap(struct drm_i915_gem_object *obj,
> + unsigned int first,
> + unsigned int npages)
> +{
> + struct sg_page_iter sg_iter;
> + struct page **pages;
> + void *addr;
> + int i;
> +
> + if (first + npages > obj->pages->nents) {
> + DRM_DEBUG_DRIVER("Invalid page count\n");
> + return NULL;
> + }
> +
> + pages = drm_malloc_ab(npages, sizeof(*pages));
> + if (pages == NULL) {
> + DRM_DEBUG_DRIVER("Failed to get space for pages\n");
> + return NULL;
> + }
> +
> + i = 0;
> + for_each_sg_page(obj->pages->sgl, &sg_iter, npages, first)
> + pages[i++] = sg_page_iter_page(&sg_iter);
I think there is a gotcha with for_each_sg_page where the nents has to
be larger than offset for it to return anything. Or in other words, it
will account all skipped pages against npages, even before it gets to
the desired offset.
Looking at the implementation, I even think nents is not equivalent to
npages when page coalescing is active which this helper should handle.
So I think you would need to do your own checking against npages and
just pass in the maximum nents for the object and the offset.
Just basically add "if (i == npages") break;" like the vmap_batch
implementation was doing.
> +
> + addr = vmap(pages, npages, 0, PAGE_KERNEL);
> + if (addr == NULL)
> + DRM_DEBUG_DRIVER("Failed to vmap pages\n");
> + drm_free_large(pages);
> +
> + return addr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 1f3eef6..d269957 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -110,9 +110,7 @@ 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;
> - struct sg_page_iter sg_iter;
> - struct page **pages;
> - int ret, i;
> + int ret;
>
> ret = i915_mutex_lock_interruptible(dev);
> if (ret)
> @@ -131,16 +129,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>
> ret = -ENOMEM;
>
> - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
> - if (pages == NULL)
> - goto err_unpin;
> -
> - i = 0;
> - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> - pages[i++] = sg_page_iter_page(&sg_iter);
> -
> - obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL);
> - drm_free_large(pages);
> + obj->dma_buf_vmapping = i915_gem_object_vmap(obj, 0, obj->pages->nents);
This is also not correct, obj->pages->nents can be smaller than
obj->base.size >> PAGE_SHIFT when coalescing is used. So I think you
need to pass in the latter here.
>
> if (!obj->dma_buf_vmapping)
> goto err_unpin;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 45ce45a..fe6d14a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2064,27 +2064,6 @@ void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
> i915_gem_object_ggtt_unpin(ringbuf->obj);
> }
>
> -static u32 *vmap_obj(struct drm_i915_gem_object *obj)
> -{
> - struct sg_page_iter sg_iter;
> - struct page **pages;
> - void *addr;
> - int i;
> -
> - pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
> - if (pages == NULL)
> - return NULL;
> -
> - i = 0;
> - for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> - pages[i++] = sg_page_iter_page(&sg_iter);
> -
> - addr = vmap(pages, i, 0, PAGE_KERNEL);
> - drm_free_large(pages);
> -
> - return addr;
> -}
> -
> int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
> struct intel_ringbuffer *ringbuf)
> {
> @@ -2103,7 +2082,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
> return ret;
> }
>
> - ringbuf->virtual_start = vmap_obj(obj);
> + ringbuf->virtual_start =
> + i915_gem_object_vmap(obj, 0, obj->pages->nents);
> if (ringbuf->virtual_start == NULL) {
> i915_gem_object_ggtt_unpin(obj);
> return -ENOMEM;
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list