On Thu, Jan 28, 2016 at 02:09:37PM +0200, Ville Syrjälä wrote: > On Thu, Jan 28, 2016 at 11:45:24AM +0000, Chris Wilson wrote: > > On Thu, Jan 28, 2016 at 10:55:16AM +0000, Daniele Ceraolo Spurio wrote: > > > > > > > > > On 27/01/16 16:39, Ville Syrjälä wrote: > > > >On Wed, Jan 27, 2016 at 03:43:49PM +0000,daniele.ceraolospurio@xxxxxxxxx wrote: > > > >>From: Daniele Ceraolo Spurio<daniele.ceraolospurio@xxxxxxxxx> > > > >> > > > >>While running some tests on the scheduler patches with rpm enabled I > > > >>came across a corruption in the ringbuffer, which was root-caused to > > > >>the GPU being suspended while commands were being emitted to the > > > >>ringbuffer. The access to memory was failing because the GPU needs to > > > >>be awake when accessing stolen memory (where my ringbuffer was located). > > > >>Since we have this constraint it looks like a sensible idea to check > > > >>that we hold a refcount when we access the rungbuffer. > > > >> > > > >>v2: move the check from ring_begin to ringbuffer iomap time (Chris) > > > >>v3: update comment (Chris) > > > >> > > > >>Cc: John Harrison<John.C.Harrison@xxxxxxxxx> > > > >>Cc: Chris Wilson<chris@xxxxxxxxxxxxxxxxxx> > > > >>Signed-off-by: Daniele Ceraolo Spurio<daniele.ceraolospurio@xxxxxxxxx> > > > >>--- > > > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++ > > > >> 1 file changed, 3 insertions(+) > > > >> > > > >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > >>index 6f5b511..133321a 100644 > > > >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > >>@@ -2119,6 +2119,9 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > > > >> return ret; > > > >> } > > > >>+ /* Access through the GTT requires the device to be awake. */ > > > >>+ assert_rpm_wakelock_held(dev_priv); > > > >>+ > > > >Hmm. This function doesn't actually acces the ring buffer, so it's a bit > > > >odd to see this here. > > > > > > I had it inring_begin initially, but Chris suggested moving it here > > > because we pin the ringbuffer before accessing it. Do you have a > > > different place in mind for where this should be added or would you > > > be happy with a simple comment update? > > > > This function we call in order to acquire access to the ring iomap for > > the request. At the beginning of the request, we should be pinning > > everything we need to build the request. If writing through the GTT we > > should be ensuring that the device is also awake. The oddity is that > > this is not yet explicit and the asymmetry still exists between > > legacy/execlists. > > Yeah, with ringbuffer mode this gets executed exactly once, so more or > less useless at the moment. With execlists I suppose it might catch > something on CHV/BXT. It shouldn't. We hold the wakeref for execbuf request construction, and that is more or less the only time we create a request in execlists. (Although it doesn't have to be that way!) -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
