[PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[Kenneth Graunke]

Sandybridge/Ivybridge, Bay Trail, and Haswell all have slightly
different page table entry formats.  Rather than polluting one function
with generation checks, simply use a function pointer and set up the
correct PTE encoding function at startup.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/i915_drv.h     |  6 ++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd2d7f1..8b58997 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -430,6 +430,9 @@ struct i915_gtt {
 				   struct sg_table *st,
 				   unsigned int pg_start,
 				   enum i915_cache_level cache_level);
+	uint32_t (*pte_encode)(struct drm_device *dev,
+			       dma_addr_t addr,
+			       enum i915_cache_level level);
 };
 #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
 
@@ -451,6 +454,9 @@ struct i915_hw_ppgtt {
 			       struct sg_table *st,
 			       unsigned int pg_start,
 			       enum i915_cache_level cache_level);
+	uint32_t (*pte_encode)(struct drm_device *dev,
+			       dma_addr_t addr,
+			       enum i915_cache_level level);
 	int (*enable)(struct drm_device *dev);
 	void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
 };
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 50df194..9db65c1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -154,9 +154,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
 	unsigned last_pte, i;
 
-	scratch_pte = gen6_pte_encode(ppgtt->dev,
-				      ppgtt->scratch_page_dma_addr,
-				      I915_CACHE_LLC);
+	scratch_pte = ppgtt->pte_encode(ppgtt->dev,
+					ppgtt->scratch_page_dma_addr,
+					I915_CACHE_LLC);
 
 	while (num_entries) {
 		last_pte = first_pte + num_entries;
@@ -191,8 +191,8 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
 		dma_addr_t page_addr;
 
 		page_addr = sg_page_iter_dma_address(&sg_iter);
-		pt_vaddr[act_pte] = gen6_pte_encode(ppgtt->dev, page_addr,
-						    cache_level);
+		pt_vaddr[act_pte] = ppgtt->pte_encode(ppgtt->dev, page_addr,
+						      cache_level);
 		if (++act_pte == I915_PPGTT_PT_ENTRIES) {
 			kunmap_atomic(pt_vaddr);
 			act_pt++;
@@ -236,6 +236,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	first_pd_entry_in_global_pt =
 		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
 
+	ppgtt->pte_encode = gen6_pte_encode;
 	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
 	ppgtt->enable = gen6_ppgtt_enable;
 	ppgtt->clear_range = gen6_ppgtt_clear_range;
@@ -438,7 +439,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_page_iter_dma_address(&sg_iter);
-		iowrite32(gen6_pte_encode(dev, addr, level), &gtt_entries[i]);
+		iowrite32(dev_priv->gtt.pte_encode(dev, addr, level),
+			  &gtt_entries[i]);
 		i++;
 	}
 
@@ -450,7 +452,7 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
 	 */
 	if (i != 0)
 		WARN_ON(readl(&gtt_entries[i-1])
-			!= gen6_pte_encode(dev, addr, level));
+			!= dev_priv->gtt.pte_encode(dev, addr, level));
 
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
@@ -475,8 +477,9 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 		 first_entry, num_entries, max_entries))
 		num_entries = max_entries;
 
-	scratch_pte = gen6_pte_encode(dev, dev_priv->gtt.scratch_page_dma,
-				      I915_CACHE_LLC);
+	scratch_pte = dev_priv->gtt.pte_encode(dev,
+					       dev_priv->gtt.scratch_page_dma,
+					       I915_CACHE_LLC);
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
@@ -823,6 +826,7 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	} else {
 		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
 		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
+		dev_priv->gtt.pte_encode = gen6_pte_encode;
 	}
 
 	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
-- 
1.8.2.1

Re: [PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[Jani Nikula]

On Thu, 18 Apr 2013, Kenneth Graunke <kenneth@whitecape.org> wrote:
> Sandybridge/Ivybridge, Bay Trail, and Haswell all have slightly
> different page table entry formats.  Rather than polluting one function
> with generation checks, simply use a function pointer and set up the
> correct PTE encoding function at startup.
>
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  6 ++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +++++++++++++---------
>  2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bd2d7f1..8b58997 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -430,6 +430,9 @@ struct i915_gtt {
>  				   struct sg_table *st,
>  				   unsigned int pg_start,
>  				   enum i915_cache_level cache_level);
> +	uint32_t (*pte_encode)(struct drm_device *dev,
> +			       dma_addr_t addr,
> +			       enum i915_cache_level level);

To keep the gen6_gtt_pte_t typedef internal to i915_gem_gtt.c, or to
have the same return type in the function pointers and the functions. I
think I'd find the latter more aesthetically pleasing, but I'm not
insisting. Either way,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  };
>  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
>  
> @@ -451,6 +454,9 @@ struct i915_hw_ppgtt {
>  			       struct sg_table *st,
>  			       unsigned int pg_start,
>  			       enum i915_cache_level cache_level);
> +	uint32_t (*pte_encode)(struct drm_device *dev,
> +			       dma_addr_t addr,
> +			       enum i915_cache_level level);
>  	int (*enable)(struct drm_device *dev);
>  	void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
>  };
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 50df194..9db65c1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -154,9 +154,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
>  	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
>  	unsigned last_pte, i;
>  
> -	scratch_pte = gen6_pte_encode(ppgtt->dev,
> -				      ppgtt->scratch_page_dma_addr,
> -				      I915_CACHE_LLC);
> +	scratch_pte = ppgtt->pte_encode(ppgtt->dev,
> +					ppgtt->scratch_page_dma_addr,
> +					I915_CACHE_LLC);
>  
>  	while (num_entries) {
>  		last_pte = first_pte + num_entries;
> @@ -191,8 +191,8 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
>  		dma_addr_t page_addr;
>  
>  		page_addr = sg_page_iter_dma_address(&sg_iter);
> -		pt_vaddr[act_pte] = gen6_pte_encode(ppgtt->dev, page_addr,
> -						    cache_level);
> +		pt_vaddr[act_pte] = ppgtt->pte_encode(ppgtt->dev, page_addr,
> +						      cache_level);
>  		if (++act_pte == I915_PPGTT_PT_ENTRIES) {
>  			kunmap_atomic(pt_vaddr);
>  			act_pt++;
> @@ -236,6 +236,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	first_pd_entry_in_global_pt =
>  		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
>  
> +	ppgtt->pte_encode = gen6_pte_encode;
>  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->enable = gen6_ppgtt_enable;
>  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> @@ -438,7 +439,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
>  
>  	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
>  		addr = sg_page_iter_dma_address(&sg_iter);
> -		iowrite32(gen6_pte_encode(dev, addr, level), &gtt_entries[i]);
> +		iowrite32(dev_priv->gtt.pte_encode(dev, addr, level),
> +			  &gtt_entries[i]);
>  		i++;
>  	}
>  
> @@ -450,7 +452,7 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
>  	 */
>  	if (i != 0)
>  		WARN_ON(readl(&gtt_entries[i-1])
> -			!= gen6_pte_encode(dev, addr, level));
> +			!= dev_priv->gtt.pte_encode(dev, addr, level));
>  
>  	/* This next bit makes the above posting read even more important. We
>  	 * want to flush the TLBs only after we're certain all the PTE updates
> @@ -475,8 +477,9 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
>  		 first_entry, num_entries, max_entries))
>  		num_entries = max_entries;
>  
> -	scratch_pte = gen6_pte_encode(dev, dev_priv->gtt.scratch_page_dma,
> -				      I915_CACHE_LLC);
> +	scratch_pte = dev_priv->gtt.pte_encode(dev,
> +					       dev_priv->gtt.scratch_page_dma,
> +					       I915_CACHE_LLC);
>  	for (i = 0; i < num_entries; i++)
>  		iowrite32(scratch_pte, &gtt_base[i]);
>  	readl(gtt_base);
> @@ -823,6 +826,7 @@ int i915_gem_gtt_init(struct drm_device *dev)
>  	} else {
>  		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
>  		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
> +		dev_priv->gtt.pte_encode = gen6_pte_encode;
>  	}
>  
>  	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
> -- 
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[Kenneth Graunke]

Sandybridge/Ivybridge, Bay Trail, and Haswell all have slightly
different page table entry formats.  Rather than polluting one function
with generation checks, simply use a function pointer and set up the
correct PTE encoding function at startup.

v2: Move the gen6_gtt_pte_t typedef to i915_drv.h so that the function
    pointers and implementations have identical signatures.  Also remove
    inline keyword on gen6_pte_encode.  Both suggested by Jani Nikula.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com> [v1]
Tested-by: Daniel Leung <daniel.leung@linux.intel.com> [v1]
---
 drivers/gpu/drm/i915/i915_drv.h     |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 30 ++++++++++++++++--------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd2d7f1..d80bced 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -395,6 +395,8 @@ enum i915_cache_level {
 	I915_CACHE_LLC_MLC, /* gen6+, in docs at least! */
 };
 
+typedef uint32_t gen6_gtt_pte_t;
+
 /* The Graphics Translation Table is the way in which GEN hardware translates a
  * Graphics Virtual Address into a Physical Address. In addition to the normal
  * collateral associated with any va->pa translations GEN hardware also has a
@@ -430,6 +432,9 @@ struct i915_gtt {
 				   struct sg_table *st,
 				   unsigned int pg_start,
 				   enum i915_cache_level cache_level);
+	gen6_gtt_pte_t (*pte_encode)(struct drm_device *dev,
+				     dma_addr_t addr,
+				     enum i915_cache_level level);
 };
 #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
 
@@ -451,6 +456,9 @@ struct i915_hw_ppgtt {
 			       struct sg_table *st,
 			       unsigned int pg_start,
 			       enum i915_cache_level cache_level);
+	gen6_gtt_pte_t (*pte_encode)(struct drm_device *dev,
+				     dma_addr_t addr,
+				     enum i915_cache_level level);
 	int (*enable)(struct drm_device *dev);
 	void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
 };
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 50df194..92e147f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -28,8 +28,6 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-typedef uint32_t gen6_gtt_pte_t;
-
 /* PPGTT stuff */
 #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
 
@@ -44,9 +42,9 @@ typedef uint32_t gen6_gtt_pte_t;
 #define GEN6_PTE_CACHE_LLC_MLC		(3 << 1)
 #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
 
-static inline gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
-					     dma_addr_t addr,
-					     enum i915_cache_level level)
+static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
+				      dma_addr_t addr,
+				      enum i915_cache_level level)
 {
 	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
 	pte |= GEN6_PTE_ADDR_ENCODE(addr);
@@ -154,9 +152,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
 	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
 	unsigned last_pte, i;
 
-	scratch_pte = gen6_pte_encode(ppgtt->dev,
-				      ppgtt->scratch_page_dma_addr,
-				      I915_CACHE_LLC);
+	scratch_pte = ppgtt->pte_encode(ppgtt->dev,
+					ppgtt->scratch_page_dma_addr,
+					I915_CACHE_LLC);
 
 	while (num_entries) {
 		last_pte = first_pte + num_entries;
@@ -191,8 +189,8 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
 		dma_addr_t page_addr;
 
 		page_addr = sg_page_iter_dma_address(&sg_iter);
-		pt_vaddr[act_pte] = gen6_pte_encode(ppgtt->dev, page_addr,
-						    cache_level);
+		pt_vaddr[act_pte] = ppgtt->pte_encode(ppgtt->dev, page_addr,
+						      cache_level);
 		if (++act_pte == I915_PPGTT_PT_ENTRIES) {
 			kunmap_atomic(pt_vaddr);
 			act_pt++;
@@ -236,6 +234,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	first_pd_entry_in_global_pt =
 		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
 
+	ppgtt->pte_encode = gen6_pte_encode;
 	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
 	ppgtt->enable = gen6_ppgtt_enable;
 	ppgtt->clear_range = gen6_ppgtt_clear_range;
@@ -438,7 +437,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_page_iter_dma_address(&sg_iter);
-		iowrite32(gen6_pte_encode(dev, addr, level), &gtt_entries[i]);
+		iowrite32(dev_priv->gtt.pte_encode(dev, addr, level),
+			  &gtt_entries[i]);
 		i++;
 	}
 
@@ -450,7 +450,7 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
 	 */
 	if (i != 0)
 		WARN_ON(readl(&gtt_entries[i-1])
-			!= gen6_pte_encode(dev, addr, level));
+			!= dev_priv->gtt.pte_encode(dev, addr, level));
 
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
@@ -475,8 +475,9 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
 		 first_entry, num_entries, max_entries))
 		num_entries = max_entries;
 
-	scratch_pte = gen6_pte_encode(dev, dev_priv->gtt.scratch_page_dma,
-				      I915_CACHE_LLC);
+	scratch_pte = dev_priv->gtt.pte_encode(dev,
+					       dev_priv->gtt.scratch_page_dma,
+					       I915_CACHE_LLC);
 	for (i = 0; i < num_entries; i++)
 		iowrite32(scratch_pte, &gtt_base[i]);
 	readl(gtt_base);
@@ -823,6 +824,7 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	} else {
 		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
 		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
+		dev_priv->gtt.pte_encode = gen6_pte_encode;
 	}
 
 	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
-- 
1.8.2.1

[PATCH 2/3] drm/i915: Fix page table entries for Bay Trail.

[Kenneth Graunke]

On Bay Trail, bit 1 means "writeable by the GPU."  Failing to set that
means basically anything using the GPU will cause hangs.

v2: Drop accidental inline keyword on byt_pte_encode.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Jani Nikula <jani.nikula@intel.com> [v1]
Tested-by: Daniel Leung <daniel.leung@linux.intel.com> [v1]
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

To address Jani's comment about PPGTT - Daniel thinks Jesse has patches
for aliasing PPGTT floating around that could land at some point.  Also,
if we get real PPGTT, we'll also need this.  Either way, setting it up
is trivial and leaving it out could lead to bugs later.

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 92e147f..62058dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -73,6 +73,27 @@ static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
 	return pte;
 }
 
+#define BYT_PTE_WRITEABLE		(1 << 1)
+#define BYT_PTE_SNOOPED_BY_CPU_CACHES	(1 << 2)
+
+static gen6_gtt_pte_t byt_pte_encode(struct drm_device *dev,
+				     dma_addr_t addr,
+				     enum i915_cache_level level)
+{
+	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
+	pte |= GEN6_PTE_ADDR_ENCODE(addr);
+
+	/* Mark the page as writeable.  Other platforms don't have a
+	 * setting for read-only/writable, so this matches that behavior.
+	 */
+	pte |= BYT_PTE_WRITEABLE;
+
+	if (level != I915_CACHE_NONE)
+		pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES;
+
+	return pte;
+}
+
 static int gen6_ppgtt_enable(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -234,7 +255,11 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	first_pd_entry_in_global_pt =
 		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
 
-	ppgtt->pte_encode = gen6_pte_encode;
+	if (IS_VALLEYVIEW(dev)) {
+		ppgtt->pte_encode = byt_pte_encode;
+	} else {
+		ppgtt->pte_encode = gen6_pte_encode;
+	}
 	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
 	ppgtt->enable = gen6_ppgtt_enable;
 	ppgtt->clear_range = gen6_ppgtt_clear_range;
@@ -824,7 +849,11 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	} else {
 		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
 		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
-		dev_priv->gtt.pte_encode = gen6_pte_encode;
+		if (IS_VALLEYVIEW(dev)) {
+			dev_priv->gtt.pte_encode = byt_pte_encode;
+		} else {
+			dev_priv->gtt.pte_encode = gen6_pte_encode;
+		}
 	}
 
 	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
-- 
1.8.2.1

[PATCH 3/3] drm/i915: Split out Haswell code from gen6_pte_encode.

[Kenneth Graunke]

Now that we have function pointers, it's cleaner to just create a new
per-platform PTE encoding function.

This should be identical in behavior to the previous code.

v2: Drop accidental inline keyword on hsw_pte_encode.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Jani Nikula <jani.nikula@intel.com> [v1]
Tested-by: Daniel Leung <daniel.leung@linux.intel.com> [v1]
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 62058dc..ce024bd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -51,20 +51,13 @@ static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
 
 	switch (level) {
 	case I915_CACHE_LLC_MLC:
-		/* Haswell doesn't set L3 this way */
-		if (IS_HASWELL(dev))
-			pte |= GEN6_PTE_CACHE_LLC;
-		else
-			pte |= GEN6_PTE_CACHE_LLC_MLC;
+		pte |= GEN6_PTE_CACHE_LLC_MLC;
 		break;
 	case I915_CACHE_LLC:
 		pte |= GEN6_PTE_CACHE_LLC;
 		break;
 	case I915_CACHE_NONE:
-		if (IS_HASWELL(dev))
-			pte |= HSW_PTE_UNCACHED;
-		else
-			pte |= GEN6_PTE_UNCACHED;
+		pte |= GEN6_PTE_UNCACHED;
 		break;
 	default:
 		BUG();
@@ -94,6 +87,19 @@ static gen6_gtt_pte_t byt_pte_encode(struct drm_device *dev,
 	return pte;
 }
 
+static gen6_gtt_pte_t hsw_pte_encode(struct drm_device *dev,
+				     dma_addr_t addr,
+				     enum i915_cache_level level)
+{
+	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
+	pte |= GEN6_PTE_ADDR_ENCODE(addr);
+
+	if (level != I915_CACHE_NONE)
+		pte |= GEN6_PTE_CACHE_LLC;
+
+	return pte;
+}
+
 static int gen6_ppgtt_enable(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -255,7 +261,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	first_pd_entry_in_global_pt =
 		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
 
-	if (IS_VALLEYVIEW(dev)) {
+	if (IS_HASWELL(dev)) {
+		ppgtt->pte_encode = hsw_pte_encode;
+	} else if (IS_VALLEYVIEW(dev)) {
 		ppgtt->pte_encode = byt_pte_encode;
 	} else {
 		ppgtt->pte_encode = gen6_pte_encode;
@@ -849,7 +857,9 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	} else {
 		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
 		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
-		if (IS_VALLEYVIEW(dev)) {
+		if (IS_HASWELL(dev)) {
+			dev_priv->gtt.pte_encode = hsw_pte_encode;
+		} else if (IS_VALLEYVIEW(dev)) {
 			dev_priv->gtt.pte_encode = byt_pte_encode;
 		} else {
 			dev_priv->gtt.pte_encode = gen6_pte_encode;
-- 
1.8.2.1

Re: [PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[Jani Nikula]

On the series,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


On Mon, 22 Apr 2013, Kenneth Graunke <kenneth@whitecape.org> wrote:
> Sandybridge/Ivybridge, Bay Trail, and Haswell all have slightly
> different page table entry formats.  Rather than polluting one function
> with generation checks, simply use a function pointer and set up the
> correct PTE encoding function at startup.
>
> v2: Move the gen6_gtt_pte_t typedef to i915_drv.h so that the function
>     pointers and implementations have identical signatures.  Also remove
>     inline keyword on gen6_pte_encode.  Both suggested by Jani Nikula.
>
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com> [v1]
> Tested-by: Daniel Leung <daniel.leung@linux.intel.com> [v1]
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  8 ++++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 30 ++++++++++++++++--------------
>  2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bd2d7f1..d80bced 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -395,6 +395,8 @@ enum i915_cache_level {
>  	I915_CACHE_LLC_MLC, /* gen6+, in docs at least! */
>  };
>  
> +typedef uint32_t gen6_gtt_pte_t;
> +
>  /* The Graphics Translation Table is the way in which GEN hardware translates a
>   * Graphics Virtual Address into a Physical Address. In addition to the normal
>   * collateral associated with any va->pa translations GEN hardware also has a
> @@ -430,6 +432,9 @@ struct i915_gtt {
>  				   struct sg_table *st,
>  				   unsigned int pg_start,
>  				   enum i915_cache_level cache_level);
> +	gen6_gtt_pte_t (*pte_encode)(struct drm_device *dev,
> +				     dma_addr_t addr,
> +				     enum i915_cache_level level);
>  };
>  #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT)
>  
> @@ -451,6 +456,9 @@ struct i915_hw_ppgtt {
>  			       struct sg_table *st,
>  			       unsigned int pg_start,
>  			       enum i915_cache_level cache_level);
> +	gen6_gtt_pte_t (*pte_encode)(struct drm_device *dev,
> +				     dma_addr_t addr,
> +				     enum i915_cache_level level);
>  	int (*enable)(struct drm_device *dev);
>  	void (*cleanup)(struct i915_hw_ppgtt *ppgtt);
>  };
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 50df194..92e147f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -28,8 +28,6 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> -typedef uint32_t gen6_gtt_pte_t;
> -
>  /* PPGTT stuff */
>  #define GEN6_GTT_ADDR_ENCODE(addr)	((addr) | (((addr) >> 28) & 0xff0))
>  
> @@ -44,9 +42,9 @@ typedef uint32_t gen6_gtt_pte_t;
>  #define GEN6_PTE_CACHE_LLC_MLC		(3 << 1)
>  #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
>  
> -static inline gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
> -					     dma_addr_t addr,
> -					     enum i915_cache_level level)
> +static gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
> +				      dma_addr_t addr,
> +				      enum i915_cache_level level)
>  {
>  	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
>  	pte |= GEN6_PTE_ADDR_ENCODE(addr);
> @@ -154,9 +152,9 @@ static void gen6_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
>  	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
>  	unsigned last_pte, i;
>  
> -	scratch_pte = gen6_pte_encode(ppgtt->dev,
> -				      ppgtt->scratch_page_dma_addr,
> -				      I915_CACHE_LLC);
> +	scratch_pte = ppgtt->pte_encode(ppgtt->dev,
> +					ppgtt->scratch_page_dma_addr,
> +					I915_CACHE_LLC);
>  
>  	while (num_entries) {
>  		last_pte = first_pte + num_entries;
> @@ -191,8 +189,8 @@ static void gen6_ppgtt_insert_entries(struct i915_hw_ppgtt *ppgtt,
>  		dma_addr_t page_addr;
>  
>  		page_addr = sg_page_iter_dma_address(&sg_iter);
> -		pt_vaddr[act_pte] = gen6_pte_encode(ppgtt->dev, page_addr,
> -						    cache_level);
> +		pt_vaddr[act_pte] = ppgtt->pte_encode(ppgtt->dev, page_addr,
> +						      cache_level);
>  		if (++act_pte == I915_PPGTT_PT_ENTRIES) {
>  			kunmap_atomic(pt_vaddr);
>  			act_pt++;
> @@ -236,6 +234,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	first_pd_entry_in_global_pt =
>  		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
>  
> +	ppgtt->pte_encode = gen6_pte_encode;
>  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->enable = gen6_ppgtt_enable;
>  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> @@ -438,7 +437,8 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
>  
>  	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
>  		addr = sg_page_iter_dma_address(&sg_iter);
> -		iowrite32(gen6_pte_encode(dev, addr, level), &gtt_entries[i]);
> +		iowrite32(dev_priv->gtt.pte_encode(dev, addr, level),
> +			  &gtt_entries[i]);
>  		i++;
>  	}
>  
> @@ -450,7 +450,7 @@ static void gen6_ggtt_insert_entries(struct drm_device *dev,
>  	 */
>  	if (i != 0)
>  		WARN_ON(readl(&gtt_entries[i-1])
> -			!= gen6_pte_encode(dev, addr, level));
> +			!= dev_priv->gtt.pte_encode(dev, addr, level));
>  
>  	/* This next bit makes the above posting read even more important. We
>  	 * want to flush the TLBs only after we're certain all the PTE updates
> @@ -475,8 +475,9 @@ static void gen6_ggtt_clear_range(struct drm_device *dev,
>  		 first_entry, num_entries, max_entries))
>  		num_entries = max_entries;
>  
> -	scratch_pte = gen6_pte_encode(dev, dev_priv->gtt.scratch_page_dma,
> -				      I915_CACHE_LLC);
> +	scratch_pte = dev_priv->gtt.pte_encode(dev,
> +					       dev_priv->gtt.scratch_page_dma,
> +					       I915_CACHE_LLC);
>  	for (i = 0; i < num_entries; i++)
>  		iowrite32(scratch_pte, &gtt_base[i]);
>  	readl(gtt_base);
> @@ -823,6 +824,7 @@ int i915_gem_gtt_init(struct drm_device *dev)
>  	} else {
>  		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
>  		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
> +		dev_priv->gtt.pte_encode = gen6_pte_encode;
>  	}
>  
>  	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
> -- 
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Add PTE encoding function to the gtt/ppgtt vtables.

[Daniel Vetter]

On Mon, Apr 22, 2013 at 11:13:18AM +0300, Jani Nikula wrote:
> 
> On the series,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

All merged, thanks for the patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch