diff mbox

[14/26] drm/i915: Complete page table structures

Message ID 1395121738-29126-15-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky March 18, 2014, 5:48 a.m. UTC 
Move the remaining members over to the new page table structures.

This can be squashed with the previous commit if desire. The reasoning
is the same as that patch. I simply felt it is easier to review if split.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Conflicts:
	drivers/gpu/drm/i915/i915_drv.h
	drivers/gpu/drm/i915/i915_gem_gtt.c
---
 drivers/gpu/drm/i915/i915_debugfs.c   |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c   | 85 +++++++++++++----------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h   | 15 +++----
 drivers/gpu/drm/i915/i915_gpu_error.c |  1 -
 4 files changed, 38 insertions(+), 65 deletions(-)

Comments

Chris Wilson March 18, 2014, 9:09 a.m. UTC | #1 
On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote:
> Move the remaining members over to the new page table structures.
> 
> This can be squashed with the previous commit if desire. The reasoning
> is the same as that patch. I simply felt it is easier to review if split.

I'm not liking the shorter names much. Is there precedence elsewhere
(e.g. daddr)?
-Chris
Ben Widawsky March 22, 2014, 8:10 p.m. UTC | #2 
On Tue, Mar 18, 2014 at 09:09:45AM +0000, Chris Wilson wrote:
> On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote:
> > Move the remaining members over to the new page table structures.
> > 
> > This can be squashed with the previous commit if desire. The reasoning
> > is the same as that patch. I simply felt it is easier to review if split.
> 
> I'm not liking the shorter names much. Is there precedence elsewhere
> (e.g. daddr)?
> -Chris
> 

I'm not particularly attached to "daddr." It was fun to say in my head.
A lot of code does use "daddr" but it seems to vary between "dma",
"device", data", "destination" Not exactly precedence.

Initially I had the prefix p[td]_daddr, but I thought you might complain
about it because it's implicit. dma_addr seemed kinda redundant to me.

Recommendation?

> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson March 22, 2014, 9:14 p.m. UTC | #3 
On Sat, Mar 22, 2014 at 01:10:47PM -0700, Ben Widawsky wrote:
> On Tue, Mar 18, 2014 at 09:09:45AM +0000, Chris Wilson wrote:
> > On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote:
> > > Move the remaining members over to the new page table structures.
> > > 
> > > This can be squashed with the previous commit if desire. The reasoning
> > > is the same as that patch. I simply felt it is easier to review if split.
> > 
> > I'm not liking the shorter names much. Is there precedence elsewhere
> > (e.g. daddr)?
> > -Chris
> > 
> 
> I'm not particularly attached to "daddr." It was fun to say in my head.
> A lot of code does use "daddr" but it seems to vary between "dma",
> "device", data", "destination" Not exactly precedence.
> 
> Initially I had the prefix p[td]_daddr, but I thought you might complain
> about it because it's implicit. dma_addr seemed kinda redundant to me.
> 
> Recommendation?

I am still attached to dma_addr, as it seems to be common (or
dma_address) for dma-mapped addresses. But by the end, I had stopped
caring, so

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

I'll come back to this if nobody has a better idea and see if I can make
some sensisble suggestions, or just give in completely.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b226788..5f3666a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1788,7 +1788,7 @@  static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev, int verb
 static void print_ppgtt(struct seq_file *m, struct i915_hw_ppgtt *ppgtt, const char *name)
 {
 	seq_printf(m, "%s:\n", name);
-	seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset);
+	seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd.pd_offset);
 }
 
 static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev, bool verbose)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5b283f2..d91a545 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -223,7 +223,7 @@  static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	int used_pd = ppgtt->num_pd_entries / I915_PDES_PER_PD;
 
 	for (i = used_pd - 1; i >= 0; i--) {
-		dma_addr_t addr = ppgtt->pd_dma_addr[i];
+		dma_addr_t addr = ppgtt->pdp.pagedir[i].daddr;
 		ret = gen8_write_pdp(ring, i, addr, synchronous);
 		if (ret)
 			return ret;
@@ -341,7 +341,6 @@  static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
 	for (i = 0; i < ppgtt->num_pd_pages; i++) {
 		gen8_free_page_tables(&ppgtt->pdp.pagedir[i]);
 		gen8_free_page_directories(&ppgtt->pdp.pagedir[i]);
-		kfree(ppgtt->gen8_pt_dma_addr[i]);
 	}
 }
 
@@ -353,14 +352,14 @@  static void gen8_ppgtt_dma_unmap_pages(struct i915_hw_ppgtt *ppgtt)
 	for (i = 0; i < ppgtt->num_pd_pages; i++) {
 		/* TODO: In the future we'll support sparse mappings, so this
 		 * will have to change. */
-		if (!ppgtt->pd_dma_addr[i])
+		if (!ppgtt->pdp.pagedir[i].daddr)
 			continue;
 
-		pci_unmap_page(hwdev, ppgtt->pd_dma_addr[i], PAGE_SIZE,
+		pci_unmap_page(hwdev, ppgtt->pdp.pagedir[i].daddr, PAGE_SIZE,
 			       PCI_DMA_BIDIRECTIONAL);
 
 		for (j = 0; j < I915_PDES_PER_PD; j++) {
-			dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
+			dma_addr_t addr = ppgtt->pdp.pagedir[i].page_tables[j].daddr;
 			if (addr)
 				pci_unmap_page(hwdev, addr, PAGE_SIZE,
 					       PCI_DMA_BIDIRECTIONAL);
@@ -380,31 +379,18 @@  static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 	gen8_ppgtt_free(ppgtt);
 }
 
-static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
-{
-	int i;
-
-	for (i = 0; i < ppgtt->num_pd_pages; i++) {
-		ppgtt->gen8_pt_dma_addr[i] = kcalloc(I915_PDES_PER_PD,
-						     sizeof(dma_addr_t),
-						     GFP_KERNEL);
-		if (!ppgtt->gen8_pt_dma_addr[i])
-			return -ENOMEM;
-	}
-
-	return 0;
-}
-
 static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
 {
 	int i, j;
 
 	for (i = 0; i < ppgtt->num_pd_pages; i++) {
+		struct i915_pagedir *pd = &ppgtt->pdp.pagedir[i];
 		for (j = 0; j < I915_PDES_PER_PD; j++) {
-			struct i915_pagetab *pt = &ppgtt->pdp.pagedir[i].page_tables[j];
+			struct i915_pagetab *pt = &pd->page_tables[j];
 			pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
 			if (!pt->page)
 				goto unwind_out;
+
 		}
 	}
 
@@ -464,9 +450,7 @@  static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
 
 	ppgtt->num_pd_entries = max_pdp * I915_PDES_PER_PD;
 
-	ret = gen8_ppgtt_allocate_dma(ppgtt);
-	if (!ret)
-		return ret;
+	return 0;
 
 	/* TODO: Check this for all cases */
 err_out:
@@ -488,7 +472,7 @@  static int gen8_ppgtt_setup_page_directories(struct i915_hw_ppgtt *ppgtt,
 	if (ret)
 		return ret;
 
-	ppgtt->pd_dma_addr[pdpe] = pd_addr;
+	ppgtt->pdp.pagedir[pdpe].daddr = pd_addr;
 
 	return 0;
 }
@@ -498,17 +482,18 @@  static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt,
 					const int pde)
 {
 	dma_addr_t pt_addr;
-	struct page *p;
+	struct i915_pagedir *pd = &ppgtt->pdp.pagedir[pdpe];
+	struct i915_pagetab *pt = &pd->page_tables[pde];
+	struct page *p = pt->page;
 	int ret;
 
-	p = ppgtt->pdp.pagedir[pdpe].page_tables[pde].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);
 	if (ret)
 		return ret;
 
-	ppgtt->gen8_pt_dma_addr[pdpe][pde] = pt_addr;
+	pt->daddr = pt_addr;
 
 	return 0;
 }
@@ -564,7 +549,7 @@  static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 		gen8_ppgtt_pde_t *pd_vaddr;
 		pd_vaddr = kmap_atomic(ppgtt->pdp.pagedir[i].page);
 		for (j = 0; j < I915_PDES_PER_PD; j++) {
-			dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
+			dma_addr_t addr = ppgtt->pdp.pagedir[i].page_tables[j].daddr;
 			pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
 						      I915_CACHE_LLC);
 		}
@@ -604,14 +589,15 @@  static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 	scratch_pte = vm->pte_encode(vm->scratch.addr, I915_CACHE_LLC, true);
 
 	pd_addr = (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm +
-		ppgtt->pd_offset / sizeof(gen6_gtt_pte_t);
+		ppgtt->pd.pd_offset / sizeof(gen6_gtt_pte_t);
 
 	seq_printf(m, "  VM %p (pd_offset %x-%x):\n", vm,
-		   ppgtt->pd_offset, ppgtt->pd_offset + ppgtt->num_pd_entries);
+		   ppgtt->pd.pd_offset,
+		   ppgtt->pd.pd_offset + ppgtt->num_pd_entries);
 	for (pde = 0; pde < ppgtt->num_pd_entries; pde++) {
 		u32 expected;
 		gen6_gtt_pte_t *pt_vaddr;
-		dma_addr_t pt_addr = ppgtt->pt_dma_addr[pde];
+		dma_addr_t pt_addr = ppgtt->pd.page_tables[pde].daddr;
 		pd_entry = readl(pd_addr + pde);
 		expected = (GEN6_PDE_ADDR_ENCODE(pt_addr) | GEN6_PDE_VALID);
 
@@ -654,8 +640,8 @@  static void gen6_map_single(struct i915_hw_ppgtt *ppgtt,
 {
 	struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;
 	uint32_t pd_entry;
-	gen6_gtt_pte_t __iomem *pd_addr =
-		(gen6_gtt_pte_t __iomem*)dev_priv->gtt.gsm + ppgtt->pd_offset / sizeof(gen6_gtt_pte_t);
+	gen6_gtt_pte_t __iomem *pd_addr = (gen6_gtt_pte_t __iomem*)dev_priv->gtt.gsm;
+	pd_addr	+= ppgtt->pd.pd_offset / sizeof(gen6_gtt_pte_t);
 
 	pd_entry = GEN6_PDE_ADDR_ENCODE(daddr);
 	pd_entry |= GEN6_PDE_VALID;
@@ -670,18 +656,18 @@  static void gen6_map_page_tables(struct i915_hw_ppgtt *ppgtt)
 	struct drm_i915_private *dev_priv = ppgtt->base.dev->dev_private;
 	int i;
 
-	WARN_ON(ppgtt->pd_offset & 0x3f);
+	WARN_ON(ppgtt->pd.pd_offset & 0x3f);
 	for (i = 0; i < ppgtt->num_pd_entries; i++)
-		gen6_map_single(ppgtt, i, ppgtt->pt_dma_addr[i]);
+		gen6_map_single(ppgtt, i, ppgtt->pd.page_tables[i].daddr);
 
 	readl(dev_priv->gtt.gsm);
 }
 
 static uint32_t get_pd_offset(struct i915_hw_ppgtt *ppgtt)
 {
-	BUG_ON(ppgtt->pd_offset & 0x3f);
+	BUG_ON(ppgtt->pd.pd_offset & 0x3f);
 
-	return (ppgtt->pd_offset / 64) << 16;
+	return (ppgtt->pd.pd_offset / 64) << 16;
 }
 
 static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
@@ -964,19 +950,16 @@  static void gen6_ppgtt_dma_unmap_pages(struct i915_hw_ppgtt *ppgtt)
 {
 	int i;
 
-	if (ppgtt->pt_dma_addr) {
-		for (i = 0; i < ppgtt->num_pd_entries; i++)
-			pci_unmap_page(ppgtt->base.dev->pdev,
-				       ppgtt->pt_dma_addr[i],
-				       4096, PCI_DMA_BIDIRECTIONAL);
-	}
+	for (i = 0; i < ppgtt->num_pd_entries; i++)
+		pci_unmap_page(ppgtt->base.dev->pdev,
+			       ppgtt->pd.page_tables[i].daddr,
+			       4096, PCI_DMA_BIDIRECTIONAL);
 }
 
 static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
 {
 	int i;
 
-	kfree(ppgtt->pt_dma_addr);
 	for (i = 0; i < ppgtt->num_pd_entries; i++)
 		__free_page(ppgtt->pd.page_tables[i].page);
 	kfree(ppgtt->pd.page_tables);
@@ -1071,14 +1054,6 @@  static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
 		return ret;
 	}
 
-	ppgtt->pt_dma_addr = kcalloc(ppgtt->num_pd_entries, sizeof(dma_addr_t),
-				     GFP_KERNEL);
-	if (!ppgtt->pt_dma_addr) {
-		drm_mm_remove_node(&ppgtt->node);
-		gen6_ppgtt_free(ppgtt);
-		return -ENOMEM;
-	}
-
 	return 0;
 }
 
@@ -1100,7 +1075,7 @@  static int gen6_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt)
 			return -EIO;
 		}
 
-		ppgtt->pt_dma_addr[i] = pt_addr;
+		ppgtt->pd.page_tables[i].daddr = pt_addr;
 	}
 
 	return 0;
@@ -1142,7 +1117,7 @@  static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.total = ppgtt->num_pd_entries * GEN6_PTES_PER_PT * PAGE_SIZE;
 	ppgtt->debug_dump = gen6_dump_ppgtt;
 
-	ppgtt->pd_offset =
+	ppgtt->pd.pd_offset =
 		ppgtt->node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t);
 
 	gen6_map_page_tables(ppgtt);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 2c7b378..d30f6de 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -308,10 +308,16 @@  struct i915_gtt {
 
 struct i915_pagetab {
 	struct page *page;
+	dma_addr_t daddr;
 };
 
 struct i915_pagedir {
 	struct page *page; /* NULL for GEN6-GEN7 */
+	union {
+		uint32_t pd_offset;
+		dma_addr_t daddr;
+	};
+
 	struct i915_pagetab *page_tables;
 };
 
@@ -327,17 +333,10 @@  struct i915_hw_ppgtt {
 	unsigned num_pd_entries;
 	unsigned num_pd_pages; /* gen8+ */
 	union {
-		uint32_t pd_offset;
-		dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPS];
-	};
-	union {
-		dma_addr_t *pt_dma_addr;
-		dma_addr_t *gen8_pt_dma_addr[GEN8_LEGACY_PDPS];
-	};
-	union {
 		struct i915_pagedirpo pdp;
 		struct i915_pagedir pd;
 	};
+
 	int (*enable)(struct i915_hw_ppgtt *ppgtt);
 	int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
 			 struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 63266ae..d7ac688 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,6 @@  static void i915_gem_record_fences(struct drm_device *dev,
 	}
 }
 
-
 static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct drm_i915_error_state *error,
 					struct intel_ring_buffer *ring,