On Thu, Dec 07, 2017 at 04:27:17PM +0000, Chris Wilson wrote:
> In quite a few places, we have a list iteration over the vma on an
> object that only want to inspect GGTT vma. By construction, these are
> placed at the start of the list, so we have copied that knowledge into
> many callsites. Pull that knowledge back to i915_vma.h and provide a
> for_each_ggtt_vma() to tidy up the code.
>
> Suggested-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem.c | 18 ++++--------------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +----
> drivers/gpu/drm/i915/i915_gem_tiling.c | 10 ++--------
> drivers/gpu/drm/i915/i915_vma.h | 14 +++++++++++++-
> 5 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 28294470ae31..7b41a1799a03 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -111,8 +111,8 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
> u64 size = 0;
> struct i915_vma *vma;
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (i915_vma_is_ggtt(vma) && drm_mm_node_allocated(&vma->node))
> + for_each_ggtt_vma(vma, obj) {
> + if (drm_mm_node_allocated(&vma->node))
> size += vma->node.size;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 67dc11effc8e..c7b5db78fbb4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -714,10 +714,7 @@ flush_write_domain(struct drm_i915_gem_object *obj, unsigned int flush_domains)
> intel_fb_obj_flush(obj,
> fb_write_origin(obj, I915_GEM_DOMAIN_GTT));
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!i915_vma_is_ggtt(vma))
> - break;
> -
> + for_each_ggtt_vma(vma, obj) {
> if (vma->iomap)
> continue;
>
> @@ -1569,10 +1566,7 @@ static void i915_gem_object_bump_inactive_ggtt(struct drm_i915_gem_object *obj)
>
> GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!i915_vma_is_ggtt(vma))
> - break;
> -
> + for_each_ggtt_vma(vma, obj) {
> if (i915_vma_is_active(vma))
> continue;
>
> @@ -2051,12 +2045,8 @@ static void __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> drm_vma_node_unmap(&obj->base.vma_node,
> obj->base.dev->anon_inode->i_mapping);
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!i915_vma_is_ggtt(vma))
> - break;
> -
> + for_each_ggtt_vma(vma, obj)
> i915_vma_unset_userfault(vma);
> - }
> }
>
> /**
> @@ -3822,7 +3812,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> * dropped the fence as all snoopable access is
> * supposed to be linear.
> */
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> + for_each_ggtt_vma(vma, obj) {
> ret = i915_vma_put_fence(vma);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1faea9a17b5a..d701b79b6319 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3623,10 +3623,7 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
> bool ggtt_bound = false;
> struct i915_vma *vma;
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (vma->vm != &ggtt->base)
> - continue;
> -
> + for_each_ggtt_vma(vma, obj) {
> if (!i915_vma_unbind(vma))
> continue;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index b85d7ebd9bee..d9dc9df523b5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -205,10 +205,7 @@ i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj,
> if (tiling_mode == I915_TILING_NONE)
> return 0;
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!i915_vma_is_ggtt(vma))
> - break;
> -
> + for_each_ggtt_vma(vma, obj) {
> if (i915_vma_fence_prepare(vma, tiling_mode, stride))
> continue;
>
> @@ -285,10 +282,7 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
> }
> mutex_unlock(&obj->mm.lock);
>
> - list_for_each_entry(vma, &obj->vma_list, obj_link) {
> - if (!i915_vma_is_ggtt(vma))
> - break;
> -
> + for_each_ggtt_vma(vma, obj) {
> vma->fence_size =
> i915_gem_fence_size(i915, vma->size, tiling, stride);
> vma->fence_alignment =
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index f636243eb8f7..d58da80c0dd2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -408,5 +408,17 @@ i915_vma_unpin_fence(struct i915_vma *vma)
> __i915_vma_unpin_fence(vma);
> }
>
> -#endif
> +/**
> + * for_each_ggtt_vma - Iterate over the GGTT VMA belonging to an object.
> + * V - the #i915_vma iterator
> + * OBJ - the #drm_i915_gem_object
> + *
> + * GGTT VMA are placed at the being of the object's vma_list, see
> + * vma_create(), so we can stop our walk as soon as we see a ppgtt VMA,
> + * or the list is empty ofc.
> + */
> +#define for_each_ggtt_vma(V, OBJ) \
> + list_for_each_entry(V, &(OBJ)->vma_list, obj_link) \
> + if (!i915_vma_is_ggtt(vma)) break; else
Definitely like that we document this assumption a bit better and
encapsulate it.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> +#endif
> --
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
