[PATCH v6 0/7] Reorganise calls to vmap() GEM objects

[PATCH v6 0/7] Reorganise calls to vmap() GEM objects

[Dave Gordon]

Alex Dai and Chris Wilson have both recently posted patches to
rationalise the use of vmap() for mapping GEM objects into kernel
virtual space. However, they addressed different areas, with Alex's
patch being derived from the copy_batch() code, whereas Chris' patch
refactored the dma-buf and ringbuffer code.

So this patchset unifies the two, copying Chris' interfaces which
unite pin-and-vmap for convenient lifecycle management, but using
Alex's code underneath to permit partial mappings. And finally
there's a little optimisation I've added for "small" objects e.g.
ringbuffers and contexts, which are expected to be the objects most
commonly handled by this code.

v5:
  Added another of Chris' patches, introducing drm_malloc_gfp().

  Split Chris' original patch into three, of which two are actually
  minor unrelated improvements, and only one is actually addresses
  the vmap() reorganisation [Tvrtko Ursulin]

  Decided not to hold onto vmappings after the pin count goes to
  zero. This may reduce the benefit of Chris' scheme somewhat, but
  does avoid any increased risk of exhausting kernel vmap space on
  32-bit kernels [Tvrtko Ursulin]. Potentially, the vunmap() could
  be moved back to the put_pages() stage (thus extending the cache
  lifetime) if a suitable notifier were written, but that's not
  included here.

v6:
  Addressed a few review comments by Tvrtko & Chris; the main
  functional change is that i915_gem_object_vmap_range() now takes
  a range in bytes rather than pages, so we use sg_nents_for_len()
  to validate it against the actual scatterlist.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Alex Dai <yu.dai@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Alex Dai (1):
  drm/i915: introduce and use i915_gem_object_vmap_range()

Chris Wilson (3):
  drm,i915: introduce drm_malloc_gfp()
  drm/i915: move locking in i915_gem_unmap_dma_buf()
  drm/i915: deduplicate intel_pin_and_map_ringbuffer_obj() error
    handling

Dave Gordon (3):
  drm: add parameter-order checking to drm memory allocators
  drm/i915: optimise i915_gem_object_vmap_range() for small objects
  drm/i915: refactor duplicate object vmap functions (reworked again)

 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |   2 +-
 drivers/gpu/drm/i915/i915_cmd_parser.c       |  32 +--------
 drivers/gpu/drm/i915/i915_drv.h              |  26 +++++--
 drivers/gpu/drm/i915/i915_gem.c              | 103 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c       |  53 ++------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  12 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c          |   5 +-
 drivers/gpu/drm/i915/i915_gem_userptr.c      |  15 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c      |  52 +++++---------
 include/drm/drm_mem_util.h                   |  44 +++++++++++-
 10 files changed, 202 insertions(+), 142 deletions(-)

-- 
1.9.1

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

[PATCH v6 1/7] drm,i915: introduce drm_malloc_gfp()

[Dave Gordon]

From: Chris Wilson <chris@chris-wilson.co.uk>

I have instances where I want to use drm_malloc_ab() but with a custom
gfp mask. And with those, where I want a temporary allocation, I want
to try a high-order kmalloc() before using a vmalloc().

So refactor my usage into drm_malloc_gfp().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 +++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  5 +++--
 drivers/gpu/drm/i915/i915_gem_userptr.c    | 15 ++++-----------
 include/drm/drm_mem_util.h                 | 19 +++++++++++++++++++
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1328bc5..f734b3c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1775,11 +1775,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
-			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (exec2_list == NULL)
-		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
-					   args->buffer_count);
+	exec2_list = drm_malloc_gfp(sizeof(*exec2_list),
+				    args->buffer_count,
+				    GFP_TEMPORARY);
 	if (exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
 			  args->buffer_count);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 49e4f26..6af2462 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3416,8 +3416,9 @@ struct i915_vma *
 	int ret = -ENOMEM;
 
 	/* Allocate a temporary list of source pages for random access. */
-	page_addr_list = drm_malloc_ab(obj->base.size / PAGE_SIZE,
-				       sizeof(dma_addr_t));
+	page_addr_list = drm_malloc_gfp(obj->base.size / PAGE_SIZE,
+					sizeof(dma_addr_t),
+					GFP_TEMPORARY);
 	if (!page_addr_list)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 4b09c84..4c98c8c 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -494,10 +494,7 @@ struct get_pages_work {
 	ret = -ENOMEM;
 	pinned = 0;
 
-	pvec = kmalloc(npages*sizeof(struct page *),
-		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (pvec == NULL)
-		pvec = drm_malloc_ab(npages, sizeof(struct page *));
+	pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY);
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
@@ -634,14 +631,10 @@ struct get_pages_work {
 	pvec = NULL;
 	pinned = 0;
 	if (obj->userptr.mm->mm == current->mm) {
-		pvec = kmalloc(num_pages*sizeof(struct page *),
-			       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
+		pvec = drm_malloc_gfp(num_pages, sizeof(struct page *), GFP_TEMPORARY);
 		if (pvec == NULL) {
-			pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
-			if (pvec == NULL) {
-				__i915_gem_userptr_set_active(obj, false);
-				return -ENOMEM;
-			}
+			__i915_gem_userptr_set_active(obj, false);
+			return -ENOMEM;
 		}
 
 		pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages,
diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index e42495a..741ce75 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -54,6 +54,25 @@ static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
 			 GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 
+static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
+{
+	if (size != 0 && nmemb > SIZE_MAX / size)
+		return NULL;
+
+	if (size * nmemb <= PAGE_SIZE)
+	    return kmalloc(nmemb * size, gfp);
+
+	if (gfp & __GFP_RECLAIMABLE) {
+		void *ptr = kmalloc(nmemb * size,
+				    gfp | __GFP_NOWARN | __GFP_NORETRY);
+		if (ptr)
+			return ptr;
+	}
+
+	return __vmalloc(size * nmemb,
+			 gfp | __GFP_HIGHMEM, PAGE_KERNEL);
+}
+
 static __inline void drm_free_large(void *ptr)
 {
 	kvfree(ptr);
-- 
1.9.1

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

[PATCH v6 2/7] drm: add parameter-order checking to drm memory allocators

[Dave Gordon]

After the recent addition of drm_malloc_gfp(), it was noticed that
some callers of these functions has swapped the parameters in the
call - it's supposed to be 'number of members' and 'sizeof(element)',
but a few callers had got the size first and the count second. This
isn't otherwise detected because they're both type 'size_t', and
the implementation at present just multiplies them anyway, so the
result is still right. But some future implementation might treat
them differently (e.g. allowing 0 elements but not zero size), so
let's add some compile-time checks and complain if the second (size)
parameter isn't a sizeof() expression, or at least a compile-time
constant.

This patch also fixes those callers where the order was wrong.

v6: removed duplicate BUILD_BUG_ON_MSG(); avoided renaming functions
    by shadowing them with #defines and then calling the function
    (non-recursively!) from inside the #define [Chris Wilson]

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>Cc: dri-
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c   |  8 ++++----
 include/drm/drm_mem_util.h                   | 27 ++++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 1aba01a..9ae4a71 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -340,7 +340,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	 */
 	bos = drm_malloc_ab(args->nr_bos, sizeof(*bos));
 	relocs = drm_malloc_ab(args->nr_relocs, sizeof(*relocs));
-	stream = drm_malloc_ab(1, args->stream_size);
+	stream = drm_malloc_ab(args->stream_size, sizeof(*stream));
 	cmdbuf = etnaviv_gpu_cmdbuf_new(gpu, ALIGN(args->stream_size, 8) + 8,
 					args->nr_bos);
 	if (!bos || !relocs || !stream || !cmdbuf) {
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f734b3c..1a136d9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1686,8 +1686,8 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	}
 
 	/* Copy in the exec list from userland */
-	exec_list = drm_malloc_ab(sizeof(*exec_list), args->buffer_count);
-	exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count);
+	exec_list = drm_malloc_ab(args->buffer_count, sizeof(*exec_list));
+	exec2_list = drm_malloc_ab(args->buffer_count, sizeof(*exec2_list));
 	if (exec_list == NULL || exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
 			  args->buffer_count);
@@ -1775,8 +1775,8 @@ static bool only_mappable_for_reloc(unsigned int flags)
 		return -EINVAL;
 	}
 
-	exec2_list = drm_malloc_gfp(sizeof(*exec2_list),
-				    args->buffer_count,
+	exec2_list = drm_malloc_gfp(args->buffer_count,
+				    sizeof(*exec2_list),
 				    GFP_TEMPORARY);
 	if (exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index 741ce75..5b0111c 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -29,7 +29,7 @@
 
 #include <linux/vmalloc.h>
 
-static __inline__ void *drm_calloc_large(size_t nmemb, size_t size)
+static __inline__ void *drm_calloc_large(size_t nmemb, const size_t size)
 {
 	if (size != 0 && nmemb > SIZE_MAX / size)
 		return NULL;
@@ -41,8 +41,15 @@ static __inline__ void *drm_calloc_large(size_t nmemb, size_t size)
 			 GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
 }
 
+#define	drm_calloc_large(nmemb, size)					\
+({									\
+	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
+		"Non-constant 'size' - check argument ordering?");	\
+	(drm_calloc_large)(nmemb, size);				\
+})
+
 /* Modeled after cairo's malloc_ab, it's like calloc but without the zeroing. */
-static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
+static __inline__ void *drm_malloc_ab(size_t nmemb, const size_t size)
 {
 	if (size != 0 && nmemb > SIZE_MAX / size)
 		return NULL;
@@ -54,7 +61,14 @@ static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
 			 GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 
-static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
+#define	drm_malloc_ab(nmemb, size)					\
+({									\
+	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
+		"Non-constant 'size' - check argument ordering?");	\
+	(drm_malloc_ab)(nmemb, size);					\
+})
+
+static __inline__ void *drm_malloc_gfp(size_t nmemb, const size_t size, gfp_t gfp)
 {
 	if (size != 0 && nmemb > SIZE_MAX / size)
 		return NULL;
@@ -73,6 +87,13 @@ static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
 			 gfp | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 
+#define	drm_malloc_gfp(nmemb, size, gfp)				\
+({									\
+	BUILD_BUG_ON_MSG(!__builtin_constant_p(size),			\
+		"Non-constant 'size' - check argument ordering?");	\
+	(drm_malloc_gfp)(nmemb, size, gfp);				\
+})
+
 static __inline void drm_free_large(void *ptr)
 {
 	kvfree(ptr);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v6 3/7] drm/i915: introduce and use i915_gem_object_vmap_range()

[Dave Gordon]

From: Alex Dai <yu.dai@intel.com>

There are several places inside driver where a GEM object is mapped
to kernel virtual space. The mapping may be done either for the whole
object or only a subset of it.

This patch introduces a function i915_gem_object_vmap_range() to
implement the common functionality. The code itself is extracted and
adapted from that in vmap_batch(), but also replaces vmap_obj() and the
open-coded version in i915_gem_dmabuf_vmap().

v2: use obj->pages->nents for iteration within i915_gem_object_vmap;
    break when it finishes all desired pages. The caller must pass the
    actual page count required. [Tvrtko Ursulin]

v4: renamed to i915_gem_object_vmap_range() to make its function
    clearer. [Dave Gordon]

v5: use Chris Wilson's new drm_malloc_gfp() rather than kmalloc() or
    drm_malloc_ab(). [Dave Gordon]

v6: changed range checking to not use pages->nents. [Tvrtko Ursulin]
    Use sg_nents_for_len() for range check instead. [Dave Gordon]
    Pass range parameters in bytes rather than pages (both callers
    were converting from bytes to pages anyway, so this reduces the
    number of places where the conversion is done).

With this change, we have only one vmap() in the whole driver :)

Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c  | 32 +-----------------
 drivers/gpu/drm/i915/i915_drv.h         |  4 +++
 drivers/gpu/drm/i915/i915_gem.c         | 58 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 16 ++-------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++------------
 5 files changed, 68 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 814d894..1b2515d 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -863,37 +863,7 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
 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 i915_gem_object_vmap_range(obj, start, len);
 }
 
 /* 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 a4dcb74..12b0717 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2983,6 +2983,10 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 	obj->pages_pin_count--;
 }
 
+void *__must_check i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
+					      unsigned long start,
+					      unsigned long nbytes);
+
 int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
 int i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			 struct intel_engine_cs *to,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3d31d3a..8f12e73 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2400,6 +2400,64 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+/**
+ * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space
+ * @obj: the GEM object to be mapped
+ * @start: offset in bytes of the start of the range to be mapped
+ * @len: length in bytes of the range to be mapped
+ *
+ * Map a given range of a GEM object into kernel virtual space. The range will
+ * be extended at both ends, if necessary, to span a whole number of pages. The
+ * caller must make sure the associated pages are gathered and pinned before
+ * calling this function, and is responsible for unmapping the returned address
+ * when it is no longer required.
+ *
+ * Returns the address at which the object has been mapped, or NULL on failure.
+ */
+void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
+				 unsigned long start,
+				 unsigned long nbytes)
+{
+	struct scatterlist *sg = obj->pages->sgl;
+	struct sg_page_iter sg_iter;
+	struct page **pages;
+	unsigned long first, npages, i;
+	int nents;
+	void *addr;
+
+	/* Check requested range against underlying sg list */
+	nents = sg_nents_for_len(sg, start + nbytes);
+	if (nents < 0) {
+		DRM_DEBUG_DRIVER("Invalid page count\n");
+		return NULL;
+	}
+
+	/* Work in pages from now on */
+	first = start >> PAGE_SHIFT;
+	npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first;
+
+	pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY);
+	if (pages == NULL) {
+		DRM_DEBUG_DRIVER("Failed to get space for pages\n");
+		return NULL;
+	}
+
+	i = 0;
+	for_each_sg_page(sg, &sg_iter, nents, first) {
+		pages[i] = sg_page_iter_page(&sg_iter);
+		if (++i >= npages)
+			break;
+	}
+	WARN_ON(i != npages);
+
+	addr = vmap(pages, npages, 0, PAGE_KERNEL);
+	if (addr == NULL)
+		DRM_DEBUG_DRIVER("Failed to vmap pages\n");
+	drm_free_large(pages);
+
+	return addr;
+}
+
 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 1f3eef6..aee4149 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,8 @@ 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_range(obj, 0,
+						dma_buf->size >> PAGE_SHIFT);
 
 	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..434a452 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_range(obj, 0,
+						ringbuf->size >> PAGE_SHIFT);
 		if (ringbuf->virtual_start == NULL) {
 			i915_gem_object_ggtt_unpin(obj);
 			return -ENOMEM;
-- 
1.9.1

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

Re: [PATCH v6 3/7] drm/i915: introduce and use i915_gem_object_vmap_range()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 15887 bytes --]

Hi Alex,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.5-rc6 next-20160229]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Dave-Gordon/Reorganise-calls-to-vmap-GEM-objects/20160301-052138
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_select_subconnector_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_mode_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_left_margin_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_right_margin_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_top_margin_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_bottom_margin_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_brightness_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_contrast_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_overscan_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_saturation_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'tv_hue_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'scaling_mode_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'aspect_ratio_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'dirty_info_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'suggested_x_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'suggested_y_property'
   include/drm/drm_crtc.h:2144: warning: No description found for parameter 'allow_fb_modifiers'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2364: warning: No description found for parameter 'connector'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work'
   drivers/gpu/drm/drm_dp_mst_topology.c:2364: warning: No description found for parameter 'connector'
   drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags'
   include/drm/drmP.h:168: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:184: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:202: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:247: warning: No description found for parameter 'dev'
   include/drm/drmP.h:247: warning: No description found for parameter 'data'
   include/drm/drmP.h:247: warning: No description found for parameter 'file_priv'
   include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
   include/drm/drmP.h:280: warning: No description found for parameter '_func'
   include/drm/drmP.h:280: warning: No description found for parameter '_flags'
   include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data '
   include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver '
   include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list '
   include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node '
   include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor '
   include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device '
   drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps'
   drivers/gpu/drm/i915/i915_gem.c:1461: warning: No description found for parameter 'req'
   drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'readonly'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1727: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'size'
   drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'tiling_mode'
   drivers/gpu/drm/i915/i915_gem.c:2015: warning: No description found for parameter 'fenced'
   drivers/gpu/drm/i915/i915_gem.c:2015: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
>> drivers/gpu/drm/i915/i915_gem.c:2420: warning: No description found for parameter 'nbytes'
>> drivers/gpu/drm/i915/i915_gem.c:2420: warning: Excess function parameter 'len' description in 'i915_gem_object_vmap_range'
   drivers/gpu/drm/i915/i915_gem.c:2972: warning: No description found for parameter 'ring'
   drivers/gpu/drm/i915/i915_gem.c:3103: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:3153: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:3153: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
   drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'vm'
   drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'ggtt_view'
   drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'alignment'
   drivers/gpu/drm/i915/i915_gem.c:3525: warning: No description found for parameter 'flags'
   drivers/gpu/drm/i915/i915_gem.c:3780: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3780: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/i915_gem.c:3855: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3855: warning: No description found for parameter 'cache_level'
   drivers/gpu/drm/i915/i915_gem.c:4129: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:4129: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   Warning: didn't use docs for i915_hotplug_interrupt_update
   Warning: didn't use docs for ilk_update_display_irq
   Warning: didn't use docs for ilk_update_gt_irq
   Warning: didn't use docs for snb_update_pm_irq
   Warning: didn't use docs for bdw_update_port_irq
   Warning: didn't use docs for bdw_update_pipe_irq
   Warning: didn't use docs for ibx_display_interrupt_update
   Warning: didn't use docs for i915_enable_asle_pipestat
   Warning: didn't use docs for ivybridge_parity_work
   Warning: didn't use docs for i915_reset_and_wakeup
   Warning: didn't use docs for i915_handle_error
   Warning: didn't use docs for intel_irq_install
   Warning: didn't use docs for intel_irq_uninstall

vim +/nbytes +2420 drivers/gpu/drm/i915/i915_gem.c

  2404	 * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space
  2405	 * @obj: the GEM object to be mapped
  2406	 * @start: offset in bytes of the start of the range to be mapped
  2407	 * @len: length in bytes of the range to be mapped
  2408	 *
  2409	 * Map a given range of a GEM object into kernel virtual space. The range will
  2410	 * be extended at both ends, if necessary, to span a whole number of pages. The
  2411	 * caller must make sure the associated pages are gathered and pinned before
  2412	 * calling this function, and is responsible for unmapping the returned address
  2413	 * when it is no longer required.
  2414	 *
  2415	 * Returns the address at which the object has been mapped, or NULL on failure.
  2416	 */
  2417	void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
  2418					 unsigned long start,
  2419					 unsigned long nbytes)
> 2420	{
  2421		struct scatterlist *sg = obj->pages->sgl;
  2422		struct sg_page_iter sg_iter;
  2423		struct page **pages;
  2424		unsigned long first, npages, i;
  2425		int nents;
  2426		void *addr;
  2427	
  2428		/* Check requested range against underlying sg list */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6229 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v6 3/7] drm/i915: introduce and use i915_gem_object_vmap_range()

[Tvrtko Ursulin]

On 29/02/16 21:16, Dave Gordon wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> There are several places inside driver where a GEM object is mapped
> to kernel virtual space. The mapping may be done either for the whole
> object or only a subset of it.
>
> This patch introduces a function i915_gem_object_vmap_range() to
> implement the common functionality. The code itself is extracted and
> adapted from that in vmap_batch(), but also replaces vmap_obj() and the
> open-coded version in i915_gem_dmabuf_vmap().
>
> v2: use obj->pages->nents for iteration within i915_gem_object_vmap;
>      break when it finishes all desired pages. The caller must pass the
>      actual page count required. [Tvrtko Ursulin]
>
> v4: renamed to i915_gem_object_vmap_range() to make its function
>      clearer. [Dave Gordon]
>
> v5: use Chris Wilson's new drm_malloc_gfp() rather than kmalloc() or
>      drm_malloc_ab(). [Dave Gordon]
>
> v6: changed range checking to not use pages->nents. [Tvrtko Ursulin]
>      Use sg_nents_for_len() for range check instead. [Dave Gordon]
>      Pass range parameters in bytes rather than pages (both callers
>      were converting from bytes to pages anyway, so this reduces the
>      number of places where the conversion is done).
>
> With this change, we have only one vmap() in the whole driver :)
>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_cmd_parser.c  | 32 +-----------------
>   drivers/gpu/drm/i915/i915_drv.h         |  4 +++
>   drivers/gpu/drm/i915/i915_gem.c         | 58 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 16 ++-------
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++------------
>   5 files changed, 68 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 814d894..1b2515d 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -863,37 +863,7 @@ void i915_cmd_parser_fini_ring(struct intel_engine_cs *ring)
>   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 i915_gem_object_vmap_range(obj, start, len);
>   }
>
>   /* 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 a4dcb74..12b0717 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2983,6 +2983,10 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>   	obj->pages_pin_count--;
>   }
>
> +void *__must_check i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
> +					      unsigned long start,
> +					      unsigned long nbytes);
> +
>   int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
>   int i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   			 struct intel_engine_cs *to,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3d31d3a..8f12e73 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2400,6 +2400,64 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>
> +/**
> + * i915_gem_object_vmap_range - map some or all of a GEM object into kernel space
> + * @obj: the GEM object to be mapped
> + * @start: offset in bytes of the start of the range to be mapped
> + * @len: length in bytes of the range to be mapped

kbuild spotted this kerneldoc issue.

> + *
> + * Map a given range of a GEM object into kernel virtual space. The range will
> + * be extended at both ends, if necessary, to span a whole number of pages. The
> + * caller must make sure the associated pages are gathered and pinned before
> + * calling this function, and is responsible for unmapping the returned address
> + * when it is no longer required.
> + *
> + * Returns the address at which the object has been mapped, or NULL on failure.
> + */
> +void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
> +				 unsigned long start,
> +				 unsigned long nbytes)
> +{
> +	struct scatterlist *sg = obj->pages->sgl;
> +	struct sg_page_iter sg_iter;
> +	struct page **pages;
> +	unsigned long first, npages, i;
> +	int nents;
> +	void *addr;
> +
> +	/* Check requested range against underlying sg list */
> +	nents = sg_nents_for_len(sg, start + nbytes);
> +	if (nents < 0) {
> +		DRM_DEBUG_DRIVER("Invalid page count\n");
> +		return NULL;
> +	}

I think this is needless overhead. The helper will iterate the whole sg 
chain while we know the size in obj->base.size and finding out the real 
nents is of little (no) use to the code below.

> +
> +	/* Work in pages from now on */
> +	first = start >> PAGE_SHIFT;
> +	npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first;

And this looks like weak API if the caller can pass non page aligned 
start and size and the function will silently vmap something else.

It should assert and fail on both I think, or it may have been simpler 
to keep it working in page units.

> +
> +	pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY);
> +	if (pages == NULL) {
> +		DRM_DEBUG_DRIVER("Failed to get space for pages\n");
> +		return NULL;
> +	}
> +
> +	i = 0;
> +	for_each_sg_page(sg, &sg_iter, nents, first) {
> +		pages[i] = sg_page_iter_page(&sg_iter);
> +		if (++i >= npages)
> +			break;
> +	}
> +	WARN_ON(i != npages);
> +
> +	addr = vmap(pages, npages, 0, PAGE_KERNEL);
> +	if (addr == NULL)
> +		DRM_DEBUG_DRIVER("Failed to vmap pages\n");
> +	drm_free_large(pages);
> +
> +	return addr;
> +}
> +
>   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 1f3eef6..aee4149 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,8 @@ 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_range(obj, 0,
> +						dma_buf->size >> PAGE_SHIFT);

This is still in pages. (Although as said below I think it should remain 
and API be reverted back.)

>
>   	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..434a452 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_range(obj, 0,
> +						ringbuf->size >> PAGE_SHIFT);

Here also.

>   		if (ringbuf->virtual_start == NULL) {
>   			i915_gem_object_ggtt_unpin(obj);
>   			return -ENOMEM;
>

Regards,

Tvrtko

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

Re: [PATCH v6 3/7] drm/i915: introduce and use i915_gem_object_vmap_range()

[Dave Gordon]

On 01/03/16 10:12, Tvrtko Ursulin wrote:
>
> On 29/02/16 21:16, Dave Gordon wrote:
>> From: Alex Dai <yu.dai@intel.com>
>>
>> There are several places inside driver where a GEM object is mapped
>> to kernel virtual space. The mapping may be done either for the whole
>> object or only a subset of it.
>>
>> This patch introduces a function i915_gem_object_vmap_range() to
>> implement the common functionality. The code itself is extracted and
>> adapted from that in vmap_batch(), but also replaces vmap_obj() and the
>> open-coded version in i915_gem_dmabuf_vmap().
>>
>> v2: use obj->pages->nents for iteration within i915_gem_object_vmap;
>>      break when it finishes all desired pages. The caller must pass the
>>      actual page count required. [Tvrtko Ursulin]
>>
>> v4: renamed to i915_gem_object_vmap_range() to make its function
>>      clearer. [Dave Gordon]
>>
>> v5: use Chris Wilson's new drm_malloc_gfp() rather than kmalloc() or
>>      drm_malloc_ab(). [Dave Gordon]
>>
>> v6: changed range checking to not use pages->nents. [Tvrtko Ursulin]
>>      Use sg_nents_for_len() for range check instead. [Dave Gordon]
>>      Pass range parameters in bytes rather than pages (both callers
>>      were converting from bytes to pages anyway, so this reduces the
>>      number of places where the conversion is done).
>>
>> With this change, we have only one vmap() in the whole driver :)
>>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---

[snip]

>> +/**
>> + * i915_gem_object_vmap_range - map some or all of a GEM object into
>> kernel space
>> + * @obj: the GEM object to be mapped
>> + * @start: offset in bytes of the start of the range to be mapped
>> + * @len: length in bytes of the range to be mapped
>
> kbuild spotted this kerneldoc issue.

Ah yes, that parameter got renamed

>> + *
>> + * Map a given range of a GEM object into kernel virtual space. The
>> range will
>> + * be extended at both ends, if necessary, to span a whole number of
>> pages. The
>> + * caller must make sure the associated pages are gathered and pinned
>> before
>> + * calling this function, and is responsible for unmapping the
>> returned address
>> + * when it is no longer required.
>> + *
>> + * Returns the address at which the object has been mapped, or NULL
>> on failure.
>> + */
>> +void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
>> +                 unsigned long start,
>> +                 unsigned long nbytes)
>> +{
>> +    struct scatterlist *sg = obj->pages->sgl;
>> +    struct sg_page_iter sg_iter;
>> +    struct page **pages;
>> +    unsigned long first, npages, i;
>> +    int nents;
>> +    void *addr;
>> +
>> +    /* Check requested range against underlying sg list */
>> +    nents = sg_nents_for_len(sg, start + nbytes);
>> +    if (nents < 0) {
>> +        DRM_DEBUG_DRIVER("Invalid page count\n");
>> +        return NULL;
>> +    }
>
> I think this is needless overhead. The helper will iterate the whole sg
> chain while we know the size in obj->base.size and finding out the real
> nents is of little (no) use to the code below.

It was more of a consistency check between the GEM object on the one 
hand, and the SGlist implementation on the other; since none of the 
other SG interfaces actually work in any useful quantities (e.g. pages 
or bytes; 'nents' is particularly useless, as it is affected by the 
details of the way the underlying pages have been mapped, in particular 
how many separate chunks they're in).

>> +
>> +    /* Work in pages from now on */
>> +    first = start >> PAGE_SHIFT;
>> +    npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first;
>
> And this looks like weak API if the caller can pass non page aligned
> start and size and the function will silently vmap something else.
>
> It should assert and fail on both I think, or it may have been simpler
> to keep it working in page units.

It is exactly the API required by copy_batch() (which is where this code 
came from), and it's a perfectly well-defined API: the start offset is 
rounded down to the start of the containing page, the end address (start 
+ len) is rounded up to the next page boundary, and that defines what 
gets mapped i.e. the smallest page-aligned region containing the range 
specified.

But I could let (all) the callers do that conversion instead ...

>> +    pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY);
>> +    if (pages == NULL) {
>> +        DRM_DEBUG_DRIVER("Failed to get space for pages\n");
>> +        return NULL;
>> +    }
>> +
>> +    i = 0;
>> +    for_each_sg_page(sg, &sg_iter, nents, first) {
>> +        pages[i] = sg_page_iter_page(&sg_iter);
>> +        if (++i >= npages)
>> +            break;
>> +    }
>> +    WARN_ON(i != npages);
>> +
>> +    addr = vmap(pages, npages, 0, PAGE_KERNEL);
>> +    if (addr == NULL)
>> +        DRM_DEBUG_DRIVER("Failed to vmap pages\n");
>> +    drm_free_large(pages);
>> +
>> +    return addr;
>> +}
>> +
>>   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 1f3eef6..aee4149 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,8 @@ 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_range(obj, 0,
>> +                        dma_buf->size >> PAGE_SHIFT);
>
> This is still in pages. (Although as said below I think it should remain
> and API be reverted back.)

Ah yes, I missed this call and the one below, 'cos they get fixed up by 
the last patch of the series (which *does* adopt the new interface).

>>       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..434a452 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_range(obj, 0,
>> +                        ringbuf->size >> PAGE_SHIFT);
>
> Here also.

Actually ... if we work in pages but adopt a convention that length==0 
means "the whole object" then only copy_batch() would have to do any 
page-aligning calculations; all other callers always want the whole 
object so it's nice to make the interface convenient for them :)

I'll see how that works out ...

.Dave.

>>           if (ringbuf->virtual_start == NULL) {
>>               i915_gem_object_ggtt_unpin(obj);
>>               return -ENOMEM;
>>
>
> Regards,
> Tvrtko

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

[PATCH v6 4/7] drm/i915: optimise i915_gem_object_vmap_range() for small objects

[Dave Gordon]

We're using this function for ringbuffers and other "small" objects, so
it's worth avoiding an extra malloc()/free() cycle if the page array is
small enough to put on the stack. Here we've chosen an arbitrary cutoff
of 32 (4k) pages, which is big enough for a ringbuffer (4 pages) or a
context image (currently up to 22 pages).

v5:
    change name of local array [Chris Wilson]

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8f12e73..9410c72 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2420,7 +2420,8 @@ void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
 {
 	struct scatterlist *sg = obj->pages->sgl;
 	struct sg_page_iter sg_iter;
-	struct page **pages;
+	struct page *stack_pages[32];
+	struct page **pages = stack_pages;
 	unsigned long first, npages, i;
 	int nents;
 	void *addr;
@@ -2436,10 +2437,13 @@ void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
 	first = start >> PAGE_SHIFT;
 	npages = DIV_ROUND_UP(start + nbytes, PAGE_SIZE) - first;
 
-	pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY);
-	if (pages == NULL) {
-		DRM_DEBUG_DRIVER("Failed to get space for pages\n");
-		return NULL;
+	if (npages > ARRAY_SIZE(stack_pages)) {
+		/* Too big for stack -- allocate temporary array instead */
+		pages = drm_malloc_gfp(npages, sizeof(*pages), GFP_TEMPORARY);
+		if (pages == NULL) {
+			DRM_DEBUG_DRIVER("Failed to get space for pages\n");
+			return NULL;
+		}
 	}
 
 	i = 0;
@@ -2453,7 +2457,8 @@ void *i915_gem_object_vmap_range(struct drm_i915_gem_object *obj,
 	addr = vmap(pages, npages, 0, PAGE_KERNEL);
 	if (addr == NULL)
 		DRM_DEBUG_DRIVER("Failed to vmap pages\n");
-	drm_free_large(pages);
+	if (pages != stack_pages)
+		drm_free_large(pages);
 
 	return addr;
 }
-- 
1.9.1

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

[PATCH v6 5/7] drm/i915: move locking in i915_gem_unmap_dma_buf()

[Dave Gordon]

From: Chris Wilson <chris@chris-wilson.co.uk>

The struct_mutex doesn't need to be (and therefore shouldn't be)
held around the various non-i915 operations in this function. We
only need the struct_mutex when we get to the unpin_pages() call.

Extracted from Chris Wilson's patch:
    drm/i915: Refactor duplicate object vmap functions
in preparation for the reimplementation of the same.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index aee4149..68e21d1 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -95,14 +95,12 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
 
-	mutex_lock(&obj->base.dev->struct_mutex);
-
 	dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
 	sg_free_table(sg);
 	kfree(sg);
 
+	mutex_lock(&obj->base.dev->struct_mutex);
 	i915_gem_object_unpin_pages(obj);
-
 	mutex_unlock(&obj->base.dev->struct_mutex);
 }
 
-- 
1.9.1

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

[PATCH v6 6/7] drm/i915: deduplicate intel_pin_and_map_ringbuffer_obj() error handling

[Dave Gordon]

From: Chris Wilson <chris@chris-wilson.co.uk>

Replace multiple "unpin(); return errno;" sequences with
a branch to a single common label for all the error paths.

Extracted from Chris Wilson's patch:
    drm/i915: Refactor duplicate object vmap functions
in preparation for the reimplementation of the same.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 434a452..15e2d29 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2077,16 +2077,14 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 			return ret;
 
 		ret = i915_gem_object_set_to_cpu_domain(obj, true);
-		if (ret) {
-			i915_gem_object_ggtt_unpin(obj);
-			return ret;
-		}
+		if (ret)
+			goto unpin;
 
 		ringbuf->virtual_start = i915_gem_object_vmap_range(obj, 0,
 						ringbuf->size >> PAGE_SHIFT);
 		if (ringbuf->virtual_start == NULL) {
-			i915_gem_object_ggtt_unpin(obj);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto unpin;
 		}
 	} else {
 		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
@@ -2094,10 +2092,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 			return ret;
 
 		ret = i915_gem_object_set_to_gtt_domain(obj, true);
-		if (ret) {
-			i915_gem_object_ggtt_unpin(obj);
-			return ret;
-		}
+		if (ret)
+			goto unpin;
 
 		/* Access through the GTT requires the device to be awake. */
 		assert_rpm_wakelock_held(dev_priv);
@@ -2105,14 +2101,18 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
 						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
 		if (ringbuf->virtual_start == NULL) {
-			i915_gem_object_ggtt_unpin(obj);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto unpin;
 		}
 	}
 
 	ringbuf->vma = i915_gem_obj_to_ggtt(obj);
 
 	return 0;
+
+unpin:
+	i915_gem_object_ggtt_unpin(obj);
+	return ret;
 }
 
 static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
-- 
1.9.1

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

[PATCH v6 7/7] drm/i915: refactor duplicate object vmap functions (reworked again)

[Dave Gordon]

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@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         | 22 +++++++++++++-----
 drivers/gpu/drm/i915/i915_gem.c         | 40 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 37 ++++--------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++-----
 4 files changed, 66 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 12b0717..754441a 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 9410c72..b17423d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2235,6 +2235,12 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
+	/* vmapping should have been dropped when pages_pin_count went to 0 */
+	if (WARN_ON(obj->vmapping)) {
+		vunmap(obj->vmapping);
+		obj->vmapping = NULL;
+	}
+
 	i915_gem_object_invalidate(obj);
 
 	return 0;
@@ -2463,6 +2469,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);
+
+		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 {
-- 
1.9.1

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

✗ Fi.CI.BAT: warning for Reorganise calls to vmap() GEM objects (rev4)

[Patchwork]

== Series Details ==

Series: Reorganise calls to vmap() GEM objects (rev4)
URL   : https://patchwork.freedesktop.org/series/3688/
State : warning

== Summary ==

Series 3688v4 Reorganise calls to vmap() GEM objects
http://patchwork.freedesktop.org/api/1.0/series/3688/revisions/4/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ilk-hp8440p)
                pass       -> SKIP       (ivb-t430s)
        Subgroup force-load-detect:
                fail       -> SKIP       (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-x220t)
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:169  pass:138  dwarn:0   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25 
hsw-brixbox      total:169  pass:153  dwarn:1   dfail:0   fail:0   skip:15 
hsw-gt2          total:169  pass:157  dwarn:1   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:169  pass:115  dwarn:2   dfail:0   fail:0   skip:52 
ivb-t430s        total:169  pass:152  dwarn:0   dfail:0   fail:1   skip:16 
skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
snb-dellxps      total:169  pass:144  dwarn:1   dfail:0   fail:1   skip:23 
snb-x220t        total:169  pass:144  dwarn:1   dfail:0   fail:2   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1499/

deb861ea08c6d5bbf682fb40ea1f0471a448aafd drm-intel-nightly: 2016y-03m-01d-11h-12m-16s UTC integration manifest
42b2faf48a67fb45ee1661d61b184d84a56c8448 drm/i915: refactor duplicate object vmap functions (reworked again)
23511e6942ef6fbd46b3291b1031a74e29495b5c drm/i915: deduplicate intel_pin_and_map_ringbuffer_obj() error handling
10275d84adb4918d7d116e43b9ca26d45298e71c drm/i915: move locking in i915_gem_unmap_dma_buf()
4a06fa2941a4e4fff8d173bc163cc1861e48494e drm/i915: optimise i915_gem_object_vmap_range() for small objects
2dc9da3af74e2e627267724ab01b341c2855a3cc drm/i915: introduce and use i915_gem_object_vmap_range()
ba80092458ccebcbc7c80215c123fb8a50a7f472 drm: add parameter-order checking to drm memory allocators
2a898fccfc38ba6664d96fbcdce0333ea1da3a7a drm,i915: introduce drm_malloc_gfp()

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