| Message ID | 20171207162717.904-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > 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 for_each_if() ? > > +#endif > -- > 2.15.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Ville Syrjälä (2017-12-07 16:40:49) > On Thu, Dec 07, 2017 at 04:27:17PM +0000, Chris Wilson wrote: > > +#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 > > for_each_if() ? for_each_if() doesn't perform a break. for_each_until() ? I think that may be a bit too magical. -Chris
On Thu, Dec 07, 2017 at 04:46:37PM +0000, Chris Wilson wrote: > Quoting Ville Syrjälä (2017-12-07 16:40:49) > > On Thu, Dec 07, 2017 at 04:27:17PM +0000, Chris Wilson wrote: > > > +#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 > > > > for_each_if() ? > > for_each_if() doesn't perform a break. Ah. I probably should have read the comment :) > > for_each_until() ? I think that may be a bit too magical. Maybe. I guess open coding it here is fine until the pattern starts to repeat all over the place.
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@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > 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@ffwll.ch> > > +#endif > -- > 2.15.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Patchwork (2017-12-08 00:32:47) > == Series Details == > > Series: drm/i915: Refactor common list iteration over GGTT vma (rev3) > URL : https://patchwork.freedesktop.org/series/35048/ > State : warning > > == Summary == > > Test kms_pipe_crc_basic: > Subgroup read-crc-pipe-b: > incomplete -> PASS (shard-hsw) > Test kms_chv_cursor_fail: > Subgroup pipe-b-128x128-right-edge: > pass -> SKIP (shard-snb) > Test kms_draw_crc: > Subgroup fill-fb: > pass -> SKIP (shard-snb) > Subgroup draw-method-rgb565-pwrite-xtiled: > pass -> SKIP (shard-snb) > Test kms_plane_multiple: > Subgroup atomic-pipe-b-tiling-x: > pass -> SKIP (shard-snb) > Test gem_tiled_swapping: > Subgroup non-threaded: > pass -> INCOMPLETE (shard-hsw) fdo#104009 > Test drv_module_reload: > Subgroup basic-reload: > pass -> DMESG-WARN (shard-hsw) fdo#102707 > Test kms_frontbuffer_tracking: > Subgroup fbc-1p-offscren-pri-shrfb-draw-render: > fail -> PASS (shard-snb) fdo#101623 > Test kms_setmode: > Subgroup basic: > fail -> PASS (shard-hsw) fdo#99912 Whatever bug may be lurking hides very well. Thanks for the suggestion and review, applied. -Chris
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 +#endif
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@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- 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(-)