[PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Chris Wilson]

When we enable the GuC, we enable an alternative mechanism for doing
post-GGTT update invalidation. Likewise, when we disable the GuC, we
restore the previous method. Assert that we change between known
endpoints, so that we can catch if we accidentally clobber some other
gen and if we change the invalidate routine without updating guc.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 1489c3af7145..4ff854e6413c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3095,13 +3095,17 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
 
 void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 {
+	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
+
 	i915->ggtt.invalidate = guc_ggtt_invalidate;
 }
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
-		i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	/* We should only be called after i915_ggtt_enable_guc() */
+	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
+
+	i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
-- 
2.11.0

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

✓ Fi.CI.BAT: success for drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Assert that we switch between known ggtt->invalidate functions
URL   : https://patchwork.freedesktop.org/series/25150/
State : success

== Summary ==

Series 25150v1 drm/i915/guc: Assert that we switch between known ggtt->invalidate functions
https://patchwork.freedesktop.org/api/1.0/series/25150/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:432s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:585s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:512s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:477s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:431s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:569s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:435s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:469s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:502s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:434s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:407s

b479b5c889c24e5a7cf4beb064f0037fdc2d568d drm-tip: 2017y-06m-01d-08h-55m-16s UTC integration manifest
675b5c6 drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4853/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Chris Wilson]

On Thu, Jun 01, 2017 at 09:36:58AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/guc: Assert that we switch between known ggtt->invalidate functions
> URL   : https://patchwork.freedesktop.org/series/25150/
> State : success
> 
> == Summary ==
> 
> Series 25150v1 drm/i915/guc: Assert that we switch between known ggtt->invalidate functions
> https://patchwork.freedesktop.org/api/1.0/series/25150/revisions/1/mbox/
> 
> fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:444s
> fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:432s
> fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:585s
> fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:512s
> fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:488s
> fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:477s
> fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:431s
> fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
> fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
> fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:489s
> fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
> fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:467s
> fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:569s
> fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:455s
> fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:435s
> fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:469s
> fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:502s
> fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:434s
> fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:542s
> fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:407s
> 
> b479b5c889c24e5a7cf4beb064f0037fdc2d568d drm-tip: 2017y-06m-01d-08h-55m-16s UTC integration manifest
> 675b5c6 drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

And pushed. Thanks for the review,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Michal Wajdeczko]

On Thu, Jun 01, 2017 at 10:04:46AM +0100, Chris Wilson wrote:
> When we enable the GuC, we enable an alternative mechanism for doing
> post-GGTT update invalidation. Likewise, when we disable the GuC, we
> restore the previous method. Assert that we change between known
> endpoints, so that we can catch if we accidentally clobber some other
> gen and if we change the invalidate routine without updating guc.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1489c3af7145..4ff854e6413c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3095,13 +3095,17 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
>  
>  void i915_ggtt_enable_guc(struct drm_i915_private *i915)
>  {
> +	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
> +
>  	i915->ggtt.invalidate = guc_ggtt_invalidate;
>  }
>  
>  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
>  {
> -	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> -		i915->ggtt.invalidate = gen6_ggtt_invalidate;
> +	/* We should only be called after i915_ggtt_enable_guc() */
> +	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
> +
> +	i915->ggtt.invalidate = gen6_ggtt_invalidate;
>  }

While this looks correct today, it may not work in the future if we
will need somethig other than gen6_ggtt_invalidate() as base invalidate
function or guc_gtt_invalidate() as the one for the guc. Just a head up.

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

>  
>  void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Chris Wilson]

On Thu, Jun 01, 2017 at 12:03:18PM +0200, Michal Wajdeczko wrote:
> On Thu, Jun 01, 2017 at 10:04:46AM +0100, Chris Wilson wrote:
> > When we enable the GuC, we enable an alternative mechanism for doing
> > post-GGTT update invalidation. Likewise, when we disable the GuC, we
> > restore the previous method. Assert that we change between known
> > endpoints, so that we can catch if we accidentally clobber some other
> > gen and if we change the invalidate routine without updating guc.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 1489c3af7145..4ff854e6413c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -3095,13 +3095,17 @@ int i915_ggtt_enable_hw(struct drm_i915_private *dev_priv)
> >  
> >  void i915_ggtt_enable_guc(struct drm_i915_private *i915)
> >  {
> > +	GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
> > +
> >  	i915->ggtt.invalidate = guc_ggtt_invalidate;
> >  }
> >  
> >  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
> >  {
> > -	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> > -		i915->ggtt.invalidate = gen6_ggtt_invalidate;
> > +	/* We should only be called after i915_ggtt_enable_guc() */
> > +	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
> > +
> > +	i915->ggtt.invalidate = gen6_ggtt_invalidate;
> >  }
> 
> While this looks correct today, it may not work in the future if we
> will need somethig other than gen6_ggtt_invalidate() as base invalidate
> function or guc_gtt_invalidate() as the one for the guc. Just a head up.

That's one of the reasons for adding it. So that if we forget to make
the change, it gets caught.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Joonas Lahtinen]

On to, 2017-06-01 at 12:03 +0200, Michal Wajdeczko wrote:
> On Thu, Jun 01, 2017 at 10:04:46AM +0100, Chris Wilson wrote:
> >  
> >  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
> >  {
> > -	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> > -		i915->ggtt.invalidate = gen6_ggtt_invalidate;
> > +	/* We should only be called after i915_ggtt_enable_guc() */
> > +	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
> > +
> > +	i915->ggtt.invalidate = gen6_ggtt_invalidate;
> >  }
> 
> While this looks correct today, it may not work in the future if we
> will need somethig other than gen6_ggtt_invalidate() as base invalidate
> function or guc_gtt_invalidate() as the one for the guc. Just a head up.

Currently the assignment is directly to gen6_ggtt_invalidate, no
questions asked. So I don't think the assert could be much more :)

Maybe GuC code should backup the invalidate function before overriding.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Chris Wilson]

On Thu, Jun 01, 2017 at 01:24:55PM +0300, Joonas Lahtinen wrote:
> On to, 2017-06-01 at 12:03 +0200, Michal Wajdeczko wrote:
> > On Thu, Jun 01, 2017 at 10:04:46AM +0100, Chris Wilson wrote:
> > >  
> > >  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
> > >  {
> > > -	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> > > -		i915->ggtt.invalidate = gen6_ggtt_invalidate;
> > > +	/* We should only be called after i915_ggtt_enable_guc() */
> > > +	GEM_BUG_ON(i915->ggtt.invalidate != guc_ggtt_invalidate);
> > > +
> > > +	i915->ggtt.invalidate = gen6_ggtt_invalidate;
> > >  }
> > 
> > While this looks correct today, it may not work in the future if we
> > will need somethig other than gen6_ggtt_invalidate() as base invalidate
> > function or guc_gtt_invalidate() as the one for the guc. Just a head up.
> 
> Currently the assignment is directly to gen6_ggtt_invalidate, no
> questions asked. So I don't think the assert could be much more :)
> 
> Maybe GuC code should backup the invalidate function before overriding.

Possibly, but I hope the alternatives are a little better known so that
we don't have to do save/restore of function pointers too often.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Joonas Lahtinen]

On to, 2017-06-01 at 10:04 +0100, Chris Wilson wrote:
> When we enable the GuC, we enable an alternative mechanism for doing
> post-GGTT update invalidation. Likewise, when we disable the GuC, we
> restore the previous method. Assert that we change between known
> endpoints, so that we can catch if we accidentally clobber some other
> gen and if we change the invalidate routine without updating guc.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>

This was R-b'd by Michal already, a dup patch?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Chris Wilson]

On Thu, Jun 01, 2017 at 01:44:07PM +0300, Joonas Lahtinen wrote:
> On to, 2017-06-01 at 10:04 +0100, Chris Wilson wrote:
> > When we enable the GuC, we enable an alternative mechanism for doing
> > post-GGTT update invalidation. Likewise, when we disable the GuC, we
> > restore the previous method. Assert that we change between known
> > endpoints, so that we can catch if we accidentally clobber some other
> > gen and if we change the invalidate routine without updating guc.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> 
> This was R-b'd by Michal already, a dup patch?

This is the second step to turn the v4.11 bugfix into a warning that it
never happens again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Assert that we switch between known ggtt->invalidate functions

[Joonas Lahtinen]

On to, 2017-06-01 at 12:01 +0100, Chris Wilson wrote:
> On Thu, Jun 01, 2017 at 01:44:07PM +0300, Joonas Lahtinen wrote:
> > 
> > On to, 2017-06-01 at 10:04 +0100, Chris Wilson wrote:
> > > 
> > > When we enable the GuC, we enable an alternative mechanism for doing
> > > post-GGTT update invalidation. Likewise, when we disable the GuC, we
> > > restore the previous method. Assert that we change between known
> > > endpoints, so that we can catch if we accidentally clobber some other
> > > gen and if we change the invalidate routine without updating guc.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > > Cc: Michel Thierry <michel.thierry@intel.com>
> > 
> > This was R-b'd by Michal already, a dup patch?
> 
> This is the second step to turn the v4.11 bugfix into a warning that it
> never happens again.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx