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

[Niranjana Vishwanathapura]

On Thu, Sep 22, 2022 at 12:54:09PM +0300, Jani Nikula wrote:
>On Wed, 21 Sep 2022, Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h
>> new file mode 100644
>> index 000000000000..725febfd6a53
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer_common.h
>> @@ -0,0 +1,47 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +
>> +#ifndef __I915_GEM_EXECBUFFER_COMMON_H
>> +#define __I915_GEM_EXECBUFFER_COMMON_H
>> +
>> +#include <drm/drm_syncobj.h>
>> +
>> +#include "gt/intel_context.h"
>
>You don't need these includes. Most of it can be handled using forward
>declarations. You'll need <linux/types.h>
>

Thanks Jani,
Sure, here and everywhere I will remove the unwanted includes and use
forward declarations instead.

>> +
>> +struct eb_fence {
>> +	struct drm_syncobj *syncobj;
>> +	struct dma_fence *dma_fence;
>> +	u64 value;
>> +	struct dma_fence_chain *chain_fence;
>> +};
>> +
>> +int __eb_pin_engine(struct intel_context *ce, struct i915_gem_ww_ctx *ww,
>> +		    bool throttle, bool nonblock);
>> +void __eb_unpin_engine(struct intel_context *ce);
>> +int __eb_select_engine(struct intel_context *ce);
>> +void __eb_put_engine(struct intel_context *context, struct intel_gt *gt);
>> +
>> +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,
>
>struct i915_sched_attr is passed by value, so you either need to turn
>that into a pointer, or you need the definition. The definition is just
>a wrapper around an int. (For strict type safety or for future proofing
>or what, I don't know.) And this all brings me to my pet peeve about
>gem/gt headers.
>
>To get that definition of a struct wrapper around an int, you need to
>include i915_scheduler_types.h, which recursively includes a total of 16
>headers. Touch any of those files, and you get a rebuild butterfly
>effect.
>
>28% of i915 header files, when modified, cause the rebuild of 83% of the
>driver. Please let's not make it worse.
>

Ok. I think it is passed by value as it is just a wrapper around an int.
I am just moving this function to a separate file.
Will keep it as such, but will forward declare it insted of including
the i915_scheduler_types.h.

Regards,
Niranjana

>
>BR,
>Jani.
>
>> +		    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 */
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center