| Message ID | 1522752747-7836-6-git-send-email-kevin.rogovin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting kevin.rogovin@intel.com (2018-04-03 13:52:27) > From: Kevin Rogovin <kevin.rogovin@intel.com> > > Add documentation to a number of the function pointer fields of > intel_engine_cs. > > Signed-off-by: Kevin Rogovin <kevin.rogovin@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 >
HI, -----Original Message----- From: Joonas Lahtinen [mailto:joonas.lahtinen@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
+ 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
On Wed, 04 Apr 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 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 &struct_name->member or &struct_name.member. BR, Jani.
Quoting Jani Nikula (2018-04-04 12:48:55) > On Wed, 04 Apr 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 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 > &struct_name->member or &struct_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
On Wed, 04 Apr 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 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.
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. + */ 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. + */ 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). + */ void (*emit_breadcrumb)(struct i915_request *rq, u32 *cs); int emit_breadcrumb_sz;