On Wed, Apr 13, 2016 at 09:45:03AM +0100, Tvrtko Ursulin wrote: > > On 12/04/16 17:21, Chris Wilson wrote: > >On Tue, Apr 12, 2016 at 04:56:51PM +0100, Tvrtko Ursulin wrote: > >>From: Chris Wilson <[email protected]> > >> > >>By tracking the iomapping on the VMA itself, we can share that area > >>between multiple users. Also by only revoking the iomapping upon > >>unbinding from the mappable portion of the GGTT, we can keep that iomap > >>across multiple invocations (e.g. execlists context pinning). > >> > >>v2: > >> * Rebase on nightly; > >> * added kerneldoc. (Tvrtko Ursulin) > >> > >>Signed-off-by: Chris Wilson <[email protected]> > >>Signed-off-by: Tvrtko Ursulin <[email protected]> > >>--- > >> drivers/gpu/drm/i915/i915_gem.c | 2 ++ > >> drivers/gpu/drm/i915/i915_gem_gtt.c | 38 > >> +++++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/i915_gem_gtt.h | 38 > >> +++++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_fbdev.c | 8 +++----- > >> 4 files changed, 81 insertions(+), 5 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem.c > >>b/drivers/gpu/drm/i915/i915_gem.c > >>index b37ffea8b458..6a485630595e 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem.c > >>+++ b/drivers/gpu/drm/i915/i915_gem.c > >>@@ -3393,6 +3393,8 @@ static int __i915_vma_unbind(struct i915_vma *vma, > >>bool wait) > >> ret = i915_gem_object_put_fence(obj); > >> if (ret) > >> return ret; > >>+ > >>+ i915_vma_iounmap(vma); > >> } > >> > >> trace_i915_vma_unbind(vma); > >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>index c5cb04907525..b2a8a14e8dcb 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>@@ -3626,3 +3626,41 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj, > >> return obj->base.size; > >> } > >> } > >>+ > >>+void *i915_vma_iomap(struct drm_i915_private *dev_priv, > >>+ struct i915_vma *vma) > >>+{ > >>+ struct drm_i915_gem_object *obj = vma->obj; > >>+ struct i915_ggtt *ggtt = &dev_priv->ggtt; > >>+ > >>+ if (WARN_ON(!obj->map_and_fenceable)) > >>+ return ERR_PTR(-ENODEV); > >>+ > >>+ BUG_ON(!vma->is_ggtt); > >>+ BUG_ON(vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL); > >>+ BUG_ON((vma->bound & GLOBAL_BIND) == 0); > >>+ > >>+ if (vma->iomap == NULL) { > >>+ void *ptr; > > > >We could extract ggtt from the is_ggtt vma->vm that would remove the > >dev_priv parameter and make the callers a bit tidier. > > > >static inline struct i915_ggtt *to_ggtt(struct i915_address_space *vm) > >{ > > BUG_ON(!i915_is_ggtt(vm)); > > return container_of(vm, struct i915_ggtt, base); > >} > > > >>+ > >>+ ptr = ioremap_wc(ggtt->mappable_base + vma->node.start, > >>+ obj->base.size); > >>+ if (ptr == NULL) { > >>+ int ret; > >>+ > >>+ /* Too many areas already allocated? */ > >>+ ret = i915_gem_evict_vm(vma->vm, true); > >>+ if (ret) > >>+ return ERR_PTR(ret); > >>+ > >>+ ptr = ioremap_wc(ggtt->mappable_base + vma->node.start, > >>+ obj->base.size); > > > >No, we really don't want to create a new ioremap for every caller when > >we already have one, ggtt->mappable. Hence, > > io-mapping: Specify mapping size for io_mapping_map_wc > >being its preceeding patch. The difference is huge on Braswell. > > I was following the principle of least change for this one since it > does remain the same in that respect as the current code. Goal was > to unblock the GuC early unpin / execlist no retired queue series > and not get into the trap of inflating the dependencies too much. I > thought to add this and default context cleanup but you keep adding > things to the pile. :)
Tip of the iceberg. Only we have lots of icebergs. And the occasional dragon. > I'll have a look at your io_mapping stuff to see how much it would > add to the series. Remember I said I'll add the stable ctx id > patches as well. Do we need to come up with a plan here? We more or less own io_mapping, for this it is just one patch to add a pass-through parameter to ioremap_wc that's required for 32bit. I could mutter about the patch before this being quite a major bugfix... > I could have two separate series to simplify dependencies a bit: > > 1. GuC premature unpin and > 2. execlist no retired queue. > > The stack for the first one could look like (not as patches but > conceptually): > > 1. default context cleanup > 2. any io_mapping patches of yours > 3. i915_vma_iomap or WC pin_map as you suggested in the other reply. > 4. previous / pinned context > > Execlist no retired queue would then be: > > 1. stable ctx id > 2. removal of i915_gem_request_unreference__unlocked > 3. execlist no retired queue > > If I did not forget about something. > > At any point in any series we could add things like > i915_gem_request.c or those patches which move around the context > init. Could I see you some drm_mm.c patches after that? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
