diff mbox

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

Message ID 1366316916-4164-3-git-send-email-kenneth@whitecape.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kenneth Graunke April 18, 2013, 8:28 p.m. UTC
On Bay Trail, bit 1 means "writeable by the GPU."  Failing to set that
means basically anything using the GPU will cause hangs.

Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Comments

Jani Nikula April 19, 2013, 6:08 a.m. UTC | #1
On Thu, 18 Apr 2013, Kenneth Graunke <kenneth@whitecape.org> wrote:
> On Bay Trail, bit 1 means "writeable by the GPU."  Failing to set that
> means basically anything using the GPU will cause hangs.
>
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9db65c1..db5c654 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -75,6 +75,27 @@ static inline 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 inline gen6_gtt_pte_t byt_pte_encode(struct drm_device *dev,

inline doesn't really make sense for functions used through
pointers... I wonder if the compiler should warn about taking address of
inline functions, or whether it just happily non-inlines. Too lazy to
check. ;)

Oh, this applies for patch 1 too; please drop inline from
gen6_pte_encode() there.

> +					    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;
> @@ -236,7 +257,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;
> +	}

HAS_ALIASING_PPGTT(dev) returns false for VLV. Do we need this in
preparation for future, or can we just drop this?

>  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->enable = gen6_ppgtt_enable;
>  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> @@ -826,7 +851,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;
> +		}

The rest looks good. With the bikesheds addressed (by patches or
counter-arguments ;)

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

>  	}
>  
>  	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
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9db65c1..db5c654 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -75,6 +75,27 @@  static inline 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 inline 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;
@@ -236,7 +257,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;
@@ -826,7 +851,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,