[Intel-gfx] [PATCH 17/21] drm/i915: Drop struct_mutex from around i915_retire_requests()

[Tvrtko Ursulin]

On 25/09/2019 09:43, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-24 16:25:29)
>>
>> On 02/09/2019 05:02, Chris Wilson wrote:
>>> @@ -449,8 +447,6 @@ tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>>>        struct i915_request *rq;
>>>        int err;
>>>    
>>> -     lockdep_assert_held(&tl->gt->i915->drm.struct_mutex); /* lazy rq refs */
>>> -
>>>        err = intel_timeline_pin(tl);
>>>        if (err) {
>>>                rq = ERR_PTR(err);
>>> @@ -461,10 +457,14 @@ tl_write(struct intel_timeline *tl, struct intel_engine_cs *engine, u32 value)
>>>        if (IS_ERR(rq))
>>>                goto out_unpin;
>>>    
>>> +     i915_request_get(rq);
>>> +
>>>        err = emit_ggtt_store_dw(rq, tl->hwsp_offset, value);
>>>        i915_request_add(rq);
>>> -     if (err)
>>> +     if (err) {
>>> +             i915_request_put(rq);
>>>                rq = ERR_PTR(err);
>>> +     }
>>>    
>>>    out_unpin:
>>>        intel_timeline_unpin(tl);
>>> @@ -500,7 +500,6 @@ static int live_hwsp_engine(void *arg)
>>>        struct intel_timeline **timelines;
>>>        struct intel_engine_cs *engine;
>>>        enum intel_engine_id id;
>>> -     intel_wakeref_t wakeref;
>>>        unsigned long count, n;
>>>        int err = 0;
>>>    
>>> @@ -515,14 +514,13 @@ static int live_hwsp_engine(void *arg)
>>>        if (!timelines)
>>>                return -ENOMEM;
>>>    
>>> -     mutex_lock(&i915->drm.struct_mutex);
>>> -     wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>>> -
>>>        count = 0;
>>>        for_each_engine(engine, i915, id) {
>>>                if (!intel_engine_can_store_dword(engine))
>>>                        continue;
>>>    
>>> +             intel_engine_pm_get(engine);
>>> +
>>>                for (n = 0; n < NUM_TIMELINES; n++) {
>>>                        struct intel_timeline *tl;
>>>                        struct i915_request *rq;
>>> @@ -530,22 +528,26 @@ static int live_hwsp_engine(void *arg)
>>>                        tl = checked_intel_timeline_create(i915);
>>>                        if (IS_ERR(tl)) {
>>>                                err = PTR_ERR(tl);
>>> -                             goto out;
>>> +                             break;
>>>                        }
>>>    
>>>                        rq = tl_write(tl, engine, count);
>>>                        if (IS_ERR(rq)) {
>>>                                intel_timeline_put(tl);
>>>                                err = PTR_ERR(rq);
>>> -                             goto out;
>>> +                             break;
>>>                        }
>>>    
>>>                        timelines[count++] = tl;
>>> +                     i915_request_put(rq);
>>
>> This was a leak until now?
> 
> No, we added a get into tl_write() so that we own a reference to the
> request, just so that ownership is clear across the waits (and it can't
> accidentally be retired).

/me scrolls up, scrolls down.. rubs eyes.. yep, I was blind.

[snip]

>> Essentially looks fine. Provisional, meaning keep it if you do some
>> small tweaks:
> 
> Nothing seemed to be required.

Ack.

Regards,

Tvrtko