On 2/18/2015 11:27 AM, Mika Kuoppala wrote: > Michel Thierry writes: > >> From: Ben Widawsky >> >> 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 >> Signed-off-by: Michel Thierry (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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx