Re: [Intel-gfx] [PATCH v2] drm/i915: Re-enable GTT following a device reset
Quoting Ville Syrjälä (2017-09-06 13:13:05)
> On Wed, Sep 06, 2017 at 12:14:05PM +0100, Chris Wilson wrote:
> > Ville Syrjälä spotted that PGETBL_CTL was losing its enable bit upon a
> > reset. That was causing the display to show garbage on his 945gm. On my
> > i915gm the effect was far more severe; re-enabling the display following
> > the reset without PGETBL_CTL being enabled lead to an immediate hard
> > hang.
> >
> > We do have a routine to re-enable PGETBL_CTL which is applicable to
> > gen2-4, although on gen4 it is documented that a graphics reset doesn't
> > alter the register (no such wording is given for gen3) and should be safe
> > to call to punch back in the enable bit. However, that leaves the question
> > of whether we need to completely re-initialise the register and the
> > rest of the GSM. For g33/pnv/gen4+, where we do have a configurable
> > page table, its contents do seem to be kept, and so we should be able to
> > recover without having to reinitialise the GTT from scratch (as prior to
> > g33, that register is configured by the BIOS and we leave alone except
> > for the enable bit).
> >
> > This appears to have been broken by commit 5fbd0418eef2 ("drm/i915:
> > Re-enable GGTT earlier during resume on pre-gen6 platforms"), which
> > moved the intel_enable_gtt() from i915_gem_init_hw() (also used by
> > reset) to add it earlier during hw init and resume, missing the reset
> > path.
> >
> > v2: Find the culprit, rearrange ggtt_enable to be before gem_init_hw to
> > match init/resume
> >
> > Reported-by: Ville Syrjälä
> > Fixes: 5fbd0418eef2 ("drm/i915: Re-enable GGTT earlier during resume on
> > pre-gen6 platforms")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101852
> > Signed-off-by: Chris Wilson
> > Cc: Ville Syrjälä
> > Cc: Daniel Vetter
> > Reviewed-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 12 +---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index f10a078e3a55..ff70fc45ba7c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1892,9 +1892,15 @@ void i915_reset(struct drm_i915_private *i915,
> > unsigned int flags)
> >
> > /*
> >* Everything depends on having the GTT running, so we need to start
> > - * there. Fortunately we don't need to do this unless we reset the
> > - * chip at a PCI level.
> > - *
> > + * there.
> > + */
> > + ret = i915_ggtt_enable_hw(i915);
> > + if (ret) {
> > + DRM_ERROR("Failed to re-enable GGTT following reset %d\n",
> > ret);
> > + goto error;
> > + }
>
> I do wonder a bit whether the hardware might object to the fact that
> we restore fences before the GGTT gets re-enabled. But I suppose it's
> possible there's no linkage between the two until someone actually
> accesses the GGTT...
That's a reasonable suggestion. The real challenge is finding a name for
each step ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Re-enable GTT following a device reset
On Wed, Sep 06, 2017 at 12:14:05PM +0100, Chris Wilson wrote:
> Ville Syrjälä spotted that PGETBL_CTL was losing its enable bit upon a
> reset. That was causing the display to show garbage on his 945gm. On my
> i915gm the effect was far more severe; re-enabling the display following
> the reset without PGETBL_CTL being enabled lead to an immediate hard
> hang.
>
> We do have a routine to re-enable PGETBL_CTL which is applicable to
> gen2-4, although on gen4 it is documented that a graphics reset doesn't
> alter the register (no such wording is given for gen3) and should be safe
> to call to punch back in the enable bit. However, that leaves the question
> of whether we need to completely re-initialise the register and the
> rest of the GSM. For g33/pnv/gen4+, where we do have a configurable
> page table, its contents do seem to be kept, and so we should be able to
> recover without having to reinitialise the GTT from scratch (as prior to
> g33, that register is configured by the BIOS and we leave alone except
> for the enable bit).
>
> This appears to have been broken by commit 5fbd0418eef2 ("drm/i915:
> Re-enable GGTT earlier during resume on pre-gen6 platforms"), which
> moved the intel_enable_gtt() from i915_gem_init_hw() (also used by
> reset) to add it earlier during hw init and resume, missing the reset
> path.
>
> v2: Find the culprit, rearrange ggtt_enable to be before gem_init_hw to
> match init/resume
>
> Reported-by: Ville Syrjälä
> Fixes: 5fbd0418eef2 ("drm/i915: Re-enable GGTT earlier during resume on
> pre-gen6 platforms")
Ouch. So it was me that broke this. Sorry about that folks.
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101852
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_drv.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f10a078e3a55..ff70fc45ba7c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1892,9 +1892,15 @@ void i915_reset(struct drm_i915_private *i915,
> unsigned int flags)
>
> /*
>* Everything depends on having the GTT running, so we need to start
> - * there. Fortunately we don't need to do this unless we reset the
> - * chip at a PCI level.
> - *
> + * there.
> + */
> + ret = i915_ggtt_enable_hw(i915);
> + if (ret) {
> + DRM_ERROR("Failed to re-enable GGTT following reset %d\n", ret);
> + goto error;
> + }
> +
> + /*
>* Next we need to restore the context, but we don't use those
>* yet either...
>*
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Re-enable GTT following a device reset
On Wed, Sep 06, 2017 at 12:14:05PM +0100, Chris Wilson wrote:
> Ville Syrjälä spotted that PGETBL_CTL was losing its enable bit upon a
> reset. That was causing the display to show garbage on his 945gm. On my
> i915gm the effect was far more severe; re-enabling the display following
> the reset without PGETBL_CTL being enabled lead to an immediate hard
> hang.
>
> We do have a routine to re-enable PGETBL_CTL which is applicable to
> gen2-4, although on gen4 it is documented that a graphics reset doesn't
> alter the register (no such wording is given for gen3) and should be safe
> to call to punch back in the enable bit. However, that leaves the question
> of whether we need to completely re-initialise the register and the
> rest of the GSM. For g33/pnv/gen4+, where we do have a configurable
> page table, its contents do seem to be kept, and so we should be able to
> recover without having to reinitialise the GTT from scratch (as prior to
> g33, that register is configured by the BIOS and we leave alone except
> for the enable bit).
>
> This appears to have been broken by commit 5fbd0418eef2 ("drm/i915:
> Re-enable GGTT earlier during resume on pre-gen6 platforms"), which
> moved the intel_enable_gtt() from i915_gem_init_hw() (also used by
> reset) to add it earlier during hw init and resume, missing the reset
> path.
>
> v2: Find the culprit, rearrange ggtt_enable to be before gem_init_hw to
> match init/resume
>
> Reported-by: Ville Syrjälä
> Fixes: 5fbd0418eef2 ("drm/i915: Re-enable GGTT earlier during resume on
> pre-gen6 platforms")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101852
> Signed-off-by: Chris Wilson
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_drv.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f10a078e3a55..ff70fc45ba7c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1892,9 +1892,15 @@ void i915_reset(struct drm_i915_private *i915,
> unsigned int flags)
>
> /*
>* Everything depends on having the GTT running, so we need to start
> - * there. Fortunately we don't need to do this unless we reset the
> - * chip at a PCI level.
> - *
> + * there.
> + */
> + ret = i915_ggtt_enable_hw(i915);
> + if (ret) {
> + DRM_ERROR("Failed to re-enable GGTT following reset %d\n", ret);
> + goto error;
> + }
I do wonder a bit whether the hardware might object to the fact that
we restore fences before the GGTT gets re-enabled. But I suppose it's
possible there's no linkage between the two until someone actually
accesses the GGTT...
CCID/PPGTT also seems to get re-enabled before this but since that's
gen6+ stuff it doesn't matter.
This does help my 945gm, so
Tested-by: Ville Syrjälä
Reviewed-by: Ville Syrjälä
> +
> + /*
>* Next we need to restore the context, but we don't use those
>* yet either...
>*
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Re-enable GTT following a device reset
Ville Syrjälä spotted that PGETBL_CTL was losing its enable bit upon a
reset. That was causing the display to show garbage on his 945gm. On my
i915gm the effect was far more severe; re-enabling the display following
the reset without PGETBL_CTL being enabled lead to an immediate hard
hang.
We do have a routine to re-enable PGETBL_CTL which is applicable to
gen2-4, although on gen4 it is documented that a graphics reset doesn't
alter the register (no such wording is given for gen3) and should be safe
to call to punch back in the enable bit. However, that leaves the question
of whether we need to completely re-initialise the register and the
rest of the GSM. For g33/pnv/gen4+, where we do have a configurable
page table, its contents do seem to be kept, and so we should be able to
recover without having to reinitialise the GTT from scratch (as prior to
g33, that register is configured by the BIOS and we leave alone except
for the enable bit).
This appears to have been broken by commit 5fbd0418eef2 ("drm/i915:
Re-enable GGTT earlier during resume on pre-gen6 platforms"), which
moved the intel_enable_gtt() from i915_gem_init_hw() (also used by
reset) to add it earlier during hw init and resume, missing the reset
path.
v2: Find the culprit, rearrange ggtt_enable to be before gem_init_hw to
match init/resume
Reported-by: Ville Syrjälä
Fixes: 5fbd0418eef2 ("drm/i915: Re-enable GGTT earlier during resume on
pre-gen6 platforms")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101852
Signed-off-by: Chris Wilson
Cc: Ville Syrjälä
Cc: Daniel Vetter
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_drv.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f10a078e3a55..ff70fc45ba7c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1892,9 +1892,15 @@ void i915_reset(struct drm_i915_private *i915, unsigned
int flags)
/*
* Everything depends on having the GTT running, so we need to start
-* there. Fortunately we don't need to do this unless we reset the
-* chip at a PCI level.
-*
+* there.
+*/
+ ret = i915_ggtt_enable_hw(i915);
+ if (ret) {
+ DRM_ERROR("Failed to re-enable GGTT following reset %d\n", ret);
+ goto error;
+ }
+
+ /*
* Next we need to restore the context, but we don't use those
* yet either...
*
--
2.14.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
