[Intel-gfx] [PATCH v3] drm/i915: check that rpm ref is held when accessing ringbuf in stolen mem

[Chris Wilson]

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 at intel.com  wrote:
> > > >>From: Daniele Ceraolo Spurio<daniele.ceraolospurio at intel.com>
> > > >>
> > > >>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 at Intel.com>
> > > >>Cc: Chris Wilson<chris at chris-wilson.co.uk>
> > > >>Signed-off-by: Daniele Ceraolo Spurio<daniele.ceraolospurio at intel.com>
> > > >>---
> > > >>  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