| Message ID | 20200810103103.303818-19-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Correct the locking hierarchy in gem. | expand |
On 8/10/20 12:30 PM, Maarten Lankhorst wrote: > We have the ordering of timeline->mutex vs resv_lock wrong, > convert the i915_pin_vma and intel_context_pin as well to > future-proof this. > > We may need to do future changes to do this more transaction-like, > and only get down to a single i915_gem_ww_ctx, but for now this > should work. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 57 +++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index c6f6370283cf..e94976976571 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -1195,24 +1195,39 @@ static struct intel_context *oa_pin_context(struct i915_perf_stream *stream) > struct i915_gem_engines_iter it; > struct i915_gem_context *ctx = stream->ctx; > struct intel_context *ce; > - int err; > + struct i915_gem_ww_ctx ww; > + int err = -ENODEV; > > for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { > if (ce->engine != stream->engine) /* first match! */ > continue; > > - /* > - * As the ID is the gtt offset of the context's vma we > - * pin the vma to ensure the ID remains fixed. > - */ > - err = intel_context_pin(ce); > - if (err == 0) { > - stream->pinned_ctx = ce; > - break; > - } > + err = 0; > + break; > } > i915_gem_context_unlock_engines(ctx); > > + if (err) > + return ERR_PTR(err); > + > + i915_gem_ww_ctx_init(&ww, true); > +retry: > + /* > + * As the ID is the gtt offset of the context's vma we > + * pin the vma to ensure the ID remains fixed. > + */ > + err = intel_context_pin_ww(ce, &ww); > + if (err == -EDEADLK) { > + err = i915_gem_ww_ctx_backoff(&ww); > + if (!err) > + goto retry; > + } > + i915_gem_ww_ctx_fini(&ww); > + Hmm. Didn't we keep an intel_context_pin() that does exactly the above without recoding the whole ww transaction? Or do you plan to remove that? With that taken into account, Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>
Op 12-08-2020 om 21:53 schreef Thomas Hellström (Intel): > > On 8/10/20 12:30 PM, Maarten Lankhorst wrote: >> We have the ordering of timeline->mutex vs resv_lock wrong, >> convert the i915_pin_vma and intel_context_pin as well to >> future-proof this. >> >> We may need to do future changes to do this more transaction-like, >> and only get down to a single i915_gem_ww_ctx, but for now this >> should work. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_perf.c | 57 +++++++++++++++++++++++--------- >> 1 file changed, 42 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> index c6f6370283cf..e94976976571 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -1195,24 +1195,39 @@ static struct intel_context *oa_pin_context(struct i915_perf_stream *stream) >> struct i915_gem_engines_iter it; >> struct i915_gem_context *ctx = stream->ctx; >> struct intel_context *ce; >> - int err; >> + struct i915_gem_ww_ctx ww; >> + int err = -ENODEV; >> for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { >> if (ce->engine != stream->engine) /* first match! */ >> continue; >> - /* >> - * As the ID is the gtt offset of the context's vma we >> - * pin the vma to ensure the ID remains fixed. >> - */ >> - err = intel_context_pin(ce); >> - if (err == 0) { >> - stream->pinned_ctx = ce; >> - break; >> - } >> + err = 0; >> + break; >> } >> i915_gem_context_unlock_engines(ctx); >> + if (err) >> + return ERR_PTR(err); >> + >> + i915_gem_ww_ctx_init(&ww, true); >> +retry: >> + /* >> + * As the ID is the gtt offset of the context's vma we >> + * pin the vma to ensure the ID remains fixed. >> + */ >> + err = intel_context_pin_ww(ce, &ww); >> + if (err == -EDEADLK) { >> + err = i915_gem_ww_ctx_backoff(&ww); >> + if (!err) >> + goto retry; >> + } >> + i915_gem_ww_ctx_fini(&ww); >> + > > Hmm. Didn't we keep an intel_context_pin() that does exactly the above without recoding the whole ww transaction? Or do you plan to remove that? > > With that taken into account, > > Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com> > > Yeah, I want to remove that eventually, might need to change i915_perf even more to fully do this. Thanks for reviewing. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index c6f6370283cf..e94976976571 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1195,24 +1195,39 @@ static struct intel_context *oa_pin_context(struct i915_perf_stream *stream) struct i915_gem_engines_iter it; struct i915_gem_context *ctx = stream->ctx; struct intel_context *ce; - int err; + struct i915_gem_ww_ctx ww; + int err = -ENODEV; for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) { if (ce->engine != stream->engine) /* first match! */ continue; - /* - * As the ID is the gtt offset of the context's vma we - * pin the vma to ensure the ID remains fixed. - */ - err = intel_context_pin(ce); - if (err == 0) { - stream->pinned_ctx = ce; - break; - } + err = 0; + break; } i915_gem_context_unlock_engines(ctx); + if (err) + return ERR_PTR(err); + + i915_gem_ww_ctx_init(&ww, true); +retry: + /* + * As the ID is the gtt offset of the context's vma we + * pin the vma to ensure the ID remains fixed. + */ + err = intel_context_pin_ww(ce, &ww); + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + + if (err) + return ERR_PTR(err); + + stream->pinned_ctx = ce; return stream->pinned_ctx; } @@ -1923,15 +1938,22 @@ emit_oa_config(struct i915_perf_stream *stream, { struct i915_request *rq; struct i915_vma *vma; + struct i915_gem_ww_ctx ww; int err; vma = get_oa_vma(stream, oa_config); if (IS_ERR(vma)) return PTR_ERR(vma); - err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL | PIN_HIGH); + i915_gem_ww_ctx_init(&ww, true); +retry: + err = i915_gem_object_lock(vma->obj, &ww); + if (err) + goto err; + + err = i915_vma_pin_ww(vma, &ww, 0, 0, PIN_GLOBAL | PIN_HIGH); if (err) - goto err_vma_put; + goto err; intel_engine_pm_get(ce->engine); rq = i915_request_create(ce); @@ -1953,11 +1975,9 @@ emit_oa_config(struct i915_perf_stream *stream, goto err_add_request; } - i915_vma_lock(vma); err = i915_request_await_object(rq, vma->obj, 0); if (!err) err = i915_vma_move_to_active(vma, rq, 0); - i915_vma_unlock(vma); if (err) goto err_add_request; @@ -1971,7 +1991,14 @@ emit_oa_config(struct i915_perf_stream *stream, i915_request_add(rq); err_vma_unpin: i915_vma_unpin(vma); -err_vma_put: +err: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + + i915_gem_ww_ctx_fini(&ww); i915_vma_put(vma); return err; }
We have the ordering of timeline->mutex vs resv_lock wrong, convert the i915_pin_vma and intel_context_pin as well to future-proof this. We may need to do future changes to do this more transaction-like, and only get down to a single i915_gem_ww_ctx, but for now this should work. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 57 +++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 15 deletions(-)