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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC