[PATCH] drm/i915/gt: Handle errors for i915_gem_object_trylock

[PATCH] drm/i915/gt: Handle errors for i915_gem_object_trylock

[Jiasheng Jiang]

As the potential failure of the i915_gem_object_trylock(),
it should be better to check it and return error if fails.

Fixes: 94ce0d65076c ("drm/i915/gt: Setup a default migration context on the GT")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/gpu/drm/i915/gt/selftest_migrate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c
index fa4293d2944f..79c6c68f7316 100644
--- a/drivers/gpu/drm/i915/gt/selftest_migrate.c
+++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c
@@ -465,7 +465,11 @@ create_init_lmem_internal(struct intel_gt *gt, size_t sz, bool try_lmem)
 			return obj;
 	}
 
-	i915_gem_object_trylock(obj, NULL);
+	if (!i915_gem_object_trylock(obj, NULL)) {
+		i915_gem_object_put(obj);
+		return ERR_PTR(-EBUSY);
+	}
+
 	err = i915_gem_object_pin_pages(obj);
 	if (err) {
 		i915_gem_object_unlock(obj);
-- 
2.25.1

Re: [PATCH] drm/i915/gt: Handle errors for i915_gem_object_trylock

[Tvrtko Ursulin]

+ Thomas, Matt

On 02/03/2022 06:19, Jiasheng Jiang wrote:
> As the potential failure of the i915_gem_object_trylock(),
> it should be better to check it and return error if fails.
> 
> Fixes: 94ce0d65076c ("drm/i915/gt: Setup a default migration context on the GT")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>   drivers/gpu/drm/i915/gt/selftest_migrate.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> index fa4293d2944f..79c6c68f7316 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_migrate.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> @@ -465,7 +465,11 @@ create_init_lmem_internal(struct intel_gt *gt, size_t sz, bool try_lmem)
>   			return obj;
>   	}
>   
> -	i915_gem_object_trylock(obj, NULL);

Guys why is this a trylock to start with? (Since being added in 
94ce0d65076c ("drm/i915/gt: Setup a default migration context on the GT").

Surely it can't ever fail since the object has just been created.

Regards,

Tvrtko

> +	if (!i915_gem_object_trylock(obj, NULL)) {
> +		i915_gem_object_put(obj);
> +		return ERR_PTR(-EBUSY);
> +	}
> +
>   	err = i915_gem_object_pin_pages(obj);
>   	if (err) {
>   		i915_gem_object_unlock(obj);

Re: [PATCH] drm/i915/gt: Handle errors for i915_gem_object_trylock

[Hellstrom, Thomas]

On Wed, 2022-03-02 at 10:37 +0000, Tvrtko Ursulin wrote:
> 
> + Thomas, Matt
> 
> On 02/03/2022 06:19, Jiasheng Jiang wrote:
> > As the potential failure of the i915_gem_object_trylock(),
> > it should be better to check it and return error if fails.
> > 
> > Fixes: 94ce0d65076c ("drm/i915/gt: Setup a default migration
> > context on the GT")
> > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > ---
> >   drivers/gpu/drm/i915/gt/selftest_migrate.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > index fa4293d2944f..79c6c68f7316 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_migrate.c
> > @@ -465,7 +465,11 @@ create_init_lmem_internal(struct intel_gt *gt,
> > size_t sz, bool try_lmem)
> >                         return obj;
> >         }
> >   
> > -       i915_gem_object_trylock(obj, NULL);
> 
> Guys why is this a trylock to start with? (Since being added in 
> 94ce0d65076c ("drm/i915/gt: Setup a default migration context on the
> GT").
> 
> Surely it can't ever fail since the object has just been created.

Typically in some situations, we want to create locked objects when
we're already in a ww transaction and might not have access to the ww
context, in which case an ordinary sleeping lock would trigger a
lockdep splat, so a trylock is used instead since it will never fail.

I once introduced a i915_gem_object_lock_isolated() wrapper around
trylock to annotate this situation, but that was removed during the
obj->mm.lock removal IIRC. Similarly TTM has an option to create an
object locked.

So here we should probably BUG on a trylock failure, if anything.
 In the long run we should probably mimic TTM and introduce an
interface to create an object locked.

/Thomas




> 
> Regards,

> 
> Tvrtko





> 
> > +       if (!i915_gem_object_trylock(obj, NULL)) {
> > +               i915_gem_object_put(obj);
> > +               return ERR_PTR(-EBUSY);
> > +       }
> > +
> >         err = i915_gem_object_pin_pages(obj);
> >         if (err) {
> >                 i915_gem_object_unlock(obj);

----------------------------------------------------------------------
Intel Sweden AB
Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden
Registration Number: 556189-6027

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.