[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 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre