Re: [Intel-gfx] [PATCH 2/2] drm/i915/selftests: add igt_vma_move_to_active_unlocked

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Andrzej,

On Mon, Oct 24, 2022 at 04:05:57PM +0200, Andrzej Hajda wrote:
> On 21.10.2022 17:39, Andi Shyti wrote:
> > Hi Andrzej,
> > 
> > [...]
> > 
> > > +static inline int __must_check
> > > +igt_vma_move_to_active_unlocked(struct i915_vma *vma, struct i915_request *rq,
> > > +				unsigned int flags)
> > > +{
> > > +	int err;
> > > +
> > > +	i915_vma_lock(vma);
> > > +	err = _i915_vma_move_to_active(vma, rq, &rq->fence, flags);
> > > +	i915_vma_unlock(vma);
> > > +	return err;
> > > +}
> > > +
> > 
> > there are calls to i915_vma_move_to_active also outside
> > selftests, why not having a i915_move_to_active_unlocked() in
> > i915_vma.h?
> 
> As I said before, Chris suggested real users of this call should use locking
> explicitly.

Yeah, sure... I was just thinking about it... no big opinion,
besides I don't hink my proposal in Patch 1 makes things easier.

> > Besides here you break also the bisect, because between patch 1
> > and 2 the i915_move_to_avtive would also call
> > i915_request_await_object(). Right or am I getting confused?
> 
> Hmm, looking at v2, I do not see breakage. Patch 1 moves all occurrences of
> i915_request_await_object inside i915_vma_move_to_active.
> Patch 2, just replaces sequence of calls with call to new helper.

Are you sure?

I might be getting confused, but in Patch 1
"i915_vma_move_to_active()" takes "i915_request_await_object()"
inside. This affects all the calls to "i915_vma_move_to_active()"
in the selftests that are not actually requesting
"i915_request_await_object()".

We need to wait for Patch 2 in order to have a local redefinition
of "i915_vma_move_to_active()" for those selftests.

Andi