On Tue, Oct 07, 2014 at 06:11:00PM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
>
> The driver currently lets callers pin global, and then tries to do
> things correctly inside the function. Doing so has two downsides:
> 1. It's not possible to exclusively pin to a global, or an aliasing
> address space.
> 2. It's difficult to read, and understand.
>
> The eventual goal when realized should fix both of the issues. This patch
> which should have no functional impact begins to address these issues
> without intentionally breaking things.
>
> v2: Replace PIN_GLOBAL with PIN_ALIASING in _pin(). Copy paste error
>
> v3: Rebased/reworked with flag conflict from negative relocations
>
> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 14 ++++++++------
> drivers/gpu/drm/i915/i915_gem.c | 31 +++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 ++-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
> drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++++-
> 5 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3c725ec..6b60e90 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2396,11 +2396,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
> void i915_gem_free_object(struct drm_gem_object *obj);
> void i915_gem_vma_destroy(struct i915_vma *vma);
>
> -#define PIN_MAPPABLE 0x1
> -#define PIN_NONBLOCK 0x2
> -#define PIN_GLOBAL 0x4
> -#define PIN_OFFSET_BIAS 0x8
> -#define PIN_OFFSET_MASK (~4095)
> +#define PIN_MAPPABLE (1<<0)
> +#define PIN_NONBLOCK (1<<1)
> +#define PIN_GLOBAL (1<<2)
> +#define PIN_ALIASING (1<<3)
> +#define PIN_GLOBAL_ALIASED (PIN_ALIASING | PIN_GLOBAL)
> +#define PIN_OFFSET_BIAS (1<<4)
> +#define PIN_OFFSET_MASK (PAGE_MASK)
#define rename should be split out.
> int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> struct i915_address_space *vm,
> uint32_t alignment,
> @@ -2618,7 +2620,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> unsigned flags)
> {
> return i915_gem_object_pin(obj, i915_obj_to_ggtt(obj),
> - alignment, flags | PIN_GLOBAL);
> + alignment, flags | PIN_GLOBAL_ALIASED);
> }
>
> static inline int
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7745d22..dfb20e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3421,8 +3421,12 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> unsigned long end =
> flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
> struct i915_vma *vma;
> + u32 vma_bind_flags = 0;
> int ret;
>
> + if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> + flags |= PIN_GLOBAL;
> +
> fence_size = i915_gem_get_gtt_size(dev,
> obj->base.size,
> obj->tiling_mode);
> @@ -3508,9 +3512,11 @@ search_free:
>
> WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
>
> + if (flags & PIN_GLOBAL_ALIASED)
> + vma_bind_flags = GLOBAL_BIND | ALIASING_BIND;
> +
> trace_i915_vma_bind(vma, flags);
> - i915_gem_vma_bind(vma, obj->cache_level,
> - flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> + i915_gem_vma_bind(vma, obj->cache_level, vma_bind_flags);
>
> return vma;
>
> @@ -3716,9 +3722,14 @@ 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))
> - i915_gem_vma_bind(vma, cache_level,
> - obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
> + if (drm_mm_node_allocated(&vma->node)) {
> + u32 bind_flags = 0;
> + if (obj->has_global_gtt_mapping)
> + bind_flags |= GLOBAL_BIND;
> + if (obj->has_aliasing_ppgtt_mapping)
> + bind_flags |= ALIASING_BIND;
> + i915_gem_vma_bind(vma, cache_level, bind_flags);
We should have a vma_rebind function for use here and in the gtt restore
code.
> + }
> }
>
> list_for_each_entry(vma, &obj->vma_list, vma_link)
> @@ -4114,8 +4125,14 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> return PTR_ERR(vma);
> }
>
> - if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> - i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> + if (flags & PIN_GLOBAL_ALIASED) {
> + u32 bind_flags = 0;
> + if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> + bind_flags |= GLOBAL_BIND;
> + if (flags & PIN_ALIASING && !obj->has_aliasing_ppgtt_mapping)
> + bind_flags |= ALIASING_BIND;
> + i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
> + }
>
> vma->pin_count++;
> if (flags & PIN_MAPPABLE)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4564988..92191f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -362,7 +362,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> list_first_entry(&target_i915_obj->vma_list,
> typeof(*vma), vma_link);
> i915_gem_vma_bind(vma, target_i915_obj->cache_level,
> - GLOBAL_BIND);
> + GLOBAL_BIND | ALIASING_BIND);
If you read the comment above then it's clear we actually want a global
binding here specifically. Adding the aliasing_bind flag is confusing.
> }
>
> /* Validate that the target is in a valid r/w GPU domain */
> @@ -533,6 +533,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
> flags = 0;
> if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
> flags |= PIN_MAPPABLE;
> + /* FIXME: What kind of bind does Chris want? */
This is the place where the aliasing bind should have been. Presuming
follow-up patches indeed rework the binding then this will badly break
snb. Or any gen7 machine booted with i915.enable_ppgtt=1.
> if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
> flags |= PIN_GLOBAL;
> if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0c203f4..d725883 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1336,8 +1336,16 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> * Unfortunately above, we've just wiped out the mappings
> * without telling our object about it. So we need to fake it.
> */
> - obj->has_global_gtt_mapping = 0;
> - i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> + if (obj->has_global_gtt_mapping || obj->has_aliasing_ppgtt_mapping) {
> + u32 bind_flags = 0;
> + if (obj->has_global_gtt_mapping)
> + bind_flags |= GLOBAL_BIND;
> + if (obj->has_aliasing_ppgtt_mapping)
> + bind_flags |= ALIASING_BIND;
> + obj->has_global_gtt_mapping = 0;
> + obj->has_aliasing_ppgtt_mapping = 0;
> + i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
> + }
> }
>
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d5c14af..5fd7fa9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -155,8 +155,12 @@ 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. */
> +
> +/* Only use this if you know you want a strictly global binding */
> #define GLOBAL_BIND (1<<0)
> -#define PTE_READ_ONLY (1<<1)
> +/* Only use this if you know you want a strictly aliased binding */
> +#define ALIASING_BIND (1<<1)
> +#define PTE_READ_ONLY (1<<2)
> void (*bind_vma)(struct i915_vma *vma,
> enum i915_cache_level cache_level,
> u32 flags);
> --
> 2.0.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
