Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-06 Thread Keith Packard
On Thu, 05 Jan 2012 21:41:34 -0800, Keith Packard kei...@keithp.com wrote: I talked with Eric about this and we decided that the whole splitting out of the i/o functions just doesn't make any sense. That makes this series very similar to Daniel's patches, so I'll rebase my bug fixes on top of

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-05 Thread Daniel Vetter
On Wed, Jan 04, 2012 at 06:22:41PM -0800, Keith Packard wrote: On Wed, 4 Jan 2012 19:12:57 +0100, Daniel Vetter dan...@ffwll.ch wrote: The Correct? was just to check my understanding of your concern, I still think its invalid. You've cut away the second part of my mail where I explain why

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-05 Thread Keith Packard
On Thu, 5 Jan 2012 12:29:08 +0100, Daniel Vetter dan...@ffwll.ch wrote: - The reset code (running from a workqueue) does hold sturct mutex. It's the hangcheck and error state capture code running from softirq/timer context causing issues. Right, I mis-wrote; I meant the hangcheck timer

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-05 Thread Daniel Vetter
Looks like we managed to clear up our mutual confusion here ;-) On Thu, Jan 05, 2012 at 07:49:12AM -0800, Keith Packard wrote: On Thu, 5 Jan 2012 12:29:08 +0100, Daniel Vetter dan...@ffwll.ch wrote: - The reset code (running from a workqueue) does hold sturct mutex. It's the hangcheck

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-05 Thread Keith Packard
On Thu, 5 Jan 2012 17:59:47 +0100, Daniel Vetter dan...@ffwll.ch wrote: Absolutely agreed, maybe with the adadendum to only try to make things faster if it's actually a problem and shows up in a fast-path we care about. Here's a longer series that does a bunch of cleanup before trying to fix

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-05 Thread Keith Packard
On Thu, 05 Jan 2012 16:29:43 -0800, Keith Packard kei...@keithp.com wrote: Here's a longer series that does a bunch of cleanup before trying to fix things. Patches marked with '***' fix bugs. The patch marked with '...' is the optimization to inline the spinlocks. I talked with Eric about

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-04 Thread Daniel Vetter
On Wed, Jan 4, 2012 at 00:33, Keith Packard kei...@keithp.com wrote: On Tue, 3 Jan 2012 22:49:52 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: Nope, current hangcheck blows up, and we have an i-g-t testcase for it (which the commit msg clearly states). There are also numerous bug

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-04 Thread Daniel Vetter
On Wed, Jan 04, 2012 at 09:54:08AM -0800, Keith Packard wrote: On Wed, 4 Jan 2012 18:11:18 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: Ah, I think I see you're concern: Between the time we reset the gpu and the time we fix up the forcewake state somebody might sneak in and see an

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-04 Thread Keith Packard
On Wed, 4 Jan 2012 19:12:57 +0100, Daniel Vetter dan...@ffwll.ch wrote: The Correct? was just to check my understanding of your concern, I still think its invalid. You've cut away the second part of my mail where I explain why and I don't see you responding to that here. Also micro-optimizing

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Keith Packard
On Wed, 14 Dec 2011 13:57:03 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: The problem this patch solves is that the forcewake accounting necessary for register reads is protected by dev-struct_mutex. But the hangcheck and error_capture code need to access registers without grabbing

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Daniel Vetter
On Tue, Jan 3, 2012 at 19:51, Keith Packard kei...@keithp.com wrote: On Wed, 14 Dec 2011 13:57:03 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: The problem this patch solves is that the forcewake accounting necessary for register reads is protected by dev-struct_mutex. But the

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Keith Packard
On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: I'm a bit confused by this. With the current code forcewake is protected by dev-struct_mutex. Which doesn't work out because we need to access registers that require the forcewake dance from non-process context.

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Ben Widawsky
On 01/03/2012 01:13 PM, Keith Packard wrote: On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: I'm a bit confused by this. With the current code forcewake is protected by dev-struct_mutex. Which doesn't work out because we need to access registers that require

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Daniel Vetter
On Tue, Jan 3, 2012 at 22:13, Keith Packard kei...@keithp.com wrote: On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: I'm a bit confused by this. With the current code forcewake is protected by dev-struct_mutex. Which doesn't work out because we need to access

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Chris Wilson
On Tue, 03 Jan 2012 13:49:36 -0800, Ben Widawsky b...@bwidawsk.net wrote: The atomic ops stuff was simply there to help reduce the races (even if we don't have the lock, we can still safely increment the variable). It should be safe to get rid of with the spinlock in place. My only gripe

Re: [Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2012-01-03 Thread Keith Packard
On Tue, 3 Jan 2012 22:49:52 +0100, Daniel Vetter daniel.vet...@ffwll.ch wrote: Nope, current hangcheck blows up, and we have an i-g-t testcase for it (which the commit msg clearly states). There are also numerous bug reports where a dying gpu results in tons of

[Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

2011-12-14 Thread Daniel Vetter
The problem this patch solves is that the forcewake accounting necessary for register reads is protected by dev-struct_mutex. But the hangcheck and error_capture code need to access registers without grabbing this mutex because we hold it while waiting for the gpu. So a new lock is required.