[intel-agp] Rewrite GTT on resume

[intel-agp] Rewrite GTT on resume

[Keith Packard]

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

On my Intel chipset (965GM), the GTT is entirely erased across
suspend/resume. This patch simply re-plays the current mapping at resume
time to restore the table. 

I noticed this once I started relying on persistent GTT mappings across
VT switch in our GEM work -- the old X server and DRM code carefully
unbind all memory from the GTT on VT switch, but GEM does not bother.

I placed the list management and rewrite code in the generic layer on
the assumption that it will be needed on other hardware, but I did not
add the rewrite call to anything other than the Intel resume function.


commit 1233057731935fb5e9fd115d3d2985802ab636c8
Author: Keith Packard <keithp@keithp.com>
Date:   Tue Jun 3 20:34:54 2008 -0700

    [INTEL-AGP] Re-write GATT on resume
    
    Keep a list of current GATT mappings. At resume time, rewrite them into the
    GATT. This is needed on Intel (at least) as the entire GATT is cleared
    across suspend/resume.

diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
index c69f795..253f56f 100644
--- a/drivers/char/agp/agp.h
+++ b/drivers/char/agp/agp.h
@@ -148,6 +148,8 @@ struct agp_bridge_data {
 	char minor_version;
 	struct list_head list;
 	u32 apbase_config;
+	/* list of agp_memory mapped to the aperture */
+	struct list_head mapped_list;
 };
 
 #define KB(x)	((x) * 1024)
diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
index b1bdd01..a5f0ce2 100644
--- a/drivers/char/agp/backend.c
+++ b/drivers/char/agp/backend.c
@@ -183,6 +183,7 @@ static int agp_backend_initialize(struct agp_bridge_data *bridge)
 		rc = -EINVAL;
 		goto err_out;
 	}
+	INIT_LIST_HEAD(&bridge->mapped_list);
 
 	return 0;
 
diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
index 7fc0c99..cf99078 100644
--- a/drivers/char/agp/generic.c
+++ b/drivers/char/agp/generic.c
@@ -426,6 +426,8 @@ int agp_bind_memory(struct agp_memory *curr, off_t pg_start)
 
 	curr->is_bound = TRUE;
 	curr->pg_start = pg_start;
+	list_add (&curr->mapped_list, &agp_bridge->mapped_list);
+	
 	return 0;
 }
 EXPORT_SYMBOL(agp_bind_memory);
@@ -458,10 +460,30 @@ int agp_unbind_memory(struct agp_memory *curr)
 
 	curr->is_bound = FALSE;
 	curr->pg_start = 0;
+	list_del (&curr->mapped_list);
 	return 0;
 }
 EXPORT_SYMBOL(agp_unbind_memory);
 
+/**
+ *	agp_rebind_emmory  -  Rewrite the entire GATT, useful on resume
+ */
+int agp_rebind_memory (void)
+{
+	struct agp_memory *curr;
+	int ret_val;
+	
+	list_for_each_entry(curr, &agp_bridge->mapped_list, mapped_list) {
+		ret_val = curr->bridge->driver->insert_memory(curr,
+							      curr->pg_start,
+							      curr->type);
+		if (ret_val != 0)
+			return ret_val;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(agp_rebind_memory);
+
 /* End - Routines for handling swapping of agp_memory into the GATT */
 
 
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index eeea50a..7f1a96e 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -2176,6 +2176,7 @@ static void __devexit agp_intel_remove(struct pci_dev *pdev)
 static int agp_intel_resume(struct pci_dev *pdev)
 {
 	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
+	int ret_val;
 
 	pci_restore_state(pdev);
 
@@ -2203,6 +2204,10 @@ static int agp_intel_resume(struct pci_dev *pdev)
 	else if (bridge->driver == &intel_i965_driver)
 		intel_i915_configure();
 
+	ret_val = agp_rebind_memory();
+	if (ret_val != 0)
+		return ret_val;
+
 	return 0;
 }
 #endif
diff --git a/include/linux/agp_backend.h b/include/linux/agp_backend.h
index 03e3454..2fbfe4d 100644
--- a/include/linux/agp_backend.h
+++ b/include/linux/agp_backend.h
@@ -32,6 +32,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/list.h>
+
 #ifndef TRUE
 #define TRUE 1
 #endif
@@ -88,6 +90,8 @@ struct agp_memory {
 	u8 is_bound;
 	u8 is_flushed;
         u8 vmalloc_flag;
+	/* list of agp_memory mapped to the aperture */
+	struct list_head mapped_list;
 };
 
 #define AGP_NORMAL_MEMORY 0
@@ -106,6 +110,7 @@ extern struct agp_memory *agp_allocate_memory(struct agp_bridge_data *, size_t,
 extern int agp_copy_info(struct agp_bridge_data *, struct agp_kern_info *);
 extern int agp_bind_memory(struct agp_memory *, off_t);
 extern int agp_unbind_memory(struct agp_memory *);
+extern int agp_rebind_memory (void);
 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 *);

-- 
keith.packard@intel.com

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

Re: [intel-agp] Rewrite GTT on resume

[Andrew Morton]

On Tue, 03 Jun 2008 20:41:20 -0700
Keith Packard <keithp@keithp.com> wrote:

> On my Intel chipset (965GM), the GTT is entirely erased across
> suspend/resume. This patch simply re-plays the current mapping at resume
> time to restore the table. 
> 
> I noticed this once I started relying on persistent GTT mappings across
> VT switch in our GEM work -- the old X server and DRM code carefully
> unbind all memory from the GTT on VT switch, but GEM does not bother.
> 
> I placed the list management and rewrite code in the generic layer on
> the assumption that it will be needed on other hardware, but I did not
> add the rewrite call to anything other than the Intel resume function.
> 
> 
> commit 1233057731935fb5e9fd115d3d2985802ab636c8
> Author: Keith Packard <keithp@keithp.com>
> Date:   Tue Jun 3 20:34:54 2008 -0700
> 
>     [INTEL-AGP] Re-write GATT on resume
>     
>     Keep a list of current GATT mappings. At resume time, rewrite them into the
>     GATT. This is needed on Intel (at least) as the entire GATT is cleared
>     across suspend/resume.

Please send a signed-off-by:?

> diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
> index c69f795..253f56f 100644
> --- a/drivers/char/agp/agp.h
> +++ b/drivers/char/agp/agp.h
> @@ -148,6 +148,8 @@ struct agp_bridge_data {
>  	char minor_version;
>  	struct list_head list;
>  	u32 apbase_config;
> +	/* list of agp_memory mapped to the aperture */
> +	struct list_head mapped_list;
>  };

Is it safe to use this without locking?

Re: [intel-agp] Rewrite GTT on resume

[Keith Packard]

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

On Fri, 2008-06-06 at 13:50 -0700, Andrew Morton wrote:

> Please send a signed-off-by:?

See below.

> Is it safe to use this without locking?

I don't know. There is a mutex in the agp frontend exposed to user mode,
but that is not used by the internal kernel interfaces used by drm. This
scares me a bit, but the agp code doesn't appear to have any kind of
internal data structures that might be messed up, so it 'should' be OK.

This version adds a spinlock around all of the mapped_list manipulation.

From 26b58f13ecbe95a7eaca7434aeb885def057b835 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Tue, 3 Jun 2008 20:34:54 -0700
Subject: [PATCH] [INTEL-AGP] Re-write GATT on resume

Keep a list of current GATT mappings. At resume time, rewrite them into the
GATT. This is needed on Intel (at least) as the entire GATT is cleared
across suspend/resume. The list is protected by a spinlock.

Signed-off-by: Keith Packard <keithp@keithp.com>

diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
index c69f795..5fdbbd7 100644
--- a/drivers/char/agp/agp.h
+++ b/drivers/char/agp/agp.h
@@ -148,6 +148,9 @@ struct agp_bridge_data {
 	char minor_version;
 	struct list_head list;
 	u32 apbase_config;
+	/* list of agp_memory mapped to the aperture */
+	struct list_head mapped_list;
+	spinlock_t mapped_lock;
 };
 
 #define KB(x)	((x) * 1024)
diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
index b1bdd01..463f04b 100644
--- a/drivers/char/agp/backend.c
+++ b/drivers/char/agp/backend.c
@@ -183,6 +183,8 @@ static int agp_backend_initialize(struct agp_bridge_data *bridge)
 		rc = -EINVAL;
 		goto err_out;
 	}
+	INIT_LIST_HEAD(&bridge->mapped_list);
+	spin_lock_init(&bridge->mapped_lock);
 
 	return 0;
 
diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
index 7fc0c99..4741438 100644
--- a/drivers/char/agp/generic.c
+++ b/drivers/char/agp/generic.c
@@ -426,6 +426,10 @@ int agp_bind_memory(struct agp_memory *curr, off_t pg_start)
 
 	curr->is_bound = TRUE;
 	curr->pg_start = pg_start;
+	spin_lock(&agp_bridge->mapped_lock);
+	list_add (&curr->mapped_list, &agp_bridge->mapped_list);
+	spin_unlock(&agp_bridge->mapped_lock);
+	
 	return 0;
 }
 EXPORT_SYMBOL(agp_bind_memory);
@@ -458,10 +462,34 @@ int agp_unbind_memory(struct agp_memory *curr)
 
 	curr->is_bound = FALSE;
 	curr->pg_start = 0;
+	spin_lock(&curr->bridge->mapped_lock);
+	list_del (&curr->mapped_list);
+	spin_unlock(&curr->bridge->mapped_lock);
 	return 0;
 }
 EXPORT_SYMBOL(agp_unbind_memory);
 
+/**
+ *	agp_rebind_emmory  -  Rewrite the entire GATT, useful on resume
+ */
+int agp_rebind_memory (void)
+{
+	struct agp_memory *curr;
+	int ret_val;
+	
+	spin_lock(&agp_bridge->mapped_lock);
+	list_for_each_entry(curr, &agp_bridge->mapped_list, mapped_list) {
+		ret_val = curr->bridge->driver->insert_memory(curr,
+							      curr->pg_start,
+							      curr->type);
+		if (ret_val != 0)
+			return ret_val;
+	}
+	spin_unlock(&agp_bridge->mapped_lock);
+	return 0;
+}
+EXPORT_SYMBOL(agp_rebind_memory);
+
 /* End - Routines for handling swapping of agp_memory into the GATT */
 
 
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index eeea50a..7f1a96e 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -2176,6 +2176,7 @@ static void __devexit agp_intel_remove(struct pci_dev *pdev)
 static int agp_intel_resume(struct pci_dev *pdev)
 {
 	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
+	int ret_val;
 
 	pci_restore_state(pdev);
 
@@ -2203,6 +2204,10 @@ static int agp_intel_resume(struct pci_dev *pdev)
 	else if (bridge->driver == &intel_i965_driver)
 		intel_i915_configure();
 
+	ret_val = agp_rebind_memory();
+	if (ret_val != 0)
+		return ret_val;
+
 	return 0;
 }
 #endif
diff --git a/include/linux/agp_backend.h b/include/linux/agp_backend.h
index 03e3454..2fbfe4d 100644
--- a/include/linux/agp_backend.h
+++ b/include/linux/agp_backend.h
@@ -32,6 +32,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/list.h>
+
 #ifndef TRUE
 #define TRUE 1
 #endif
@@ -88,6 +90,8 @@ struct agp_memory {
 	u8 is_bound;
 	u8 is_flushed;
         u8 vmalloc_flag;
+	/* list of agp_memory mapped to the aperture */
+	struct list_head mapped_list;
 };
 
 #define AGP_NORMAL_MEMORY 0
@@ -106,6 +110,7 @@ extern struct agp_memory *agp_allocate_memory(struct agp_bridge_data *, size_t,
 extern int agp_copy_info(struct agp_bridge_data *, struct agp_kern_info *);
 extern int agp_bind_memory(struct agp_memory *, off_t);
 extern int agp_unbind_memory(struct agp_memory *);
+extern int agp_rebind_memory (void);
 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 *);

-- 
keith.packard@intel.com

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

Re: [intel-agp] Rewrite GTT on resume

[Pavel Machek]

Hi!

> From 26b58f13ecbe95a7eaca7434aeb885def057b835 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp@keithp.com>
> Date: Tue, 3 Jun 2008 20:34:54 -0700
> Subject: [PATCH] [INTEL-AGP] Re-write GATT on resume
> 
> Keep a list of current GATT mappings. At resume time, rewrite them into the
> GATT. This is needed on Intel (at least) as the entire GATT is cleared
> across suspend/resume. The list is protected by a spinlock.

Is this needed in case of iommu enabled, agp disabled, too?

> diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> index eeea50a..7f1a96e 100644
> --- a/drivers/char/agp/intel-agp.c
> +++ b/drivers/char/agp/intel-agp.c
> @@ -2176,6 +2176,7 @@ static void __devexit agp_intel_remove(struct pci_dev *pdev)
>  static int agp_intel_resume(struct pci_dev *pdev)
>  {
>  	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
> +	int ret_val;
>  
>  	pci_restore_state(pdev);
>  
> @@ -2203,6 +2204,10 @@ static int agp_intel_resume(struct pci_dev *pdev)
>  	else if (bridge->driver == &intel_i965_driver)
>  		intel_i915_configure();
>  
> +	ret_val = agp_rebind_memory();
> +	if (ret_val != 0)
> +		return ret_val;
> +
>  	return 0;
>  }
>  #endif

Should this move early (sysdev?) because other drivers may depend on
iommu working?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [intel-agp] Rewrite GTT on resume

[Keith Packard]

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

On Sun, 2008-06-08 at 09:13 +0200, Pavel Machek wrote:

> Is this needed in case of iommu enabled, agp disabled, too?

I don't understand this question; if AGP isn't being used, then there
won't be any AGP mappings to rewrite.

> Should this move early (sysdev?) because other drivers may depend on
> iommu working?

I don't need AGP running until userspace is active again; however, that
may change once we move graphics mode setting into the kernel. Dave will
assuredly have a better idea about this than I do.

-- 
keith.packard@intel.com

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