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

Michel Thierry
2015-07-07T13:42:08Z

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);
>> + }
>> + }
>> }
>>