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

[Tvrtko Ursulin]

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.

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