On 01/09/2022 06:09, Niranjana Vishwanathapura wrote:
On Wed, Aug 31, 2022 at 08:38:48AM +0100, Tvrtko Ursulin wrote:
On 27/08/2022 20:43, Andi Shyti wrote:
From: Niranjana Vishwanathapura <niranjana.vishwanathapura@xxxxxxxxx>
Implement new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
works in vm_bind mode. The vm_bind mode only works with
this new execbuf3 ioctl.
The new execbuf3 ioctl will not have any list of objects to validate
bind as all required objects binding would have been requested by the
userspace before submitting the execbuf3.
And the legacy support like relocations etc are removed.
Signed-off-by: Niranjana Vishwanathapura
<niranjana.vishwanathapura@xxxxxxxxx>
Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
---
[snip]
+static void signal_fence_array(const struct i915_execbuffer *eb,
+ struct dma_fence * const fence)
+{
+ unsigned int n;
+
+ for (n = 0; n < eb->num_fences; n++) {
+ struct drm_syncobj *syncobj;
+ unsigned int flags;
+
+ syncobj = ptr_unpack_bits(eb->fences[n].syncobj, &flags, 2);
+ if (!(flags & I915_TIMELINE_FENCE_SIGNAL))
+ continue;
+
+ if (eb->fences[n].chain_fence) {
+ drm_syncobj_add_point(syncobj,
+ eb->fences[n].chain_fence,
+ fence,
+ eb->fences[n].value);
+ /*
+ * The chain's ownership is transferred to the
+ * timeline.
+ */
+ eb->fences[n].chain_fence = NULL;
+ } else {
+ drm_syncobj_replace_fence(syncobj, fence);
+ }
+ }
+}
Semi-random place to ask - how many of the code here is direct copy of
existing functions from i915_gem_execbuffer.c? There seems to be some
100% copies at least. And then some more with small tweaks. Spend some
time and try to figure out some code sharing?
During VM_BIND design review, maintainers expressed thought on keeping
execbuf3 completely separate and not touch the legacy execbuf path.
Got a link so this maintainer can see what exactly was said? Just to
make sure there isn't any misunderstanding on what "completely separate"
means to different people.
I also think, execbuf3 should be fully separate. We can do some code
sharing where is a close 100% copy (there is a TODO in cover letter).
There are some changes like the timeline fence array handling here
which looks similar, but the uapi is not exactly the same. Probably,
we should keep them separate and not try to force code sharing at
least at this point.
Okay did not spot that TODO in the cover. But fair since it is RFC to be
unfinished.
I do however think it should be improved before considering the merge.
Because looking at the patch, 100% copies are:
for_each_batch_create_order
for_each_batch_add_order
eb_throttle
eb_pin_timeline
eb_pin_engine
eb_put_engine
__free_fence_array
put_fence_array
await_fence_array
signal_fence_array
retire_requests
eb_request_add
eb_requests_get
eb_requests_put
eb_find_context
Quite a lot.
Then there is a bunch of almost same functions which could be shared if
there weren't two incompatible local struct i915_execbuffer's.
Especially given when the out fence TODO item gets handled a chunk more
will also become a 100% copy.
This could be done by having a common struct i915_execbuffer and then
eb2 and eb3 specific parts which inherit from it. After that is done it
should be easier to see if it makes sense to do something more and how.
Regards,
Tvrtko
