[Intel-gfx] [PATCH 1/2] drm/i915/gtt: Fix pte clear range

[Chris Wilson]

On Mon, Oct 31, 2016 at 05:24:45PM +0200, Mika Kuoppala wrote:
> Comparing pte index to a number of entries is wrong
> when clearing a range of pte entries. Use end marker
> of 'one past' to correctly point adequate number of
> ptes to the scratch page.
> 
> Fixes: d209b9c3cd28 ("drm/i915/gtt: Split gen8_ppgtt_clear_pte_range")
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98282
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e7afad5..cda263c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -712,13 +712,13 @@ static int gen8_48b_mm_switch(struct i915_hw_ppgtt *ppgtt,
>   */
>  static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  				struct i915_page_table *pt,
> -				uint64_t start,
> -				uint64_t length)
> +				const uint64_t start,
> +				const uint64_t length)
>  {
>  	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> -	unsigned int pte_start = gen8_pte_index(start);
> -	unsigned int num_entries = gen8_pte_count(start, length);
> -	uint64_t pte;
> +	const unsigned int num_entries = gen8_pte_count(start, length);
> +	unsigned int pte = gen8_pte_index(start);
> +	unsigned int pte_end = pte + num_entries;
>  	gen8_pte_t *pt_vaddr;
>  	gen8_pte_t scratch_pte = gen8_pte_encode(vm->scratch_page.daddr,
>  						 I915_CACHE_LLC);
> @@ -726,17 +726,20 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
>  	if (WARN_ON(!px_page(pt)))
>  		return false;
>  
> -	bitmap_clear(pt->used_ptes, pte_start, num_entries);
> +	bitmap_clear(pt->used_ptes, pte, num_entries);
>  
>  	if (bitmap_empty(pt->used_ptes, GEN8_PTES)) {
>  		free_pt(vm->dev, pt);
>  		return true;
>  	}
>  
> +	if (WARN_ON_ONCE(pte_end > GEN8_PTES))
> +		pte_end = GEN8_PTES;

Internal programming error, if you hit all the upper layers are dead,
i.e. bug on. GEM_BUG_ON.

And the assert should be earlier since you have already used the invalid
values (i.e. pte+num_entries).

> +
>  	pt_vaddr = kmap_px(pt);
>  
> -	for (pte = pte_start; pte < num_entries; pte++)

Nice catch.
R-b on this part, less enamoured with the reset.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre