[PATCH] Revert "drm/i915: Don't save/restore hardware status page address register"

[PATCH] Revert "drm/i915: Don't save/restore hardware status page address register"

[Chris Wilson]

This reverts commit a7a75c8f70d6f6a2f16c9f627f938bbee2d32718.

There are two different variations on how Intel hardware addresses the
"Hardware Status Page". One as a location in physical memory and the
other as an offset into the virtual memory of the GPU, used in more
recent chipsets. (The HWS itself is a cacheable region of memory which
the GPU can write to without requiring CPU synchronisation, used for
updating various details of hardware state, such as the position of
the GPU head in the ringbuffer, the last breadcrumb seqno, etc).

These two types of addresses were updated in different locations of code
- one inline with the ringbuffer initialisation, and the other during
device initialisation. (The HWS page is logically associated with
the rings, and there is one HWS page per ring.) During resume, only the
ringbuffers were being re-initialised along with the virtual HWS page,
leaving the older physical address HWS untouched. This then caused a
hang on the older gen3/4 (915GM, 945GM, 965GM) the first time we tried
to synchronise the GPU as the breadcrumbs were never being updated.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Jan Niehusmann <jan@gondor.com>
Reported-by: Justin P. Mattock <justinmattock@gmail.com>
Reported-and-tested-by: Michael "brot" Groh <brot@minad.de>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_suspend.c |    6 ++++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4496505..5004724 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -383,6 +383,7 @@ typedef struct drm_i915_private {
 	u32 saveDSPACNTR;
 	u32 saveDSPBCNTR;
 	u32 saveDSPARB;
+	u32 saveHWS;
 	u32 savePIPEACONF;
 	u32 savePIPEBCONF;
 	u32 savePIPEASRC;
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 7e992a8..da47415 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -796,6 +796,9 @@ int i915_save_state(struct drm_device *dev)
 
 	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
 
+	/* Hardware status page */
+	dev_priv->saveHWS = I915_READ(HWS_PGA);
+
 	i915_save_display(dev);
 
 	/* Interrupt state */
@@ -842,6 +845,9 @@ int i915_restore_state(struct drm_device *dev)
 
 	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
 
+	/* Hardware status page */
+	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
+
 	i915_restore_display(dev);
 
 	/* Interrupt state */
-- 
1.7.4.1

Re: [PATCH] Revert "drm/i915: Don't save/restore hardware status page address register"

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3229 bytes --]

On 2011.03.23 18:16:55 +0000, Chris Wilson wrote:
> This reverts commit a7a75c8f70d6f6a2f16c9f627f938bbee2d32718.
> 
> There are two different variations on how Intel hardware addresses the
> "Hardware Status Page". One as a location in physical memory and the
> other as an offset into the virtual memory of the GPU, used in more
> recent chipsets. (The HWS itself is a cacheable region of memory which
> the GPU can write to without requiring CPU synchronisation, used for
> updating various details of hardware state, such as the position of
> the GPU head in the ringbuffer, the last breadcrumb seqno, etc).
> 
> These two types of addresses were updated in different locations of code
> - one inline with the ringbuffer initialisation, and the other during
> device initialisation. (The HWS page is logically associated with
> the rings, and there is one HWS page per ring.) During resume, only the
> ringbuffers were being re-initialised along with the virtual HWS page,
> leaving the older physical address HWS untouched. This then caused a
> hang on the older gen3/4 (915GM, 945GM, 965GM) the first time we tried
> to synchronise the GPU as the breadcrumbs were never being updated.

oops, right, I ignored those old devices.

> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Reported-by: Jan Niehusmann <jan@gondor.com>
> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
> Reported-and-tested-by: Michael "brot" Groh <brot@minad.de>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |    1 +
>  drivers/gpu/drm/i915/i915_suspend.c |    6 ++++++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4496505..5004724 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -383,6 +383,7 @@ typedef struct drm_i915_private {
>  	u32 saveDSPACNTR;
>  	u32 saveDSPBCNTR;
>  	u32 saveDSPARB;
> +	u32 saveHWS;
>  	u32 savePIPEACONF;
>  	u32 savePIPEBCONF;
>  	u32 savePIPEASRC;
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 7e992a8..da47415 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -796,6 +796,9 @@ int i915_save_state(struct drm_device *dev)
>  
>  	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
>  
> +	/* Hardware status page */
> +	dev_priv->saveHWS = I915_READ(HWS_PGA);
> +
>  	i915_save_display(dev);
>  
>  	/* Interrupt state */
> @@ -842,6 +845,9 @@ int i915_restore_state(struct drm_device *dev)
>  
>  	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
>  
> +	/* Hardware status page */
> +	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
> +

We'd better only save/restore this for !I915_NEED_GFX_HWS(dev),
note that HWS_PGA is not valid hw status register for new chip, e.g gen6.

>  	i915_restore_display(dev);
>  
>  	/* Interrupt state */
> -- 
> 1.7.4.1

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Dont save/restore hardware status page address register"

[Chris Wilson]

On Thu, 24 Mar 2011 09:41:27 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> We'd better only save/restore this for !I915_NEED_GFX_HWS(dev),
> note that HWS_PGA is not valid hw status register for new chip, e.g gen6.

Yes, but we should take it one step at a time. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre