Re: [PATCH v3 09/17] drm/i915/gen8: Add 4 level support in insert_entries and clear_range

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

On 7/7/2015 1:51 PM, Goel, Akash wrote:
> On 7/1/2015 8:57 PM, Michel Thierry wrote:
>>   static void
>> @@ -781,9 +793,9 @@ gen8_ppgtt_insert_pte_entries(struct
>> i915_address_space *vm,
>>       struct i915_hw_ppgtt *ppgtt =
>>           container_of(vm, struct i915_hw_ppgtt, base);
>>       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 pdpe = gen8_pdpe_index(start);
>> +    unsigned pde = gen8_pde_index(start);
>> +    unsigned pte = gen8_pte_index(start);
>>
>>       pt_vaddr = NULL;
>>
>> @@ -801,7 +813,8 @@ gen8_ppgtt_insert_pte_entries(struct
>> i915_address_space *vm,
>>               kunmap_px(ppgtt, pt_vaddr);
>>               pt_vaddr = NULL;
>>               if (++pde == I915_PDES) {
>> -                pdpe++;
>> +                if (++pdpe == I915_PDPES_PER_PDP(vm->dev))
>> +                    break;
>
> Can the same pdpe check (for Page directory pointer boundary) be added
> in the gen8_ppgtt_clear_pte_range function also, to make it consistent
> with gen8_ppgtt_insert_pte_entries and this will also obviate the need
> for gen8_clamp_pdp macro.
>
I will change gen8_ppgtt_clear_pte_range to stop at PDP boundary as you 
suggests (and clamp_pdp will go away).

>>                   pde = 0;
>>               }
>>               pte = 0;
>> @@ -820,11 +833,25 @@ static void gen8_ppgtt_insert_entries(struct
>> i915_address_space *vm,
>>   {
>>       struct i915_hw_ppgtt *ppgtt =
>>           container_of(vm, struct i915_hw_ppgtt, base);
>> -    struct i915_page_directory_pointer *pdp = &ppgtt->pdp; /* FIXME:
>> 48b */
>>       struct sg_page_iter sg_iter;
>>
>>       __sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl),
>> 0);
>> -    gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter, start,
>> cache_level);
>> +
>> +    if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
>> +        gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
>> +                          cache_level);
>> +    } else {
>> +        struct i915_page_directory_pointer *pdp;
>> +        uint64_t templ4, pml4e;
>> +        uint64_t length = (uint64_t)sg_nents(pages->sgl) << PAGE_SHIFT;
>> +
>> +        gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4,
>> pml4e) {
>> +            uint64_t pdp_start = start;
>> +
>
> Isn't the 'pdp_start' dispensable here ? ‘start’ can be used directly.
>
Yes, and the same applies in gen8_ppgtt_clear_range, pdp_len and 
pdp_start are redundant there.

>> +            gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter,
>> +                              pdp_start, cache_level);
>> +        }
>> +    }
>>   }
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx