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

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 24.10.2022 17:08, Andi Shyti wrote:
> 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()".

Apparently I've forgot to answer this comment. Let's do it now.
Currently every call to i915_vma_move_to_active is prepended with 
i915_request_await_object, the only exception is 
prepare_shadow_batch_buffer.
And selftests always calls i915_request_await_object before either 
directly, either via move_to_active helpers.
Patch 1 transforms all these calls, so maybe looking at patch2 confuses you?
I have double checked things, did not find any issue.
If I missed sth please let me know.

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

And this does not seems to be true, patch 1 alone is independent.

Regards
Andrzej


> 
> Andi