On 2/18/2015 11:27 AM, Mika Kuoppala wrote:
Michel Thierry <michel.thierry@xxxxxxxxx> writes:From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx> 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@xxxxxxxxxxxx> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx> (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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
