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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/03/16 12:08, Chris Wilson wrote:
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@xxxxxxxxx>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Alex Dai <yu.dai@xxxxxxxxx>
---
  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

The patch is not pointless -- at the very least, it:
+ reduces the size of "struct drm_i915_gem_object" (OK, only by 4 bytes)
+ replaces special-function code for dmabufs with more generic code that can be reused for other objects (for now, ringbuffers; next GuC-shared objects -- see Alex's patch "drm/i915/guc: Simplify code by keeping vmap of guc_client object" which will eliminate lot of short-term kmap_atomics with persistent kmaps). + provides a shorthand for the sequence of { get_pages(), pin_pages(), vmap() } so we don't have to open-code it (and deal with all the error paths) in several different places

Thus there is an engineering benefit even if this version doesn't provide any performance benefit. And if, as the next step, you want to extend the vmap lifetime, you just have to remove those few lines in i915_gem_object_unpin_pages() and incorporate the notifier that you prototyped earlier -- if it actually provides any performance boost.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux