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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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


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