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

[Jani Nikula]

On Wed, 04 Apr 2018, Joonas Lahtinen  wrote:
> To be perfectly clear: Are we working towards a clean make htmldocs and
> CI nagging when it gets broken?

I'd like that to be the goal, yes. I'm not sure if anyone's working
towards that.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

Quoting Jani Nikula (2018-04-04 12:48:55)
> On Wed, 04 Apr 2018, Joonas Lahtinen  wrote:
> > + Jani for Sphinx
> >
> > Quoting Rogovin, Kevin (2018-04-03 17:34:49)
> >> I am somewhat tempted to just drop this patch or add more documentation. 
> >> The function pointers are used in the code common
> >> to the legacy way and LRC way of submitting batchbuffers to the GPU, so 
> >> they should have somekind of contract to what they are
> >> supposed to do... but spelling out that contract might be a bit much...
> >> 
> >> Opinions?
> >
> > No big feelings to either direction, you could add a documentation block
> > for the flow nearby.
> >
> > If the struct members are referred to from documentation blocks, how far
> > are we from generating warnings if a patch renames something that
> > becomes non-existent in .rst or documentation block? (this one for Jani)
> 
> So first of all, the comments here are not kernel-doc comments, just
> regular comments. It's just free text.
> 
> If you want them to be kernel-doc comments, included to some fancy
> generated documentation, you'll have to follow the guide at [1], wrap
> them in /** and */ and add the @member: tag at the start.
> 
> Specifically, struct::member is not a thing. If you want to reference
> documented struct members in kernel-doc comments, you'll need to use
> _name->member or _name.member.

That was known, but thanks for reminding. What I was weighing was what's the
likelihood of noticing if some struct members get renamed in a patch and
the documentation breaks.

To be perfectly clear: Are we working towards a clean make htmldocs and
CI nagging when it gets broken?

Regards, Joonas

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Wed, 04 Apr 2018, Joonas Lahtinen  wrote:
> + Jani for Sphinx
>
> Quoting Rogovin, Kevin (2018-04-03 17:34:49)
>> I am somewhat tempted to just drop this patch or add more documentation. The 
>> function pointers are used in the code common
>> to the legacy way and LRC way of submitting batchbuffers to the GPU, so they 
>> should have somekind of contract to what they are
>> supposed to do... but spelling out that contract might be a bit much...
>> 
>> Opinions?
>
> No big feelings to either direction, you could add a documentation block
> for the flow nearby.
>
> If the struct members are referred to from documentation blocks, how far
> are we from generating warnings if a patch renames something that
> becomes non-existent in .rst or documentation block? (this one for Jani)

So first of all, the comments here are not kernel-doc comments, just
regular comments. It's just free text.

If you want them to be kernel-doc comments, included to some fancy
generated documentation, you'll have to follow the guide at [1], wrap
them in /** and */ and add the @member: tag at the start.

Specifically, struct::member is not a thing. If you want to reference
documented struct members in kernel-doc comments, you'll need to use
_name->member or _name.member.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

+ Jani for Sphinx

Quoting Rogovin, Kevin (2018-04-03 17:34:49)
> I am somewhat tempted to just drop this patch or add more documentation. The 
> function pointers are used in the code common
> to the legacy way and LRC way of submitting batchbuffers to the GPU, so they 
> should have somekind of contract to what they are
> supposed to do... but spelling out that contract might be a bit much...
> 
> Opinions?

No big feelings to either direction, you could add a documentation block
for the flow nearby.

If the struct members are referred to from documentation blocks, how far
are we from generating warnings if a patch renames something that
becomes non-existent in .rst or documentation block? (this one for Jani)

Regards, Joonas

> 
>  -Kevin
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rogovin, Kevin]

HI,


-Original Message-
From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com] 

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

I like that since it is shorter :)

>> +
>> +   /* 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."?

Agreed; reserved is word to use here.

>> +   /* 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 :)

I am somewhat tempted to just drop this patch or add more documentation. The 
function pointers are used in the code common
to the legacy way and LRC way of submitting batchbuffers to the GPU, so they 
should have somekind of contract to what they are
supposed to do... but spelling out that contract might be a bit much...

Opinions?

 -Kevin
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

Quoting kevin.rogo...@intel.com (2018-04-03 13:52:27)
> From: Kevin Rogovin 
> 
> Add documentation to a number of the function pointer fields of
> intel_engine_cs.
> 
> Signed-off-by: Kevin Rogovin 
> ---
>  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_INVALIDATEBIT(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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx