| Message ID | 1417781506-1730-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 05, 2014 at 12:11:45PM +0000, Tvrtko Ursulin wrote: > +struct i915_ggtt_view { > + enum i915_ggtt_view_type type; > + > + struct sg_table *pages; > +}; Minor nit on semantics, not really worth a resend (except when you need one anyway because of the detailed review): Imo the sg_table should be in the vma directly, not in the gtt_view. Otherwise we can't just do a memcmp if the view paramaters grow more fancy than just the type key. And maybe call it sg_table, not pages, to avoid confusion with the real obj->pages backing storage pointer. Anyway just bikesheds ;-) -Daniel > + > +extern const struct i915_ggtt_view i915_ggtt_view_normal; > + > enum i915_cache_level; > + > /** > * A VMA represents a GEM BO that is bound into an address space. Therefore, a > * VMA's presence cannot be guaranteed before binding, or after unbinding the > @@ -129,6 +142,15 @@ struct i915_vma { > #define PTE_READ_ONLY (1<<2) > unsigned int bound : 4; > > + /** > + * Support different GGTT views into the same object. > + * This means there can be multiple VMA mappings per object and per VM. > + * i915_ggtt_view_type is used to distinguish between those entries. > + * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also > + * assumed in GEM functions which take no ggtt view parameter. > + */ > + struct i915_ggtt_view ggtt_view; > + > /** This object's place on the active/inactive lists */ > struct list_head mm_list; > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index c4536e1..f97479a 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err, > break; > > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->vm == vm && vma->pin_count > 0) { > + if (vma->vm == vm && vma->pin_count > 0) > capture_bo(err++, vma); > - break; > - } > } > > return err - first; > @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv, > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->vm == vm && vma->pin_count > 0) { > + if (vma->vm == vm && vma->pin_count > 0) > i++; > - break; > - } > } > error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx]; > > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Things like reliable GGTT mappings and mirrored 2d-on-3d display will need > to map objects into the same address space multiple times. > > Added a GGTT view concept and linked it with the VMA to distinguish between > multiple instances per address space. > > New objects and GEM functions which do not take this new view as a parameter > assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the > previous behaviour. > > This now means that objects can have multiple VMA entries so the code which > assumed there will only be one also had to be modified. > > Alternative GGTT views are supposed to borrow DMA addresses from obj->pages > which is DMA mapped on first VMA instantiation and unmapped on the last one > going away. > > v2: > * Removed per view special casing in i915_gem_ggtt_prepare / > finish_object in favour of creating and destroying DMA mappings > on first VMA instantiation and last VMA destruction. (Daniel Vetter) > * Simplified i915_vma_unbind which does not need to count the GGTT views. > (Daniel Vetter) > * Also moved obj->map_and_fenceable reset under the same check. > * Checkpatch cleanups. > > v3: > * Only retire objects once the last VMA is unbound. > > v4: > * Keep scatter-gather table for alternative views persistent for the > lifetime of the VMA. > * Propagate binding errors to callers and handle appropriately. > > For: VIZ-4544 > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 5 +- > drivers/gpu/drm/i915/i915_drv.h | 46 +++++++++++-- > drivers/gpu/drm/i915/i915_gem.c | 101 ++++++++++++++++++----------- > drivers/gpu/drm/i915/i915_gem_context.c | 11 +++- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++- > drivers/gpu/drm/i915/i915_gem_gtt.c | 70 +++++++++++++++++--- > drivers/gpu/drm/i915/i915_gem_gtt.h | 22 +++++++ > drivers/gpu/drm/i915/i915_gpu_error.c | 8 +-- > 8 files changed, 206 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 6c16939..bd08289 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > seq_puts(m, " (pp"); > else > seq_puts(m, " (g"); > - seq_printf(m, "gtt offset: %08lx, size: %08lx)", > - vma->node.start, vma->node.size); > + seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)", > + vma->node.start, vma->node.size, > + vma->ggtt_view.type); > } > if (obj->stolen) > seq_printf(m, " (stolen: %08lx)", obj->stolen->start); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 049482f..b2f6f7d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma); > #define PIN_GLOBAL 0x4 > #define PIN_OFFSET_BIAS 0x8 > #define PIN_OFFSET_MASK (~4095) > +int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + uint32_t alignment, > + uint64_t flags, > + const struct i915_ggtt_view *view); > +static inline > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > struct i915_address_space *vm, > uint32_t alignment, > - uint64_t flags); > + uint64_t flags) > +{ > + return i915_gem_object_pin_view(obj, vm, alignment, flags, > + &i915_ggtt_view_normal); > +} > + > +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > + u32 flags); > int __must_check i915_vma_unbind(struct i915_vma *vma); > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > @@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, > > void i915_gem_restore_fences(struct drm_device *dev); > > +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, > + struct i915_address_space *vm, > + enum i915_ggtt_view_type view); > +static inline > unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, > - struct i915_address_space *vm); > + struct i915_address_space *vm) > +{ > + return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL); > +} > bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o); > bool i915_gem_obj_bound(struct drm_i915_gem_object *o, > struct i915_address_space *vm); > unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, > struct i915_address_space *vm); > +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + const struct i915_ggtt_view *view); > +static inline > struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm); > + struct i915_address_space *vm) > +{ > + return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal); > +} > + > +struct i915_vma * > +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + const struct i915_ggtt_view *view); > + > +static inline > struct i915_vma * > i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm); > + struct i915_address_space *vm) > +{ > + return i915_gem_obj_lookup_or_create_vma_view(obj, vm, > + &i915_ggtt_view_normal); > +} Hi, We also need a _vma_view version of i915_gem_obj_bound; i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be that the normal view is gone but a different view is still active (it is also used in gpu_error and debug_fs, but I don't think it's a problem there). > > struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj); > static inline bool i915_gem_obj_is_pinned(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 c1c1141..9244f0b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2087,8 +2087,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, > /* For the unbound phase, this should be a no-op! */ > list_for_each_entry_safe(vma, v, > &obj->vma_list, vma_link) > - if (i915_vma_unbind(vma)) > - break; > + i915_vma_unbind(vma); > > if (i915_gem_object_put_pages(obj) == 0) > count += obj->base.size >> PAGE_SHIFT; > @@ -2297,17 +2296,14 @@ void i915_vma_move_to_active(struct i915_vma *vma, > static void > i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) > { > - struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > - struct i915_address_space *vm; > struct i915_vma *vma; > > BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS); > BUG_ON(!obj->active); > > - list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > - vma = i915_gem_obj_to_vma(obj, vm); > - if (vma && !list_empty(&vma->mm_list)) > - list_move_tail(&vma->mm_list, &vm->inactive_list); > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > + if (!list_empty(&vma->mm_list)) > + list_move_tail(&vma->mm_list, &vma->vm->inactive_list); > } > > intel_fb_obj_flush(obj, true); > @@ -3062,10 +3058,8 @@ int i915_vma_unbind(struct i915_vma *vma) > * cause memory corruption through use-after-free. > */ > > - /* Throw away the active reference before moving to the unbound list */ > - i915_gem_object_retire(obj); > - > - if (i915_is_ggtt(vma->vm)) { > + if (i915_is_ggtt(vma->vm) && > + vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > i915_gem_object_finish_gtt(obj); > > /* release the fence reg _after_ flushing */ > @@ -3079,8 +3073,15 @@ int i915_vma_unbind(struct i915_vma *vma) > vma->unbind_vma(vma); > > list_del_init(&vma->mm_list); > - if (i915_is_ggtt(vma->vm)) > - obj->map_and_fenceable = false; > + if (i915_is_ggtt(vma->vm)) { > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { > + obj->map_and_fenceable = false; > + } else if (vma->ggtt_view.pages) { > + sg_free_table(vma->ggtt_view.pages); > + kfree(vma->ggtt_view.pages); > + vma->ggtt_view.pages = NULL; > + } > + } > > drm_mm_remove_node(&vma->node); > i915_gem_vma_destroy(vma); > @@ -3088,6 +3089,10 @@ int i915_vma_unbind(struct i915_vma *vma) > /* Since the unbound list is global, only move to that list if > * no more VMAs exist. */ > if (list_empty(&obj->vma_list)) { > + /* Throw away the active reference before > + * moving to the unbound list. */ > + i915_gem_object_retire(obj); > + > i915_gem_gtt_finish_object(obj); > list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); > } > @@ -3498,7 +3503,8 @@ static struct i915_vma * > i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > struct i915_address_space *vm, > unsigned alignment, > - uint64_t flags) > + uint64_t flags, > + const struct i915_ggtt_view *view) > { > struct drm_device *dev = obj->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3548,7 +3554,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > > i915_gem_object_pin_pages(obj); > > - vma = i915_gem_obj_lookup_or_create_vma(obj, vm); > + vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view); > if (IS_ERR(vma)) > goto err_unpin; > > @@ -3578,15 +3584,19 @@ search_free: > if (ret) > goto err_remove_node; > > + trace_i915_vma_bind(vma, flags); > + ret = i915_vma_bind(vma, obj->cache_level, > + flags & PIN_GLOBAL ? GLOBAL_BIND : 0); > + if (ret) > + goto err_finish_gtt; > + > list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > list_add_tail(&vma->mm_list, &vm->inactive_list); > > - trace_i915_vma_bind(vma, flags); > - vma->bind_vma(vma, obj->cache_level, > - flags & PIN_GLOBAL ? GLOBAL_BIND : 0); > - > return vma; > > +err_finish_gtt: > + i915_gem_gtt_finish_object(obj); > err_remove_node: > drm_mm_remove_node(&vma->node); > err_free_vma: > @@ -3789,9 +3799,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, > } > > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (drm_mm_node_allocated(&vma->node)) > - vma->bind_vma(vma, cache_level, > - vma->bound & GLOBAL_BIND); > + if (drm_mm_node_allocated(&vma->node)) { > + ret = i915_vma_bind(vma, cache_level, > + vma->bound & GLOBAL_BIND); > + if (ret); > + return ret; > + } > } > > list_for_each_entry(vma, &obj->vma_list, vma_link) > @@ -4144,10 +4157,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags) > } > > int > -i915_gem_object_pin(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm, > - uint32_t alignment, > - uint64_t flags) > +i915_gem_object_pin_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + uint32_t alignment, > + uint64_t flags, > + const struct i915_ggtt_view *view) > { > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > struct i915_vma *vma; > @@ -4163,7 +4177,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, > if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE)) > return -EINVAL; > > - vma = i915_gem_obj_to_vma(obj, vm); > + vma = i915_gem_obj_to_vma_view(obj, vm, view); > if (vma) { > if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT)) > return -EBUSY; > @@ -4173,7 +4187,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, > "bo is already pinned with incorrect alignment:" > " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d," > " obj->map_and_fenceable=%d\n", > - i915_gem_obj_offset(obj, vm), alignment, > + i915_gem_obj_offset_view(obj, vm, view->type), > + alignment, > !!(flags & PIN_MAPPABLE), > obj->map_and_fenceable); > ret = i915_vma_unbind(vma); > @@ -4186,13 +4201,17 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, > > bound = vma ? vma->bound : 0; > if (vma == NULL || !drm_mm_node_allocated(&vma->node)) { > - vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags); > + vma = i915_gem_object_bind_to_vm(obj, vm, alignment, > + flags, view); > if (IS_ERR(vma)) > return PTR_ERR(vma); > } > > - if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) > - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); > + if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) { > + ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); > + if (ret) > + return ret; > + } > > if ((bound ^ vma->bound) & GLOBAL_BIND) { > bool mappable, fenceable; > @@ -4528,12 +4547,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > intel_runtime_pm_put(dev_priv); > } > > -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm) > +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + const struct i915_ggtt_view *view) > { > struct i915_vma *vma; > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->vm == vm) > + if (vma->vm == vm && vma->ggtt_view.type == view->type) > return vma; > > return NULL; > @@ -5147,8 +5167,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) > } > > /* All the new VM stuff */ > -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, > - struct i915_address_space *vm) > +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, > + struct i915_address_space *vm, > + enum i915_ggtt_view_type view) > { > struct drm_i915_private *dev_priv = o->base.dev->dev_private; > struct i915_vma *vma; > @@ -5156,7 +5177,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, > WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base); > > list_for_each_entry(vma, &o->vma_list, vma_link) { > - if (vma->vm == vm) > + if (vma->vm == vm && vma->ggtt_view.type == view) > return vma->node.start; > > } > @@ -5307,7 +5328,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) > struct i915_vma *vma; > > list_for_each_entry(vma, &obj->vma_list, vma_link) { > - if (vma->vm == ggtt) > + if (vma->vm != ggtt) > + continue; > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > return vma; > } I'd prefer to have a single if (vma->vm == ggtt && type = normal view). Everything else looks good to me. -Michel > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 5cd2b97..4ef8815 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -590,9 +590,14 @@ static int do_switch(struct intel_engine_cs *ring, > goto unpin_out; > > vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state); > - if (!(vma->bound & GLOBAL_BIND)) > - vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, > - GLOBAL_BIND); > + if (!(vma->bound & GLOBAL_BIND)) { > + ret = i915_vma_bind(vma, > + to->legacy_hw_ctx.rcs_state->cache_level, > + GLOBAL_BIND); > + /* This shouldn't ever fail. */ > + if (WARN_ONCE(ret, "GGTT context bind failed!")) > + goto unpin_out; > + } > > if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(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 0c25f62..3927d93 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -360,9 +360,12 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, > * through the ppgtt for non_secure batchbuffers. */ > if (unlikely(IS_GEN6(dev) && > reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION && > - !(target_vma->bound & GLOBAL_BIND))) > - target_vma->bind_vma(target_vma, target_i915_obj->cache_level, > - GLOBAL_BIND); > + !(target_vma->bound & GLOBAL_BIND))) { > + ret = i915_vma_bind(target_vma, target_i915_obj->cache_level, > + GLOBAL_BIND); > + if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!")) > + return ret; > + } > > /* Validate that the target is in a valid r/w GPU domain */ > if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index ac03a38..73c1c0b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -30,6 +30,8 @@ > #include "i915_trace.h" > #include "intel_drv.h" > > +const struct i915_ggtt_view i915_ggtt_view_normal; > + > static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv); > static void chv_setup_private_ppat(struct drm_i915_private *dev_priv); > > @@ -1341,9 +1343,12 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > /* The bind_vma code tries to be smart about tracking mappings. > * Unfortunately above, we've just wiped out the mappings > * without telling our object about it. So we need to fake it. > + * > + * Bind is not expected to fail since this is only called on > + * resume and assumption is all requirements exist already. > */ > vma->bound &= ~GLOBAL_BIND; > - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); > + WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND)); > } > > > @@ -1538,7 +1543,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma, > AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; > > BUG_ON(!i915_is_ggtt(vma->vm)); > - intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags); > + intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags); > vma->bound = GLOBAL_BIND; > } > > @@ -1588,7 +1593,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, > if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { > if (!(vma->bound & GLOBAL_BIND) || > (cache_level != obj->cache_level)) { > - vma->vm->insert_entries(vma->vm, obj->pages, > + vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages, > vma->node.start, > cache_level, flags); > vma->bound |= GLOBAL_BIND; > @@ -1600,7 +1605,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, > (cache_level != obj->cache_level))) { > struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt; > appgtt->base.insert_entries(&appgtt->base, > - vma->obj->pages, > + vma->ggtt_view.pages, > vma->node.start, > cache_level, flags); > vma->bound |= LOCAL_BIND; > @@ -2165,7 +2170,8 @@ int i915_gem_gtt_init(struct drm_device *dev) > } > > static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm) > + struct i915_address_space *vm, > + const struct i915_ggtt_view *view) > { > struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL); > if (vma == NULL) > @@ -2176,6 +2182,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, > INIT_LIST_HEAD(&vma->exec_list); > vma->vm = vm; > vma->obj = obj; > + vma->ggtt_view = *view; > > switch (INTEL_INFO(vm->dev)->gen) { > case 9: > @@ -2210,14 +2217,59 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, > } > > struct i915_vma * > -i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, > - struct i915_address_space *vm) > +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj, > + struct i915_address_space *vm, > + const struct i915_ggtt_view *view) > { > struct i915_vma *vma; > > - vma = i915_gem_obj_to_vma(obj, vm); > + vma = i915_gem_obj_to_vma_view(obj, vm, view); > if (!vma) > - vma = __i915_gem_vma_create(obj, vm); > + vma = __i915_gem_vma_create(obj, vm, view); > > return vma; > } > + > +static inline > +int i915_get_vma_pages(struct i915_vma *vma) > +{ > + if (vma->ggtt_view.pages) > + return 0; > + > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > + vma->ggtt_view.pages = vma->obj->pages; > + else > + WARN_ONCE(1, "GGTT view %u not implemented!\n", > + vma->ggtt_view.type); > + > + if (!vma->ggtt_view.pages) { > + DRM_ERROR("Failed to get pages for VMA view type %u!\n", > + vma->ggtt_view.type); > + return -EINVAL; > + } > + > + return 0; > +} > + > +/** > + * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space. > + * @vma: VMA to map > + * @cache_level: mapping cache level > + * @flags: flags like global or local mapping > + * > + * DMA addresses are taken from the scatter-gather table of this object (or of > + * this VMA in case of non-default GGTT views) and PTE entries set up. > + * Note that DMA addresses are also the only part of the SG table we care about. > + */ > +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > + u32 flags) > +{ > + int ret = i915_get_vma_pages(vma); > + > + if (ret) > + return ret; > + > + vma->bind_vma(vma, cache_level, flags); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index dd849df..e377c7d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -109,7 +109,20 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; > #define GEN8_PPAT_ELLC_OVERRIDE (0<<2) > #define GEN8_PPAT(i, x) ((uint64_t) (x) << ((i) * 8)) > > +enum i915_ggtt_view_type { > + I915_GGTT_VIEW_NORMAL = 0, > +}; > + > +struct i915_ggtt_view { > + enum i915_ggtt_view_type type; > + > + struct sg_table *pages; > +}; > + > +extern const struct i915_ggtt_view i915_ggtt_view_normal; > + > enum i915_cache_level; > + > /** > * A VMA represents a GEM BO that is bound into an address space. Therefore, a > * VMA's presence cannot be guaranteed before binding, or after unbinding the > @@ -129,6 +142,15 @@ struct i915_vma { > #define PTE_READ_ONLY (1<<2) > unsigned int bound : 4; > > + /** > + * Support different GGTT views into the same object. > + * This means there can be multiple VMA mappings per object and per VM. > + * i915_ggtt_view_type is used to distinguish between those entries. > + * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also > + * assumed in GEM functions which take no ggtt view parameter. > + */ > + struct i915_ggtt_view ggtt_view; > + > /** This object's place on the active/inactive lists */ > struct list_head mm_list; > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index c4536e1..f97479a 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err, > break; > > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->vm == vm && vma->pin_count > 0) { > + if (vma->vm == vm && vma->pin_count > 0) > capture_bo(err++, vma); > - break; > - } > } > > return err - first; > @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv, > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > list_for_each_entry(vma, &obj->vma_list, vma_link) > - if (vma->vm == vm && vma->pin_count > 0) { > + if (vma->vm == vm && vma->pin_count > 0) > i++; > - break; > - } > } > error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx]; >
On Tue, Dec 09, 2014 at 03:23:35PM +0000, Michel Thierry wrote: > On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote: > >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > >Things like reliable GGTT mappings and mirrored 2d-on-3d display will need > >to map objects into the same address space multiple times. > > > >Added a GGTT view concept and linked it with the VMA to distinguish between > >multiple instances per address space. > > > >New objects and GEM functions which do not take this new view as a parameter > >assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the > >previous behaviour. > > > >This now means that objects can have multiple VMA entries so the code which > >assumed there will only be one also had to be modified. > > > >Alternative GGTT views are supposed to borrow DMA addresses from obj->pages > >which is DMA mapped on first VMA instantiation and unmapped on the last one > >going away. > > > >v2: > > * Removed per view special casing in i915_gem_ggtt_prepare / > > finish_object in favour of creating and destroying DMA mappings > > on first VMA instantiation and last VMA destruction. (Daniel Vetter) > > * Simplified i915_vma_unbind which does not need to count the GGTT views. > > (Daniel Vetter) > > * Also moved obj->map_and_fenceable reset under the same check. > > * Checkpatch cleanups. > > > >v3: > > * Only retire objects once the last VMA is unbound. > > > >v4: > > * Keep scatter-gather table for alternative views persistent for the > > lifetime of the VMA. > > * Propagate binding errors to callers and handle appropriately. > > > >For: VIZ-4544 > >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >--- > > drivers/gpu/drm/i915/i915_debugfs.c | 5 +- > > drivers/gpu/drm/i915/i915_drv.h | 46 +++++++++++-- > > drivers/gpu/drm/i915/i915_gem.c | 101 ++++++++++++++++++----------- > > drivers/gpu/drm/i915/i915_gem_context.c | 11 +++- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 70 +++++++++++++++++--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 22 +++++++ > > drivers/gpu/drm/i915/i915_gpu_error.c | 8 +-- > > 8 files changed, 206 insertions(+), 66 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >index 6c16939..bd08289 100644 > >--- a/drivers/gpu/drm/i915/i915_debugfs.c > >+++ b/drivers/gpu/drm/i915/i915_debugfs.c > >@@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > > seq_puts(m, " (pp"); > > else > > seq_puts(m, " (g"); > >- seq_printf(m, "gtt offset: %08lx, size: %08lx)", > >- vma->node.start, vma->node.size); > >+ seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)", > >+ vma->node.start, vma->node.size, > >+ vma->ggtt_view.type); > > } > > if (obj->stolen) > > seq_printf(m, " (stolen: %08lx)", obj->stolen->start); > >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >index 049482f..b2f6f7d 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma); > > #define PIN_GLOBAL 0x4 > > #define PIN_OFFSET_BIAS 0x8 > > #define PIN_OFFSET_MASK (~4095) > >+int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj, > >+ struct i915_address_space *vm, > >+ uint32_t alignment, > >+ uint64_t flags, > >+ const struct i915_ggtt_view *view); > >+static inline > > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > > struct i915_address_space *vm, > > uint32_t alignment, > >- uint64_t flags); > >+ uint64_t flags) > >+{ > >+ return i915_gem_object_pin_view(obj, vm, alignment, flags, > >+ &i915_ggtt_view_normal); > >+} > >+ > >+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > >+ u32 flags); > > int __must_check i915_vma_unbind(struct i915_vma *vma); > > int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); > > void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); > >@@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, > > void i915_gem_restore_fences(struct drm_device *dev); > >+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, > >+ struct i915_address_space *vm, > >+ enum i915_ggtt_view_type view); > >+static inline > > unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, > >- struct i915_address_space *vm); > >+ struct i915_address_space *vm) > >+{ > >+ return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL); > >+} > > bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o); > > bool i915_gem_obj_bound(struct drm_i915_gem_object *o, > > struct i915_address_space *vm); > > unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, > > struct i915_address_space *vm); > >+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj, > >+ struct i915_address_space *vm, > >+ const struct i915_ggtt_view *view); > >+static inline > > struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, > >- struct i915_address_space *vm); > >+ struct i915_address_space *vm) > >+{ > >+ return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal); > >+} > >+ > >+struct i915_vma * > >+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj, > >+ struct i915_address_space *vm, > >+ const struct i915_ggtt_view *view); > >+ > >+static inline > > struct i915_vma * > > i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, > >- struct i915_address_space *vm); > >+ struct i915_address_space *vm) > >+{ > >+ return i915_gem_obj_lookup_or_create_vma_view(obj, vm, > >+ &i915_ggtt_view_normal); > >+} > > We also need a _vma_view version of i915_gem_obj_bound; > i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be > that the normal view is gone but a different view is still active (it is > also used in gpu_error and debug_fs, but I don't think it's a problem > there). Where did you see the need for the new obj_bound variant? Probably best to reply to the patch newly with just the relevant part quoted. -Daniel
On 12/10/2014 09:16 AM, Daniel Vetter wrote: > On Tue, Dec 09, 2014 at 03:23:35PM +0000, Michel Thierry wrote: >> We also need a _vma_view version of i915_gem_obj_bound; >> i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be >> that the normal view is gone but a different view is still active (it is >> also used in gpu_error and debug_fs, but I don't think it's a problem >> there). > > Where did you see the need for the new obj_bound variant? Probably best to > reply to the patch newly with just the relevant part quoted. It is not in the patch but in the i915_gem_object_ggtt_unpin. Which is: i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj) { struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); BUG_ON(!vma); BUG_ON(vma->pin_count == 0); BUG_ON(!i915_gem_obj_ggtt_bound(obj)); if (--vma->pin_count == 0) obj->pin_mappable = false; } The concern is the mismatch in semantics between i915_gem_obj_to_ggtt and i915_gem_obj_ggtt_bound. Former implies normal VMA while the latter hasn't been touched so it returns true on _any_ GGTT bound VMA. I don't think this BUG_ON can trigger since normal VMA exists by the virtue of BUG_ON(!vma), but I do agree that there is a mismatch in "documentation" (BUG_ONs). So making i915_gem_obj_ggtt_bound also imply a normal view would be correct it seems. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 6c16939..bd08289 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_puts(m, " (pp"); else seq_puts(m, " (g"); - seq_printf(m, "gtt offset: %08lx, size: %08lx)", - vma->node.start, vma->node.size); + seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)", + vma->node.start, vma->node.size, + vma->ggtt_view.type); } if (obj->stolen) seq_printf(m, " (stolen: %08lx)", obj->stolen->start); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 049482f..b2f6f7d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma); #define PIN_GLOBAL 0x4 #define PIN_OFFSET_BIAS 0x8 #define PIN_OFFSET_MASK (~4095) +int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + uint32_t alignment, + uint64_t flags, + const struct i915_ggtt_view *view); +static inline int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, struct i915_address_space *vm, uint32_t alignment, - uint64_t flags); + uint64_t flags) +{ + return i915_gem_object_pin_view(obj, vm, alignment, flags, + &i915_ggtt_view_normal); +} + +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, + u32 flags); int __must_check i915_vma_unbind(struct i915_vma *vma); int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); @@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev, void i915_gem_restore_fences(struct drm_device *dev); +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, + struct i915_address_space *vm, + enum i915_ggtt_view_type view); +static inline unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, - struct i915_address_space *vm); + struct i915_address_space *vm) +{ + return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL); +} bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o); bool i915_gem_obj_bound(struct drm_i915_gem_object *o, struct i915_address_space *vm); unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, struct i915_address_space *vm); +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + const struct i915_ggtt_view *view); +static inline struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, - struct i915_address_space *vm); + struct i915_address_space *vm) +{ + return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal); +} + +struct i915_vma * +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + const struct i915_ggtt_view *view); + +static inline struct i915_vma * i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, - struct i915_address_space *vm); + struct i915_address_space *vm) +{ + return i915_gem_obj_lookup_or_create_vma_view(obj, vm, + &i915_ggtt_view_normal); +} struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj); static inline bool i915_gem_obj_is_pinned(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 c1c1141..9244f0b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2087,8 +2087,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv, /* For the unbound phase, this should be a no-op! */ list_for_each_entry_safe(vma, v, &obj->vma_list, vma_link) - if (i915_vma_unbind(vma)) - break; + i915_vma_unbind(vma); if (i915_gem_object_put_pages(obj) == 0) count += obj->base.size >> PAGE_SHIFT; @@ -2297,17 +2296,14 @@ void i915_vma_move_to_active(struct i915_vma *vma, static void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) { - struct drm_i915_private *dev_priv = obj->base.dev->dev_private; - struct i915_address_space *vm; struct i915_vma *vma; BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS); BUG_ON(!obj->active); - list_for_each_entry(vm, &dev_priv->vm_list, global_link) { - vma = i915_gem_obj_to_vma(obj, vm); - if (vma && !list_empty(&vma->mm_list)) - list_move_tail(&vma->mm_list, &vm->inactive_list); + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (!list_empty(&vma->mm_list)) + list_move_tail(&vma->mm_list, &vma->vm->inactive_list); } intel_fb_obj_flush(obj, true); @@ -3062,10 +3058,8 @@ int i915_vma_unbind(struct i915_vma *vma) * cause memory corruption through use-after-free. */ - /* Throw away the active reference before moving to the unbound list */ - i915_gem_object_retire(obj); - - if (i915_is_ggtt(vma->vm)) { + if (i915_is_ggtt(vma->vm) && + vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { i915_gem_object_finish_gtt(obj); /* release the fence reg _after_ flushing */ @@ -3079,8 +3073,15 @@ int i915_vma_unbind(struct i915_vma *vma) vma->unbind_vma(vma); list_del_init(&vma->mm_list); - if (i915_is_ggtt(vma->vm)) - obj->map_and_fenceable = false; + if (i915_is_ggtt(vma->vm)) { + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { + obj->map_and_fenceable = false; + } else if (vma->ggtt_view.pages) { + sg_free_table(vma->ggtt_view.pages); + kfree(vma->ggtt_view.pages); + vma->ggtt_view.pages = NULL; + } + } drm_mm_remove_node(&vma->node); i915_gem_vma_destroy(vma); @@ -3088,6 +3089,10 @@ int i915_vma_unbind(struct i915_vma *vma) /* Since the unbound list is global, only move to that list if * no more VMAs exist. */ if (list_empty(&obj->vma_list)) { + /* Throw away the active reference before + * moving to the unbound list. */ + i915_gem_object_retire(obj); + i915_gem_gtt_finish_object(obj); list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); } @@ -3498,7 +3503,8 @@ static struct i915_vma * i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, struct i915_address_space *vm, unsigned alignment, - uint64_t flags) + uint64_t flags, + const struct i915_ggtt_view *view) { struct drm_device *dev = obj->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -3548,7 +3554,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, i915_gem_object_pin_pages(obj); - vma = i915_gem_obj_lookup_or_create_vma(obj, vm); + vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view); if (IS_ERR(vma)) goto err_unpin; @@ -3578,15 +3584,19 @@ search_free: if (ret) goto err_remove_node; + trace_i915_vma_bind(vma, flags); + ret = i915_vma_bind(vma, obj->cache_level, + flags & PIN_GLOBAL ? GLOBAL_BIND : 0); + if (ret) + goto err_finish_gtt; + list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); list_add_tail(&vma->mm_list, &vm->inactive_list); - trace_i915_vma_bind(vma, flags); - vma->bind_vma(vma, obj->cache_level, - flags & PIN_GLOBAL ? GLOBAL_BIND : 0); - return vma; +err_finish_gtt: + i915_gem_gtt_finish_object(obj); err_remove_node: drm_mm_remove_node(&vma->node); err_free_vma: @@ -3789,9 +3799,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, } list_for_each_entry(vma, &obj->vma_list, vma_link) - if (drm_mm_node_allocated(&vma->node)) - vma->bind_vma(vma, cache_level, - vma->bound & GLOBAL_BIND); + if (drm_mm_node_allocated(&vma->node)) { + ret = i915_vma_bind(vma, cache_level, + vma->bound & GLOBAL_BIND); + if (ret); + return ret; + } } list_for_each_entry(vma, &obj->vma_list, vma_link) @@ -4144,10 +4157,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags) } int -i915_gem_object_pin(struct drm_i915_gem_object *obj, - struct i915_address_space *vm, - uint32_t alignment, - uint64_t flags) +i915_gem_object_pin_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + uint32_t alignment, + uint64_t flags, + const struct i915_ggtt_view *view) { struct drm_i915_private *dev_priv = obj->base.dev->dev_private; struct i915_vma *vma; @@ -4163,7 +4177,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE)) return -EINVAL; - vma = i915_gem_obj_to_vma(obj, vm); + vma = i915_gem_obj_to_vma_view(obj, vm, view); if (vma) { if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT)) return -EBUSY; @@ -4173,7 +4187,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, "bo is already pinned with incorrect alignment:" " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d," " obj->map_and_fenceable=%d\n", - i915_gem_obj_offset(obj, vm), alignment, + i915_gem_obj_offset_view(obj, vm, view->type), + alignment, !!(flags & PIN_MAPPABLE), obj->map_and_fenceable); ret = i915_vma_unbind(vma); @@ -4186,13 +4201,17 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, bound = vma ? vma->bound : 0; if (vma == NULL || !drm_mm_node_allocated(&vma->node)) { - vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags); + vma = i915_gem_object_bind_to_vm(obj, vm, alignment, + flags, view); if (IS_ERR(vma)) return PTR_ERR(vma); } - if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); + if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) { + ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); + if (ret) + return ret; + } if ((bound ^ vma->bound) & GLOBAL_BIND) { bool mappable, fenceable; @@ -4528,12 +4547,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) intel_runtime_pm_put(dev_priv); } -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj, - struct i915_address_space *vm) +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + const struct i915_ggtt_view *view) { struct i915_vma *vma; list_for_each_entry(vma, &obj->vma_list, vma_link) - if (vma->vm == vm) + if (vma->vm == vm && vma->ggtt_view.type == view->type) return vma; return NULL; @@ -5147,8 +5167,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) } /* All the new VM stuff */ -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, - struct i915_address_space *vm) +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o, + struct i915_address_space *vm, + enum i915_ggtt_view_type view) { struct drm_i915_private *dev_priv = o->base.dev->dev_private; struct i915_vma *vma; @@ -5156,7 +5177,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base); list_for_each_entry(vma, &o->vma_list, vma_link) { - if (vma->vm == vm) + if (vma->vm == vm && vma->ggtt_view.type == view) return vma->node.start; } @@ -5307,7 +5328,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) struct i915_vma *vma; list_for_each_entry(vma, &obj->vma_list, vma_link) { - if (vma->vm == ggtt) + if (vma->vm != ggtt) + continue; + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) return vma; } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 5cd2b97..4ef8815 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -590,9 +590,14 @@ static int do_switch(struct intel_engine_cs *ring, goto unpin_out; vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state); - if (!(vma->bound & GLOBAL_BIND)) - vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, - GLOBAL_BIND); + if (!(vma->bound & GLOBAL_BIND)) { + ret = i915_vma_bind(vma, + to->legacy_hw_ctx.rcs_state->cache_level, + GLOBAL_BIND); + /* This shouldn't ever fail. */ + if (WARN_ONCE(ret, "GGTT context bind failed!")) + goto unpin_out; + } if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(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 0c25f62..3927d93 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -360,9 +360,12 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, * through the ppgtt for non_secure batchbuffers. */ if (unlikely(IS_GEN6(dev) && reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION && - !(target_vma->bound & GLOBAL_BIND))) - target_vma->bind_vma(target_vma, target_i915_obj->cache_level, - GLOBAL_BIND); + !(target_vma->bound & GLOBAL_BIND))) { + ret = i915_vma_bind(target_vma, target_i915_obj->cache_level, + GLOBAL_BIND); + if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!")) + return ret; + } /* Validate that the target is in a valid r/w GPU domain */ if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index ac03a38..73c1c0b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -30,6 +30,8 @@ #include "i915_trace.h" #include "intel_drv.h" +const struct i915_ggtt_view i915_ggtt_view_normal; + static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv); static void chv_setup_private_ppat(struct drm_i915_private *dev_priv); @@ -1341,9 +1343,12 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) /* The bind_vma code tries to be smart about tracking mappings. * Unfortunately above, we've just wiped out the mappings * without telling our object about it. So we need to fake it. + * + * Bind is not expected to fail since this is only called on + * resume and assumption is all requirements exist already. */ vma->bound &= ~GLOBAL_BIND; - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); + WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND)); } @@ -1538,7 +1543,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma, AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; BUG_ON(!i915_is_ggtt(vma->vm)); - intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags); + intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags); vma->bound = GLOBAL_BIND; } @@ -1588,7 +1593,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { if (!(vma->bound & GLOBAL_BIND) || (cache_level != obj->cache_level)) { - vma->vm->insert_entries(vma->vm, obj->pages, + vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages, vma->node.start, cache_level, flags); vma->bound |= GLOBAL_BIND; @@ -1600,7 +1605,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, (cache_level != obj->cache_level))) { struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt; appgtt->base.insert_entries(&appgtt->base, - vma->obj->pages, + vma->ggtt_view.pages, vma->node.start, cache_level, flags); vma->bound |= LOCAL_BIND; @@ -2165,7 +2170,8 @@ int i915_gem_gtt_init(struct drm_device *dev) } static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, - struct i915_address_space *vm) + struct i915_address_space *vm, + const struct i915_ggtt_view *view) { struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL); if (vma == NULL) @@ -2176,6 +2182,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, INIT_LIST_HEAD(&vma->exec_list); vma->vm = vm; vma->obj = obj; + vma->ggtt_view = *view; switch (INTEL_INFO(vm->dev)->gen) { case 9: @@ -2210,14 +2217,59 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj, } struct i915_vma * -i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj, - struct i915_address_space *vm) +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj, + struct i915_address_space *vm, + const struct i915_ggtt_view *view) { struct i915_vma *vma; - vma = i915_gem_obj_to_vma(obj, vm); + vma = i915_gem_obj_to_vma_view(obj, vm, view); if (!vma) - vma = __i915_gem_vma_create(obj, vm); + vma = __i915_gem_vma_create(obj, vm, view); return vma; } + +static inline +int i915_get_vma_pages(struct i915_vma *vma) +{ + if (vma->ggtt_view.pages) + return 0; + + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) + vma->ggtt_view.pages = vma->obj->pages; + else + WARN_ONCE(1, "GGTT view %u not implemented!\n", + vma->ggtt_view.type); + + if (!vma->ggtt_view.pages) { + DRM_ERROR("Failed to get pages for VMA view type %u!\n", + vma->ggtt_view.type); + return -EINVAL; + } + + return 0; +} + +/** + * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space. + * @vma: VMA to map + * @cache_level: mapping cache level + * @flags: flags like global or local mapping + * + * DMA addresses are taken from the scatter-gather table of this object (or of + * this VMA in case of non-default GGTT views) and PTE entries set up. + * Note that DMA addresses are also the only part of the SG table we care about. + */ +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, + u32 flags) +{ + int ret = i915_get_vma_pages(vma); + + if (ret) + return ret; + + vma->bind_vma(vma, cache_level, flags); + + return 0; +} diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index dd849df..e377c7d 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -109,7 +109,20 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; #define GEN8_PPAT_ELLC_OVERRIDE (0<<2) #define GEN8_PPAT(i, x) ((uint64_t) (x) << ((i) * 8)) +enum i915_ggtt_view_type { + I915_GGTT_VIEW_NORMAL = 0, +}; + +struct i915_ggtt_view { + enum i915_ggtt_view_type type; + + struct sg_table *pages; +}; + +extern const struct i915_ggtt_view i915_ggtt_view_normal; + enum i915_cache_level; + /** * A VMA represents a GEM BO that is bound into an address space. Therefore, a * VMA's presence cannot be guaranteed before binding, or after unbinding the @@ -129,6 +142,15 @@ struct i915_vma { #define PTE_READ_ONLY (1<<2) unsigned int bound : 4; + /** + * Support different GGTT views into the same object. + * This means there can be multiple VMA mappings per object and per VM. + * i915_ggtt_view_type is used to distinguish between those entries. + * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also + * assumed in GEM functions which take no ggtt view parameter. + */ + struct i915_ggtt_view ggtt_view; + /** This object's place on the active/inactive lists */ struct list_head mm_list; diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index c4536e1..f97479a 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err, break; list_for_each_entry(vma, &obj->vma_list, vma_link) - if (vma->vm == vm && vma->pin_count > 0) { + if (vma->vm == vm && vma->pin_count > 0) capture_bo(err++, vma); - break; - } } return err - first; @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv, list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { list_for_each_entry(vma, &obj->vma_list, vma_link) - if (vma->vm == vm && vma->pin_count > 0) { + if (vma->vm == vm && vma->pin_count > 0) i++; - break; - } } error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];