Re: [PATCH v5 2/5] drm/i915: Create the locked version of the request create

Hi Andrzej,

...

Make version of the request creation that doesn't hold any lock.

Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: stable@vger.kernel.org Reviewed-by: Nirmoy Das nirmoy.das@intel.com

---

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@intel.com

Thanks!

Andi