Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Since we now allow the intel_context_unpin() to run unserialised, we
> risk our operations under the intel_context_lock_pinned() being run as
> the context is unpinned (and thus invalidating our state). We can
> atomically acquire the pin, testing to see if it is pinned in the
> process, thus ensuring that the state remains consistent during the
> course of the whole operation.
>
> Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 +++++++---
> drivers/gpu/drm/i915/gt/intel_context.h | 7 ++++++-
> drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++------
> drivers/gpu/drm/i915/i915_perf.c | 13 +++++--------
> 4 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 88f6253f5405..a2e57e62af30 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1236,12 +1236,14 @@ gen8_modify_rpcs(struct intel_context *ce, struct intel_sseu sseu)
> * image, or into the registers directory, does not stick). Pristine
> * and idle contexts will be configured on pinning.
> */
> - if (!intel_context_is_pinned(ce))
> + if (!intel_context_pin_if_active(ce))
> return 0;
>
> rq = intel_engine_create_kernel_request(ce->engine);
> - if (IS_ERR(rq))
> - return PTR_ERR(rq);
> + if (IS_ERR(rq)) {
> + ret = PTR_ERR(rq);
> + goto out_unpin;
> + }
>
> /* Serialise with the remote context */
> ret = intel_context_prepare_remote_request(ce, rq);
> @@ -1249,6 +1251,8 @@ gen8_modify_rpcs(struct intel_context *ce, struct intel_sseu sseu)
> ret = gen8_emit_rpcs_config(rq, ce, sseu);
>
> i915_request_add(rq);
> +out_unpin:
> + intel_context_unpin(ce);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 0f5ae4ff3b10..63073ebc6cf1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -76,9 +76,14 @@ static inline void intel_context_unlock_pinned(struct intel_context *ce)
>
> int __intel_context_do_pin(struct intel_context *ce);
>
> +static inline bool intel_context_pin_if_active(struct intel_context *ce)
> +{
> + return atomic_inc_not_zero(&ce->pin_count);
> +}
> +
> static inline int intel_context_pin(struct intel_context *ce)
> {
> - if (likely(atomic_inc_not_zero(&ce->pin_count)))
> + if (likely(intel_context_pin_if_active(ce)))
> return 0;
>
> return __intel_context_do_pin(ce);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 0ac98e39eb75..db184536acef 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -321,16 +321,15 @@ static void print_context_stats(struct seq_file *m,
>
> for_each_gem_engine(ce,
> i915_gem_context_lock_engines(ctx), it) {
> - intel_context_lock_pinned(ce);
> - if (intel_context_is_pinned(ce)) {
> + if (intel_context_pin_if_active(ce)) {
> rcu_read_lock();
> if (ce->state)
> per_file_stats(0,
> ce->state->obj, &kstats);
> per_file_stats(0, ce->ring->vma->obj, &kstats);
> rcu_read_unlock();
> + intel_context_unpin(ce);
> }
> - intel_context_unlock_pinned(ce);
> }
> i915_gem_context_unlock_engines(ctx);
>
> @@ -1513,15 +1512,14 @@ static int i915_context_status(struct seq_file *m, void *unused)
>
> for_each_gem_engine(ce,
> i915_gem_context_lock_engines(ctx), it) {
> - intel_context_lock_pinned(ce);
> - if (intel_context_is_pinned(ce)) {
> + if (intel_context_pin_if_active(ce)) {
> seq_printf(m, "%s: ", ce->engine->name);
> if (ce->state)
> describe_obj(m, ce->state->obj);
> describe_ctx_ring(m, ce->ring);
> seq_putc(m, '\n');
> + intel_context_unpin(ce);
> }
> - intel_context_unlock_pinned(ce);
> }
> i915_gem_context_unlock_engines(ctx);
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 84350c7bc711..c7a7b676f079 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2203,17 +2203,14 @@ static int gen8_configure_context(struct i915_gem_context *ctx,
> if (ce->engine->class != RENDER_CLASS)
> continue;
>
> - err = intel_context_lock_pinned(ce);
> - if (err)
> - break;
> + /* Otherwise OA settings will be set upon first use */
> + if (!intel_context_pin_if_active(ce))
> + continue;
>
> flex->value = intel_sseu_make_rpcs(ctx->i915, &ce->sseu);
> + err = gen8_modify_context(ce, flex, count);
>
> - /* Otherwise OA settings will be set upon first use */
> - if (intel_context_is_pinned(ce))
> - err = gen8_modify_context(ce, flex, count);
> -
> - intel_context_unlock_pinned(ce);
> + intel_context_unpin(ce);
> if (err)
> break;
> }
> --
> 2.25.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
