[RFC] AGP initial support for chipset flushing..

[RFC] AGP initial support for chipset flushing..

[Dave Airlie]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 601 bytes --]


Hi,

We've uncovered a need when using the new memory manager to flush the 
chipset global write buffers on certain intel chipset due to a lack of 
coherency..

The attached patches add a new AGP interface  for this purpose and 
implements this in the Intel AGP driver. This stuff is based of some 
guesswork in the 915 case from comments in the documentation :).

Unfortuantely the 965 BIOS doesn't set this stuff up properly and it 
doesn't use a standard BAR address, so I have to do it by hand, I'd 
appreciate any commentary particularly in the setting up of the resource 
stuff.

Regards,
Dave.

[-- Attachment #2: Type: TEXT/x-diff, Size: 6128 bytes --]

From b9dcf514d0f1f61dc482cac622ffd2d79d500bf8 Mon Sep 17 00:00:00 2001
From: Dave Airlie <airlied@linux.ie>
Date: Mon, 29 Oct 2007 15:14:03 +1000
Subject: [PATCH] agp: add chipset flushing support to AGP interface

This bumps the AGP interface to 0.103.

Certain Intel chipsets contains a global write buffer, and this can require
flushing from the drm or X.org to make sure all data has hit RAM before
initiating a GPU transfer, due to a lack of coherency with the integrated
graphics device and this buffer.

This just adds generic support to the AGP interfaces, a follow-on patch
will add support to the Intel driver to use this interface.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/char/agp/agp.h          |    3 ++-
 drivers/char/agp/backend.c      |    2 +-
 drivers/char/agp/compat_ioctl.c |    4 ++++
 drivers/char/agp/compat_ioctl.h |    2 ++
 drivers/char/agp/frontend.c     |   11 +++++++++++
 drivers/char/agp/generic.c      |    7 +++++++
 include/linux/agp_backend.h     |    1 +
 include/linux/agpgart.h         |    1 +
 8 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
index b83824c..9ec9374 100644
--- a/drivers/char/agp/agp.h
+++ b/drivers/char/agp/agp.h
@@ -117,7 +117,8 @@ struct agp_bridge_driver {
 	void (*free_by_type)(struct agp_memory *);
 	void *(*agp_alloc_page)(struct agp_bridge_data *);
 	void (*agp_destroy_page)(void *, int flags);
-        int (*agp_type_to_mask_type) (struct agp_bridge_data *, int);
+	int (*agp_type_to_mask_type) (struct agp_bridge_data *, int);
+	void (*chipset_flush)(struct agp_bridge_data *);
 };
 
 struct agp_bridge_data {
diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
index 832ded2..f9c180c 100644
--- a/drivers/char/agp/backend.c
+++ b/drivers/char/agp/backend.c
@@ -43,7 +43,7 @@
  * fix some real stupidity. It's only by chance we can bump
  * past 0.99 at all due to some boolean logic error. */
 #define AGPGART_VERSION_MAJOR 0
-#define AGPGART_VERSION_MINOR 102
+#define AGPGART_VERSION_MINOR 103
 static const struct agp_version agp_current_version =
 {
 	.major = AGPGART_VERSION_MAJOR,
diff --git a/drivers/char/agp/compat_ioctl.c b/drivers/char/agp/compat_ioctl.c
index ecd4248..3927579 100644
--- a/drivers/char/agp/compat_ioctl.c
+++ b/drivers/char/agp/compat_ioctl.c
@@ -273,6 +273,10 @@ long compat_agp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case AGPIOC_UNBIND32:
 		ret_val = compat_agpioc_unbind_wrap(curr_priv, (void __user *) arg);
 		break;
+
+	case AGPIOC_CHIPSET_FLUSH32:
+		ret_val = agpioc_chipset_flush_wrap(curr_priv);
+		break;
 	}
 
 ioctl_out:
diff --git a/drivers/char/agp/compat_ioctl.h b/drivers/char/agp/compat_ioctl.h
index 71939d6..0c9678a 100644
--- a/drivers/char/agp/compat_ioctl.h
+++ b/drivers/char/agp/compat_ioctl.h
@@ -39,6 +39,7 @@
 #define AGPIOC_DEALLOCATE32 _IOW (AGPIOC_BASE, 7, compat_int_t)
 #define AGPIOC_BIND32       _IOW (AGPIOC_BASE, 8, compat_uptr_t)
 #define AGPIOC_UNBIND32     _IOW (AGPIOC_BASE, 9, compat_uptr_t)
+#define AGPIOC_CHIPSET_FLUSH32 _IO (AGPIOC_BASE, 10)
 
 struct agp_info32 {
 	struct agp_version version;	/* version of the driver        */
@@ -101,5 +102,6 @@ void agp_free_memory_wrap(struct agp_memory *memory);
 struct agp_memory *agp_allocate_memory_wrap(size_t pg_count, u32 type);
 struct agp_memory *agp_find_mem_by_key(int key);
 struct agp_client *agp_find_client_by_pid(pid_t id);
+int agpioc_chipset_flush_wrap(struct agp_file_private *priv);
 
 #endif /* _AGP_COMPAT_H */
diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c
index 7791e98..9bd5a95 100644
--- a/drivers/char/agp/frontend.c
+++ b/drivers/char/agp/frontend.c
@@ -960,6 +960,13 @@ static int agpioc_unbind_wrap(struct agp_file_private *priv, void __user *arg)
 	return agp_unbind_memory(memory);
 }
 
+int agpioc_chipset_flush_wrap(struct agp_file_private *priv)
+{
+	DBG("");
+	agp_flush_chipset(agp_bridge);
+	return 0;
+}
+
 static int agp_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
@@ -1033,6 +1040,10 @@ static int agp_ioctl(struct inode *inode, struct file *file,
 	case AGPIOC_UNBIND:
 		ret_val = agpioc_unbind_wrap(curr_priv, (void __user *) arg);
 		break;
+	       
+	case AGPIOC_CHIPSET_FLUSH:
+		ret_val = agpioc_chipset_flush_wrap(curr_priv);
+		break;
 	}
 
 ioctl_out:
diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
index 64b2f6d..8c67b4f 100644
--- a/drivers/char/agp/generic.c
+++ b/drivers/char/agp/generic.c
@@ -80,6 +80,13 @@ static int agp_get_key(void)
 	return -1;
 }
 
+void agp_flush_chipset(struct agp_bridge_data *bridge)
+{
+	if (bridge->driver->chipset_flush)
+		bridge->driver->chipset_flush(bridge);
+}
+EXPORT_SYMBOL(agp_flush_chipset);
+
 /*
  * Use kmalloc if possible for the page list. Otherwise fall back to
  * vmalloc. This speeds things up and also saves memory for small AGP
diff --git a/include/linux/agp_backend.h b/include/linux/agp_backend.h
index abc521c..03e3454 100644
--- a/include/linux/agp_backend.h
+++ b/include/linux/agp_backend.h
@@ -109,6 +109,7 @@ extern int agp_unbind_memory(struct agp_memory *);
 extern void agp_enable(struct agp_bridge_data *, u32);
 extern struct agp_bridge_data *agp_backend_acquire(struct pci_dev *);
 extern void agp_backend_release(struct agp_bridge_data *);
+extern void agp_flush_chipset(struct agp_bridge_data *);
 
 #endif				/* __KERNEL__ */
 #endif				/* _AGP_BACKEND_H */
diff --git a/include/linux/agpgart.h b/include/linux/agpgart.h
index 09fbf7e..62aef58 100644
--- a/include/linux/agpgart.h
+++ b/include/linux/agpgart.h
@@ -38,6 +38,7 @@
 #define AGPIOC_DEALLOCATE _IOW (AGPIOC_BASE, 7, int)
 #define AGPIOC_BIND       _IOW (AGPIOC_BASE, 8, struct agp_bind*)
 #define AGPIOC_UNBIND     _IOW (AGPIOC_BASE, 9, struct agp_unbind*)
+#define AGPIOC_CHIPSET_FLUSH _IO (AGPIOC_BASE, 10)
 
 #define AGP_DEVICE      "/dev/agpgart"
 
-- 
1.5.2.4


[-- Attachment #3: Type: TEXT/x-diff, Size: 6440 bytes --]

From fd4959b6ad931e6392e10ee466334d0fb2a05b94 Mon Sep 17 00:00:00 2001
From: Dave Airlie <airlied@linux.ie>
Date: Mon, 29 Oct 2007 18:06:10 +1000
Subject: [PATCH] intel-agp: add chipset flushing support

This adds support for flushing the chipsets on the 915, 945, 965 and G33
families of Intel chips.

The BIOS doesn't seem to always allocate the BAR on the 965 chipsets
so I have to use pci resource code to create a resource

It adds an export for pcibios_align_resource.
---
 arch/x86/pci/i386.c          |    2 +-
 drivers/char/agp/intel-agp.c |  102 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 42ba0e2..103b9df 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -72,7 +72,7 @@ pcibios_align_resource(void *data, struct resource *res,
 		}
 	}
 }
-
+EXPORT_SYMBOL(pcibios_align_resource);
 
 /*
  *  Handle resources of PCI devices.  If the world were perfect, we could
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index d879619..5be0d48 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -69,9 +69,11 @@ extern int agp_memory_reserved;
 #define I915_GMCH_GMS_STOLEN_64M	(0x7 << 4)
 #define G33_GMCH_GMS_STOLEN_128M       (0x8 << 4)
 #define G33_GMCH_GMS_STOLEN_256M       (0x9 << 4)
+#define I915_IFPADDR    0x60
 
 /* Intel 965G registers */
 #define I965_MSAC 0x62
+#define I965_IFPADDR    0x70
 
 /* Intel 7505 registers */
 #define INTEL_I7505_APSIZE	0x74
@@ -113,6 +115,8 @@ static struct _intel_private {
 	 * popup and for the GTT.
 	 */
 	int gtt_entries;			/* i830+ */
+	void __iomem *flush_page;
+	struct resource ifp_resource;
 } intel_private;
 
 static int intel_i810_fetch_size(void)
@@ -769,6 +773,73 @@ static struct agp_memory *intel_i830_alloc_by_type(size_t pg_count,int type)
 	return NULL;
 }
 
+static int intel_alloc_chipset_flush_resource(void)
+{
+	int ret;
+	ret = pci_bus_alloc_resource(agp_bridge->dev->bus, &intel_private.ifp_resource, PAGE_SIZE,
+				     PAGE_SIZE, PCIBIOS_MIN_MEM, 0,
+				     pcibios_align_resource, agp_bridge->dev);
+	if (ret != 0)
+		return ret;
+
+	printk("intel priv bus start %08lx\n", intel_private.ifp_resource.start);
+	return 0;
+}
+
+static void intel_i915_setup_chipset_flush(void)
+{
+	int ret;
+	u32 temp;
+
+	pci_read_config_dword(agp_bridge->dev, I915_IFPADDR, &temp);
+	if (!(temp & 0x1)) {
+		intel_alloc_chipset_flush_resource();
+
+		pci_write_config_dword(agp_bridge->dev, I915_IFPADDR, (intel_private.ifp_resource.start & 0xffffffff) | 0x1);
+	} else {
+		temp &= ~1;
+
+		intel_private.ifp_resource.start = temp;
+		intel_private.ifp_resource.end = temp + PAGE_SIZE;
+		ret = request_resource(&iomem_resource, &intel_private.ifp_resource);
+		if (ret) {
+			intel_private.ifp_resource.start = 0;
+			printk("Failed inserting resource into tree\n");
+		}
+	}
+}
+
+static void intel_i965_g33_setup_chipset_flush(void)
+{
+	u32 temp_hi, temp_lo;
+	int ret;
+
+	pci_read_config_dword(agp_bridge->dev, I965_IFPADDR + 4, &temp_hi);
+	pci_read_config_dword(agp_bridge->dev, I965_IFPADDR, &temp_lo);
+
+	if (!(temp_lo & 0x1)) {
+
+		intel_alloc_chipset_flush_resource();
+
+		pci_write_config_dword(agp_bridge->dev, I965_IFPADDR + 4, (intel_private.ifp_resource.start >> 32));
+		pci_write_config_dword(agp_bridge->dev, I965_IFPADDR, (intel_private.ifp_resource.start & 0xffffffff) | 0x1);
+		intel_private.flush_page = ioremap_nocache(intel_private.ifp_resource.start, PAGE_SIZE);
+	} else {
+		u64 l64;
+		
+		temp_lo &= ~0x1;
+		l64 = ((u64)temp_hi << 32) | temp_lo;
+
+		intel_private.ifp_resource.start = l64;
+		intel_private.ifp_resource.end = l64 + PAGE_SIZE;
+		ret = request_resource(&iomem_resource, &intel_private.ifp_resource);
+		if (!ret) {
+			intel_private.ifp_resource.start = 0;
+			printk("Failed inserting resource into tree\n");
+		}
+	}
+}
+
 static int intel_i915_configure(void)
 {
 	struct aper_size_info_fixed *current_size;
@@ -797,15 +868,43 @@ static int intel_i915_configure(void)
 	}
 
 	global_cache_flush();
+
+	/* setup a resource for this object */
+	memset(&intel_private.ifp_resource, 0, sizeof(intel_private.ifp_resource));
+
+	intel_private.ifp_resource.name = "Intel Flush Page";
+	intel_private.ifp_resource.flags = IORESOURCE_MEM;
+
+	/* Setup chipset flush for 915 */
+	if (IS_I965 || IS_G33) {
+		intel_i965_g33_setup_chipset_flush();
+	} else {
+		intel_i915_setup_chipset_flush();
+	}
+
+	if (intel_private.ifp_resource.start) {
+		intel_private.flush_page = ioremap_nocache(intel_private.ifp_resource.start, PAGE_SIZE);
+		if (!intel_private.flush_page)
+			printk("unable to ioremap flush  page - no chipset flushing");
+	}
+	
 	return 0;
 }
 
 static void intel_i915_cleanup(void)
 {
+	if (intel_private.flush_page)
+		iounmap(intel_private.flush_page);
 	iounmap(intel_private.gtt);
 	iounmap(intel_private.registers);
 }
 
+static void intel_i915_chipset_flush(struct agp_bridge_data *bridge)
+{
+	if (intel_private.flush_page)
+		writel(1, intel_private.flush_page);
+}
+
 static int intel_i915_insert_entries(struct agp_memory *mem,off_t pg_start,
 				int type)
 {
@@ -1722,6 +1821,7 @@ static const struct agp_bridge_driver intel_915_driver = {
 	.agp_alloc_page		= agp_generic_alloc_page,
 	.agp_destroy_page	= agp_generic_destroy_page,
 	.agp_type_to_mask_type  = intel_i830_type_to_mask_type,
+	.chipset_flush		= intel_i915_chipset_flush,
 };
 
 static const struct agp_bridge_driver intel_i965_driver = {
@@ -1747,6 +1847,7 @@ static const struct agp_bridge_driver intel_i965_driver = {
        .agp_alloc_page         = agp_generic_alloc_page,
        .agp_destroy_page       = agp_generic_destroy_page,
        .agp_type_to_mask_type  = intel_i830_type_to_mask_type,
+	.chipset_flush		= intel_i915_chipset_flush,
 };
 
 static const struct agp_bridge_driver intel_7505_driver = {
@@ -1796,6 +1897,7 @@ static const struct agp_bridge_driver intel_g33_driver = {
 	.agp_alloc_page         = agp_generic_alloc_page,
 	.agp_destroy_page       = agp_generic_destroy_page,
 	.agp_type_to_mask_type  = intel_i830_type_to_mask_type,
+	.chipset_flush		= intel_i915_chipset_flush,
 };
 
 static int find_gmch(u16 device)
-- 
1.5.2.4

Re: [RFC] AGP initial support for chipset flushing..

[Keith Packard]

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


On Mon, 2007-10-29 at 08:15 +0000, Dave Airlie wrote:

> The attached patches add a new AGP interface  for this purpose and 
> implements this in the Intel AGP driver. This stuff is based of some 
> guesswork in the 915 case from comments in the documentation :).

The relevant register lives in device 0, which is why this is an AGP
interface and not just hidden inside the DRM driver directly.

-- 
keith.packard@intel.com

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

Re: [RFC] AGP initial support for chipset flushing..

[Jesse Barnes]

On Monday, October 29, 2007 1:15 am Dave Airlie wrote:
> Hi,
>
> We've uncovered a need when using the new memory manager to flush the
> chipset global write buffers on certain intel chipset due to a lack
> of coherency..
>
> The attached patches add a new AGP interface  for this purpose and
> implements this in the Intel AGP driver. This stuff is based of some
> guesswork in the 915 case from comments in the documentation :).

In this case, we're performing basically a dma_sync*(...DMA_TO_DEVICE) 
right?  Can we be sure that a single flush is sufficient?  Is there any 
window between when we flush and when we start accessing memory with 
the device that we could get into more caching trouble?

> Unfortuantely the 965 BIOS doesn't set this stuff up properly and it
> doesn't use a standard BAR address, so I have to do it by hand, I'd
> appreciate any commentary particularly in the setting up of the
> resource stuff.

Looks reasonable, I'm not sure we can do much better.  The only concern 
I have is that allocating some more PCI space like that may end up 
clobbering some *other* hidden BIOS mapping, but there's not a whole 
lot we can do about that.

Jesse

Re: [RFC] AGP initial support for chipset flushing..

[Dave Airlie]

> 
> In this case, we're performing basically a dma_sync*(...DMA_TO_DEVICE) 
> right?  Can we be sure that a single flush is sufficient?  Is there any 
> window between when we flush and when we start accessing memory with 
> the device that we could get into more caching trouble?

Not that I can think off, but I don't work for the company who screwed up 
the coherency :-), and I don't have the docs, so please investigate for me 
;-)

> Looks reasonable, I'm not sure we can do much better.  The only concern 
> I have is that allocating some more PCI space like that may end up 
> clobbering some *other* hidden BIOS mapping, but there's not a whole 
> lot we can do about that.

Again I'm trying to workaround broken BIOS.. nothing I can do.

Dave.

Re: [RFC] AGP initial support for chipset flushing..

[Jesse Barnes]

On Monday, October 29, 2007 12:52 pm Dave Airlie wrote:
> > In this case, we're performing basically a
> > dma_sync*(...DMA_TO_DEVICE) right?  Can we be sure that a single
> > flush is sufficient?  Is there any window between when we flush and
> > when we start accessing memory with the device that we could get
> > into more caching trouble?
>
> Not that I can think off, but I don't work for the company who
> screwed up the coherency :-), and I don't have the docs, so please
> investigate for me ;-)

It *looks* like it'll be enough.  I assume Keith has talked to the 
chipset guys to confirm this.

> > Looks reasonable, I'm not sure we can do much better.  The only
> > concern I have is that allocating some more PCI space like that may
> > end up clobbering some *other* hidden BIOS mapping, but there's not
> > a whole lot we can do about that.
>
> Again I'm trying to workaround broken BIOS.. nothing I can do.

Right, BIOSes are so much fun to deal with.  One other thing:  it looks 
like the flush mmio space has to be allocated above the top of DRAM but 
below 4G.  I wonder if there's an easy way to guarantee this with the 
pci_bus* routines...

Jesse

Re: [RFC] AGP initial support for chipset flushing..

[Keith Packard]

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


On Mon, 2007-10-29 at 12:47 -0700, Jesse Barnes wrote:

> In this case, we're performing basically a dma_sync*(...DMA_TO_DEVICE) 
> right?

But this is just for the GPU; every other DMA device in the system is
cache-coherent.

>   Can we be sure that a single flush is sufficient?  Is there any 
> window between when we flush and when we start accessing memory with 
> the device that we could get into more caching trouble?

An uncached write to this page will not complete until the buffers are
completely flushed.

> Looks reasonable, I'm not sure we can do much better.  The only concern 
> I have is that allocating some more PCI space like that may end up 
> clobbering some *other* hidden BIOS mapping, but there's not a whole 
> lot we can do about that.

This isn't a hidden mapping; the i965 doesn't allocate space for it in
the BIOS.

-- 
keith.packard@intel.com

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

Re: [RFC] AGP initial support for chipset flushing..

[Jesse Barnes]

On Monday, October 29, 2007 1:12 pm Keith Packard wrote:
> On Mon, 2007-10-29 at 12:47 -0700, Jesse Barnes wrote:
> > In this case, we're performing basically a
> > dma_sync*(...DMA_TO_DEVICE) right?
>
> But this is just for the GPU; every other DMA device in the system is
> cache-coherent.

Right.

> >   Can we be sure that a single flush is sufficient?  Is there any
> > window between when we flush and when we start accessing memory
> > with the device that we could get into more caching trouble?
>
> An uncached write to this page will not complete until the buffers
> are completely flushed.

Yeah, so we should be safe.

> > Looks reasonable, I'm not sure we can do much better.  The only
> > concern I have is that allocating some more PCI space like that may
> > end up clobbering some *other* hidden BIOS mapping, but there's not
> > a whole lot we can do about that.
>
> This isn't a hidden mapping; the i965 doesn't allocate space for it
> in the BIOS.

I know, that's what I'm worried about.  If the BIOS is broken enough to 
not allocate MMIO space for the flush page, it may also be broken 
enough that our hand crafted MMIO space allocation will end up 
conflicting with some unreported BIOS area, which would be bad.

Jesse