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).
> > @@ -681,7 +670,9 @@ static int live_hwsp_wrap(void *arg)
> > if (!intel_engine_can_store_dword(engine))
> > continue;
> >
> > + intel_engine_pm_get(engine);
> > rq = i915_request_create(engine->kernel_context);
> > + intel_engine_pm_put(engine);
> > if (IS_ERR(rq)) {
> > err = PTR_ERR(rq);
> > goto out;
> > @@ -747,16 +738,12 @@ static int live_hwsp_wrap(void *arg)
> > }
> >
>
> No i915_request_put in this one and I can't see why it would be different.
It's not using tl_write with the added ref.
> > diff --git a/drivers/gpu/drm/i915/selftests/igt_flush_test.c b/drivers/gpu/drm/i915/selftests/igt_flush_test.c
> > index d3b5eb402d33..2a5fbe46ea9f 100644
> > --- a/drivers/gpu/drm/i915/selftests/igt_flush_test.c
> > +++ b/drivers/gpu/drm/i915/selftests/igt_flush_test.c
> > @@ -12,31 +12,25 @@
> >
> > #include "igt_flush_test.h"
> >
> > -int igt_flush_test(struct drm_i915_private *i915, unsigned int flags)
> > +int igt_flush_test(struct drm_i915_private *i915)
> > {
> > int ret = intel_gt_is_wedged(&i915->gt) ? -EIO : 0;
> > - int repeat = !!(flags & I915_WAIT_LOCKED);
> >
> > cond_resched();
> >
> > - do {
> > - if (i915_gem_wait_for_idle(i915, flags, HZ / 5) == -ETIME) {
> > - pr_err("%pS timed out, cancelling all further testing.\n",
> > - __builtin_return_address(0));
> > + i915_retire_requests(i915);
>
> Do you need this one or it would be better without, for clarity, given
> there is one at the end? i915_gem_wait_for_idle will retire all it can.
It's to aide kick starting of the idle-barrier. Eventually this is
pulled together again.
> > + if (i915_gem_wait_for_idle(i915, 0, HZ / 5) == -ETIME) {
> > + pr_err("%pS timed out, cancelling all further testing.\n",
> > + __builtin_return_address(0));
> >
> > - GEM_TRACE("%pS timed out.\n",
> > - __builtin_return_address(0));
> > - GEM_TRACE_DUMP();
> > + GEM_TRACE("%pS timed out.\n",
> > + __builtin_return_address(0));
> > + GEM_TRACE_DUMP();
> >
> > - intel_gt_set_wedged(&i915->gt);
> > - repeat = 0;
> > - ret = -EIO;
> > - }
> > -
> > - /* Ensure we also flush after wedging. */
> > - if (flags & I915_WAIT_LOCKED)
> > - i915_retire_requests(i915);
> > - } while (repeat--);
> > + intel_gt_set_wedged(&i915->gt);
> > + ret = -EIO;
> > + }
> > + i915_retire_requests(i915);
> >
> > return ret;
> > }
> Essentially looks fine. Provisional, meaning keep it if you do some
> small tweaks:
Nothing seemed to be required.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
