On Tue, Jul 23, 2013 at 06:54:43PM +0200, Daniel Vetter wrote:
> On Sun, Jul 21, 2013 at 07:08:13PM -0700, Ben Widawsky wrote:
> > Building on the last patch which created the new function pointers in
> > the VM for bind/unbind, here we actually put those new function pointers
> > to use.
> >
> > Split out as a separate patch to aid in review. I'm fine with squashing
> > into the previous patch if people request it.
> >
> > v2: Updated to address the smart ggtt which can do aliasing as needed
> > Make sure we bind to global gtt when mappable and fenceable. I thought
> > we could get away without this initialy, but we cannot.
> >
> > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
>
> Meta review on the patch split: If you create new functions in a prep
> patch, then switch and then kill the old functions it's much harder to
> review whether any unwanted functional changes have been introduced.
> Reviewers have to essentially keep both the old and new code open and
> compare by hand. And generally the really hard regression in gem have
> been due to such deeply-hidden accidental changes, and we frankly don't
> yet have the test coverage to just gloss over this.
>
> If you instead first prepare the existing functions by changing the
> arguments and logic, and then once everything is in place switch over to
> vfuncs in the 2nd patch changes will be in-place. In-place changes are
> much easier to review since diff compresses away unchanged parts.
>
> Second reason for this approach is that the functions stay at the same
> place in the source code file, which reduces the amount of spurious
> conflicts when rebasing a large set of patches around such changes ...
>
> I need to ponder this more.
> -Daniel
ping
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 10 ------
> > drivers/gpu/drm/i915/i915_gem.c | 37 +++++++++------------
> > drivers/gpu/drm/i915/i915_gem_context.c | 7 ++--
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 ++++++++--------
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 53 ++----------------------------
> > 5 files changed, 37 insertions(+), 99 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f3f2825..8d6aa34 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1933,18 +1933,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> >
> > /* i915_gem_gtt.c */
> > void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
> > -void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> > - struct drm_i915_gem_object *obj,
> > - enum i915_cache_level cache_level);
> > -void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
> > - struct drm_i915_gem_object *obj);
> > -
> > void i915_gem_restore_gtt_mappings(struct drm_device *dev);
> > int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
> > -/* FIXME: this is never okay with full PPGTT */
> > -void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
> > - enum i915_cache_level cache_level);
> > -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
> > void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
> > void i915_gem_init_global_gtt(struct drm_device *dev);
> > void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 9ea6424..63297d7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2653,12 +2653,9 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> >
> > trace_i915_gem_object_unbind(obj, vm);
> >
> > - if (obj->has_global_gtt_mapping && i915_is_ggtt(vm))
> > - i915_gem_gtt_unbind_object(obj);
> > - if (obj->has_aliasing_ppgtt_mapping) {
> > - i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj);
> > - obj->has_aliasing_ppgtt_mapping = 0;
> > - }
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > + vm->unmap_vma(vma);
> > +
> > i915_gem_gtt_finish_object(obj);
> > i915_gem_object_unpin_pages(obj);
> >
> > @@ -2666,7 +2663,6 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> > if (i915_is_ggtt(vm))
> > obj->map_and_fenceable = true;
> >
> > - vma = i915_gem_obj_to_vma(obj, vm);
> > list_del(&vma->mm_list);
> > list_del(&vma->vma_link);
> > drm_mm_remove_node(&vma->node);
> > @@ -3372,7 +3368,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > enum i915_cache_level cache_level)
> > {
> > struct drm_device *dev = obj->base.dev;
> > - drm_i915_private_t *dev_priv = dev->dev_private;
> > struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > int ret;
> >
> > @@ -3407,13 +3402,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > return ret;
> > }
> >
> > - if (obj->has_global_gtt_mapping)
> > - i915_gem_gtt_bind_object(obj, cache_level);
> > - if (obj->has_aliasing_ppgtt_mapping)
> > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
> > - obj, cache_level);
> > -
> > - i915_gem_obj_set_color(obj, vma->vm, cache_level);
> > + vm->map_vma(vma, cache_level, 0);
> > + i915_gem_obj_set_color(obj, vm, cache_level);
> > }
> >
> > if (cache_level == I915_CACHE_NONE) {
> > @@ -3695,6 +3685,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > bool map_and_fenceable,
> > bool nonblocking)
> > {
> > + const u32 flags = map_and_fenceable ? GLOBAL_BIND : 0;
> > + struct i915_vma *vma;
> > int ret;
> >
> > if (WARN_ON(obj->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> > @@ -3702,6 +3694,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >
> > WARN_ON(map_and_fenceable && !i915_is_ggtt(vm));
> >
> > + /* FIXME: Use vma for bounds check */
> > if (i915_gem_obj_bound(obj, vm)) {
> > if ((alignment &&
> > i915_gem_obj_offset(obj, vm) & (alignment - 1)) ||
> > @@ -3720,20 +3713,22 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > }
> >
> > if (!i915_gem_obj_bound(obj, vm)) {
> > - struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > -
> > ret = i915_gem_object_bind_to_vm(obj, vm, alignment,
> > map_and_fenceable,
> > nonblocking);
> > if (ret)
> > return ret;
> >
> > - if (!dev_priv->mm.aliasing_ppgtt)
> > - i915_gem_gtt_bind_object(obj, obj->cache_level);
> > - }
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > + vm->map_vma(vma, obj->cache_level, flags);
> > + } else
> > + vma = i915_gem_obj_to_vma(obj, vm);
> >
> > + /* Objects are created map and fenceable. If we bind an object
> > + * the first time, and we had aliasing PPGTT (and didn't request
> > + * GLOBAL), we'll need to do this on the second bind.*/
> > if (!obj->has_global_gtt_mapping && map_and_fenceable)
> > - i915_gem_gtt_bind_object(obj, obj->cache_level);
> > + vm->map_vma(vma, obj->cache_level, GLOBAL_BIND);
> >
> > obj->pin_count++;
> > obj->pin_mappable |= map_and_fenceable;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 873577d..cc7c0b4 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -417,8 +417,11 @@ static int do_switch(struct i915_hw_context *to)
> > return ret;
> > }
> >
> > - if (!to->obj->has_global_gtt_mapping)
> > - i915_gem_gtt_bind_object(to->obj, to->obj->cache_level);
> > + if (!to->obj->has_global_gtt_mapping) {
> > + struct i915_vma *vma = i915_gem_obj_to_vma(to->obj,
> > + &dev_priv->gtt.base);
> > + vma->vm->map_vma(vma, to->obj->cache_level, GLOBAL_BIND);
> > + }
> >
> > if (!to->is_initialized || is_default_context(to))
> > hw_flags |= MI_RESTORE_INHIBIT;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 8d2643b..6359ef2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -197,8 +197,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> > if (unlikely(IS_GEN6(dev) &&
> > reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
> > !target_i915_obj->has_global_gtt_mapping)) {
> > - i915_gem_gtt_bind_object(target_i915_obj,
> > - target_i915_obj->cache_level);
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > + vma->vm->map_vma(vma, target_i915_obj->cache_level,
> > + GLOBAL_BIND);
> > }
> >
> > /* Validate that the target is in a valid r/w GPU domain */
> > @@ -404,10 +405,12 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > struct i915_address_space *vm,
> > bool *need_reloc)
> > {
> > - struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> > bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4;
> > bool need_fence, need_mappable;
> > + u32 flags = (entry->flags & EXEC_OBJECT_NEEDS_GTT) &&
> > + !obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
> > + struct i915_vma *vma;
> > int ret;
> >
> > need_fence =
> > @@ -421,6 +424,7 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > if (ret)
> > return ret;
> >
> > + vma = i915_gem_obj_to_vma(obj, vm);
> > entry->flags |= __EXEC_OBJECT_HAS_PIN;
> >
> > if (has_fenced_gpu_access) {
> > @@ -436,14 +440,6 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > }
> > }
> >
> > - /* Ensure ppgtt mapping exists if needed */
> > - if (dev_priv->mm.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) {
> > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
> > - obj, obj->cache_level);
> > -
> > - obj->has_aliasing_ppgtt_mapping = 1;
> > - }
> > -
> > if (entry->offset != i915_gem_obj_offset(obj, vm)) {
> > entry->offset = i915_gem_obj_offset(obj, vm);
> > *need_reloc = true;
> > @@ -454,9 +450,7 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj,
> > obj->base.pending_write_domain = I915_GEM_DOMAIN_RENDER;
> > }
> >
> > - if (entry->flags & EXEC_OBJECT_NEEDS_GTT &&
> > - !obj->has_global_gtt_mapping)
> > - i915_gem_gtt_bind_object(obj, obj->cache_level);
> > + vm->map_vma(vma, obj->cache_level, flags);
> >
> > return 0;
> > }
> > @@ -1047,8 +1041,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > * batch" bit. Hence we need to pin secure batches into the global gtt.
> > * hsw should have this fixed, but let's be paranoid and do it
> > * unconditionally for now. */
> > - if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
> > - i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
> > + if (flags & I915_DISPATCH_SECURE &&
> > + !batch_obj->has_global_gtt_mapping) {
> > + struct i915_vma *vma = i915_gem_obj_to_vma(batch_obj, vm);
> > + vm->map_vma(vma, batch_obj->cache_level, GLOBAL_BIND);
> > + }
> >
> > ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->objects);
> > if (ret)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 03e6179..1de49a0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -414,18 +414,6 @@ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev)
> > dev_priv->mm.aliasing_ppgtt = NULL;
> > }
> >
> > -void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> > - struct drm_i915_gem_object *obj,
> > - enum i915_cache_level cache_level)
> > -{
> > - struct i915_address_space *vm = &ppgtt->base;
> > - unsigned long obj_offset = i915_gem_obj_offset(obj, vm);
> > -
> > - vm->insert_entries(vm, obj->pages,
> > - obj_offset >> PAGE_SHIFT,
> > - cache_level);
> > -}
> > -
> > static void __always_unused gen6_ppgtt_map_vma(struct i915_vma *vma,
> > enum i915_cache_level cache_level,
> > u32 flags)
> > @@ -437,16 +425,6 @@ static void __always_unused gen6_ppgtt_map_vma(struct i915_vma *vma,
> > gen6_ppgtt_insert_entries(vma->vm, vma->obj->pages, entry, cache_level);
> > }
> >
> > -void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
> > - struct drm_i915_gem_object *obj)
> > -{
> > - struct i915_address_space *vm = &ppgtt->base;
> > - unsigned long obj_offset = i915_gem_obj_offset(obj, vm);
> > -
> > - vm->clear_range(vm, obj_offset >> PAGE_SHIFT,
> > - obj->base.size >> PAGE_SHIFT);
> > -}
> > -
> > static void __always_unused gen6_ppgtt_unmap_vma(struct i915_vma *vma)
> > {
> > const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> > @@ -507,8 +485,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> > gen6_write_pdes(dev_priv->mm.aliasing_ppgtt);
> >
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> > + struct i915_vma *vma = i915_gem_obj_to_vma(obj,
> > + &dev_priv->gtt.base);
> > i915_gem_clflush_object(obj);
> > - i915_gem_gtt_bind_object(obj, obj->cache_level);
> > + vma->vm->map_vma(vma, obj->cache_level, 0);
> > }
> >
> > i915_gem_chipset_flush(dev);
> > @@ -664,33 +644,6 @@ static void gen6_ggtt_map_vma(struct i915_vma *vma,
> > }
> > }
> >
> > -void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
> > - enum i915_cache_level cache_level)
> > -{
> > - struct drm_device *dev = obj->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT;
> > -
> > - dev_priv->gtt.base.insert_entries(&dev_priv->gtt.base, obj->pages,
> > - entry,
> > - cache_level);
> > -
> > - obj->has_global_gtt_mapping = 1;
> > -}
> > -
> > -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
> > -{
> > - struct drm_device *dev = obj->base.dev;
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT;
> > -
> > - dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
> > - entry,
> > - obj->base.size >> PAGE_SHIFT);
> > -
> > - obj->has_global_gtt_mapping = 0;
> > -}
> > -
> > static void gen6_ggtt_unmap_vma(struct i915_vma *vma)
> > {
> > struct drm_device *dev = vma->vm->dev;
> > --
> > 1.8.3.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
