[PATCH 1/2] drm/i915/gen9: Clean up MOCS table definitions

[PATCH 1/2] drm/i915/gen9: Clean up MOCS table definitions

[Imre Deak]

Use named struct initializers for clarity. Also fix the target cache
definition to reflect its role in GEN9 onwards. On GEN8 a TC value of 0
meant ELLC but on GEN9+ it means the TC and LRU controls are taken from
the PTE.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 79 ++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 23b8545..5006a92 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -66,9 +66,10 @@ struct drm_i915_mocs_table {
 #define L3_WB			3
 
 /* Target cache */
-#define ELLC			0
-#define LLC			1
-#define LLC_ELLC		2
+#define LE_TC_PAGETABLE		0
+#define LE_TC_LLC		1
+#define LE_TC_LLC_ELLC		2
+#define LE_TC_LLC_ELLC_ALT	3
 
 /*
  * MOCS tables
@@ -96,34 +97,58 @@ struct drm_i915_mocs_table {
  *       end.
  */
 static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
-	/* { 0x00000009, 0x0010 } */
-	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
-	/* { 0x00000038, 0x0030 } */
-	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
-	/* { 0x0000003b, 0x0030 } */
-	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+	{
+	  .control_value = LE_CACHEABILITY(LE_UC) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
+	},
+	{
+	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	},
+	{
+	  .control_value = LE_CACHEABILITY(LE_WB) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =   L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	},
 };
 
 /* NOTE: the LE_TGT_CACHE is not used on Broxton */
 static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
-	/* { 0x00000009, 0x0010 } */
-	{ (LE_CACHEABILITY(LE_UC) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(0) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC)) },
-	/* { 0x00000038, 0x0030 } */
-	{ (LE_CACHEABILITY(LE_PAGETABLE) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) },
-	/* { 0x0000003b, 0x0030 } */
-	{ (LE_CACHEABILITY(LE_WB) | LE_TGT_CACHE(LLC_ELLC) | LE_LRUM(3) |
-	   LE_AOM(0) | LE_RSC(0) | LE_SCC(0) | LE_PFM(0) | LE_SCF(0)),
-	  (L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB)) }
+	{
+	  .control_value = LE_CACHEABILITY(LE_UC) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
+	},
+	{
+	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	},
+	{
+	  .control_value = LE_CACHEABILITY(LE_WB) |
+			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
+			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
+			   LE_PFM(0) | LE_SCF(0),
+
+	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	},
 };
 
 /**
-- 
2.5.0

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

[PATCH 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Imre Deak]

Setting a write-back cache policy in the MOCS entry definition also
implies snooping, which has a considerable overhead. This is
unexpected for a few reasons:
- From user-space's point of view since it didn't want a coherent
  surface (it didn't set the buffer as such via the set caching IOCTL).
- There is a separate MOCS entry field for snooping (which we never
  set).
- This MOCS table is about caching in (e)LLC and there is no (e)LLC on
  BXT. There is a separate table for L3 cache control.

Considering the above the current behavior of snooping looks like an
unintentional side-effect of the WB setting. Changing it to be PTE
based cacheability gets rid of the snooping without any ill-effects.
For a coherent surface the application would use a separate MOCS entry
(at index 1) and call the set caching IOCTL to setup the PTE entries
for the corresponding buffer to be snooped.

This resulted in 70% improvement in synthetic texturing benchmarks.

Kudos to Valtteri Rantala, Eero Tamminen and Michael T Frederick and
Ville who helped to narrow the source of problem to the kernel and to
the snooping behaviour in particular.

CC: Valtteri Rantala <valtteri.rantala@intel.com>
CC: Eero Tamminen <eero.t.tamminen@intel.com>
CC: Michael T Frederick <michael.t.frederick@intel.com>
CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 5006a92..23c7dd1 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -142,7 +142,7 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
 	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
 	},
 	{
-	  .control_value = LE_CACHEABILITY(LE_WB) |
+	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
 			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
 			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
 			   LE_PFM(0) | LE_SCF(0),
-- 
2.5.0

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

Re: [PATCH 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Chris Wilson]

On Mon, Apr 25, 2016 at 03:23:21PM +0300, Imre Deak wrote:
> Setting a write-back cache policy in the MOCS entry definition also
> implies snooping, which has a considerable overhead. This is
> unexpected for a few reasons:
> - From user-space's point of view since it didn't want a coherent
>   surface (it didn't set the buffer as such via the set caching IOCTL).
> - There is a separate MOCS entry field for snooping (which we never
>   set).
> - This MOCS table is about caching in (e)LLC and there is no (e)LLC on
>   BXT. There is a separate table for L3 cache control.
> 
> Considering the above the current behavior of snooping looks like an
> unintentional side-effect of the WB setting. Changing it to be PTE
> based cacheability gets rid of the snooping without any ill-effects.
> For a coherent surface the application would use a separate MOCS entry
> (at index 1) and call the set caching IOCTL to setup the PTE entries
> for the corresponding buffer to be snooped.
> 
> This resulted in 70% improvement in synthetic texturing benchmarks.
> 
> Kudos to Valtteri Rantala, Eero Tamminen and Michael T Frederick and
> Ville who helped to narrow the source of problem to the kernel and to
> the snooping behaviour in particular.
> 
> CC: Valtteri Rantala <valtteri.rantala@intel.com>
> CC: Eero Tamminen <eero.t.tamminen@intel.com>
> CC: Michael T Frederick <michael.t.frederick@intel.com>
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 5006a92..23c7dd1 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -142,7 +142,7 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
>  	},
>  	{
> -	  .control_value = LE_CACHEABILITY(LE_WB) |
> +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
>  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
>  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
>  			   LE_PFM(0) | LE_SCF(0),

This is index 2? This should *be* the snooping entry?

Index 0: uncached
Index 1: follow pte
Index 2: snoop

Aim I missing something? Why isn't this a userspce bug for requesting a
mocs setting it didn't wnat? In my kernel this makes mocs_table[1] ==
mocs_table[2].
-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 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Imre Deak]

On ma, 2016-04-25 at 13:37 +0100, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 03:23:21PM +0300, Imre Deak wrote:
> > Setting a write-back cache policy in the MOCS entry definition also
> > implies snooping, which has a considerable overhead. This is
> > unexpected for a few reasons:
> > - From user-space's point of view since it didn't want a coherent
> >   surface (it didn't set the buffer as such via the set caching
> > IOCTL).
> > - There is a separate MOCS entry field for snooping (which we never
> >   set).
> > - This MOCS table is about caching in (e)LLC and there is no (e)LLC
> > on
> >   BXT. There is a separate table for L3 cache control.
> > 
> > Considering the above the current behavior of snooping looks like
> > an
> > unintentional side-effect of the WB setting. Changing it to be PTE
> > based cacheability gets rid of the snooping without any ill-
> > effects.
> > For a coherent surface the application would use a separate MOCS
> > entry
> > (at index 1) and call the set caching IOCTL to setup the PTE
> > entries
> > for the corresponding buffer to be snooped.
> > 
> > This resulted in 70% improvement in synthetic texturing benchmarks.
> > 
> > Kudos to Valtteri Rantala, Eero Tamminen and Michael T Frederick
> > and
> > Ville who helped to narrow the source of problem to the kernel and
> > to
> > the snooping behaviour in particular.
> > 
> > CC: Valtteri Rantala <valtteri.rantala@intel.com>
> > CC: Eero Tamminen <eero.t.tamminen@intel.com>
> > CC: Michael T Frederick <michael.t.frederick@intel.com>
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_mocs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_mocs.c
> > b/drivers/gpu/drm/i915/intel_mocs.c
> > index 5006a92..23c7dd1 100644
> > --- a/drivers/gpu/drm/i915/intel_mocs.c
> > +++ b/drivers/gpu/drm/i915/intel_mocs.c
> > @@ -142,7 +142,7 @@ static const struct drm_i915_mocs_entry
> > broxton_mocs_table[] = {
> >  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) |
> > L3_CACHEABILITY(L3_WB),
> >  	},
> >  	{
> > -	  .control_value = LE_CACHEABILITY(LE_WB) |
> > +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> >  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> >  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) |
> > LE_SCC(0) |
> >  			   LE_PFM(0) | LE_SCF(0),
> 
> This is index 2? This should *be* the snooping entry?
> 
> Index 0: uncached
> Index 1: follow pte
> Index 2: snoop
> 
> Aim I missing something? Why isn't this a userspce bug for requesting
> a
> mocs setting it didn't wnat? In my kernel this makes mocs_table[1] ==
> mocs_table[2].

I don't think there is or can be any snooping entry. On CHV for example
we can only setup snooping via the PTE, so there we necessarily have to
use entry 1 + PTE setup (set caching IOCTL). This is also what both
Windows and Mesa assumes apparently.

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

Re: [PATCH 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Chris Wilson]

On Mon, Apr 25, 2016 at 03:39:54PM +0300, Imre Deak wrote:
> On ma, 2016-04-25 at 13:37 +0100, Chris Wilson wrote:
> > On Mon, Apr 25, 2016 at 03:23:21PM +0300, Imre Deak wrote:
> > > Setting a write-back cache policy in the MOCS entry definition also
> > > implies snooping, which has a considerable overhead. This is
> > > unexpected for a few reasons:
> > > - From user-space's point of view since it didn't want a coherent
> > >   surface (it didn't set the buffer as such via the set caching
> > > IOCTL).
> > > - There is a separate MOCS entry field for snooping (which we never
> > >   set).
> > > - This MOCS table is about caching in (e)LLC and there is no (e)LLC
> > > on
> > >   BXT. There is a separate table for L3 cache control.
> > > 
> > > Considering the above the current behavior of snooping looks like
> > > an
> > > unintentional side-effect of the WB setting. Changing it to be PTE
> > > based cacheability gets rid of the snooping without any ill-
> > > effects.
> > > For a coherent surface the application would use a separate MOCS
> > > entry
> > > (at index 1) and call the set caching IOCTL to setup the PTE
> > > entries
> > > for the corresponding buffer to be snooped.
> > > 
> > > This resulted in 70% improvement in synthetic texturing benchmarks.
> > > 
> > > Kudos to Valtteri Rantala, Eero Tamminen and Michael T Frederick
> > > and
> > > Ville who helped to narrow the source of problem to the kernel and
> > > to
> > > the snooping behaviour in particular.
> > > 
> > > CC: Valtteri Rantala <valtteri.rantala@intel.com>
> > > CC: Eero Tamminen <eero.t.tamminen@intel.com>
> > > CC: Michael T Frederick <michael.t.frederick@intel.com>
> > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_mocs.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_mocs.c
> > > b/drivers/gpu/drm/i915/intel_mocs.c
> > > index 5006a92..23c7dd1 100644
> > > --- a/drivers/gpu/drm/i915/intel_mocs.c
> > > +++ b/drivers/gpu/drm/i915/intel_mocs.c
> > > @@ -142,7 +142,7 @@ static const struct drm_i915_mocs_entry
> > > broxton_mocs_table[] = {
> > >  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) |
> > > L3_CACHEABILITY(L3_WB),
> > >  	},
> > >  	{
> > > -	  .control_value = LE_CACHEABILITY(LE_WB) |
> > > +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> > >  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> > >  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) |
> > > LE_SCC(0) |
> > >  			   LE_PFM(0) | LE_SCF(0),
> > 
> > This is index 2? This should *be* the snooping entry?
> > 
> > Index 0: uncached
> > Index 1: follow pte
> > Index 2: snoop
> > 
> > Aim I missing something? Why isn't this a userspce bug for requesting
> > a
> > mocs setting it didn't wnat? In my kernel this makes mocs_table[1] ==
> > mocs_table[2].
> 
> I don't think there is or can be any snooping entry. On CHV for example
> we can only setup snooping via the PTE, so there we necessarily have to
> use entry 1 + PTE setup (set caching IOCTL). This is also what both
> Windows and Mesa assumes apparently.

Then the issue is not we've enabled snooping via mocs, but that the mocs
entry is completely bogus. Please update your commit message :)
-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 2/2] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config

[Imre Deak]

On ma, 2016-04-25 at 13:49 +0100, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 03:39:54PM +0300, Imre Deak wrote:
> > On ma, 2016-04-25 at 13:37 +0100, Chris Wilson wrote:
> > > On Mon, Apr 25, 2016 at 03:23:21PM +0300, Imre Deak wrote:
> > > > Setting a write-back cache policy in the MOCS entry definition
> > > > also
> > > > implies snooping, which has a considerable overhead. This is
> > > > unexpected for a few reasons:
> > > > - From user-space's point of view since it didn't want a
> > > > coherent
> > > >   surface (it didn't set the buffer as such via the set caching
> > > > IOCTL).
> > > > - There is a separate MOCS entry field for snooping (which we
> > > > never
> > > >   set).
> > > > - This MOCS table is about caching in (e)LLC and there is no
> > > > (e)LLC
> > > > on
> > > >   BXT. There is a separate table for L3 cache control.
> > > > 
> > > > Considering the above the current behavior of snooping looks
> > > > like
> > > > an
> > > > unintentional side-effect of the WB setting. Changing it to be
> > > > PTE
> > > > based cacheability gets rid of the snooping without any ill-
> > > > effects.
> > > > For a coherent surface the application would use a separate
> > > > MOCS
> > > > entry
> > > > (at index 1) and call the set caching IOCTL to setup the PTE
> > > > entries
> > > > for the corresponding buffer to be snooped.
> > > > 
> > > > This resulted in 70% improvement in synthetic texturing
> > > > benchmarks.
> > > > 
> > > > Kudos to Valtteri Rantala, Eero Tamminen and Michael T
> > > > Frederick
> > > > and
> > > > Ville who helped to narrow the source of problem to the kernel
> > > > and
> > > > to
> > > > the snooping behaviour in particular.
> > > > 
> > > > CC: Valtteri Rantala <valtteri.rantala@intel.com>
> > > > CC: Eero Tamminen <eero.t.tamminen@intel.com>
> > > > CC: Michael T Frederick <michael.t.frederick@intel.com>
> > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_mocs.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_mocs.c
> > > > b/drivers/gpu/drm/i915/intel_mocs.c
> > > > index 5006a92..23c7dd1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_mocs.c
> > > > +++ b/drivers/gpu/drm/i915/intel_mocs.c
> > > > @@ -142,7 +142,7 @@ static const struct drm_i915_mocs_entry
> > > > broxton_mocs_table[] = {
> > > >  	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) |
> > > > L3_CACHEABILITY(L3_WB),
> > > >  	},
> > > >  	{
> > > > -	  .control_value = LE_CACHEABILITY(LE_WB) |
> > > > +	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> > > >  			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> > > >  			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0)
> > > > |
> > > > LE_SCC(0) |
> > > >  			   LE_PFM(0) | LE_SCF(0),
> > > 
> > > This is index 2? This should *be* the snooping entry?
> > > 
> > > Index 0: uncached
> > > Index 1: follow pte
> > > Index 2: snoop
> > > 
> > > Aim I missing something? Why isn't this a userspce bug for
> > > requesting
> > > a
> > > mocs setting it didn't wnat? In my kernel this makes
> > > mocs_table[1] ==
> > > mocs_table[2].
> > 
> > I don't think there is or can be any snooping entry. On CHV for
> > example
> > we can only setup snooping via the PTE, so there we necessarily
> > have to
> > use entry 1 + PTE setup (set caching IOCTL). This is also what both
> > Windows and Mesa assumes apparently.
> 
> Then the issue is not we've enabled snooping via mocs, but that the
> mocs
> entry is completely bogus. Please update your commit message :)

Yes it is bogus, that is what I meant by "incorrect MOCS config" in the
subject. Tbh, the specification is really unclear what these bits
control, it doesn't mention at all for example that the WB setting
would affect snooping behavior. It doesn't either have much sense to me
to set any of these MOCS entry fields other than their default on BXT,
since they all (should) control only e(LLC) cacheability which is
irrelevant on BXT. I will ask for a clarification for this in BSpec.

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

Re: [PATCH 1/2] drm/i915/gen9: Clean up MOCS table definitions

[Chris Wilson]

On Mon, Apr 25, 2016 at 03:23:20PM +0300, Imre Deak wrote:
> Use named struct initializers for clarity. Also fix the target cache
> definition to reflect its role in GEN9 onwards. On GEN8 a TC value of 0
> meant ELLC but on GEN9+ it means the TC and LRU controls are taken from
> the PTE.
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 79 ++++++++++++++++++++++++++-------------
>  1 file changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 23b8545..5006a92 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -66,9 +66,10 @@ struct drm_i915_mocs_table {
>  #define L3_WB			3
>  
>  /* Target cache */
> -#define ELLC			0
> -#define LLC			1
> -#define LLC_ELLC		2
> +#define LE_TC_PAGETABLE		0
> +#define LE_TC_LLC		1
> +#define LE_TC_LLC_ELLC		2
> +#define LE_TC_LLC_ELLC_ALT	3
>  
>  /*
>   * MOCS tables
> @@ -96,34 +97,58 @@ struct drm_i915_mocs_table {
>   *       end.
>   */
>  static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> -	/* { 0x00000009, 0x0010 } */

Hmm, we lose this convenience. Since we have the same table in userspace
to check the ABI is maintained, it would be nice to keep around.

Also mention that you ran igt/gem_mocs_settings to confirm no changes.
-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 1/2] drm/i915/gen9: Clean up MOCS table definitions

[Imre Deak]

On ma, 2016-04-25 at 13:30 +0100, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 03:23:20PM +0300, Imre Deak wrote:
> > Use named struct initializers for clarity. Also fix the target cache
> > definition to reflect its role in GEN9 onwards. On GEN8 a TC value of 0
> > meant ELLC but on GEN9+ it means the TC and LRU controls are taken from
> > the PTE.
> > 
> > No functional change.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_mocs.c | 79 ++++++++++++++++++++++++++-------------
> >  1 file changed, 52 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> > index 23b8545..5006a92 100644
> > --- a/drivers/gpu/drm/i915/intel_mocs.c
> > +++ b/drivers/gpu/drm/i915/intel_mocs.c
> > @@ -66,9 +66,10 @@ struct drm_i915_mocs_table {
> >  #define L3_WB			3
> >  
> >  /* Target cache */
> > -#define ELLC			0
> > -#define LLC			1
> > -#define LLC_ELLC		2
> > +#define LE_TC_PAGETABLE		0
> > +#define LE_TC_LLC		1
> > +#define LE_TC_LLC_ELLC		2
> > +#define LE_TC_LLC_ELLC_ALT	3
> >  
> >  /*
> >   * MOCS tables
> > @@ -96,34 +97,58 @@ struct drm_i915_mocs_table {
> >   *       end.
> >   */
> >  static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> > -	/* { 0x00000009, 0x0010 } */
> 
> Hmm, we lose this convenience. Since we have the same table in userspace
> to check the ABI is maintained, it would be nice to keep around.

Ok, I can add them back.

> Also mention that you ran igt/gem_mocs_settings to confirm no changes.

Ok, didn't know about this patch. After patch 2/2 there is a change in
the 3rd entry but that's expected based on the explanation of the
commit message there. So I can fix the test case accordingly.

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/gen9: Clean up MOCS table definitions

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/gen9: Clean up MOCS table definitions
URL   : https://patchwork.freedesktop.org/series/6249/
State : failure

== Summary ==

Series 6249v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6249/revisions/1/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                incomplete -> PASS       (snb-dellxps)
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> INCOMPLETE (snb-dellxps)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                skip       -> PASS       (bdw-nuci7)

bdw-nuci7        total:193  pass:181  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:192  pass:153  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:193  pass:169  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:193  pass:174  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:193  pass:136  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:193  pass:165  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:193  pass:168  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:193  pass:182  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:153  pass:123  dwarn:0   dfail:0   fail:0   skip:29 
snb-x220t        total:193  pass:155  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2061/

f814551aa7232ed36d71244dd148b48660b53a78 drm-intel-nightly: 2016y-04m-25d-11h-36m-27s UTC integration manifest
15e0467 drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config
ac664c1 drm/i915/gen9: Clean up MOCS table definitions

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