[Intel-gfx] [RFC PATCH v3 10/17] drm/i915/vm_bind: Implement I915_GEM_EXECBUFFER3 ioctl

[Niranjana Vishwanathapura]

On Mon, Sep 05, 2022 at 04:08:57PM +0100, Tvrtko Ursulin wrote:
>
>On 02/09/2022 06:41, Niranjana Vishwanathapura wrote:
>>On Thu, Sep 01, 2022 at 08:58:57AM +0100, Tvrtko Ursulin wrote:
>>>
>>>
>>>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 at intel.com>
>>>>>>
>>>>>>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 at intel.com>
>>>>>>Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>>>>>>Signed-off-by: Andi Shyti <andi.shyti at linux.intel.com>
>>>>>>---
>>>
>>>[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.
>>
>>Here is one (search for copypaste/copy-paste)
>>https://patchwork.freedesktop.org/patch/486608/?series=93447&rev=3
>>It is hard to search for old discussion threads. May be maintainers
>>can provide feedback here directly. Dave, Daniel? :)
>
>Thanks. I had a read and don't see a fundamental conflict with what I 
>said. Conclusion seemed to be to go with a new ioctl and implement 
>code sharing where it makes sense. Which is what TODO in the cover 
>letter acknowledges so there should be no disagreement really.
>
>>>>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.
>>>
>>
>>There are difinitely a few which is 100% copies hence should have a
>>shared code.
>>But some are not. Like, fence_array stuff though looks very similar,
>>the uapi structures are different between execbuf3 and legacy execbuf.
>>The internal flags are also different (eg., __EXEC3_ENGINE_PINNED vs
>>__EXEC_ENGINE_PINNED) which causes minor differences hence not a
>>100% copy.
>>
>>So, I am not convinced if it is worth carrying legacy stuff into
>>execbuf3 code. I think we need to look at these on a case by case
>>basis and see if abstracting common functionality to a separate
>>shared code makes sense or it is better to keep the code separate.
>
>No one is suggesting to carry any legacy stuff into eb3. What I'd 
>suggest is to start something like i915_gem_eb_common.h|c and stuff 
>the 100% copies from the above list in there.
>
>Common struct eb with struct eb2 and eb3 inheriting from it should do 
>the trick. Similarly eb->flags shouldn't be a hard problem to solve.
>
>Then you see what remains and whether it makes sense to consolidate further.
>

Tvrtko,

I have posted vm_bind v4 rfc series with some code sharing between two
execbuf paths. My approach was not having to touch legacy execbuff path
all over the place.

Regards,
Niranjana

>Regards,
>
>Tvrtko
>
>>>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.
>>
>>I am not a big fan of it. I think we should not try to load the execbuf3
>>code with the legacy stuff.
>>
>>Niranjana
>>
>>>
>>>Regards,
>>>
>>>Tvrtko