Re: [Intel-gfx] [PATCH] drm/i915: drop the __i915_active_call pointer packing

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@linux.intel.com Suggested-by: Daniel Vetter daniel@ffwll.ch 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@intel.com

I absolutely hate most of the pointer packing code in the i915.

With that: Reviewed-by: Matthew Brost matthew.brost@intel.com

---

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

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx