Op 06-01-2020 om 11:22 schreef Chris Wilson:
> The last remaining reason for serialising the pin/unpin of the
> intel_context is to ensure that our preallocated wakerefs are not
> consumed too early (i.e. the unpin of the previous phase does not emit
> the idle barriers for this phase before we even submit). All of the
> other operations within the context pin/unpin are supposed to be
> atomic... Therefore, we can reduce the serialisation to being just on
> the i915_active.preallocated_barriers itself and drop the nested
> pin_mutex from intel_context_unpin().
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_context.c | 18 +++++-------------
> drivers/gpu/drm/i915/i915_active.c | 19 +++++++++++++++----
> 2 files changed, 20 insertions(+), 17 deletions(-)
For whole series, except 5 and 6:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
For 5 and 6, I think they look sane but I'm not the right person to review. :)
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 4d0bc1478ccd..34ec958d400e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -86,22 +86,14 @@ int __intel_context_do_pin(struct intel_context *ce)
>
> void intel_context_unpin(struct intel_context *ce)
> {
> - if (likely(atomic_add_unless(&ce->pin_count, -1, 1)))
> + if (!atomic_dec_and_test(&ce->pin_count))
> return;
>
> - /* We may be called from inside intel_context_pin() to evict another */
> - intel_context_get(ce);
> - mutex_lock_nested(&ce->pin_mutex, SINGLE_DEPTH_NESTING);
> -
> - if (likely(atomic_dec_and_test(&ce->pin_count))) {
> - CE_TRACE(ce, "retire\n");
> + CE_TRACE(ce, "unpin\n");
> + ce->ops->unpin(ce);
>
> - ce->ops->unpin(ce);
> -
> - intel_context_active_release(ce);
> - }
> -
> - mutex_unlock(&ce->pin_mutex);
> + intel_context_get(ce);
> + intel_context_active_release(ce);
> intel_context_put(ce);
> }
>
Might want to put a comment here why intel_context_get is needed?
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index cfe09964622b..f3da5c06f331 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -605,12 +605,15 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> struct intel_engine_cs *engine)
> {
> intel_engine_mask_t tmp, mask = engine->mask;
> + struct llist_node *pos = NULL, *next;
> struct intel_gt *gt = engine->gt;
> - struct llist_node *pos, *next;
> int err;
>
> GEM_BUG_ON(i915_active_is_idle(ref));
> - GEM_BUG_ON(!llist_empty(&ref->preallocated_barriers));
> +
> + /* Wait until the previous preallocation is completed */
> + while (!llist_empty(&ref->preallocated_barriers))
> + cond_resched();
>
> /*
> * Preallocate a node for each physical engine supporting the target
> @@ -653,16 +656,24 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> GEM_BUG_ON(rcu_access_pointer(node->base.fence) != ERR_PTR(-EAGAIN));
>
> GEM_BUG_ON(barrier_to_engine(node) != engine);
> - llist_add(barrier_to_ll(node), &ref->preallocated_barriers);
> + next = barrier_to_ll(node);
> + next->next = pos;
> + if (!pos)
> + pos = next;
> intel_engine_pm_get(engine);
> }
>
> + GEM_BUG_ON(!llist_empty(&ref->preallocated_barriers));
> + llist_add_batch(next, pos, &ref->preallocated_barriers);
> +
> return 0;
>
> unwind:
> - llist_for_each_safe(pos, next, take_preallocated_barriers(ref)) {
> + while (pos) {
> struct active_node *node = barrier_from_ll(pos);
>
> + pos = pos->next;
> +
> atomic_dec(&ref->count);
> intel_engine_pm_put(barrier_to_engine(node));
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
