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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
