Re: [PATCH 17/17] drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma

Mika Kuoppala
2015-04-21T13:37:11Z

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> We have this neat abstraction between ppgtt and ggtt for (un)bind_vma
> and didn't end up using it really. What a shame, so fix this and make
> the ->bind_vma hook a bit more useful.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

This one I had problems with applying. But it looks simple enough.

Patches 13-17,
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

With 12/17 I don't yet understand all the implications so I left
that out.

-Mika

> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++++++++++++++++++++++---------------
> drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++---
> 2 files changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 458819b99a0b..763c17dd2118 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -92,6 +92,9 @@
> *
> */
>
> +static int
> +i915_get_ggtt_vma_pages(struct i915_vma *vma);
> +
> const struct i915_ggtt_view i915_ggtt_view_normal;
> const struct i915_ggtt_view i915_ggtt_view_rotated = {
> .type = I915_GGTT_VIEW_ROTATED
> @@ -143,9 +146,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
> return has_aliasing_ppgtt ? 1 : 0;
> }
>
> -static void ppgtt_bind_vma(struct i915_vma *vma,
> - enum i915_cache_level cache_level,
> - u32 unused)
> +static int ppgtt_bind_vma(struct i915_vma *vma,
> + enum i915_cache_level cache_level,
> + u32 unused)
> {
> u32 pte_flags = 0;
>
> @@ -155,6 +158,8 @@ static void ppgtt_bind_vma(struct i915_vma *vma,
>
> vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
> cache_level, pte_flags);
> +
> + return 0;
> }
>
> static void ppgtt_unbind_vma(struct i915_vma *vma)
> @@ -1898,22 +1903,26 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
> intel_gtt_clear_range(first_entry, num_entries);
> }
>
> -static void ggtt_bind_vma(struct i915_vma *vma,
> - enum i915_cache_level cache_level,
> - u32 flags)
> +static int ggtt_bind_vma(struct i915_vma *vma,
> + enum i915_cache_level cache_level,
> + u32 flags)
> {
> struct drm_device *dev = vma->vm->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *obj = vma->obj;
> struct sg_table *pages = obj->pages;
> u32 pte_flags = 0;
> + int ret;
> +
> + ret = i915_get_ggtt_vma_pages(vma);
> + if (ret)
> + return ret;
> + pages = vma->ggtt_view.pages;
>
> /* Currently applicable only to VLV */
> if (obj->gt_ro)
> pte_flags |= PTE_READ_ONLY;
>
> - if (i915_is_ggtt(vma->vm))
> - pages = vma->ggtt_view.pages;
>
> if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> vma->vm->insert_entries(vma->vm, pages,
> @@ -1929,6 +1938,8 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> vma->node.start,
> cache_level, pte_flags);
> }
> +
> + return 0;
> }
>
> static void ggtt_unbind_vma(struct i915_vma *vma)
> @@ -2749,7 +2760,7 @@ err_st_alloc:
> return ERR_PTR(ret);
> }
>
> -static inline int
> +static int
> i915_get_ggtt_vma_pages(struct i915_vma *vma)
> {
> int ret = 0;
> @@ -2793,8 +2804,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> u32 flags)
> {
> + int ret = 0;
> u32 bind_flags = 0;
> - int ret;
>
> if (vma->vm->allocate_va_range) {
> trace_i915_va_alloc(vma->vm, vma->node.start,
> @@ -2808,12 +2819,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> return ret;
> }
>
> - if (i915_is_ggtt(vma->vm)) {
> - ret = i915_get_ggtt_vma_pages(vma);
> - if (ret)
> - return 0;
> - }
> -
> if (flags & PIN_GLOBAL)
> bind_flags |= GLOBAL_BIND;
> if (flags & PIN_EXECBUF)
> @@ -2825,7 +2830,9 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> bind_flags &= ~vma->bound;
>
> if (bind_flags)
> - vma->vm->bind_vma(vma, cache_level, bind_flags);
> + ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
> + if (ret)
> + return ret;
>
> vma->bound |= bind_flags;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index fb0a04aa5363..4e6cac575cd8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -278,9 +278,9 @@ struct i915_address_space {
> * 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,
> - enum i915_cache_level cache_level,
> - u32 flags);
> + int (*bind_vma)(struct i915_vma *vma,
> + enum i915_cache_level cache_level,
> + u32 flags);
> };
>
> /* The Graphics Translation Table is the way in which GEN hardware translates a
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx