drm/i915: Make contexts non-snooped on non-LLC platforms

Commit Message

Ville Syrjälä March 31, 2014, 3:09 p.m. UTC

From: Ville Syrjälä <[e-mail]>

We don't do CPU access to GPU contexts so making the GPU access snoop
the CPU caches seems silly, and potentially expensive.

Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chris Wilson March 31, 2014, 3:39 p.m. UTC | #1

On Mon, Mar 31, 2014 at 06:09:49PM +0300, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> We don't do CPU access to GPU contexts so making the GPU access snoop
> the CPU caches seems silly, and potentially expensive.
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>

Maybe define a macro to be HAS_L3_CACHE?

The root cause is that our singular enum is not flexible enough to
account for all platform variations.

Otherwise, lgtm.
-Chris

Ville Syrjälä April 1, 2014, 10:11 a.m. UTC | #2

On Mon, Mar 31, 2014 at 04:39:37PM +0100, Chris Wilson wrote:
> On Mon, Mar 31, 2014 at 06:09:49PM +0300, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > We don't do CPU access to GPU contexts so making the GPU access snoop
> > the CPU caches seems silly, and potentially expensive.
> > 
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> Maybe define a macro to be HAS_L3_CACHE?

What should I do with such a macro?

> 
> The root cause is that our singular enum is not flexible enough to
> account for all platform variations.
> 
> Otherwise, lgtm.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Chris Wilson April 1, 2014, 10:19 a.m. UTC | #3

On Tue, Apr 01, 2014 at 01:11:02PM +0300, Ville Syrjälä wrote:
> On Mon, Mar 31, 2014 at 04:39:37PM +0100, Chris Wilson wrote:
> > On Mon, Mar 31, 2014 at 06:09:49PM +0300, [e-mail] wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > We don't do CPU access to GPU contexts so making the GPU access snoop
> > > the CPU caches seems silly, and potentially expensive.
> > > 
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > 
> > Maybe define a macro to be HAS_L3_CACHE?
> 
> What should I do with such a macro?

I am trying to express what exactly we are testing for here. It is not
exactly LLC we care about, but L3 to hide the context switch latency.
Even though Ben thinks that's a waste of our limited resources.
-Chris

Ville Syrjälä April 1, 2014, 10:37 a.m. UTC | #4

On Tue, Apr 01, 2014 at 11:19:19AM +0100, Chris Wilson wrote:
> On Tue, Apr 01, 2014 at 01:11:02PM +0300, Ville Syrjälä wrote:
> > On Mon, Mar 31, 2014 at 04:39:37PM +0100, Chris Wilson wrote:
> > > On Mon, Mar 31, 2014 at 06:09:49PM +0300, [e-mail] wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > We don't do CPU access to GPU contexts so making the GPU access snoop
> > > > the CPU caches seems silly, and potentially expensive.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > 
> > > Maybe define a macro to be HAS_L3_CACHE?
> > 
> > What should I do with such a macro?
> 
> I am trying to express what exactly we are testing for here. It is not
> exactly LLC we care about, but L3 to hide the context switch latency.
> Even though Ben thinks that's a waste of our limited resources.

VLV has L3 too, but we can't control it via the PTEs.

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6043062..746fb23 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -240,7 +240,7 @@  __create_hw_context(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	if (INTEL_INFO(dev)->gen >= 7) {
+	if (INTEL_INFO(dev)->gen >= 7 && HAS_LLC(dev)) {
 		ret = i915_gem_object_set_cache_level(ctx->obj,
 						      I915_CACHE_L3_LLC);
 		/* Failure shouldn't ever happen this early */