On Tue, May 04, 2021 at 05:41:36PM +0100, Matthew Auld wrote:
> We use some of the lower bits of the retire function pointer for
> potential flags, which is quite thorny, since the caller needs to
> remember to give the function the correct alignment with
> __i915_active_call, otherwise we might incorrectly unpack the pointer
> and jump to some garbage address later. Instead of all this let's just
> pass the flags along as a separate parameter.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Suggested-by: Daniel Vetter <daniel@xxxxxxxx>
> References: ca419f407b43 ("drm/i915: Fix crash in auto_retire")
> References: d8e44e4dd221 ("drm/i915/overlay: Fix active retire callback alignment")
> References: fd5f262db118 ("drm/i915/selftests: Fix active retire callback alignment")
> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Thanks for doing this quickly.
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_frontbuffer.c | 4 ++--
> drivers/gpu/drm/i915/display/intel_overlay.c | 5 ++---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 3 +--
> drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_context.c | 3 +--
> drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c | 3 +--
> drivers/gpu/drm/i915/gt/intel_timeline.c | 4 ++--
> drivers/gpu/drm/i915/gt/mock_engine.c | 2 +-
> .../gpu/drm/i915/gt/selftest_engine_heartbeat.c | 4 ++--
> drivers/gpu/drm/i915/i915_active.c | 14 +++++---------
> drivers/gpu/drm/i915/i915_active.h | 11 ++++++-----
> drivers/gpu/drm/i915/i915_active_types.h | 5 -----
> drivers/gpu/drm/i915/i915_vma.c | 3 +--
> drivers/gpu/drm/i915/selftests/i915_active.c | 4 ++--
> 15 files changed, 28 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index 8161d49e78ba..8e75debcce1a 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -211,7 +211,6 @@ static int frontbuffer_active(struct i915_active *ref)
> return 0;
> }
>
> -__i915_active_call
> static void frontbuffer_retire(struct i915_active *ref)
> {
> struct intel_frontbuffer *front =
> @@ -266,7 +265,8 @@ intel_frontbuffer_get(struct drm_i915_gem_object *obj)
> atomic_set(&front->bits, 0);
> i915_active_init(&front->write,
> frontbuffer_active,
> - i915_active_may_sleep(frontbuffer_retire));
> + frontbuffer_retire,
> + I915_ACTIVE_RETIRE_SLEEPS);
>
> spin_lock(&i915->fb_tracking.lock);
> if (rcu_access_pointer(obj->frontbuffer)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 428819ba18dd..f1e04c1535c7 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -383,8 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
> i830_overlay_clock_gating(dev_priv, true);
> }
>
> -__i915_active_call static void
> -intel_overlay_last_flip_retire(struct i915_active *active)
> +static void intel_overlay_last_flip_retire(struct i915_active *active)
> {
> struct intel_overlay *overlay =
> container_of(active, typeof(*overlay), last_flip);
> @@ -1401,7 +1400,7 @@ void intel_overlay_setup(struct drm_i915_private *dev_priv)
> overlay->saturation = 146;
>
> i915_active_init(&overlay->last_flip,
> - NULL, intel_overlay_last_flip_retire);
> + NULL, intel_overlay_last_flip_retire, 0);
>
> ret = get_registers(overlay, OVERLAY_NEEDS_PHYSICAL(dev_priv));
> if (ret)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index fd8ee52e17a4..188dee13e017 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1046,7 +1046,6 @@ struct context_barrier_task {
> void *data;
> };
>
> -__i915_active_call
> static void cb_retire(struct i915_active *base)
> {
> struct context_barrier_task *cb = container_of(base, typeof(*cb), base);
> @@ -1080,7 +1079,7 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> if (!cb)
> return -ENOMEM;
>
> - i915_active_init(&cb->base, NULL, cb_retire);
> + i915_active_init(&cb->base, NULL, cb_retire, 0);
> err = i915_active_acquire(&cb->base);
> if (err) {
> kfree(cb);
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index 21b1085769be..1aee5e6b1b23 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -343,7 +343,7 @@ static struct i915_vma *pd_vma_create(struct gen6_ppgtt *ppgtt, int size)
> if (!vma)
> return ERR_PTR(-ENOMEM);
>
> - i915_active_init(&vma->active, NULL, NULL);
> + i915_active_init(&vma->active, NULL, NULL, 0);
>
> kref_init(&vma->ref);
> mutex_init(&vma->pages_mutex);
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 17cf2640b082..4033184f13b9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -326,7 +326,6 @@ void intel_context_unpin(struct intel_context *ce)
> intel_context_put(ce);
> }
>
> -__i915_active_call
> static void __intel_context_retire(struct i915_active *active)
> {
> struct intel_context *ce = container_of(active, typeof(*ce), active);
> @@ -385,7 +384,7 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)
> mutex_init(&ce->pin_mutex);
>
> i915_active_init(&ce->active,
> - __intel_context_active, __intel_context_retire);
> + __intel_context_active, __intel_context_retire, 0);
> }
>
> void intel_context_fini(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
> index 0fa6c38893f7..7bf84cd21543 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c
> @@ -867,7 +867,7 @@ void intel_ggtt_init_fences(struct i915_ggtt *ggtt)
> for (i = 0; i < num_fences; i++) {
> struct i915_fence_reg *fence = &ggtt->fence_regs[i];
>
> - i915_active_init(&fence->active, NULL, NULL);
> + i915_active_init(&fence->active, NULL, NULL, 0);
> fence->ggtt = ggtt;
> fence->id = i;
> list_add_tail(&fence->link, &ggtt->fence_list);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> index c59468107598..aa0a59c5b614 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c
> @@ -98,7 +98,6 @@ static void pool_free_work(struct work_struct *wrk)
> round_jiffies_up_relative(HZ));
> }
>
> -__i915_active_call
> static void pool_retire(struct i915_active *ref)
> {
> struct intel_gt_buffer_pool_node *node =
> @@ -154,7 +153,7 @@ node_create(struct intel_gt_buffer_pool *pool, size_t sz,
> node->age = 0;
> node->pool = pool;
> node->pinned = false;
> - i915_active_init(&node->active, NULL, pool_retire);
> + i915_active_init(&node->active, NULL, pool_retire, 0);
>
> obj = i915_gem_object_create_internal(gt->i915, sz);
> if (IS_ERR(obj)) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c
> index f19cf6d2fa85..c4a126c8caef 100644
> --- a/drivers/gpu/drm/i915/gt/intel_timeline.c
> +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c
> @@ -32,7 +32,6 @@ static struct i915_vma *hwsp_alloc(struct intel_gt *gt)
> return vma;
> }
>
> -__i915_active_call
> static void __timeline_retire(struct i915_active *active)
> {
> struct intel_timeline *tl =
> @@ -104,7 +103,8 @@ static int intel_timeline_init(struct intel_timeline *timeline,
> INIT_LIST_HEAD(&timeline->requests);
>
> i915_syncmap_init(&timeline->sync);
> - i915_active_init(&timeline->active, __timeline_active, __timeline_retire);
> + i915_active_init(&timeline->active, __timeline_active,
> + __timeline_retire, 0);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index e1ba03b93ffa..32589c6625e1 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -55,7 +55,7 @@ static struct intel_ring *mock_ring(struct intel_engine_cs *engine)
> kfree(ring);
> return NULL;
> }
> - i915_active_init(&ring->vma->active, NULL, NULL);
> + i915_active_init(&ring->vma->active, NULL, NULL, 0);
> __set_bit(I915_VMA_GGTT_BIT, __i915_vma_flags(ring->vma));
> __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &ring->vma->node.flags);
> ring->vma->node.size = sz;
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> index fcde223e26ff..4896e4ccad50 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> @@ -63,7 +63,7 @@ static void pulse_put(struct pulse *p)
> kref_put(&p->kref, pulse_free);
> }
>
> -__i915_active_call static void pulse_retire(struct i915_active *active)
> +static void pulse_retire(struct i915_active *active)
> {
> pulse_put(container_of(active, struct pulse, active));
> }
> @@ -77,7 +77,7 @@ static struct pulse *pulse_create(void)
> return p;
>
> kref_init(&p->kref);
> - i915_active_init(&p->active, pulse_active, pulse_retire);
> + i915_active_init(&p->active, pulse_active, pulse_retire, 0);
>
> return p;
> }
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index aa573b078ae7..b1aa1c482c32 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -343,18 +343,15 @@ active_instance(struct i915_active *ref, u64 idx)
> void __i915_active_init(struct i915_active *ref,
> int (*active)(struct i915_active *ref),
> void (*retire)(struct i915_active *ref),
> + unsigned long flags,
> struct lock_class_key *mkey,
> struct lock_class_key *wkey)
> {
> - unsigned long bits;
> -
> debug_active_init(ref);
>
> - ref->flags = 0;
> + ref->flags = flags;
> ref->active = active;
> - ref->retire = ptr_unpack_bits(retire, &bits, 2);
> - if (bits & I915_ACTIVE_MAY_SLEEP)
> - ref->flags |= I915_ACTIVE_RETIRE_SLEEPS;
> + ref->retire = retire;
>
> spin_lock_init(&ref->tree_lock);
> ref->tree = RB_ROOT;
> @@ -1156,8 +1153,7 @@ static int auto_active(struct i915_active *ref)
> return 0;
> }
>
> -__i915_active_call static void
> -auto_retire(struct i915_active *ref)
> +static void auto_retire(struct i915_active *ref)
> {
> i915_active_put(ref);
> }
> @@ -1171,7 +1167,7 @@ struct i915_active *i915_active_create(void)
> return NULL;
>
> kref_init(&aa->ref);
> - i915_active_init(&aa->base, auto_active, auto_retire);
> + i915_active_init(&aa->base, auto_active, auto_retire, 0);
>
> return &aa->base;
> }
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index fb165d3f01cf..d0feda68b874 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -152,15 +152,16 @@ i915_active_fence_isset(const struct i915_active_fence *active)
> void __i915_active_init(struct i915_active *ref,
> int (*active)(struct i915_active *ref),
> void (*retire)(struct i915_active *ref),
> + unsigned long flags,
> struct lock_class_key *mkey,
> struct lock_class_key *wkey);
>
> /* Specialise each class of i915_active to avoid impossible lockdep cycles. */
> -#define i915_active_init(ref, active, retire) do { \
> - static struct lock_class_key __mkey; \
> - static struct lock_class_key __wkey; \
> - \
> - __i915_active_init(ref, active, retire, &__mkey, &__wkey); \
> +#define i915_active_init(ref, active, retire, flags) do { \
> + static struct lock_class_key __mkey; \
> + static struct lock_class_key __wkey; \
> + \
> + __i915_active_init(ref, active, retire, flags, &__mkey, &__wkey); \
> } while (0)
>
> struct dma_fence *
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index 6360c3e4b765..c149f348a972 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -24,11 +24,6 @@ struct i915_active_fence {
>
> struct active_node;
>
> -#define I915_ACTIVE_MAY_SLEEP BIT(0)
> -
> -#define __i915_active_call __aligned(4)
> -#define i915_active_may_sleep(fn) ptr_pack_bits(&(fn), I915_ACTIVE_MAY_SLEEP, 2)
> -
> struct i915_active {
> atomic_t count;
> struct mutex mutex;
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 468317e3b477..a6cd0fa62847 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -94,7 +94,6 @@ static int __i915_vma_active(struct i915_active *ref)
> return i915_vma_tryget(active_to_vma(ref)) ? 0 : -ENOENT;
> }
>
> -__i915_active_call
> static void __i915_vma_retire(struct i915_active *ref)
> {
> i915_vma_put(active_to_vma(ref));
> @@ -125,7 +124,7 @@ vma_create(struct drm_i915_gem_object *obj,
> vma->size = obj->base.size;
> vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
>
> - i915_active_init(&vma->active, __i915_vma_active, __i915_vma_retire);
> + i915_active_init(&vma->active, __i915_vma_active, __i915_vma_retire, 0);
>
> /* Declare ourselves safe for use inside shrinkers */
> if (IS_ENABLED(CONFIG_LOCKDEP)) {
> diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
> index 1aa52b5cc488..61bf4560d8af 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_active.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_active.c
> @@ -51,7 +51,7 @@ static int __live_active(struct i915_active *base)
> return 0;
> }
>
> -__i915_active_call static void __live_retire(struct i915_active *base)
> +static void __live_retire(struct i915_active *base)
> {
> struct live_active *active = container_of(base, typeof(*active), base);
>
> @@ -68,7 +68,7 @@ static struct live_active *__live_alloc(struct drm_i915_private *i915)
> return NULL;
>
> kref_init(&active->ref);
> - i915_active_init(&active->base, __live_active, __live_retire);
> + i915_active_init(&active->base, __live_active, __live_retire, 0);
>
> return active;
> }
> --
> 2.26.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
