[Intel-gfx] [PATCH v4 5/5] i915: add documentation to intel_engine_cs

[Joonas Lahtinen]

Quoting kevin.rogovin at intel.com (2018-04-03 13:52:27)
> From: Kevin Rogovin <kevin.rogovin at intel.com>
> 
> Add documentation to a number of the function pointer fields of
> intel_engine_cs.
> 
> Signed-off-by: Kevin Rogovin <kevin.rogovin at intel.com>
> ---
>  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
>