diff mbox series

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

Message ID 20230412113308.812468-3-andi.shyti@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Fix error propagation amongst request | expand

Commit Message

Andi Shyti April 12, 2023, 11:33 a.m. UTC 
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(-)

Comments

Andrzej Hajda April 12, 2023, 1:04 p.m. UTC | #1 
On 12.04.2023 13:33, Andi Shyti wrote:
> 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.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


> +
> +struct i915_request *
> +i915_request_create(struct intel_context *ce)
> +{
> +	struct i915_request *rq;
> +	struct intel_timeline *tl;
> +
> +	tl = intel_context_timeline_lock(ce);
> +	if (IS_ERR(tl))
> +		return ERR_CAST(tl);
> +
> +	rq = __i915_request_create_locked(ce);
> +	if (IS_ERR(rq))
> +		intel_context_timeline_unlock(tl);
>   
> -err_unlock:
> -	intel_context_timeline_unlock(tl);
>   	return rq;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index f5e1bb5e857aa..bb48bd4605c03 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -374,6 +374,8 @@ struct i915_request * __must_check
>   __i915_request_create(struct intel_context *ce, gfp_t gfp);
>   struct i915_request * __must_check
>   i915_request_create(struct intel_context *ce);
> +struct i915_request * __must_check
> +i915_request_create_locked(struct intel_context *ce);
>   
>   void __i915_request_skip(struct i915_request *rq);
>   bool i915_request_set_error_once(struct i915_request *rq, int error);
Andi Shyti April 13, 2023, 8:53 a.m. UTC | #2 
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
diff mbox series

Patch

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);
+}
+
+struct i915_request *
+i915_request_create(struct intel_context *ce)
+{
+	struct i915_request *rq;
+	struct intel_timeline *tl;
+
+	tl = intel_context_timeline_lock(ce);
+	if (IS_ERR(tl))
+		return ERR_CAST(tl);
+
+	rq = __i915_request_create_locked(ce);
+	if (IS_ERR(rq))
+		intel_context_timeline_unlock(tl);
 
-err_unlock:
-	intel_context_timeline_unlock(tl);
 	return rq;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index f5e1bb5e857aa..bb48bd4605c03 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -374,6 +374,8 @@  struct i915_request * __must_check
 __i915_request_create(struct intel_context *ce, gfp_t gfp);
 struct i915_request * __must_check
 i915_request_create(struct intel_context *ce);
+struct i915_request * __must_check
+i915_request_create_locked(struct intel_context *ce);
 
 void __i915_request_skip(struct i915_request *rq);
 bool i915_request_set_error_once(struct i915_request *rq, int error);