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 */
