[Intel-gfx] [PATCH 1/5] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Nov 21 16:30:08 UTC 2019
On 21/11/2019 13:51, Chris Wilson wrote:
> As we start peeking into requests for longer and longer, e.g.
> incorporating use of spinlocks when only protected by an
> rcu_read_lock(), we need to be careful in how we reset the request when
> recycling and need to preserve any barriers that may still be in use as
> the request is reset for reuse.
How do you tell which members should be left untouched on request_create?
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_request.c | 37 ++++++++++++++++++---------
> drivers/gpu/drm/i915/i915_scheduler.c | 6 +++++
> drivers/gpu/drm/i915/i915_scheduler.h | 1 +
> drivers/gpu/drm/i915/i915_sw_fence.c | 8 ++++++
> drivers/gpu/drm/i915/i915_sw_fence.h | 2 ++
> 5 files changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 00011f9533b6..5e5bd7d57134 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
> spin_lock(&engine->active.lock);
> locked = engine;
> }
> - list_del(&rq->sched.link);
> + list_del_init(&rq->sched.link);
> spin_unlock_irq(&locked->active.lock);
> }
>
> @@ -586,6 +586,19 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
> return kmem_cache_alloc(global.slab_requests, gfp);
> }
>
> +static void __i915_request_ctor(void *arg)
> +{
> + struct i915_request *rq = arg;
> +
> + spin_lock_init(&rq->lock);
> + i915_sched_node_init(&rq->sched);
> + i915_sw_fence_init(&rq->submit, submit_notify);
> + i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> +
> + INIT_LIST_HEAD(&rq->execute_cb);
> + rq->file_priv = NULL;
> +}
> +
> struct i915_request *
> __i915_request_create(struct intel_context *ce, gfp_t gfp)
> {
> @@ -655,24 +668,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
>
> rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>
> - spin_lock_init(&rq->lock);
> dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
> tl->fence_context, seqno);
>
> /* We bump the ref for the fence chain */
> - i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
> - i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
> + i915_sw_fence_reinit(&i915_request_get(rq)->submit);
> + i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>
> - i915_sched_node_init(&rq->sched);
> + i915_sched_node_reinit(&rq->sched);
>
> /* No zalloc, must clear what we need by hand */
> - rq->file_priv = NULL;
> rq->batch = NULL;
> rq->capture_list = NULL;
> rq->flags = 0;
>
> - INIT_LIST_HEAD(&rq->execute_cb);
For instance this member is now untouched. How do we assert it is in the
correct state before it gets used with the new request? What if it has
dangling state?
Same question for rq->file_priv? We have to be sure it is not read and
acted upon before it is set, right?
Sw_fence looks okay. Sched_node as well since it seems to have a fini
which cleans it up.
Regards,
Tvrtko
> -
> /*
> * Reserve space in the ring buffer for all the commands required to
> * eventually emit this request. This is to guarantee that the
> @@ -1533,10 +1542,14 @@ static struct i915_global_request global = { {
>
> int __init i915_global_request_init(void)
> {
> - global.slab_requests = KMEM_CACHE(i915_request,
> - SLAB_HWCACHE_ALIGN |
> - SLAB_RECLAIM_ACCOUNT |
> - SLAB_TYPESAFE_BY_RCU);
> + global.slab_requests =
> + kmem_cache_create("i915_request",
> + sizeof(struct i915_request),
> + __alignof__(struct i915_request),
> + SLAB_HWCACHE_ALIGN |
> + SLAB_RECLAIM_ACCOUNT |
> + SLAB_TYPESAFE_BY_RCU,
> + __i915_request_ctor);
> if (!global.slab_requests)
> return -ENOMEM;
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 194246548c4d..a5a6dbe6a53c 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node *node)
> INIT_LIST_HEAD(&node->signalers_list);
> INIT_LIST_HEAD(&node->waiters_list);
> INIT_LIST_HEAD(&node->link);
> +}
> +
> +void i915_sched_node_reinit(struct i915_sched_node *node)
> +{
> node->attr.priority = I915_PRIORITY_INVALID;
> node->semaphores = 0;
> node->flags = 0;
> @@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
> if (dep->flags & I915_DEPENDENCY_ALLOC)
> i915_dependency_free(dep);
> }
> + INIT_LIST_HEAD(&node->signalers_list);
>
> /* Remove ourselves from everyone who depends upon us */
> list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
> @@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node *node)
> if (dep->flags & I915_DEPENDENCY_ALLOC)
> i915_dependency_free(dep);
> }
> + INIT_LIST_HEAD(&node->waiters_list);
>
> spin_unlock_irq(&schedule_lock);
> }
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index 07d243acf553..d1dc4efef77b 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -26,6 +26,7 @@
> sched.link)
>
> void i915_sched_node_init(struct i915_sched_node *node);
> +void i915_sched_node_reinit(struct i915_sched_node *node);
>
> bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
> struct i915_sched_node *signal,
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 6a88db291252..eacc6c5ce0fd 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
> fence->flags = (unsigned long)fn;
> }
>
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence)
> +{
> + debug_fence_init(fence);
> +
> + atomic_set(&fence->pending, 1);
> + fence->error = 0;
> +}
> +
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> debug_fence_activate(fence);
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index ab7d58bd0b9d..1e90d9a51bd2 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -54,6 +54,8 @@ do { \
> __i915_sw_fence_init((fence), (fn), NULL, NULL)
> #endif
>
> +void i915_sw_fence_reinit(struct i915_sw_fence *fence);
> +
> #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
> void i915_sw_fence_fini(struct i915_sw_fence *fence);
> #else
>
More information about the Intel-gfx
mailing list