[Intel-gfx] [PATCH v6 04/19] drm/i915/gen8: Generalize PTE writing for GEN8 PPGTT

[Michel Thierry]

On 7/30/2015 5:46 AM, Goel, Akash wrote:
> On 7/29/2015 9:53 PM, Michel Thierry wrote:
>> The insert_entries function was the function used to write PTEs. For the
>> PPGTT it was "hardcoded" to only understand two level page tables, which
>> was the case for GEN7. We can reuse this for 4 level page tables, and
>> remove the concept of insert_entries, which was never viable past 2
>> level page tables anyway, but it requires a bit of rework to make the
>> function a bit more generic.
>>
>> This patch begins the generalization work, and it will be heavily used
>> upon when the 48b code is complete. The patch series attempts to make
>> each function which touches a part of code specific to the page table
>> level and here is no exception.
>>
>> v2: Rebase after Mika's ppgtt cleanup / scratch merge patch series.
>> v3: Rebase after final merged version of Mika's ppgtt/scratch patches.
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> Signed-off-by: Michel Thierry <michel.thierry at intel.com> (v2)
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 52
>> +++++++++++++++++++++++++++----------
>>   1 file changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index bd56979..f338a13 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -600,24 +600,21 @@ static int gen8_mm_switch(struct i915_hw_ppgtt
>> *ppgtt,
>>       return 0;
>>   }
>>
>> -static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>> -                   uint64_t start,
>> -                   uint64_t length,
>> -                   bool use_scratch)
>> +static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
>> +                       struct i915_page_directory_pointer *pdp,
>> +                       uint64_t start,
>> +                       uint64_t length,
>> +                       gen8_pte_t scratch_pte)
>>   {
>>       struct i915_hw_ppgtt *ppgtt =
>>           container_of(vm, struct i915_hw_ppgtt, base);
>> -    struct i915_page_directory_pointer *pdp = &ppgtt->pdp; /* FIXME:
>> 48b */
>> -    gen8_pte_t *pt_vaddr, scratch_pte;
>> +    gen8_pte_t *pt_vaddr;
>>       unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
>>       unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
>>       unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
>>       unsigned num_entries = length >> PAGE_SHIFT;
>>       unsigned last_pte, i;
>>
>> -    scratch_pte = gen8_pte_encode(px_dma(ppgtt->base.scratch_page),
>> -                      I915_CACHE_LLC, use_scratch);
>> -
>
> Sorry for the late comment.
> Would it be better to have a WARN_ON check here on NULL value of pdp
> pointer, considering the pdp will no longer be static in case of 48 bit.
>
> Actually there are already such checks used in this function for pd, pt
> and page pointers.
>
> Best regards
> Akash
>

Ok, I'll add the extra WARN_ON(!pdp).

-Michel