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
