[PATCH] drm/i915: don't clflush gem objects in stolen memory

[PATCH] drm/i915: don't clflush gem objects in stolen memory

[Imre Deak]

As explained by Chris Wilson gem objects in stolen memory are always
coherent with the GPU so we don't need to ever flush the CPU caches for
these.

This fixes a breakage - at least with the compact sg patches applied -
during the resume/restore gtt mappings path, when we tried to clflush an
FB object in stolen memory, but since stolen objects don't have backing
pages we passed an invalid page pointer to drm_clflush_page().

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8ea9df9..9cf6e84 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3019,6 +3019,13 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj)
 	if (obj->pages == NULL)
 		return;
 
+	/*
+	 * Stolen memory is always coherent with the GPU as it is explicitly
+	 * marked as wc by the system, or the system is cache-coherent.
+	 */
+	if (obj->stolen)
+		return;
+
 	/* If the GPU is snooping the contents of the CPU cache,
 	 * we do not need to manually clear the CPU cache lines.  However,
 	 * the caches are only snooped when the render cache is
-- 
1.7.10.4

Re: [PATCH] drm/i915: don't clflush gem objects in stolen memory

[Chris Wilson]

On Wed, Feb 13, 2013 at 09:56:05PM +0200, Imre Deak wrote:
> As explained by Chris Wilson gem objects in stolen memory are always
> coherent with the GPU so we don't need to ever flush the CPU caches for
> these.
> 
> This fixes a breakage - at least with the compact sg patches applied -
> during the resume/restore gtt mappings path, when we tried to clflush an
> FB object in stolen memory, but since stolen objects don't have backing
> pages we passed an invalid page pointer to drm_clflush_page().
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

To the best of my knowledge, this is correct:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Though the stolen framework landed in 3.8, tough call as to whether this
should be in 3.8 as well given the backported clflush fix... I guess we
are simply too late, so drm-intel-next +
Cc: stable@vger.kernel.org

Imre do you mind digging up the sha of both the introduction of stolen
and the clflush of unbounded upon resume?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't clflush gem objects in stolen memory

[Imre Deak]

On Wed, 2013-02-13 at 20:39 +0000, Chris Wilson wrote:
> On Wed, Feb 13, 2013 at 09:56:05PM +0200, Imre Deak wrote:
> > As explained by Chris Wilson gem objects in stolen memory are always
> > coherent with the GPU so we don't need to ever flush the CPU caches for
> > these.
> > 
> > This fixes a breakage - at least with the compact sg patches applied -
> > during the resume/restore gtt mappings path, when we tried to clflush an
> > FB object in stolen memory, but since stolen objects don't have backing
> > pages we passed an invalid page pointer to drm_clflush_page().
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> To the best of my knowledge, this is correct:
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Though the stolen framework landed in 3.8, tough call as to whether this
> should be in 3.8 as well given the backported clflush fix... I guess we
> are simply too late, so drm-intel-next +
> Cc: stable@vger.kernel.org
> 
> Imre do you mind digging up the sha of both the introduction of stolen
> and the clflush of unbounded upon resume?

Since afaics this problem relates only to _FBs_ in stolen memory the
first one is:

commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:27 2012 +0000

    drm/i915: Allocate fbcon from stolen memory


and the second:

commit a8e93126a6f10d0a14ba8407ec112b1b3a5e2e97
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Dec 8 14:28:54 2010 +0000

    drm/i915/gtt: Clear the cachelines upon resume
    
    Required for my pineview system to not barf after resuming.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>


--Imre

Re: [PATCH] drm/i915: don't clflush gem objects in stolen memory

[Daniel Vetter]

On Wed, Feb 13, 2013 at 08:39:58PM +0000, Chris Wilson wrote:
> On Wed, Feb 13, 2013 at 09:56:05PM +0200, Imre Deak wrote:
> > As explained by Chris Wilson gem objects in stolen memory are always
> > coherent with the GPU so we don't need to ever flush the CPU caches for
> > these.
> > 
> > This fixes a breakage - at least with the compact sg patches applied -
> > during the resume/restore gtt mappings path, when we tried to clflush an
> > FB object in stolen memory, but since stolen objects don't have backing
> > pages we passed an invalid page pointer to drm_clflush_page().
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> To the best of my knowledge, this is correct:
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Though the stolen framework landed in 3.8, tough call as to whether this
> should be in 3.8 as well given the backported clflush fix... I guess we
> are simply too late, so drm-intel-next +
> Cc: stable@vger.kernel.org
> 
> Imre do you mind digging up the sha of both the introduction of stolen
> and the clflush of unbounded upon resume?

Stolen allocations seem to be only in 3.9-next, so no cc: stable. Queued
up, thanks for the patch an review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: don't clflush gem objects in stolen memory

[Chris Wilson]

On Wed, Feb 13, 2013 at 10:55:24PM +0200, Imre Deak wrote:
> Since afaics this problem relates only to _FBs_ in stolen memory the
> first one is:
> 
> commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:27 2012 +0000
> 
>     drm/i915: Allocate fbcon from stolen memory

I would argue the oversight is in the general stolen enabling, not
usage. Speaking as the guilty party.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre