On 11/4/2014 12:54 PM, Daniel Vetter wrote:
> 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
Thanks for the comments Daniel,
I'll prepare another version, trying to just focus in the dynamic alloc 
part.

-Michel