Hi Andrzej,
> > Make version of the request creation that doesn't hold any
> > lock.
> >
> > Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reviewed-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_request.c | 38 +++++++++++++++++++++--------
> > drivers/gpu/drm/i915/i915_request.h | 2 ++
> > 2 files changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 630a732aaecca..58662360ac34e 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -1028,15 +1028,11 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
> > return ERR_PTR(ret);
> > }
> > -struct i915_request *
> > -i915_request_create(struct intel_context *ce)
> > +static struct i915_request *
> > +__i915_request_create_locked(struct intel_context *ce)
> > {
> > + struct intel_timeline *tl = ce->timeline;
> > struct i915_request *rq;
> > - struct intel_timeline *tl;
> > -
> > - tl = intel_context_timeline_lock(ce);
> > - if (IS_ERR(tl))
> > - return ERR_CAST(tl);
> > /* Move our oldest request to the slab-cache (if not in use!) */
> > rq = list_first_entry(&tl->requests, typeof(*rq), link);
> > @@ -1046,16 +1042,38 @@ i915_request_create(struct intel_context *ce)
> > intel_context_enter(ce);
> > rq = __i915_request_create(ce, GFP_KERNEL);
> > intel_context_exit(ce); /* active reference transferred to request */
> > +
> > if (IS_ERR(rq))
> > - goto err_unlock;
> > + return rq;
> > /* Check that we do not interrupt ourselves with a new request */
> > rq->cookie = lockdep_pin_lock(&tl->mutex);
> > return rq;
> > +}
> > +
> > +struct i915_request *
> > +i915_request_create_locked(struct intel_context *ce)
> > +{
> > + intel_context_assert_timeline_is_locked(ce->timeline);
> > +
> > + return __i915_request_create_locked(ce);
> > +}
>
> I wonder if we really need to have such granularity? Leaving only
> i915_request_create_locked and removing __i915_request_create_locked would
> simplify little bit the code,
> I guess the cost of calling intel_context_assert_timeline_is_locked twice
> sometimes is not big, or maybe it can be re-arranged, up to you.
There is some usage of such granularity in patch 4. I am adding
here the throttle on the timeline. I am adding it in the
"_locked" version to avoid potential deadlocks coming from
selftests (and from realworld?).
Here I'd love to have some comments from Chris and Matt.
I might still add this in the commit message:
"i915_request_create_locked() is now empty but will be used in
later commits where a throttle on the ringspace will be executed
to ensure exclusive ownership of the timeline."
> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
Thanks!
Andi
