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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 27/01/16 16:39, Ville Syrjälä wrote:
> On Wed, Jan 27, 2016 at 03:43:49PM +0000,daniele.ceraolospurio@intel.com  wrote:
>> From: Daniele Ceraolo Spurio<daniele.ceraolospurio@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@Intel.com>
>> Cc: Chris Wilson<chris@chris-wilson.co.uk>
>> Signed-off-by: Daniele Ceraolo Spurio<daniele.ceraolospurio@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?

Thanks,
Daniele

>>   		ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
>>   						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
>>   		if (ringbuf->virtual_start == NULL) {
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx