[Intel-gfx] [PATCH v3 17/28] drm/i915: Convert 'trace_irq' to use requests rather than seqnos

[Daniel Vetter]

On Wed, Nov 26, 2014 at 3:12 PM, John Harrison
<John.C.Harrison at intel.com> wrote:
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 8bfdac6..831fae2 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3130,4 +3130,11 @@ wait_remaining_ms_from_jiffies(unsigned long
>>> timestamp_jiffies, int to_wait_ms)
>>>         }
>>>   }
>>>   +static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
>>> +                                     struct drm_i915_gem_request *req)
>>> +{
>>> +       if (ring->trace_irq_req == NULL && ring->irq_get(ring))
>>> +               i915_gem_request_assign(&ring->trace_irq_req, req);
>>
>> This looks a bit suspiciuos. Thus far ring->trace_irq_req was essentially
>> ot protected at all by anything. But that was nothing troublesome since we
>> didn't hang a real resource of it.
>>
>> But now there's a refcounted request in that pointer, which means if we
>> race we leak. I'll skip this patch for now.
>> -Daniel
>
>
> Race how? The assignment only ever occurs from inside execbuffer submission
> at which point the driver mutex lock is held. Therefore it is very
> definitely protected. Not doing the reference count means that there is now
> the possibility of a dangling pointer and thus the possibility of going bang
> with a kernel oops.

Hm, ->trace_irq_seqno is indeed always touched from the a calling
context with dev->struct_mutex held. Somehow I've misrembered that
since the realtime/tracing folks are really freaked out about what
we're doing here. But from that pov your patch doesn't really make
things worse, so I'll pull it in.

Btw I don't see the oops really without this patch. What would blow up?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch