Re: [PATCH 00/24] PPGTT dynamic page allocations

[Michel Thierry <michel.thierry@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5155 bytes --]

On 12/19/2014 10:13 AM, Daniel Vetter wrote:
> On Fri, Dec 19, 2014 at 08:50:09AM +0000, Chris Wilson wrote:
>> On Fri, Dec 19, 2014 at 09:37:52AM +0100, Daniel Vetter wrote:
>>> On Fri, Dec 19, 2014 at 08:31:03AM +0000, Chris Wilson wrote:
>>>> On Thu, Dec 18, 2014 at 10:16:22PM +0100, Daniel Vetter wrote:
>>>>> On Thu, Dec 18, 2014 at 05:09:57PM +0000, Michel Thierry wrote:
>>>>>> This new version tries to remove as many unnecessary changes as possible from
>>>>>> the previous RFC.
>>>>>>   
>>>>>> For GEN8, it has also been extended to work in logical ring submission (lrc)
>>>>>> mode, as 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.
>>>>>>
>>>>>> This list can be seen in 3 parts:
>>>>>> [01-10] Include code rework for PPGTT (all GENs).
>>>>>> [11-14] Adds page table allocation for GEN6/GEN7
>>>>>> [15-24] Enables dynamic allocation in GEN8. It is enabled for both legacy
>>>>>> and execlist submission modes.
>>>>>>
>>>>>> Ben Widawsky (23):
>>>>>>    drm/i915: Add some extra guards in evict_vm
>>>>>>    drm/i915/trace: Fix offsets for 64b
>>>>>>    drm/i915: Rename to GEN8_LEGACY_PDPES
>>>>>>    drm/i915: Setup less PPGTT on failed pagedir
>>>>>>    drm/i915/gen8: Un-hardcode number of page directories
>>>>>>    drm/i915: Range clearing is PPGTT agnostic
>>>>>>    drm/i915: page table abstractions
>>>>>>    drm/i915: Complete page table structures
>>>>>>    drm/i915: Create page table allocators
>>>>>>    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: Update pdp switch and point unused PDPs to scratch page
>>>>>>    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, I've tried to read through this series again and I definitely see a
>>>>> bit clearer, but it's still fairly confusing to me. I think that's just
>>>>> the long history of this patch series - often it seems to do something and
>>>>> then undo it again in some later patch. Which doesn't help understanding
>>>>> it.
>>>>>
>>>>> I've replied with a few comments. Imo the way forward with this is to
>>>>> read, understand and review it from the beginning and merge while that's
>>>>> happening. It will probably take a few rounds until all the confusion is
>>>>> cleared up and we've reached the last patch.
>>>> I honestly think this is starting off in the wrong direction. The first
>>>> task, imo, is to make the current PD swappable. Then, we can introduce
>>>> infrastructure to do deferred page allocation, hopefully combining with
>>>> an approach to allow userspace to similarly defer their page allocation.
>>> Thus far things started to blow up because we need a bit too much memory
>>> in testcases so we need this. We'll probably need the pd swapping
>>> eventually, too. But only on gen7, and atm it doesn't look good for ppgtt
>>> on gen7 still. Given that I think the priorities are right for now.
>>>
>>> But yeah we need to keep this in mind for gen7 full ppgtt.
>> Certainly making the toplevel PD swappable on BDW is less of a priority,
>> since that should also be already done as part of execlists (and it is so
>> much smaller). I would like to see the gen6/7 deferred allocation
>> removed from this series, as I still consider it to be the wrong initial
>> approach (and making the current gen7 full-ppgtt swappable is rather
>> trivial).
> I don't understand why you'd want to hold up the delayed pagetable alloc
> until the pd is evictable. Imo these are fairly orthogoanl issues: pd
> eviction tries to make ggtt space reclaimable, while deferred pagetable
> alloc ensure that we don't alloc 2M of pagetables (which are system
> memory) in the lower level when not needed. So imo we can go ahead with
> both in parallel. There might be some minor conflicts between this series
> and making pd ggtt blocks evictable around pd reloading, but noting
> fundamental.
>
> Or are we talking past each another? Just wondering since you call it
> "swappable pd" and I can't really think of a way we could swap out pds to
> disk on gen7 (they just block ggtt space which can't be used for real ptes
> any more).
> -Daniel

Thanks for the quick review.

I'll make the changes (and try to tidy up the commit messages).
In particular, I agree it's better to decouple alloc and bind_vma.

-Michel


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx