[Intel-gfx] [PATCH v4 05/24] drm/i915: page table abstractions
Michel Thierry
michel.thierry at intel.com
Mon Feb 23 07:39:14 PST 2015
On 2/18/2015 11:27 AM, Mika Kuoppala wrote:
> Michel Thierry <michel.thierry at intel.com> writes:
>
>> From: Ben Widawsky <benjamin.widawsky at intel.com>
>>
>> When we move to dynamic page allocation, keeping page_directory and pagetabs as
>> separate structures will help to break actions into simpler tasks.
>>
>> To help transition the code nicely there is some wasted space in gen6/7.
>> This will be ameliorated shortly.
>>
>> Following the x86 pagetable terminology:
>> PDPE = struct i915_page_directory_pointer_entry.
>> PDE = struct i915_page_directory_entry [page_directory].
>> PTE = struct i915_page_table_entry [page_tables].
>>
>> v2: fixed mismatches after clean-up/rebase.
>>
>> v3: Clarify the names of the multiple levels of page tables (Daniel)
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> Signed-off-by: Michel Thierry <michel.thierry at intel.com> (v2, v3)
>> ---
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 177 ++++++++++++++++++------------------
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 23 ++++-
>> 2 files changed, 107 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index b48b586..98b4698 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -334,7 +334,8 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>> I915_CACHE_LLC, use_scratch);
>>
>> while (num_entries) {
>> - struct page *page_table = ppgtt->gen8_pt_pages[pdpe][pde];
>> + struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
>> + struct page *page_table = pd->page_tables[pde].page;
>>
>> last_pte = pte + num_entries;
>> if (last_pte > GEN8_PTES_PER_PAGE)
>> @@ -378,8 +379,12 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>> if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
>> break;
>>
>> - if (pt_vaddr == NULL)
>> - pt_vaddr = kmap_atomic(ppgtt->gen8_pt_pages[pdpe][pde]);
>> + if (pt_vaddr == NULL) {
>> + struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
>> + struct page *page_table = pd->page_tables[pde].page;
>> +
>> + pt_vaddr = kmap_atomic(page_table);
>> + }
>>
>> pt_vaddr[pte] =
>> gen8_pte_encode(sg_page_iter_dma_address(&sg_iter),
>> @@ -403,29 +408,33 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>> }
>> }
>>
>> -static void gen8_free_page_tables(struct page **pt_pages)
>> +static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
>> {
>> int i;
>>
>> - if (pt_pages == NULL)
>> + if (pd->page_tables == NULL)
>> return;
>>
>> for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
>> - if (pt_pages[i])
>> - __free_pages(pt_pages[i], 0);
>> + if (pd->page_tables[i].page)
>> + __free_page(pd->page_tables[i].page);
>> }
>>
>> -static void gen8_ppgtt_free(const struct i915_hw_ppgtt *ppgtt)
>> +static void gen8_free_page_directories(struct i915_page_directory_entry *pd)
> ^
> You only free one directory so why plural here?
>
>> +{
> If you free the page tables for the directory here..
>
>> + kfree(pd->page_tables);
>> + __free_page(pd->page);
>> +}
>> +
>> +static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>> {
>> int i;
>>
>> for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> - gen8_free_page_tables(ppgtt->gen8_pt_pages[i]);
>> - kfree(ppgtt->gen8_pt_pages[i]);
>> + gen8_free_page_tables(&ppgtt->pdp.page_directory[i]);
> ...this loop will be cleaner.
>
> Also consider renaming 'num_pd_pages' to 'num_pd'. But if it does
> cause a lot of rebase burden dont worry about it.
num_pd_pages will go away in patch #19, so I rather not change that.
All other comments addressed in v4.
Thanks,
-Michel
>> + gen8_free_page_directories(&ppgtt->pdp.page_directory[i]);
>> kfree(ppgtt->gen8_pt_dma_addr[i]);
>> }
>> -
>> - __free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages << PAGE_SHIFT));
>> }
>>
>> static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>> @@ -460,86 +469,75 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>> gen8_ppgtt_free(ppgtt);
>> }
>>
>> -static struct page **__gen8_alloc_page_tables(void)
>> +static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
>> {
>> - struct page **pt_pages;
>> int i;
>>
>> - pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *), GFP_KERNEL);
>> - if (!pt_pages)
>> - return ERR_PTR(-ENOMEM);
>> -
>> - for (i = 0; i < GEN8_PDES_PER_PAGE; i++) {
>> - pt_pages[i] = alloc_page(GFP_KERNEL);
>> - if (!pt_pages[i])
>> - goto bail;
>> + for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> + ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
>> + sizeof(dma_addr_t),
>> + GFP_KERNEL);
>> + if (!ppgtt->gen8_pt_dma_addr[i])
>> + return -ENOMEM;
>> }
>>
>> - return pt_pages;
>> -
>> -bail:
>> - gen8_free_page_tables(pt_pages);
>> - kfree(pt_pages);
>> - return ERR_PTR(-ENOMEM);
>> + return 0;
>> }
>>
>> -static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt,
>> - const int max_pdp)
>> +static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>> {
>> - struct page **pt_pages[GEN8_LEGACY_PDPES];
>> - int i, ret;
>> + int i, j;
>>
>> - for (i = 0; i < max_pdp; i++) {
>> - pt_pages[i] = __gen8_alloc_page_tables();
>> - if (IS_ERR(pt_pages[i])) {
>> - ret = PTR_ERR(pt_pages[i]);
>> - goto unwind_out;
>> + for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> + for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>> + struct i915_page_table_entry *pt = &ppgtt->pdp.page_directory[i].page_tables[j];
>> +
>> + pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> + if (!pt->page)
>> + goto unwind_out;
>> }
>> }
>>
>> - /* NB: Avoid touching gen8_pt_pages until last to keep the allocation,
>> - * "atomic" - for cleanup purposes.
>> - */
>> - for (i = 0; i < max_pdp; i++)
>> - ppgtt->gen8_pt_pages[i] = pt_pages[i];
>> -
>> return 0;
>>
>> unwind_out:
>> - while (i--) {
>> - gen8_free_page_tables(pt_pages[i]);
>> - kfree(pt_pages[i]);
>> - }
>> + while (i--)
>> + gen8_free_page_tables(&ppgtt->pdp.page_directory[i]);
>>
>> - return ret;
>> + return -ENOMEM;
>> }
>>
>> -static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
>> +static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
>> + const int max_pdp)
>> {
>> int i;
>>
>> - for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> - ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
>> - sizeof(dma_addr_t),
>> - GFP_KERNEL);
>> - if (!ppgtt->gen8_pt_dma_addr[i])
>> - return -ENOMEM;
>> - }
>> + for (i = 0; i < max_pdp; i++) {
>> + struct i915_page_table_entry *pt;
>>
>> - return 0;
>> -}
>> + pt = kcalloc(GEN8_PDES_PER_PAGE, sizeof(*pt), GFP_KERNEL);
>> + if (!pt)
>> + goto unwind_out;
>>
>> -static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
>> - const int max_pdp)
>> -{
>> - ppgtt->pd_pages = alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_SHIFT));
>> - if (!ppgtt->pd_pages)
>> - return -ENOMEM;
>> + ppgtt->pdp.page_directory[i].page = alloc_page(GFP_KERNEL);
>> + if (!ppgtt->pdp.page_directory[i].page)
>> + goto unwind_out;
> If you end up having alloc error here you will leak the previously
> allocated pt above.
>
> Also consider that if you do gen8_ppgtt_allocate_page_directory() and
> add null check for pd->page in gen8_free_page_directory you should be able to avoid
> the unwinding below completely.
>
>> +
>> + ppgtt->pdp.page_directory[i].page_tables = pt;
>> + }
>>
>> - ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT);
>> + ppgtt->num_pd_pages = max_pdp;
>> BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPES);
>>
>> return 0;
>> +
>> +unwind_out:
>> + while (i--) {
>> + kfree(ppgtt->pdp.page_directory[i].page_tables);
>> + __free_page(ppgtt->pdp.page_directory[i].page);
>> + }
>> +
>> + return -ENOMEM;
>> }
>>
>> static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
>> @@ -551,18 +549,19 @@ static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
>> if (ret)
>> return ret;
>>
>> - ret = gen8_ppgtt_allocate_page_tables(ppgtt, max_pdp);
>> - if (ret) {
>> - __free_pages(ppgtt->pd_pages, get_order(max_pdp << PAGE_SHIFT));
>> - return ret;
>> - }
>> + ret = gen8_ppgtt_allocate_page_tables(ppgtt);
>> + if (ret)
>> + goto err_out;
>>
>> ppgtt->num_pd_entries = max_pdp * GEN8_PDES_PER_PAGE;
>>
>> ret = gen8_ppgtt_allocate_dma(ppgtt);
>> - if (ret)
>> - gen8_ppgtt_free(ppgtt);
>> + if (!ret)
>> + return ret;
>>
>> + /* TODO: Check this for all cases */
> The check for zero return and then returning it with the comment is
> confusing. Why not just do the same pattern as in above?
>
> if (ret)
> goto err_out;
>
> return 0;
>
> -Mika
>
>> +err_out:
>> + gen8_ppgtt_free(ppgtt);
>> return ret;
>> }
>>
>> @@ -573,7 +572,7 @@ static int gen8_ppgtt_setup_page_directories(struct i915_hw_ppgtt *ppgtt,
>> int ret;
>>
>> pd_addr = pci_map_page(ppgtt->base.dev->pdev,
>> - &ppgtt->pd_pages[pd], 0,
>> + ppgtt->pdp.page_directory[pd].page, 0,
>> PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
>>
>> ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr);
>> @@ -593,7 +592,7 @@ static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt,
>> struct page *p;
>> int ret;
>>
>> - p = ppgtt->gen8_pt_pages[pd][pt];
>> + p = ppgtt->pdp.page_directory[pd].page_tables[pt].page;
>> pt_addr = pci_map_page(ppgtt->base.dev->pdev,
>> p, 0, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
>> ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pt_addr);
>> @@ -654,7 +653,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>> */
>> for (i = 0; i < max_pdp; i++) {
>> gen8_ppgtt_pde_t *pd_vaddr;
>> - pd_vaddr = kmap_atomic(&ppgtt->pd_pages[i]);
>> + pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i].page);
>> for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>> dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
>> pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
>> @@ -717,7 +716,7 @@ static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>> expected);
>> seq_printf(m, "\tPDE: %x\n", pd_entry);
>>
>> - pt_vaddr = kmap_atomic(ppgtt->pt_pages[pde]);
>> + pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[pde].page);
>> for (pte = 0; pte < I915_PPGTT_PT_ENTRIES; pte+=4) {
>> unsigned long va =
>> (pde * PAGE_SIZE * I915_PPGTT_PT_ENTRIES) +
>> @@ -922,7 +921,7 @@ static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
>> if (last_pte > I915_PPGTT_PT_ENTRIES)
>> last_pte = I915_PPGTT_PT_ENTRIES;
>>
>> - pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pt]);
>> + pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[act_pt].page);
>>
>> for (i = first_pte; i < last_pte; i++)
>> pt_vaddr[i] = scratch_pte;
>> @@ -951,7 +950,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
>> pt_vaddr = NULL;
>> for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
>> if (pt_vaddr == NULL)
>> - pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pt]);
>> + pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[act_pt].page);
>>
>> pt_vaddr[act_pte] =
>> vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
>> @@ -986,8 +985,8 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>>
>> kfree(ppgtt->pt_dma_addr);
>> for (i = 0; i < ppgtt->num_pd_entries; i++)
>> - __free_page(ppgtt->pt_pages[i]);
>> - kfree(ppgtt->pt_pages);
>> + __free_page(ppgtt->pd.page_tables[i].page);
>> + kfree(ppgtt->pd.page_tables);
>> }
>>
>> static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>> @@ -1044,22 +1043,22 @@ alloc:
>>
>> static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>> {
>> + struct i915_page_table_entry *pt;
>> int i;
>>
>> - ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
>> - GFP_KERNEL);
>> -
>> - if (!ppgtt->pt_pages)
>> + pt = kcalloc(ppgtt->num_pd_entries, sizeof(*pt), GFP_KERNEL);
>> + if (!pt)
>> return -ENOMEM;
>>
>> for (i = 0; i < ppgtt->num_pd_entries; i++) {
>> - ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL);
>> - if (!ppgtt->pt_pages[i]) {
>> + pt[i].page = alloc_page(GFP_KERNEL);
>> + if (!pt->page) {
>> gen6_ppgtt_free(ppgtt);
>> return -ENOMEM;
>> }
>> }
>>
>> + ppgtt->pd.page_tables = pt;
>> return 0;
>> }
>>
>> @@ -1094,9 +1093,11 @@ static int gen6_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt)
>> int i;
>>
>> for (i = 0; i < ppgtt->num_pd_entries; i++) {
>> + struct page *page;
>> dma_addr_t pt_addr;
>>
>> - pt_addr = pci_map_page(dev->pdev, ppgtt->pt_pages[i], 0, 4096,
>> + page = ppgtt->pd.page_tables[i].page;
>> + pt_addr = pci_map_page(dev->pdev, page, 0, 4096,
>> PCI_DMA_BIDIRECTIONAL);
>>
>> if (pci_dma_mapping_error(dev->pdev, pt_addr)) {
>> @@ -1140,7 +1141,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>> ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>> ppgtt->base.cleanup = gen6_ppgtt_cleanup;
>> ppgtt->base.start = 0;
>> - ppgtt->base.total = ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
>> + ppgtt->base.total = ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
>> ppgtt->debug_dump = gen6_dump_ppgtt;
>>
>> ppgtt->pd_offset =
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index 8f76990..d9bc375 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -187,6 +187,20 @@ struct i915_vma {
>> u32 flags);
>> };
>>
>> +struct i915_page_table_entry {
>> + struct page *page;
>> +};
>> +
>> +struct i915_page_directory_entry {
>> + struct page *page; /* NULL for GEN6-GEN7 */
>> + struct i915_page_table_entry *page_tables;
>> +};
>> +
>> +struct i915_page_directory_pointer_entry {
>> + /* struct page *page; */
>> + struct i915_page_directory_entry page_directory[GEN8_LEGACY_PDPES];
>> +};
>> +
>> struct i915_address_space {
>> struct drm_mm mm;
>> struct drm_device *dev;
>> @@ -272,11 +286,6 @@ struct i915_hw_ppgtt {
>> unsigned num_pd_entries;
>> unsigned num_pd_pages; /* gen8+ */
>> union {
>> - struct page **pt_pages;
>> - struct page **gen8_pt_pages[GEN8_LEGACY_PDPES];
>> - };
>> - struct page *pd_pages;
>> - union {
>> uint32_t pd_offset;
>> dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPES];
>> };
>> @@ -284,6 +293,10 @@ struct i915_hw_ppgtt {
>> dma_addr_t *pt_dma_addr;
>> dma_addr_t *gen8_pt_dma_addr[GEN8_LEGACY_PDPES];
>> };
>> + union {
>> + struct i915_page_directory_pointer_entry pdp;
>> + struct i915_page_directory_entry pd;
>> + };
>>
>> struct drm_i915_file_private *file_priv;
>>
>> --
>> 2.1.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5510 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150223/28c54115/attachment.bin>
More information about the Intel-gfx
mailing list