| Message ID | 1360785365-5628-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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(+)