Gen8 versions of these macros were updated a few months ago (e8ebd8e drm/i915: eliminate 'temp' in gen8_for_each macros) originally because at least one iterator could generate an out of bounds access, but also because eliminating the 'temp' parameter generated smaller and faster code. Matthew Auld recently noticed the same problem with the gen6 versions and provided a patch https://lists.freedesktop.org/archives/intel-gfx/2016-June/099334.html but while we're changing these, we might as well make them as much like the gen8 versions as possible, including the style of using "&& (..., true)" rather than ": (..., 1) : 0", and of course eliminating the redundant 'temp'. Furthermore, the "all_pdes" version is only used in one place, so we can improve code efficiency by changing both the macro parameters and the calling code to reduce extra dereferences. Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx> Cc: Matthew Auld <matthew.auld@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++----------- drivers/gpu/drm/i915/i915_gem_gtt.h | 40 ++++++++++++++++++------------------- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 5890017..8f0dbc4 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1570,13 +1570,13 @@ static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m) struct i915_page_table *unused; gen6_pte_t scratch_pte; uint32_t pd_entry; - uint32_t pte, pde, temp; + uint32_t pte, pde; uint32_t start = ppgtt->base.start, length = ppgtt->base.total; scratch_pte = vm->pte_encode(px_dma(vm->scratch_page), I915_CACHE_LLC, true, 0); - gen6_for_each_pde(unused, &ppgtt->pd, start, length, temp, pde) { + gen6_for_each_pde(unused, &ppgtt->pd, start, length, pde) { u32 expected; gen6_pte_t *pt_vaddr; const dma_addr_t pt_addr = px_dma(ppgtt->pd.page_table[pde]); @@ -1640,9 +1640,9 @@ static void gen6_write_page_range(struct drm_i915_private *dev_priv, { struct i915_ggtt *ggtt = &dev_priv->ggtt; struct i915_page_table *pt; - uint32_t pde, temp; + uint32_t pde; - gen6_for_each_pde(pt, pd, start, length, temp, pde) + gen6_for_each_pde(pt, pd, start, length, pde) gen6_write_pde(pd, pde, pt); /* Make sure write is complete before other code can use this page @@ -1875,7 +1875,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm, struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm); struct i915_page_table *pt; uint32_t start, length, start_save, length_save; - uint32_t pde, temp; + uint32_t pde; int ret; if (WARN_ON(start_in + length_in > ppgtt->base.total)) @@ -1891,7 +1891,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm, * need allocation. The second stage marks use ptes within the page * tables. */ - gen6_for_each_pde(pt, &ppgtt->pd, start, length, temp, pde) { + gen6_for_each_pde(pt, &ppgtt->pd, start, length, pde) { if (pt != vm->scratch_pt) { WARN_ON(bitmap_empty(pt->used_ptes, GEN6_PTES)); continue; @@ -1916,7 +1916,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm, start = start_save; length = length_save; - gen6_for_each_pde(pt, &ppgtt->pd, start, length, temp, pde) { + gen6_for_each_pde(pt, &ppgtt->pd, start, length, pde) { DECLARE_BITMAP(tmp_bitmap, GEN6_PTES); bitmap_zero(tmp_bitmap, GEN6_PTES); @@ -1985,15 +1985,16 @@ static void gen6_free_scratch(struct i915_address_space *vm) static void gen6_ppgtt_cleanup(struct i915_address_space *vm) { struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm); + struct i915_page_directory *pd = &ppgtt->pd; + struct drm_device *dev = vm->dev; struct i915_page_table *pt; uint32_t pde; drm_mm_remove_node(&ppgtt->node); - gen6_for_all_pdes(pt, ppgtt, pde) { + gen6_for_all_pdes(pt, pd, pde) if (pt != vm->scratch_pt) - free_pt(ppgtt->base.dev, pt); - } + free_pt(dev, pt); gen6_free_scratch(vm); } @@ -2059,9 +2060,9 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt, uint64_t start, uint64_t length) { struct i915_page_table *unused; - uint32_t pde, temp; + uint32_t pde; - gen6_for_each_pde(unused, &ppgtt->pd, start, length, temp, pde) + gen6_for_each_pde(unused, &ppgtt->pd, start, length, pde) ppgtt->pd.page_table[pde] = ppgtt->base.scratch_pt; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 163b564..aa5f31d 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -390,27 +390,27 @@ struct i915_hw_ppgtt { void (*debug_dump)(struct i915_hw_ppgtt *ppgtt, struct seq_file *m); }; -/* For each pde iterates over every pde between from start until start + length. - * If start, and start+length are not perfectly divisible, the macro will round - * down, and up as needed. The macro modifies pde, start, and length. Dev is - * only used to differentiate shift values. Temp is temp. On gen6/7, start = 0, - * and length = 2G effectively iterates over every PDE in the system. - * - * XXX: temp is not actually needed, but it saves doing the ALIGN operation. +/* + * gen6_for_each_pde() iterates over every pde from start until start+length. + * If start and start+length are not perfectly divisible, the macro will round + * down and up as needed. Start=0 and length=2G effectively iterates over + * every PDE in the system. The macro modifies ALL its parameters except 'pd', + * so each of the other parameters should preferably be a simple variable, or + * at most an lvalue with no side-effects! */ -#define gen6_for_each_pde(pt, pd, start, length, temp, iter) \ - for (iter = gen6_pde_index(start); \ - length > 0 && iter < I915_PDES ? \ - (pt = (pd)->page_table[iter]), 1 : 0; \ - iter++, \ - temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \ - temp = min_t(unsigned, temp, length), \ - start += temp, length -= temp) - -#define gen6_for_all_pdes(pt, ppgtt, iter) \ - for (iter = 0; \ - pt = ppgtt->pd.page_table[iter], iter < I915_PDES; \ - iter++) +#define gen6_for_each_pde(pt, pd, start, length, iter) \ + for (iter = gen6_pde_index(start); \ + length > 0 && iter < I915_PDES && \ + (pt = (pd)->page_table[iter], true); \ + ({ u32 temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT); \ + temp = min(temp - start, length); \ + start += temp, length -= temp; }), ++iter) + +#define gen6_for_all_pdes(pt, pd, iter) \ + for (iter = 0; \ + iter < I915_PDES && \ + (pt = (pd)->page_table[iter], true); \ + ++iter) static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift) { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
