Quoting kevin.rogovin@xxxxxxxxx (2018-04-03 13:52:27)
> From: Kevin Rogovin <kevin.rogovin@xxxxxxxxx>
>
> Add documentation to a number of the function pointer fields of
> intel_engine_cs.
>
> Signed-off-by: Kevin Rogovin <kevin.rogovin@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.h | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1f50727a5ddb..eafd1690acde 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -426,23 +426,52 @@ struct intel_engine_cs {
>
> void (*set_default_submission)(struct intel_engine_cs *engine);
>
> + /* In addition to pinning the context, returns the intel_ringbuffer
> + * to which to write commands.
/* Pin context and return intel_ring to write commands to. */
And if you have to resort to multi-line comments, make them
balanced:
/*
* Foo...
* Bar...
*/
These comments feel bit verbose for just being internal ones.
> + */
> struct intel_ring *(*context_pin)(struct intel_engine_cs *engine,
> struct i915_gem_context *ctx);
> void (*context_unpin)(struct intel_engine_cs *engine,
> struct i915_gem_context *ctx);
> +
> + /* Request room on the ringbuffer of a request in order to write
> + * commands for a request; In addition, if necessary, add commands
> + * to the buffer so that the i915_gem_context of the request
> + * is the one active for the commands.
> + */
"Reserve room from the ringbuffer for commands and emit necessary context
switching commands."?
> int (*request_alloc)(struct i915_request *rq);
> +
> + /* Called only once (and only if non-NULL) for an engine; used to
> + * initialize the global driver default context.
> + */
> int (*init_context)(struct i915_request *rq);
>
> + /* Add a GPU command to cache invalidate with EMIT_INVALIDATE,
> + * to pipeline flush with EMIT_FLUSH or to do both with EMIT_BARRIER;
> + * the GPU command is added to the buffer holding the commands of
> + * the request (i.e. calling intel_ring_begin() on
> + * i915_request::ring).
> + */
> int (*emit_flush)(struct i915_request *request, u32 mode);
> #define EMIT_INVALIDATE BIT(0)
> #define EMIT_FLUSH BIT(1)
> #define EMIT_BARRIER (EMIT_INVALIDATE | EMIT_FLUSH)
> + /* Add a batchbuffer start command; the GPU command is added to
> + * the buffer holding the commands of the request (i.e. calling
> + * intel_ring_begin() on i915_request::ring).
> + */
> int (*emit_bb_start)(struct i915_request *rq,
> u64 offset, u32 length,
> unsigned int dispatch_flags);
> #define I915_DISPATCH_SECURE BIT(0)
> #define I915_DISPATCH_PINNED BIT(1)
> #define I915_DISPATCH_RS BIT(2)
> + /* Add a memory write command that writes the global sequence number
> + * (i915_request::global_seqno) and also add an interrupt command;
> + * the GPU command is added to the buffer holding the commands of
> + * the request (i.e. calling intel_ring_begin() on
> + * i915_request::ring).
This is more about what a breadcrumb is than what this interface is
about. "Add commands for triggering a breadcrumb to be picked up" and
maybe explain elsewhere what a breadcrumb is.
So overall, try to make the comments bit less verbose and leave the
implementation detail to the implementation functions :)
Regards, Joonas
> + */
> void (*emit_breadcrumb)(struct i915_request *rq, u32 *cs);
> int emit_breadcrumb_sz;
>
> --
> 2.16.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
