[Intel-gfx] [PATCH] drm/i915: Do not use ggtt_view with (aliasing) PPGTT
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Mar 10 09:43:40 PDT 2015
Hi,
On the high level I don't like the explosion of _ppgtt_ functions and
the name in the first place. As Daniel commented a lot of those (if not
all) should not care between ppgtt and ggtt.
I do like the goal of having _ggtt_ versions which take the view, and
normal/non-suffixed/legacy ones which work with any VMA (ppgtt/ggtt) and
assume normal view in the ggtt case.
If that basically means just renaming _view to _ggtt (more or less) then
it is not that great, but if you can get some more
unification/clarification/consistency in the process then it is good.
And I am not sure that there is place for both _ggtt_ and _view prefix
in the final goal - that looks even more messy.
How many places would you have to change for partial views if you didn't
do this cleanup first? If not too much maybe do that first and then see
if there is scope for a cleanup?
Regards,
Tvrtko
On 03/09/2015 02:34 PM, Joonas Lahtinen wrote:
> GGTT views are only applicable when dealing with GGTT. Change the code to
> reject ggtt_view where it should not be used and require it when it should
> be.
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 91 ++++++++++++------------
> drivers/gpu/drm/i915/i915_gem.c | 134 ++++++++++++++++++++++++++++--------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++++++++---
> 3 files changed, 184 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dd7651b..6bc67fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2608,20 +2608,16 @@ 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)
> -{
> - return i915_gem_object_pin_view(obj, vm, alignment, flags,
> - &i915_ggtt_view_normal);
> -}
> +int __must_check
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + uint32_t alignment,
> + uint64_t flags);
> +int __must_check
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view,
> + uint32_t alignment,
> + uint64_t flags);
>
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> u32 flags);
> @@ -2785,41 +2781,55 @@ 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)
> +static inline bool i915_is_ggtt(struct i915_address_space *vm);
> +
> +unsigned long
> +i915_gem_obj_ppgtt_offset(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm);
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view);
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> {
> - return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> + return i915_gem_obj_ggtt_offset_view(o, I915_GGTT_VIEW_NORMAL);
> }
> -bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view);
> -static inline
> -bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm)
> +
> +static inline unsigned long
> +i915_gem_obj_offset(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm)
> {
> - return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
> + if (i915_is_ggtt(vm))
> + return i915_gem_obj_ggtt_offset(o);
> + return i915_gem_obj_ppgtt_offset(o, vm);
> }
>
> +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);
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view);
> +
> 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,
> +struct i915_vma *i915_gem_obj_to_ppgtt_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +struct i915_vma *i915_gem_obj_to_ggtt_vma(struct drm_i915_gem_object *obj,
> 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)
> {
> - return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> + if (i915_is_ggtt(vm))
> + return i915_gem_obj_to_ggtt_vma(obj, &i915_ggtt_view_normal);
> + return i915_gem_obj_to_ppgtt_vma(obj, vm);
> }
>
> struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> +i915_gem_obj_lookup_or_create_ppgtt_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm);
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view);
>
> static inline
> @@ -2827,8 +2837,9 @@ struct i915_vma *
> i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> struct i915_address_space *vm)
> {
> - return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> - &i915_ggtt_view_normal);
> + if (i915_is_ggtt(vm))
> + return i915_gem_obj_lookup_or_create_ggtt_vma(obj, &i915_ggtt_view_normal);
> + return i915_gem_obj_lookup_or_create_ppgtt_vma(obj, vm);
> }
>
> struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
> @@ -2861,13 +2872,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
>
> static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> {
> - return i915_gem_obj_bound(obj, i915_obj_to_ggtt(obj));
> -}
> -
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *obj)
> -{
> - return i915_gem_obj_offset(obj, i915_obj_to_ggtt(obj));
> + return i915_gem_obj_ggtt_bound_view(obj, I915_GGTT_VIEW_NORMAL);
> }
>
> static inline unsigned long
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3831cc0..d6edbef 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3517,9 +3517,9 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
> static struct i915_vma *
> i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view,
> unsigned alignment,
> - uint64_t flags,
> - const struct i915_ggtt_view *view)
> + uint64_t flags)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3531,6 +3531,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> struct i915_vma *vma;
> int ret;
>
> + if(WARN_ON(!i915_is_ggtt(vm) ^ !!ggtt_view))
> + return ERR_PTR(-EINVAL);
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3569,7 +3572,11 @@ 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_view(obj, vm, view);
> + if (i915_is_ggtt(vm))
> + vma = i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view);
> + else
> + vma = i915_gem_obj_lookup_or_create_ppgtt_vma(obj, vm);
> +
> if (IS_ERR(vma))
> goto err_unpin;
>
> @@ -4166,14 +4173,15 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
> return false;
> }
>
> -int
> -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 int
> +i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view,
> + uint32_t alignment,
> + uint64_t flags)
> {
> struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> + const bool is_ggtt = i915_is_ggtt(vm);
> struct i915_vma *vma;
> unsigned bound;
> int ret;
> @@ -4181,23 +4189,32 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> if (WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base))
> return -ENODEV;
>
> - if (WARN_ON(flags & (PIN_GLOBAL | PIN_MAPPABLE) && !i915_is_ggtt(vm)))
> + if (WARN_ON(flags & (PIN_GLOBAL | PIN_MAPPABLE)))
> return -EINVAL;
>
> if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> return -EINVAL;
>
> - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> + if (WARN_ON(!is_ggtt ^ !!ggtt_view))
> + return -EINVAL;
> +
> + vma = is_ggtt ? i915_gem_obj_to_ggtt_vma(obj, ggtt_view) :
> + i915_gem_obj_to_ppgtt_vma(obj, vm);
> +
> if (vma) {
> if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> return -EBUSY;
>
> if (i915_vma_misplaced(vma, alignment, flags)) {
> + unsigned long offset;
> + offset = is_ggtt ? i915_gem_obj_ggtt_offset_view(obj, ggtt_view->type) :
> + i915_gem_obj_ppgtt_offset(obj, vm);
> WARN(vma->pin_count,
> - "bo is already pinned with incorrect alignment:"
> + "bo is already pinned in %s with incorrect alignment:"
> " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
> " obj->map_and_fenceable=%d\n",
> - i915_gem_obj_offset_view(obj, vm, view->type),
> + is_ggtt ? "ggtt" : "ppgtt",
> + offset,
> alignment,
> !!(flags & PIN_MAPPABLE),
> obj->map_and_fenceable);
> @@ -4211,8 +4228,8 @@ i915_gem_object_pin_view(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, view);
> + vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
> + flags);
> if (IS_ERR(vma))
> return PTR_ERR(vma);
> }
> @@ -4253,6 +4270,27 @@ i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> return 0;
> }
>
> +int
> +i915_gem_object_pin(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + uint32_t alignment,
> + uint64_t flags)
> +{
> + const struct i915_ggtt_view *ggtt_view;
> + ggtt_view = i915_is_ggtt(vm) ? &i915_ggtt_view_normal : NULL;
> + return i915_gem_object_do_pin(obj, vm, ggtt_view, alignment, flags);
> +}
> +
> +int
> +i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *ggtt_view,
> + uint32_t alignment,
> + uint64_t flags)
> +{
> + return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), ggtt_view,
> + alignment, flags);
> +}
> +
> void
> i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
> {
> @@ -4558,15 +4596,24 @@ 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_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view)
> +struct i915_vma *i915_gem_obj_to_ppgtt_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> {
> struct i915_vma *vma;
> list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (vma->vm == vm && vma->ggtt_view.type == view->type)
> + if (vma->vm == vm)
> return vma;
> + return NULL;
> +}
>
> +struct i915_vma *i915_gem_obj_to_ggtt_vma(struct drm_i915_gem_object *obj,
> + const struct i915_ggtt_view *view)
> +{
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> + struct i915_vma *vma;
> + list_for_each_entry(vma, &obj->vma_list, vma_link)
> + if (vma->vm == ggtt && vma->ggtt_view.type == view->type)
> + return vma;
> return NULL;
> }
>
> @@ -5161,33 +5208,62 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
> }
>
> /* All the new VM stuff */
> -unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view)
> +unsigned long
> +i915_gem_obj_ppgtt_offset(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm)
> {
> struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> struct i915_vma *vma;
>
> + BUG_ON(vm == i915_obj_to_ggtt(o));
> WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
>
> list_for_each_entry(vma, &o->vma_list, vma_link) {
> - if (vma->vm == vm && vma->ggtt_view.type == view)
> + if (vma->vm == vm)
> return vma->node.start;
> + }
> +
> + WARN(1, "ppgtt vma for this object not found.\n");
> + return -1;
> +}
> +
> +unsigned long
> +i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view)
> +{
> + struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> + struct i915_vma *vma;
>
> + list_for_each_entry(vma, &o->vma_list, vma_link) {
> + if (vma->vm == ggtt && vma->ggtt_view.type == view)
> + return vma->node.start;
> }
> - WARN(1, "%s vma for this object not found.\n",
> - i915_is_ggtt(vm) ? "global" : "ppgtt");
> +
> + WARN(1, "global vma for this object not found.\n");
> return -1;
> }
>
> -bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
> - struct i915_address_space *vm,
> - enum i915_ggtt_view_type view)
> +bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> + struct i915_address_space *vm)
> {
> struct i915_vma *vma;
>
> list_for_each_entry(vma, &o->vma_list, vma_link)
> - if (vma->vm == vm &&
> + if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
> + return true;
> +
> + return false;
> +}
> +
> +bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
> + enum i915_ggtt_view_type view)
> +{
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
> + struct i915_vma *vma;
> +
> + list_for_each_entry(vma, &o->vma_list, vma_link)
> + if (vma->vm == ggtt &&
> vma->ggtt_view.type == view &&
> drm_mm_node_allocated(&vma->node))
> return true;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 2034f7c..eb46804 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1757,7 +1757,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->ggtt_view.pages,
> + vma->obj->pages,
> vma->node.start,
> cache_level, flags);
> vma->bound |= LOCAL_BIND;
> @@ -2331,9 +2331,10 @@ int i915_gem_gtt_init(struct drm_device *dev)
> return 0;
> }
>
> -static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> - const struct i915_ggtt_view *view)
> +static struct i915_vma *
> +__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm,
> + const struct i915_ggtt_view *ggtt_view)
> {
> struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> if (vma == NULL)
> @@ -2344,18 +2345,23 @@ 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;
>
> if (INTEL_INFO(vm->dev)->gen >= 6) {
> if (i915_is_ggtt(vm)) {
> + BUG_ON(!ggtt_view);
> + vma->ggtt_view = *ggtt_view;
> +
> vma->unbind_vma = ggtt_unbind_vma;
> vma->bind_vma = ggtt_bind_vma;
> } else {
> + BUG_ON(ggtt_view);
> vma->unbind_vma = ppgtt_unbind_vma;
> vma->bind_vma = ppgtt_bind_vma;
> }
> } else {
> BUG_ON(!i915_is_ggtt(vm));
> + BUG_ON(!ggtt_view);
> + vma->ggtt_view = *ggtt_view;
> vma->unbind_vma = i915_ggtt_unbind_vma;
> vma->bind_vma = i915_ggtt_bind_vma;
> }
> @@ -2368,23 +2374,38 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> }
>
> struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> - struct i915_address_space *vm,
> +i915_gem_obj_lookup_or_create_ppgtt_vma(struct drm_i915_gem_object *obj,
> + struct i915_address_space *vm)
> +{
> + struct i915_vma *vma;
> +
> + vma = i915_gem_obj_to_ppgtt_vma(obj, vm);
> + if (!vma)
> + vma = __i915_gem_vma_create(obj, vm, NULL);
> +
> + return vma;
> +}
> +
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_ggtt_vma(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view)
> {
> + struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
> struct i915_vma *vma;
>
> - vma = i915_gem_obj_to_vma_view(obj, vm, view);
> + vma = i915_gem_obj_to_ggtt_vma(obj, view);
> if (!vma)
> - vma = __i915_gem_vma_create(obj, vm, view);
> + vma = __i915_gem_vma_create(obj, ggtt, view);
>
> return vma;
> +
> }
>
> +
> static inline
> int i915_get_vma_pages(struct i915_vma *vma)
> {
> - if (vma->ggtt_view.pages)
> + if (!i915_is_ggtt(vma->vm) || vma->ggtt_view.pages)
> return 0;
>
> if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>
More information about the Intel-gfx
mailing list