Re: [RFC 00/38] PPGTT dynamic page allocations

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 07, 2014 at 06:10:56PM +0100, Michel Thierry wrote:
> This is based on the first 55 patches of Ben's 48b addressing work, taking
> into consideration the latest changes in (mainly aliasing) ppgtt rules.
> 
> Because of these changes in the tree, the first 17 patches of the original
> series are no longer needed, and some patches required more rework than others.
> 
> For GEN8, it has also been extended to work in logical ring submission (lrc)
> mode, as it looks like it will be the preferred mode of operation.
> I also tried to update the lrc code at the same time the ppgtt refactoring
> occurred, leaving only one patch that is exclusively for lrc.
> 
> I'm asking for comments, as this is the foundation for 48b virtual addressing
> in Broadwell. 
> 
> This list can be seen in 3 parts:
> [01-24] Include code rework for PPGTT (all GENs).
> [25-28] Adds page table allocation for GEN6/GEN7
> [29-38] Enables dynamic allocation in GEN8. It is enabled for both legacy
> and execlist submission modes.
> 
> Ben Widawsky (37):
>   drm/i915: Add some extra guards in evict_vm
>   drm/i915/trace: Fix offsets for 64b
>   drm/i915: Wrap VMA binding
>   drm/i915: Make pin global flags explicit
>   drm/i915: Split out aliasing binds
>   drm/i915: fix gtt_total_entries()
>   drm/i915: Rename to GEN8_LEGACY_PDPES
>   drm/i915: Split out verbose PPGTT dumping
>   drm/i915: s/pd/pdpe, s/pt/pde
>   drm/i915: rename map/unmap to dma_map/unmap
>   drm/i915: Setup less PPGTT on failed pagedir
>   drm/i915: Un-hardcode number of page directories
>   drm/i915: Make gen6_write_pdes gen6_map_page_tables
>   drm/i915: Range clearing is PPGTT agnostic
>   drm/i915: Page table helpers, and define renames
>   drm/i915: construct page table abstractions
>   drm/i915: Complete page table structures
>   drm/i915: Create page table allocators
>   drm/i915: Generalize GEN6 mapping
>   drm/i915: Clean up pagetable DMA map & unmap
>   drm/i915: Always dma map page table allocations
>   drm/i915: Consolidate dma mappings
>   drm/i915: Always dma map page directory allocations
>   drm/i915: Track GEN6 page table usage
>   drm/i915: Extract context switch skip logic
>   drm/i915: Track page table reload need
>   drm/i915: Initialize all contexts
>   drm/i915: Finish gen6/7 dynamic page table allocation
>   drm/i915/bdw: Use dynamic allocation idioms on free
>   drm/i915/bdw: pagedirs rework allocation
>   drm/i915/bdw: pagetable allocation rework
>   drm/i915/bdw: Make the pdp switch a bit less hacky
>   drm/i915: num_pd_pages/num_pd_entries isn't useful
>   drm/i915: Extract PPGTT param from pagedir alloc
>   drm/i915/bdw: Split out mappings
>   drm/i915/bdw: begin bitmap tracking
>   drm/i915/bdw: Dynamic page table allocations
> 
> Michel Thierry (1):
>   drm/i915/bdw: Dynamic page table allocations in lrc mode

Ok, high level review:

- The first part of this series seems to shuffle the code around in the
  vma binding code. If we actually want to fix this I think we need a
  REBIND flag and push the logic for rewriting ptes into the vma_bind
  hooks. There's no other way really to fix this, and the breakage for
  aliasing ppgtt this current code produces is what's blocking the cmd
  parser atm.

- Imo mergin the vma_bind/insert_entries hooks isn't useful, they imo
  provide good abstraction. Ofc we should move the vma_bind/unbind
  funcstion into the vm functions, since the vfunc dictionary varies by vm
  and not by vma. And they only really provide good abstraction if we
  first fix up the binding mess.

- The code massively shuffles around the pte and page table handling code,
  promising a lot better future. But I simply don't get it - to me this
  all looks like massive amounts of churn for no clear gain.

- Imo the really critical part of dynamic pagetable alloc is where exactly
  we add this new memory allocation point and how we handle failures. But
  since there's so much churn that I somehow can't see through I can't
  have a solid opinion on the proposed design.

So overall I think we should untangle this series first and throw out as
much churn as possible. E.g. the binding rework is very much separate imo.
Or someone needs to explain to my why we need all this code reflow.

With that out of the way reviewing the changes due to dynamic page table
alloc should be fairly simple. My expectation would have been that we'd
add a new interface to allocate vm ranges and essentially leave all the
current vma binding unchanged. Having the separate vm range extension is
somewhat important since the drm_mm allocator has a bit the tendency to
just walk up the address space, wasting piles of free space lower down.

Or like I've said maybe I just don't see the real issues and have way too
naive opinion here.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx