[Intel-gfx] [PATCH 10/24] drm/i915: Track GEN6 page table usage

[Daniel Vetter]

Ok this is just a very coarse high level comment. The only thing I really
looked for is very the dynamic pagetable allocation point happens and how
out-of-memory is handled in there.

But I've noticed that while trying to look for that that the patches and
code have a lot of history and often code comments and commit message
don't really agree with the code any more. I think that should be
carefully reviewed to make it less confusing.

On Thu, Dec 18, 2014 at 05:10:07PM +0000, Michel Thierry wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index c08fe8b..2eb6011 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -54,7 +54,10 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>  #define GEN6_PPGTT_PD_ENTRIES		512
>  #define GEN6_PD_SIZE			(GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE)
>  #define GEN6_PD_ALIGN			(PAGE_SIZE * 16)
> +#define GEN6_PDE_SHIFT          22
>  #define GEN6_PDE_VALID			(1 << 0)
> +#define GEN6_PDE_MASK			(GEN6_PPGTT_PD_ENTRIES-1)
> +#define NUM_PTE(pde_shift)		(1 << (pde_shift - PAGE_SHIFT))
>  
>  #define GEN7_PTE_CACHE_L3_LLC		(3 << 1)
>  
> @@ -182,9 +185,33 @@ struct i915_vma {
>  	 * 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);
> +};

So this patch changes the interface of vma->bind_vma to return errors when
the pagetable alloc fails. Afaics none of the callers get updated, and I
didn't see those adjustments in any other patch in this series. This
doesn't work (or I'm blind).

Also I'm not too happy with smashing this into ->bind_vma: This function
is also called in places where we know that the pagetables must be there
already, namely when changing pte bits in i915_gem_object_set_cache_level.

Imo we should add an explicit call to allocate required pagetables in
i915_gem_object_bind_to_vm, which is the only place which actually needs
this (I think).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch