| Message ID | 1415284765-25613-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 06, 2014 at 02:39:25PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Things like reliable GGTT mmaps and mirrored 2d-on-3d display will need > to map objects into the same address space multiple times. > > This also means that objects now can have multiple VMA entries. > > Added a GGTT view concept and linked it with the VMA to distinguish betwen > 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. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Let's hope I've picked the latest version, not sure. Please either have in-patch changelogs or coverletters with what's new to help confused maintainers ;-) Looks good overal, just a few things around the lifetime rules for sg tables and related stuff that we've discussed offline. And one nit. I think with these addressed this is ready for detailed review and merging (also applies to the -internal series). Cheers, Daniel > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/i915_drv.h | 44 +++++++++++++-- > drivers/gpu/drm/i915/i915_gem.c | 88 ++++++++++++++++++------------ > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 71 ++++++++++++++++++------ > drivers/gpu/drm/i915/i915_gem_gtt.h | 29 +++++++++- > drivers/gpu/drm/i915/i915_gpu_error.c | 8 +-- > 8 files changed, 179 insertions(+), 69 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 0a69813..3f23c51 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -154,8 +154,8 @@ 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 54691bc..50109fa 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2420,10 +2420,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); > +} > + > +void 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); > @@ -2570,18 +2583,41 @@ 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 1de94cc..b46a0f7 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2007,8 +2007,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; > @@ -2209,17 +2208,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); > @@ -2930,6 +2926,21 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) > old_write_domain); > } > > +static unsigned int count_ggtt_mappings(struct drm_i915_gem_object *obj) > +{ > + struct i915_vma *vma; > + unsigned int count = 0; > + > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > + if (vma->vm != i915_obj_to_ggtt(obj)) > + continue; > + if (vma->bound & GLOBAL_BIND) > + count++; > + } > + > + return count; > +} > + > int i915_vma_unbind(struct i915_vma *vma) > { > struct drm_i915_gem_object *obj = vma->obj; > @@ -2960,7 +2971,7 @@ int i915_vma_unbind(struct i915_vma *vma) > /* 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) && count_ggtt_mappings(obj) == 1) { This should probably be a check for view == NORMAL instead of count_mappings == 1 - finish_gtt is about cpu writes through the gtt and so only applies to the linear/contiguous mapping. > i915_gem_object_finish_gtt(obj); > > /* release the fence reg _after_ flushing */ > @@ -2983,7 +2994,7 @@ 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)) { > - i915_gem_gtt_finish_object(obj); > + i915_gem_gtt_finish_object(obj, &vma->ggtt_view); > list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); > } > > @@ -3393,7 +3404,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; > @@ -3443,7 +3455,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; > > @@ -3469,15 +3481,16 @@ search_free: > goto err_remove_node; > } > > - ret = i915_gem_gtt_prepare_object(obj); > + ret = i915_gem_gtt_prepare_object(obj, &vma->ggtt_view); > if (ret) > goto err_remove_node; > > - list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > + 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, > + i915_vma_bind(vma, obj->cache_level, > flags & PIN_GLOBAL ? GLOBAL_BIND : 0); > > return vma; > @@ -3685,7 +3698,7 @@ 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, > + i915_vma_bind(vma, cache_level, > vma->bound & GLOBAL_BIND); > } > > @@ -4040,10 +4053,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; > @@ -4059,7 +4073,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; > @@ -4069,7 +4083,7 @@ 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); > @@ -4082,13 +4096,14 @@ 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); Nit: Please align continuations for paramter lists to the opening ( checkpatch --strict can check for that. > 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); > + i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); > > if ((bound ^ vma->bound) & GLOBAL_BIND) { > bool mappable, fenceable; > @@ -4502,12 +4517,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; > @@ -5191,8 +5207,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; > @@ -5200,7 +5217,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; > > } > @@ -5349,9 +5366,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) > { > struct i915_vma *vma; > > - vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link); > - if (vma->vm != i915_obj_to_ggtt(obj)) > - return NULL; > + list_for_each_entry(vma, &obj->vma_list, vma_link) { > + if (vma->vm != i915_obj_to_ggtt(obj)) > + continue; > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > + return vma; > + } > > - return vma; > + return NULL; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 7d32571..f815565 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -574,7 +574,7 @@ static int do_switch(struct intel_engine_cs *ring, > > 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, > + i915_vma_bind(vma, to->legacy_hw_ctx.rcs_state->cache_level, > GLOBAL_BIND); > > if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index e1ed85a..07e2872 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -358,7 +358,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, > 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, > + i915_vma_bind(target_vma, target_i915_obj->cache_level, > GLOBAL_BIND); > > /* Validate that the target is in a valid r/w GPU domain */ > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index de12017..3f8881c 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); > > @@ -71,7 +73,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > } > > > -static void ppgtt_bind_vma(struct i915_vma *vma, > +static void ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, > enum i915_cache_level cache_level, > u32 flags); > static void ppgtt_unbind_vma(struct i915_vma *vma); > @@ -1194,7 +1196,7 @@ void i915_ppgtt_release(struct kref *kref) > } > > static void > -ppgtt_bind_vma(struct i915_vma *vma, > +ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, > enum i915_cache_level cache_level, > u32 flags) > { > @@ -1202,7 +1204,7 @@ ppgtt_bind_vma(struct i915_vma *vma, > if (vma->obj->gt_ro) > flags |= PTE_READ_ONLY; > > - vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start, > + vma->vm->insert_entries(vma->vm, pages, vma->node.start, > cache_level, flags); > } > > @@ -1337,7 +1339,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > * without telling our object about it. So we need to fake it. > */ > vma->bound &= ~GLOBAL_BIND; > - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); > + i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); > } > > > @@ -1364,9 +1366,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > i915_ggtt_flush(dev_priv); > } > > -int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) > +int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view) > { > - if (obj->has_dma_mapping) > + if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL) This check and the one for finish_gtt are wrong. prepare/finish_gtt set up the dma_addr fields in the sg table (if available also the iommu mappings on big core). And that _must_ be done when the first view shows up, no matter which one it is. E.g. userspace might display a rotate buffer right aways without even rendering or writing into it (i.e. leaving it black). The change around finish_gtt is actually a leak since if the last view around is the rotate one (which can happen if userspace drops the buffer but leaves it displayed) we won't clean up (and so leak) the iommu mapping. A related comment which only applies to the internal patch which uses all this: For ->vma_bind you only need to fill in the dma addr, not the page pointer - the pte writing functions don't need more. Even more some object types (stolen and some dma-buf imports) don't have any page pointers assigned, and so the rotation code would oops since the page pointer is NULL. > return 0; > > if (!dma_map_sg(&obj->base.dev->pdev->dev, > @@ -1523,7 +1526,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, > } > > > -static void i915_ggtt_bind_vma(struct i915_vma *vma, > +static void i915_ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, > enum i915_cache_level cache_level, > u32 unused) > { > @@ -1532,7 +1535,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(pages, entry, flags); > vma->bound = GLOBAL_BIND; > } > > @@ -1556,7 +1559,7 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma) > intel_gtt_clear_range(first, size); > } > > -static void ggtt_bind_vma(struct i915_vma *vma, > +static void ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, > enum i915_cache_level cache_level, > u32 flags) > { > @@ -1582,7 +1585,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, pages, > vma->node.start, > cache_level, flags); > vma->bound |= GLOBAL_BIND; > @@ -1594,7 +1597,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, > + pages, > vma->node.start, > cache_level, flags); > vma->bound |= LOCAL_BIND; > @@ -1625,7 +1628,8 @@ static void ggtt_unbind_vma(struct i915_vma *vma) > } > } > > -void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) > +void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view) > { > struct drm_device *dev = obj->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -1633,7 +1637,7 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) > > interruptible = do_idling(dev_priv); > > - if (!obj->has_dma_mapping) > + if (!obj->has_dma_mapping && view->type == I915_GGTT_VIEW_NORMAL) > dma_unmap_sg(&dev->pdev->dev, > obj->pages->sgl, obj->pages->nents, > PCI_DMA_BIDIRECTIONAL); > @@ -2135,7 +2139,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) > @@ -2146,6 +2151,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: > @@ -2184,14 +2190,43 @@ 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 > +struct sg_table *i915_ggtt_view_pages(struct i915_vma *vma) > +{ > + struct sg_table *pages = ERR_PTR(-EINVAL); > + > + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) > + pages = vma->obj->pages; > + else > + WARN(1, "Not implemented!\n"); > + > + return pages; > +} > + > +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > + u32 flags) > +{ > + struct sg_table *pages = i915_ggtt_view_pages(vma); > + > + if (pages && !IS_ERR(pages)) { > + vma->bind_vma(vma, pages, cache_level, flags); > + > + if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) { > + sg_free_table(pages); > + kfree(pages); > + } > + } > +} > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index d0562d0..db6ac60 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -109,7 +109,18 @@ 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; > +}; > + > +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 +140,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; > > @@ -161,7 +181,7 @@ struct i915_vma { > * setting the valid PTE entries to a reserved scratch page. */ > void (*unbind_vma)(struct i915_vma *vma); > /* Map an object into an address space with the given cache flags. */ > - void (*bind_vma)(struct i915_vma *vma, > + void (*bind_vma)(struct i915_vma *vma, struct sg_table *pages, > enum i915_cache_level cache_level, > u32 flags); > }; > @@ -299,7 +319,10 @@ void i915_check_and_clear_faults(struct drm_device *dev); > void i915_gem_suspend_gtt_mappings(struct drm_device *dev); > void i915_gem_restore_gtt_mappings(struct drm_device *dev); > > -int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj); > -void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); > +int __must_check > +i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view); > +void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj, > + const struct i915_ggtt_view *view); > > #endif > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index d17360b..8ebe45b 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -717,10 +717,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 11/12/2014 05:02 PM, Daniel Vetter wrote: > On Thu, Nov 06, 2014 at 02:39:25PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Things like reliable GGTT mmaps and mirrored 2d-on-3d display will need >> to map objects into the same address space multiple times. >> >> This also means that objects now can have multiple VMA entries. >> >> Added a GGTT view concept and linked it with the VMA to distinguish betwen >> 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. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Let's hope I've picked the latest version, not sure. Please either have > in-patch changelogs or coverletters with what's new to help confused > maintainers ;-) I had a cover letter and version when it was a patch series _and_ the concept split into multiple VMA and GGTT view. Once only one patch remained from the patch series and it was redesigned to kind of merge two concepts into one simplified approach I did not think cover letter makes sense any more. Will definitely version as this design goes forward. > Looks good overal, just a few things around the lifetime rules for sg > tables and related stuff that we've discussed offline. And one nit. > > I think with these addressed this is ready for detailed review and merging > (also applies to the -internal series). Excellent, thank you! Just one question below: >> -int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) >> +int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, >> + const struct i915_ggtt_view *view) >> { >> - if (obj->has_dma_mapping) >> + if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL) > > This check and the one for finish_gtt are wrong. prepare/finish_gtt set up > the dma_addr fields in the sg table (if available also the iommu mappings > on big core). And that _must_ be done when the first view shows up, no > matter which one it is. E.g. userspace might display a rotate buffer right > aways without even rendering or writing into it (i.e. leaving it black). The plan was to ensure that the "normal" view is always there first. Otherwise we can't "steal" DMA addresses and the other views do not have a proper SG table to generate DMA addresses from. So the internal code was doing that. Am I missing something? > The change around finish_gtt is actually a leak since if the last view > around is the rotate one (which can happen if userspace drops the buffer > but leaves it displayed) we won't clean up (and so leak) the iommu > mapping. So I somehow need to ensure finish_gtt on a "normal" view is done last, correct? Move it from VMA destructon to object destruction? Would this have any implications for the shrinker? Thanks, Tvrtko
On Mon, Nov 24, 2014 at 12:32:12PM +0000, Tvrtko Ursulin wrote: > > On 11/12/2014 05:02 PM, Daniel Vetter wrote: > >The change around finish_gtt is actually a leak since if the last view > >around is the rotate one (which can happen if userspace drops the buffer > >but leaves it displayed) we won't clean up (and so leak) the iommu > >mapping. > > So I somehow need to ensure finish_gtt on a "normal" view is done > last, correct? Move it from VMA destructon to object destruction? > Would this have any implications for the shrinker? Just have a obj->dma_pin_count? -Chris
On Mon, Nov 24, 2014 at 12:32:12PM +0000, Tvrtko Ursulin wrote: > > On 11/12/2014 05:02 PM, Daniel Vetter wrote: > >On Thu, Nov 06, 2014 at 02:39:25PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Things like reliable GGTT mmaps and mirrored 2d-on-3d display will need > >>to map objects into the same address space multiple times. > >> > >>This also means that objects now can have multiple VMA entries. > >> > >>Added a GGTT view concept and linked it with the VMA to distinguish betwen > >>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. > >> > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > >Let's hope I've picked the latest version, not sure. Please either have > >in-patch changelogs or coverletters with what's new to help confused > >maintainers ;-) > > I had a cover letter and version when it was a patch series _and_ the > concept split into multiple VMA and GGTT view. Once only one patch remained > from the patch series and it was redesigned to kind of merge two concepts > into one simplified approach I did not think cover letter makes sense any > more. Will definitely version as this design goes forward. > > >Looks good overal, just a few things around the lifetime rules for sg > >tables and related stuff that we've discussed offline. And one nit. > > > >I think with these addressed this is ready for detailed review and merging > >(also applies to the -internal series). > > Excellent, thank you! Just one question below: > > >>-int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) > >>+int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, > >>+ const struct i915_ggtt_view *view) > >> { > >>- if (obj->has_dma_mapping) > >>+ if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL) > > > >This check and the one for finish_gtt are wrong. prepare/finish_gtt set up > >the dma_addr fields in the sg table (if available also the iommu mappings > >on big core). And that _must_ be done when the first view shows up, no > >matter which one it is. E.g. userspace might display a rotate buffer right > >aways without even rendering or writing into it (i.e. leaving it black). > > The plan was to ensure that the "normal" view is always there first. > Otherwise we can't "steal" DMA addresses and the other views do not have a > proper SG table to generate DMA addresses from. So the internal code was > doing that. Am I missing something? > > >The change around finish_gtt is actually a leak since if the last view > >around is the rotate one (which can happen if userspace drops the buffer > >but leaves it displayed) we won't clean up (and so leak) the iommu > >mapping. > > So I somehow need to ensure finish_gtt on a "normal" view is done last, > correct? Move it from VMA destructon to object destruction? Would this have > any implications for the shrinker? Normal or not is irrelevant, all we need is _one_ vma to ensure that the dma mapping is around. Essentially this amounts to what Chris suggested, except that we already implicitly track dma_pin_count as dma_pin_count = 0; for_each_entry(obj->vma_list, entry) dma_pin_count++; The difference is that we only care about 0->1 and 1->0 transitions, so a list_empty check is all we really need. And since it doesn't matter whether we check the head node or a member of the list to check emptiness we can just look at list_empty(&vma->vma_list); in the vma_bind/unbind code to decide whether we should set up or destroy the dma mapping. Again the type of view the vma represents (normal ggtt, ppgtt, rotate or anything else really) is completely irrelevant. I hope that explains the confusion here a bit. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 0a69813..3f23c51 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -154,8 +154,8 @@ 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 54691bc..50109fa 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2420,10 +2420,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); +} + +void 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); @@ -2570,18 +2583,41 @@ 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 1de94cc..b46a0f7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2007,8 +2007,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; @@ -2209,17 +2208,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); @@ -2930,6 +2926,21 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) old_write_domain); } +static unsigned int count_ggtt_mappings(struct drm_i915_gem_object *obj) +{ + struct i915_vma *vma; + unsigned int count = 0; + + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (vma->vm != i915_obj_to_ggtt(obj)) + continue; + if (vma->bound & GLOBAL_BIND) + count++; + } + + return count; +} + int i915_vma_unbind(struct i915_vma *vma) { struct drm_i915_gem_object *obj = vma->obj; @@ -2960,7 +2971,7 @@ int i915_vma_unbind(struct i915_vma *vma) /* 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) && count_ggtt_mappings(obj) == 1) { i915_gem_object_finish_gtt(obj); /* release the fence reg _after_ flushing */ @@ -2983,7 +2994,7 @@ 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)) { - i915_gem_gtt_finish_object(obj); + i915_gem_gtt_finish_object(obj, &vma->ggtt_view); list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); } @@ -3393,7 +3404,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; @@ -3443,7 +3455,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; @@ -3469,15 +3481,16 @@ search_free: goto err_remove_node; } - ret = i915_gem_gtt_prepare_object(obj); + ret = i915_gem_gtt_prepare_object(obj, &vma->ggtt_view); if (ret) goto err_remove_node; - list_move_tail(&obj->global_list, &dev_priv->mm.bound_list); + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) + 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, + i915_vma_bind(vma, obj->cache_level, flags & PIN_GLOBAL ? GLOBAL_BIND : 0); return vma; @@ -3685,7 +3698,7 @@ 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, + i915_vma_bind(vma, cache_level, vma->bound & GLOBAL_BIND); } @@ -4040,10 +4053,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; @@ -4059,7 +4073,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; @@ -4069,7 +4083,7 @@ 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); @@ -4082,13 +4096,14 @@ 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); + i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); if ((bound ^ vma->bound) & GLOBAL_BIND) { bool mappable, fenceable; @@ -4502,12 +4517,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; @@ -5191,8 +5207,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; @@ -5200,7 +5217,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; } @@ -5349,9 +5366,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj) { struct i915_vma *vma; - vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link); - if (vma->vm != i915_obj_to_ggtt(obj)) - return NULL; + list_for_each_entry(vma, &obj->vma_list, vma_link) { + if (vma->vm != i915_obj_to_ggtt(obj)) + continue; + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) + return vma; + } - return vma; + return NULL; } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 7d32571..f815565 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -574,7 +574,7 @@ static int do_switch(struct intel_engine_cs *ring, 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, + i915_vma_bind(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND); if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index e1ed85a..07e2872 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -358,7 +358,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, 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, + i915_vma_bind(target_vma, target_i915_obj->cache_level, GLOBAL_BIND); /* Validate that the target is in a valid r/w GPU domain */ diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index de12017..3f8881c 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); @@ -71,7 +73,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) } -static void ppgtt_bind_vma(struct i915_vma *vma, +static void ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, enum i915_cache_level cache_level, u32 flags); static void ppgtt_unbind_vma(struct i915_vma *vma); @@ -1194,7 +1196,7 @@ void i915_ppgtt_release(struct kref *kref) } static void -ppgtt_bind_vma(struct i915_vma *vma, +ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, enum i915_cache_level cache_level, u32 flags) { @@ -1202,7 +1204,7 @@ ppgtt_bind_vma(struct i915_vma *vma, if (vma->obj->gt_ro) flags |= PTE_READ_ONLY; - vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start, + vma->vm->insert_entries(vma->vm, pages, vma->node.start, cache_level, flags); } @@ -1337,7 +1339,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) * without telling our object about it. So we need to fake it. */ vma->bound &= ~GLOBAL_BIND; - vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND); + i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND); } @@ -1364,9 +1366,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) i915_ggtt_flush(dev_priv); } -int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) +int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view) { - if (obj->has_dma_mapping) + if (obj->has_dma_mapping || view->type != I915_GGTT_VIEW_NORMAL) return 0; if (!dma_map_sg(&obj->base.dev->pdev->dev, @@ -1523,7 +1526,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, } -static void i915_ggtt_bind_vma(struct i915_vma *vma, +static void i915_ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, enum i915_cache_level cache_level, u32 unused) { @@ -1532,7 +1535,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(pages, entry, flags); vma->bound = GLOBAL_BIND; } @@ -1556,7 +1559,7 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma) intel_gtt_clear_range(first, size); } -static void ggtt_bind_vma(struct i915_vma *vma, +static void ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages, enum i915_cache_level cache_level, u32 flags) { @@ -1582,7 +1585,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, pages, vma->node.start, cache_level, flags); vma->bound |= GLOBAL_BIND; @@ -1594,7 +1597,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, + pages, vma->node.start, cache_level, flags); vma->bound |= LOCAL_BIND; @@ -1625,7 +1628,8 @@ static void ggtt_unbind_vma(struct i915_vma *vma) } } -void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) +void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view) { struct drm_device *dev = obj->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -1633,7 +1637,7 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj) interruptible = do_idling(dev_priv); - if (!obj->has_dma_mapping) + if (!obj->has_dma_mapping && view->type == I915_GGTT_VIEW_NORMAL) dma_unmap_sg(&dev->pdev->dev, obj->pages->sgl, obj->pages->nents, PCI_DMA_BIDIRECTIONAL); @@ -2135,7 +2139,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) @@ -2146,6 +2151,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: @@ -2184,14 +2190,43 @@ 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 +struct sg_table *i915_ggtt_view_pages(struct i915_vma *vma) +{ + struct sg_table *pages = ERR_PTR(-EINVAL); + + if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) + pages = vma->obj->pages; + else + WARN(1, "Not implemented!\n"); + + return pages; +} + +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, + u32 flags) +{ + struct sg_table *pages = i915_ggtt_view_pages(vma); + + if (pages && !IS_ERR(pages)) { + vma->bind_vma(vma, pages, cache_level, flags); + + if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) { + sg_free_table(pages); + kfree(pages); + } + } +} diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index d0562d0..db6ac60 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -109,7 +109,18 @@ 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; +}; + +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 +140,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; @@ -161,7 +181,7 @@ struct i915_vma { * setting the valid PTE entries to a reserved scratch page. */ void (*unbind_vma)(struct i915_vma *vma); /* Map an object into an address space with the given cache flags. */ - void (*bind_vma)(struct i915_vma *vma, + void (*bind_vma)(struct i915_vma *vma, struct sg_table *pages, enum i915_cache_level cache_level, u32 flags); }; @@ -299,7 +319,10 @@ void i915_check_and_clear_faults(struct drm_device *dev); void i915_gem_suspend_gtt_mappings(struct drm_device *dev); void i915_gem_restore_gtt_mappings(struct drm_device *dev); -int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj); -void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); +int __must_check +i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view); +void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj, + const struct i915_ggtt_view *view); #endif diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index d17360b..8ebe45b 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -717,10 +717,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];