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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> Signed-off-by: Andi Shyti <andi.shyti@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.

> 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