[Intel-gfx] [RFC v4 08/14] drm/i915/vm_bind: Abstract out common execbuf functions

[Niranjana Vishwanathapura]

On Thu, Sep 22, 2022 at 10:05:34AM +0100, Tvrtko Ursulin wrote:
>
>On 21/09/2022 19:17, Niranjana Vishwanathapura wrote:
>>On Wed, Sep 21, 2022 at 11:18:53AM +0100, Tvrtko Ursulin wrote:
>>>
>>>On 21/09/2022 08:09, Niranjana Vishwanathapura wrote:

<snip>

>>>
>>>Two things:
>>>
>>>1)
>>>
>>>Is there enough commonality to maybe avoid multiple arguments and 
>>>have like
>>>
>>>struct i915_execbuffer {
>>>
>>>};
>>>
>>>struct i915_execbuffer2 {
>>>    struct i915_execbuffer eb;
>>>    .. eb2 specific fields ..
>>>};
>>>
>>>struct i915_execbuffer3 {
>>>    struct i915_execbuffer eb;
>>>    .. eb3 specific fields ..
>>>};
>>>
>>>And then have the common helpers take the pointer to the common struct?
>>>
>>
>>...
>>This requires updating legacy execbuf path everywhere which doesn't look
>>like a good idea to me. As discussed during vm_bind rfc, I think it is
>>better to keep execbuf3 to itself and keep it leaner.
>
>To be clear the amount of almost the same duplicated code worries me 
>from the maintenance burden angle. I don't think we have any such 
>precedent in the driver. And AFAIR during RFC conclusion was keep the 
>ioctls separate and share code where it makes sense.
>

But if we make a common functions that tries to cater to all with lot
of 'if/else' statements, that also doesn't look good.
What I took from RFC discussion was that code should be duplicated
and only share code where is a 100% match.

>For instance eb_fences_add - could you have a common helper which 
>takes in_fence and out_fence as parameters. Passing in -1/-1 from eb3 
>and end up with even more sharing? Same approach like you did in this 
>patch by making helpers take arguments they need instead of struct eb.
>
>Eb_requests_create? Again same code if you make eb->batch_pool a 
>standalone argument passed in.
>

I am trying to avoid those things. The legacy execbuf and execbuf3 are
very different here. ie., execbuf3 doesn't support in/out fences,
the handling of batches are different and there is no batch_pool etc.
So, it would be good to have those two paths handle it separately.
Why should execbuf3 send dummy '-1 or NULL' etc when the point of
execbuf3 is to move away from legacy things?

Niranjana

>Haven't looked at more than those in this round..
>




>Regards,
>
>Tvrtko
>
>>>2)
>>>
>>>Should we prefix with i915_ everything that is now no longer static?
>>>
>>
>>Yah, makes sense, will update.
>>
>>Niranjana
>>
>>>Regards,
>>>
>>>Tvrtko
>>>
>>>>+
>>>>+struct intel_context *
>>>>+eb_find_context(struct intel_context *context, unsigned int 
>>>>context_number);
>>>>+
>>>>+int add_timeline_fence(struct drm_file *file, u32 handle, u64 point,
>>>>+               struct eb_fence *f, bool wait, bool signal);
>>>>+void put_fence_array(struct eb_fence *fences, u64 num_fences);
>>>>+int await_fence_array(struct eb_fence *fences, u64 num_fences,
>>>>+              struct i915_request *rq);
>>>>+void signal_fence_array(struct eb_fence *fences, u64 num_fences,
>>>>+            struct dma_fence * const fence);
>>>>+
>>>>+int eb_requests_add(struct i915_request **requests, unsigned 
>>>>int num_batches,
>>>>+            struct intel_context *context, struct 
>>>>i915_sched_attr sched,
>>>>+            int err);
>>>>+void eb_requests_get(struct i915_request **requests, unsigned 
>>>>int num_batches);
>>>>+void eb_requests_put(struct i915_request **requests, unsigned 
>>>>int num_batches);
>>>>+
>>>>+struct dma_fence *__eb_composite_fence_create(struct 
>>>>i915_request **requests,
>>>>+                          unsigned int num_batches,
>>>>+                          struct intel_context *context);
>>>>+
>>>>+#endif /* __I915_GEM_EXECBUFFER_COMMON_H */