[PATCH] drm/i915: Set up an MTRR covering the GTT at driver load.

[PATCH] drm/i915: Set up an MTRR covering the GTT at driver load.

[Eric Anholt]

We'd love to just be using PAT, but even on chips with PAT it gets disabled
sometimes due to an errata.  It would probably be better to have pat_enabled
exported and only bother with this when !pat_enabled.
---
 drivers/gpu/drm/i915/i915_dma.c |   30 +++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |    6 ------
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 439d271..c391568 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -968,10 +968,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto kfree_devname;
 
-        dev_priv->mm.gtt_mapping =
-		io_mapping_create_wc(dev->agp->base,
-				     dev->agp->agp_info.aper_size * 1024*1024);
-
 	/* Allow hardware batchbuffers unless told otherwise.
 	 */
 	dev_priv->allow_batchbuffer = 1;
@@ -1083,6 +1079,24 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto free_priv;
 	}
 
+        dev_priv->mm.gtt_mapping =
+		io_mapping_create_wc(dev->agp->base,
+				     dev->agp->agp_info.aper_size * 1024*1024);
+	/* Set up a WC MTRR for non-PAT systems.  This is more common than
+	 * one would think, because the kernel disables PAT on first
+	 * generation Core chips because WC PAT gets overridden by a UC
+	 * MTRR if present.  Even if a UC MTRR isn't present.
+	 */
+	dev_priv->mm.gtt_mtrr = -1;
+	dev_priv->mm.gtt_mtrr = mtrr_add(dev->agp->base,
+					 dev->agp->agp_info.aper_size *
+					 1024 * 1024,
+					 MTRR_TYPE_WRCOMB, 1);
+	if (dev_priv->mm.gtt_mtrr < 0) {
+		DRM_INFO("MTRR allocation failed\n.  Graphics "
+			 "performance may suffer.\n");
+	}
+
 #ifdef CONFIG_HIGHMEM64G
 	/* don't enable GEM on PAE - needs agp + set_memory_* interface fixes */
 	dev_priv->has_gem = 0;
@@ -1147,8 +1161,14 @@ int i915_driver_unload(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	io_mapping_free(dev_priv->mm.gtt_mapping);
+	if (dev_priv->mm.gtt_mtrr >= 0) {
+		mtrr_del(dev_priv->mm.gtt_mtrr, dev->agp->base,
+			 dev->agp->agp_info.aper_size * 1024 * 1024);
+		dev_priv->mm.gtt_mtrr = -1;
+	}
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		io_mapping_free(dev_priv->mm.gtt_mapping);
 		drm_irq_uninstall(dev);
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e135182..f471d21 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -284,6 +284,7 @@ typedef struct drm_i915_private {
 		struct drm_mm gtt_space;
 
 		struct io_mapping *gtt_mapping;
+		int gtt_mtrr;
 
 		/**
 		 * List of objects currently involved in rendering from the
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3741101..66a2dc1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3229,10 +3229,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		dev_priv->mm.wedged = 0;
 	}
 
-	dev_priv->mm.gtt_mapping = io_mapping_create_wc(dev->agp->base,
-							dev->agp->agp_info.aper_size
-							* 1024 * 1024);
-
 	mutex_lock(&dev->struct_mutex);
 	dev_priv->mm.suspended = 0;
 
@@ -3255,7 +3251,6 @@ int
 i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
 	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -3264,7 +3259,6 @@ i915_gem_leavevt_ioctl(struct drm_device *dev, void *data,
 	ret = i915_gem_idle(dev);
 	drm_irq_uninstall(dev);
 
-	io_mapping_free(dev_priv->mm.gtt_mapping);
 	return ret;
 }
 
-- 
1.5.6.5

[PATCH] x86: Work around PAGE_KERNEL_WC not getting WC in iomap_atomic_prot_pfn.

[Eric Anholt]

In the absence of PAT, PAGE_KERNEL_WC ends up mapping to a memory type that
gets UC behavior even in the presence of a WC MTRR covering the area in
question.  By swapping to PAGE_KERNEL_UC_MINUS, we can get the actual
behavior the caller wanted (WC if you can manage it, UC otherwise).

This is recovers the 40% performance improvement of using WC in the DRM
to upload vertex data.
---
 arch/x86/mm/iomap_32.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/iomap_32.c b/arch/x86/mm/iomap_32.c
index d0151d8..b5e3964 100644
--- a/arch/x86/mm/iomap_32.c
+++ b/arch/x86/mm/iomap_32.c
@@ -17,6 +17,7 @@
  */
 
 #include <asm/iomap.h>
+#include <asm/pat.h>
 #include <linux/module.h>
 
 /* Map 'pfn' using fixed map 'type' and protections 'prot'
@@ -29,6 +30,14 @@ iomap_atomic_prot_pfn(unsigned long pfn, enum km_type type, pgprot_t prot)
 
 	pagefault_disable();
 
+	/* For non-PAT systems, promote PAGE_KERNEL_WC to PAGE_KERNEL_UC_MINUS.
+	 * PAGE_KERNEL_WC maps to PWT, which translates to uncached if the
+	 * MTRR is UC or WC.  UC_MINUS gets the real intention, of the
+	 * user, which is "WC if the MTRR is WC, UC if you can't do that."
+	 */
+	if (!pat_enabled && pgprot_val(prot) == pgprot_val(PAGE_KERNEL_WC))
+	  prot = PAGE_KERNEL_UC_MINUS;
+
 	idx = type + KM_TYPE_NR*smp_processor_id();
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 	set_pte(kmap_pte-idx, pfn_pte(pfn, prot));
-- 
1.5.6.5

Re: [PATCH] drm/i915: Set up an MTRR covering the GTT at driver load.

[Roland Dreier]

 > +	dev_priv->mm.gtt_mtrr = -1;
 > +	dev_priv->mm.gtt_mtrr = mtrr_add(dev->agp->base,
 > +					 dev->agp->agp_info.aper_size *
 > +					 1024 * 1024,
 > +					 MTRR_TYPE_WRCOMB, 1);

Two assignments to gtt_mtrr in a row looks a bit odd?

 - R.

Re: [PATCH] x86: Work around PAGE_KERNEL_WC not getting WC in iomap_atomic_prot_pfn.

[Roland Dreier]

 > +	if (!pat_enabled && pgprot_val(prot) == pgprot_val(PAGE_KERNEL_WC))
 > +	  prot = PAGE_KERNEL_UC_MINUS;

indentation broke here (the "prot =" line is indented with a tab then
two spaces, instead of two tabs)

 - R.

Re: [PATCH] x86: Work around PAGE_KERNEL_WC not getting WC in iomap_atomic_prot_pfn.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Fri, 2009-01-23 at 15:15 -0800, Roland Dreier wrote:
> > +	if (!pat_enabled && pgprot_val(prot) == pgprot_val(PAGE_KERNEL_WC))
>  > +	  prot = PAGE_KERNEL_UC_MINUS;
> 
> indentation broke here (the "prot =" line is indented with a tab then
> two spaces, instead of two tabs)

Bad path matching going on in my .emacs indentation setup.  I don't get
out of the DRM driver often enough.  Or rather, I'm in nasty X code too
often :)

Fixed this and the other nitpick.  Thanks!

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] drm/i915: Set up an MTRR covering the GTT at driver load.

[Olivier Galibert]

On Fri, Jan 23, 2009 at 02:54:17PM -0800, Eric Anholt wrote:
> +	/* Set up a WC MTRR for non-PAT systems.  This is more common than
> +	 * one would think, because the kernel disables PAT on first
> +	 * generation Core chips because WC PAT gets overridden by a UC
> +	 * MTRR if present.  Even if a UC MTRR isn't present.

As a non-native speaker, I don't understand what the last sentence
means.  X gets overridden by Y even if there's no Y doesn't make
sense.  Could you do a simpler version?

  OG.