Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 86e5b12a49ba..5698c93dae8b 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para> + <sect1> + <title>Style Guidelines</title> + <para> + For consistency these documentations use American English. Abbreviations + are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and so + on. To aid in reading documentations make full use of the markup + characters kerneldoc provides: @parameter for function paramters, @member + for structure members, &structure to refernce structures and + function() for functions. These all get automatically hyperlinked if + kerneldoc for the referencec objects exists When referencing entries in + function vtables please use -<vfunc(). Note that with kerneldoc does + not support referncing struct members directly, so please add a reference + to the vtable struct somewhere in the same paragraph or at least section. + </para> + <para> + Except in special situations (to separate locked from unlocked variants) + locking requirements for functions aren't documented in the kerneldoc. + Instead locking should be check at runtime using e.g. + <code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier to + ignore documentation than runtime noise this provides more value. And on + top of that runtime checks do need to be updated when the locking rules + change, increasing the changes that they're correct. Within the + documentation the locking rules should be explained in the relevant + structures: Either in the comment for the lock explaining what it + protects, or data fields need a note about which lock protects them, or + both. + </para> + <para> + Functions which have a non-<code>void</code> return value should have a + section called "Returns" explaining the expected return values in + different cases an their meanings. Currently there's no consensus whether + that section name should be all upper-case or not, and whether it should + end in a colon or not. Go with the file-local style. Other common section + names are "Notes" with information for dangerous or tricky corner cases, + and "FIXME" where the interface could be cleaned up. + </para> + </sect1> </chapter>
<!-- Internals -->
Just a remnant from an old iteration of this patch that I've forgotten to remove: We only need the encoder to figure out whether it has been reassigned in this update already or not to figure out whether there's a conflict or not.
Reported-by: Thierry Reding thierry.reding@gmail.com Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 110f3db8dd05..76c124b2a775 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -88,8 +88,7 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
static bool check_pending_encoder_assignment(struct drm_atomic_state *state, - struct drm_encoder *new_encoder, - struct drm_connector *new_connector) + struct drm_encoder *new_encoder) { struct drm_connector *connector; struct drm_connector_state *conn_state; @@ -256,7 +255,7 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) return 0; }
- if (!check_pending_encoder_assignment(state, new_encoder, connector)) { + if (!check_pending_encoder_assignment(state, new_encoder)) { DRM_DEBUG_ATOMIC("Encoder for [CONNECTOR:%d:%s] already assigned\n", connector->base.id, connector->name);
On Tue, Dec 08, 2015 at 09:49:18AM +0100, Daniel Vetter wrote:
Just a remnant from an old iteration of this patch that I've forgotten to remove: We only need the encoder to figure out whether it has been reassigned in this update already or not to figure out whether there's a conflict or not.
Reported-by: Thierry Reding thierry.reding@gmail.com Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
I missed a few paragraphs in the docbook that need to be pulled into the fbdev vfunc docs.
Cc: Thierry Reding treding@nvidia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/DocBook/gpu.tmpl | 72 +----------------------------------------- include/drm/drm_crtc.h | 18 ++++++++++- 2 files changed, 18 insertions(+), 72 deletions(-)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 5698c93dae8b..0c02ba0d0750 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -986,10 +986,7 @@ int max_width, max_height;</synopsis> !Idrivers/gpu/drm/drm_atomic.c </sect2> <sect2> - <title>Frame Buffer Creation</title> - <synopsis>struct drm_framebuffer *(*fb_create)(struct drm_device *dev, - struct drm_file *file_priv, - struct drm_mode_fb_cmd2 *mode_cmd);</synopsis> + <title>Frame Buffer Abstraction</title> <para> Frame buffers are abstract memory objects that provide a source of pixels to scanout to a CRTC. Applications explicitly request the @@ -1008,73 +1005,6 @@ int max_width, max_height;</synopsis> and so expects TTM handles in the create ioctl and not GEM handles. </para> <para> - Drivers must first validate the requested frame buffer parameters passed - through the mode_cmd argument. In particular this is where invalid - sizes, pixel formats or pitches can be caught. - </para> - <para> - If the parameters are deemed valid, drivers then create, initialize and - return an instance of struct <structname>drm_framebuffer</structname>. - If desired the instance can be embedded in a larger driver-specific - structure. Drivers must fill its <structfield>width</structfield>, - <structfield>height</structfield>, <structfield>pitches</structfield>, - <structfield>offsets</structfield>, <structfield>depth</structfield>, - <structfield>bits_per_pixel</structfield> and - <structfield>pixel_format</structfield> fields from the values passed - through the <parameter>drm_mode_fb_cmd2</parameter> argument. They - should call the <function>drm_helper_mode_fill_fb_struct</function> - helper function to do so. - </para> - - <para> - The initialization of the new framebuffer instance is finalized with a - call to <function>drm_framebuffer_init</function> which takes a pointer - to DRM frame buffer operations (struct - <structname>drm_framebuffer_funcs</structname>). Note that this function - publishes the framebuffer and so from this point on it can be accessed - concurrently from other threads. Hence it must be the last step in the - driver's framebuffer initialization sequence. Frame buffer operations - are - <itemizedlist> - <listitem> - <synopsis>int (*create_handle)(struct drm_framebuffer *fb, - struct drm_file *file_priv, unsigned int *handle);</synopsis> - <para> - Create a handle to the frame buffer underlying memory object. If - the frame buffer uses a multi-plane format, the handle will - reference the memory object associated with the first plane. - </para> - <para> - Drivers call <function>drm_gem_handle_create</function> to create - the handle. - </para> - </listitem> - <listitem> - <synopsis>void (*destroy)(struct drm_framebuffer *framebuffer);</synopsis> - <para> - Destroy the frame buffer object and frees all associated - resources. Drivers must call - <function>drm_framebuffer_cleanup</function> to free resources - allocated by the DRM core for the frame buffer object, and must - make sure to unreference all memory objects associated with the - frame buffer. Handles created by the - <methodname>create_handle</methodname> operation are released by - the DRM core. - </para> - </listitem> - <listitem> - <synopsis>int (*dirty)(struct drm_framebuffer *framebuffer, - struct drm_file *file_priv, unsigned flags, unsigned color, - struct drm_clip_rect *clips, unsigned num_clips);</synopsis> - <para> - This optional operation notifies the driver that a region of the - frame buffer has changed in response to a DRM_IOCTL_MODE_DIRTYFB - ioctl call. - </para> - </listitem> - </itemizedlist> - </para> - <para> The lifetime of a drm framebuffer is controlled with a reference count, drivers can grab additional references with <function>drm_framebuffer_reference</function>and drop them diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4f587a5bc88f..6fe14a773def 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -172,7 +172,9 @@ struct drm_framebuffer_funcs { * Clean up framebuffer resources, specifically also unreference the * backing storage. The core guarantees to call this function for every * framebuffer successfully created by ->fb_create() in - * &drm_mode_config_funcs. + * &drm_mode_config_funcs. Drivers must also call + * drm_framebuffer_cleanup() to release DRM core resources for this + * framebuffer. */ void (*destroy)(struct drm_framebuffer *framebuffer);
@@ -187,6 +189,9 @@ struct drm_framebuffer_funcs { * copying the current screen contents to a private buffer and blending * between that and the new contents. * + * GEM based drivers should call drm_gem_handle_create() to create the + * handle. + * * RETURNS: * * 0 on success or a negative error code on failure. @@ -1727,6 +1732,17 @@ struct drm_mode_config_funcs { * requested metadata, but most of that is left to the driver. See * struct &drm_mode_fb_cmd2 for details. * + * If the parameters are deemed valid and the backing storage objects in + * the underlying memory manager all exists then the drivers to allocate + * a new &drm_framebuffer structure, subclassed to contain + * driver-specific information (like the internal native buffer object + * references). It also needs to fill out all relevant metadata, which + * should by done by calling drm_helper_mode_fill_fb_struct(). + * + * The initializing is finalized by calling drm_framebuffer_init(), + * which registers the framebuffer and makes it accessible to other + * threads. + * * RETURNS: * * A new framebuffer with an initial reference count of 1 or a negative
On Tue, Dec 08, 2015 at 09:49:19AM +0100, Daniel Vetter wrote: [...]
@@ -187,6 +189,9 @@ struct drm_framebuffer_funcs { * copying the current screen contents to a private buffer and blending * between that and the new contents. *
* GEM based drivers should call drm_gem_handle_create() to create the* handle.*
- RETURNS:
- 0 on success or a negative error code on failure.
@@ -1727,6 +1732,17 @@ struct drm_mode_config_funcs { * requested metadata, but most of that is left to the driver. See * struct &drm_mode_fb_cmd2 for details. *
* If the parameters are deemed valid and the backing storage objects in* the underlying memory manager all exists then the drivers to allocate
"... all exist, then the driver allocates a new &drm_framebuffer structure ..."?
* a new &drm_framebuffer structure, subclassed to contain* driver-specific information (like the internal native buffer object* references). It also needs to fill out all relevant metadata, which* should by done by calling drm_helper_mode_fill_fb_struct().
"should be done"
** The initializing is finalized by calling drm_framebuffer_init(),
"The initialization"
Other than that, looks good:
Reviewed-by: Thierry Reding treding@nvidia.com
On Mon, Dec 14, 2015 at 04:58:13PM +0100, Thierry Reding wrote:
On Tue, Dec 08, 2015 at 09:49:19AM +0100, Daniel Vetter wrote: [...]
@@ -187,6 +189,9 @@ struct drm_framebuffer_funcs { * copying the current screen contents to a private buffer and blending * between that and the new contents. *
* GEM based drivers should call drm_gem_handle_create() to create the* handle.*
- RETURNS:
- 0 on success or a negative error code on failure.
@@ -1727,6 +1732,17 @@ struct drm_mode_config_funcs { * requested metadata, but most of that is left to the driver. See * struct &drm_mode_fb_cmd2 for details. *
* If the parameters are deemed valid and the backing storage objects in* the underlying memory manager all exists then the drivers to allocate"... all exist, then the driver allocates a new &drm_framebuffer structure ..."?
* a new &drm_framebuffer structure, subclassed to contain* driver-specific information (like the internal native buffer object* references). It also needs to fill out all relevant metadata, which* should by done by calling drm_helper_mode_fill_fb_struct()."should be done"
** The initializing is finalized by calling drm_framebuffer_init(),"The initialization"
Other than that, looks good:
Reviewed-by: Thierry Reding treding@nvidia.com
All fixed up on both this and the previous patch applied, thanks a lot for your review. -Daniel
We want this for consistency with existing page_flip semantics.
Since this spurred quite a discussion on IRC also document why we reject even generation when the pipe is off: It's not that it's hard to implement, but userspace has a track recording proofing that it's way too easy to accidentally abuse and cause havoc. We want to make sure userspace doesn't get away with that.
v2: Somehow thought we do reject events already, but that code only existed in my imagination ... Also suggestions from Thierry.
Cc: Daniel Stone daniels@collabora.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ include/drm/drm_crtc.h | 3 ++- 3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 07ab75e22b2b..029377513b1d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -508,6 +508,22 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc, return -EINVAL; }
+ /* + * Reject event generation for when a CRTC is off and stays off. + * It wouldn't be hard to implement this, but userspace has a track + * record of happily burning through 100% cpu (or worse, crash) when the + * display pipe is suspended. To avoid all that fun just reject updates + * that ask for events since likely that indicates a bug in the + * compositor's drawing loop. This is consistent with the vblank IOCTL + * and legacy page_flip IOCTL which also reject service on a disabled + * pipe. + */ + if (state->event && !state->active && !crtc->state->active) { + DRM_DEBUG_ATOMIC("[CRTC:%d] requesting event but off\n", + crtc->base.id); + return -EINVAL; + } + return 0; }
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 76c124b2a775..9eb367cad790 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2282,6 +2282,15 @@ retry: goto fail; drm_atomic_set_fb_for_plane(plane_state, fb);
+ /* Make sure we don't accidentally do a full modeset. */ + state->allow_modeset = false; + if (!crtc_state->active) { + DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n", + crtc->base.id); + ret = -EINVAL; + goto fail; + } + ret = drm_atomic_async_commit(state); if (ret != 0) goto fail; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 6fe14a773def..6da847a6cb2f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -548,7 +548,8 @@ struct drm_crtc_funcs { * ->page_flip() operation is already pending the callback should return * -EBUSY. Pageflips on a disabled CRTC (either by setting a NULL mode * or just runtime disabled through DPMS respectively the new atomic - * "ACTIVE" state) should result in an -EINVAL error code. + * "ACTIVE" state) should result in an -EINVAL error code. Note that + * drm_atomic_helper_page_flip() checks this already for atomic drivers. */ int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
Hi,
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We want this for consistency with existing page_flip semantics.
Since this spurred quite a discussion on IRC also document why we reject even generation when the pipe is off: It's not that it's hard to implement, but userspace has a track recording proofing that it's way too easy to accidentally abuse and cause havoc. We want to make sure userspace doesn't get away with that.
v2: Somehow thought we do reject events already, but that code only existed in my imagination ... Also suggestions from Thierry.
What a beautifully-coloured shed.
Reviewed-by: Daniel Stone daniels@collabora.com
Cheers, Daniel
On Tue, Dec 08, 2015 at 12:01:57PM +0000, Daniel Stone wrote:
Hi,
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We want this for consistency with existing page_flip semantics.
Since this spurred quite a discussion on IRC also document why we reject even generation when the pipe is off: It's not that it's hard to implement, but userspace has a track recording proofing that it's way too easy to accidentally abuse and cause havoc. We want to make sure userspace doesn't get away with that.
v2: Somehow thought we do reject events already, but that code only existed in my imagination ... Also suggestions from Thierry.
What a beautifully-coloured shed.
Reviewed-by: Daniel Stone daniels@collabora.com
Added Ilia's spelling fixes and merged to drm-misc, thanks for the review. -Daniel
On Tue, Dec 8, 2015 at 3:49 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We want this for consistency with existing page_flip semantics.
Since this spurred quite a discussion on IRC also document why we reject even generation when the pipe is off: It's not that it's hard
event generation?
to implement, but userspace has a track recording proofing that it's
has a track record which proves that it's
way too easy to accidentally abuse and cause havoc. We want to make sure userspace doesn't get away with that.
v2: Somehow thought we do reject events already, but that code only existed in my imagination ... Also suggestions from Thierry.
Cc: Daniel Stone daniels@collabora.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ include/drm/drm_crtc.h | 3 ++- 3 files changed, 27 insertions(+), 1 deletion(-)
In my quest to remove all the drm_encoder_helper_funcs->save/restore hooks I somehow missed that this driver has a dummy set too - I thought all the i2c drivers only set up the slave_encoder save/restore hooks, which I've left. Remove them.
Reported-by: Stephen Rothwell sfr@canb.auug.org.au Cc: Stephen Rothwell sfr@canb.auug.org.au Cc: Thierry Reding thierry.reding@gmail.com Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 896b6aaf8c4d..79cb9208530e 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -855,18 +855,6 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) priv->dpms = mode; }
-static void -tda998x_encoder_save(struct drm_encoder *encoder) -{ - DBG(""); -} - -static void -tda998x_encoder_restore(struct drm_encoder *encoder) -{ - DBG(""); -} - static bool tda998x_encoder_mode_fixup(struct drm_encoder *encoder, const struct drm_display_mode *mode, @@ -1351,8 +1339,6 @@ static void tda998x_encoder_commit(struct drm_encoder *encoder)
static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = { .dpms = tda998x_encoder_dpms, - .save = tda998x_encoder_save, - .restore = tda998x_encoder_restore, .mode_fixup = tda998x_encoder_mode_fixup, .prepare = tda998x_encoder_prepare, .commit = tda998x_encoder_commit,
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
In my quest to remove all the drm_encoder_helper_funcs->save/restore hooks I somehow missed that this driver has a dummy set too - I thought all the i2c drivers only set up the slave_encoder save/restore hooks, which I've left. Remove them.
There was an identical patch from Rodrigo a couple of days ago
http://lists.freedesktop.org/archives/dri-devel/2015-December/096492.html
-Emil
On Tue, Dec 08, 2015 at 09:30:48AM +0000, Emil Velikov wrote:
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
In my quest to remove all the drm_encoder_helper_funcs->save/restore hooks I somehow missed that this driver has a dummy set too - I thought all the i2c drivers only set up the slave_encoder save/restore hooks, which I've left. Remove them.
There was an identical patch from Rodrigo a couple of days ago
couple = less than 1 ;-) But thanks for pointing out, merged that patch instead. -Daniel
http://lists.freedesktop.org/archives/dri-devel/2015-December/096492.html
-Emil
On Tue, Dec 08, 2015 at 11:11:01AM +0100, Daniel Vetter wrote:
On Tue, Dec 08, 2015 at 09:30:48AM +0000, Emil Velikov wrote:
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
In my quest to remove all the drm_encoder_helper_funcs->save/restore hooks I somehow missed that this driver has a dummy set too - I thought all the i2c drivers only set up the slave_encoder save/restore hooks, which I've left. Remove them.
There was an identical patch from Rodrigo a couple of days ago
couple = less than 1 ;-) But thanks for pointing out, merged that patch instead.
I'd be nice if Rodrigo's patch was copied to me. I've some patches outstanding (the atomic mode set) which I need to send to David, but this change should not conflict.
On Tue, Dec 8, 2015 at 2:15 AM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Tue, Dec 08, 2015 at 11:11:01AM +0100, Daniel Vetter wrote:
On Tue, Dec 08, 2015 at 09:30:48AM +0000, Emil Velikov wrote:
On 8 December 2015 at 08:49, Daniel Vetter daniel.vetter@ffwll.ch wrote:
In my quest to remove all the drm_encoder_helper_funcs->save/restore hooks I somehow missed that this driver has a dummy set too - I thought all the i2c drivers only set up the slave_encoder save/restore hooks, which I've left. Remove them.
There was an identical patch from Rodrigo a couple of days ago
couple = less than 1 ;-) But thanks for pointing out, merged that patch instead.
I'd be nice if Rodrigo's patch was copied to me. I've some patches outstanding (the atomic mode set) which I need to send to David, but this change should not conflict.
I'm really sorry. I should've cc'ed both of you.
-- RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hello Daniel,
Just some typo comments below.
On 09:49 AM - Dec 08 2015, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 86e5b12a49ba..5698c93dae8b 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency these documentations use American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading documentations make full use of the markupcharacters kerneldoc provides: @parameter for function paramters, @member
paramters -> parameters
for structure members, &structure to refernce structures and
refernce -> reference
function() for functions. These all get automatically hyperlinked ifkerneldoc for the referencec objects exists When referencing entries in
referencec -> referenced, missing '.' after exists
function vtables please use -<vfunc(). Note that with kerneldoc does
Isn't "with" too much here? "Note that kerneldoc does not […]"?
not support referncing struct members directly, so please add a reference
referncing -> referencing
to the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the changes that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.
Why not define (and use) a single style for naming all sections? Old documentation might not use it, but it should be doable to upgrade those old documents.
Pierre
</para>
</sect1> </chapter>
<!-- Internals -->
-- 2.5.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Dec 08, 2015 at 10:59:05AM +0100, Pierre Moreau wrote:
On 09:49 AM - Dec 08 2015, Daniel Vetter wrote:
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.Why not define (and use) a single style for naming all sections? Old documentation might not use it, but it should be doable to upgrade those old documents.
There is a massive pile of these docs, and there is a slight difference in how vfunc table section headings and function reference section headings are rendered. Given that I figured I'll start modestly.
But if you feel like making this consistent I'd definitely take a patch to do so. Same really for any of the other style guideline issues. Well, after we have some acks on this, and any large-scale style change should first do the RFC patch for this section for discussion to avoid the risk of wasting lots of time. -Daniel
Hello,
On Tuesday 08 December 2015 10:59:05 Pierre Moreau wrote:
On 09:49 AM - Dec 08 2015, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 86e5b12a49ba..5698c93dae8b 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency these documentations use American English.Abbreviations
are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC,and so
on. To aid in reading documentations make full use of the markupcharacters kerneldoc provides: @parameter for function paramters,@member
paramters -> parameters
for structure members, &structure to refernce structures andrefernce -> reference
function() for functions. These all get automatically hyperlinkedif
kerneldoc for the referencec objects exists When referencingentries in
referencec -> referenced, missing '.' after exists
function vtables please use -<vfunc(). Note that with kerneldocdoes
Isn't "with" too much here? "Note that kerneldoc does not […]"?
not support referncing struct members directly, so please add areference
referncing -> referencing
to the vtable struct somewhere in the same paragraph or at leastsection.
</para>
<para>
Except in special situations (to separate locked from unlockedvariants)
locking requirements for functions aren't documented in thekerneldoc.
Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's mucheasier to
ignore documentation than runtime noise this provides more value.And on
top of that runtime checks do need to be updated when the lockingrules
change, increasing the changes that they're correct. Within the
s/changes/chances/
documentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protectsthem, or
both.</para>
<para>
Functions which have a non-<code>void</code> return value shouldhave a
section called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus
s/an/and/
Apart from that,
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
whether
that section name should be all upper-case or not, and whether itshould
end in a colon or not. Go with the file-local style. Other commonsection
names are "Notes" with information for dangerous or tricky cornercases,
and "FIXME" where the interface could be cleaned up.Why not define (and use) a single style for naming all sections? Old documentation might not use it, but it should be doable to upgrade those old documents.
</para>
</sect1>
</chapter> <!-- Internals -->
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai... --- Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..ce4d6f017242 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para> + <sect1> + <title>Style Guidelines</title> + <para> + For consistency these documentations use American English. Abbreviations + are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and so + on. To aid in reading documentations make full use of the markup + characters kerneldoc provides: @parameter for function paramters, @member + for structure members, &structure to refernce structures and + function() for functions. These all get automatically hyperlinked if + kerneldoc for the referencec objects exists When referencing entries in + function vtables please use -<vfunc(). Note that with kerneldoc does + not support referncing struct members directly, so please add a reference + to the vtable struct somewhere in the same paragraph or at least section. + </para> + <para> + Except in special situations (to separate locked from unlocked variants) + locking requirements for functions aren't documented in the kerneldoc. + Instead locking should be check at runtime using e.g. + <code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier to + ignore documentation than runtime noise this provides more value. And on + top of that runtime checks do need to be updated when the locking rules + change, increasing the changes that they're correct. Within the + documentation the locking rules should be explained in the relevant + structures: Either in the comment for the lock explaining what it + protects, or data fields need a note about which lock protects them, or + both. + </para> + <para> + Functions which have a non-<code>void</code> return value should have a + section called "Returns" explaining the expected return values in + different cases an their meanings. Currently there's no consensus whether + that section name should be all upper-case or not, and whether it should + end in a colon or not. Go with the file-local style. Other common section + names are "Notes" with information for dangerous or tricky corner cases, + and "FIXME" where the interface could be cleaned up. + </para> + </sect1> </chapter>
<!-- Internals -->
On Wed, Dec 09, 2015 at 11:41:31AM +0100, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Ok I pulled that one in, thanks for all the comments. Like I said in this thread, this is just a start. So if anyone has an OCD doc style thing, please just add it here. -Daniel
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..ce4d6f017242 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency these documentations use American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading documentations make full use of the markupcharacters kerneldoc provides: @parameter for function paramters, @memberfor structure members, &structure to refernce structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referencec objects exists When referencing entries infunction vtables please use -<vfunc(). Note that with kerneldoc doesnot support referncing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the changes that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.</para>
</sect1> </chapter>
<!-- Internals -->
-- 2.5.1
On Wed, 09 Dec 2015, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Nah, you ignored my comment about "these documentations use American English". :/
BR, Jani.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..ce4d6f017242 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency these documentations use American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading documentations make full use of the markupcharacters kerneldoc provides: @parameter for function paramters, @memberfor structure members, &structure to refernce structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referencec objects exists When referencing entries infunction vtables please use -<vfunc(). Note that with kerneldoc doesnot support referncing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the changes that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.</para>
</sect1> </chapter>
<!-- Internals -->
On Wednesday 09 December 2015 13:21:09 Jani Nikula wrote:
On Wed, 09 Dec 2015, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Nah, you ignored my comment about "these documentations use American English". :/
Isn't documentation uncountable when meaning information recorded in a document ?
On Wed, 09 Dec 2015, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Wednesday 09 December 2015 13:21:09 Jani Nikula wrote:
On Wed, 09 Dec 2015, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Nah, you ignored my comment about "these documentations use American English". :/
Isn't documentation uncountable when meaning information recorded in a document ?
That was my comment, though not on the list. I also thought it was not necessary to recommend the use of American English.
It did occur to me perhaps documentations are countable in American English. ;)
BR, Jani.
On Wednesday 09 December 2015 16:17:47 Jani Nikula wrote:
On Wed, 09 Dec 2015, Laurent Pinchart laurent.pinchart@ideasonboard.com
wrote:
On Wednesday 09 December 2015 13:21:09 Jani Nikula wrote:
On Wed, 09 Dec 2015, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Nah, you ignored my comment about "these documentations use American English". :/
Isn't documentation uncountable when meaning information recorded in a document ?
That was my comment, though not on the list. I also thought it was not necessary to recommend the use of American English.
It did occur to me perhaps documentations are countable in American English. ;)
https://lwn.net/Articles/636286/
On Wed, Dec 09, 2015 at 01:21:09PM +0200, Jani Nikula wrote:
On Wed, 09 Dec 2015, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
Nah, you ignored my comment about "these documentations use American English". :/
Well I did type them all in, but totally failed to run git add before hitting send. Fixed now. -Daniel
BR, Jani.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..ce4d6f017242 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency these documentations use American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading documentations make full use of the markupcharacters kerneldoc provides: @parameter for function paramters, @memberfor structure members, &structure to refernce structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referencec objects exists When referencing entries infunction vtables please use -<vfunc(). Note that with kerneldoc doesnot support referncing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the changes that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases an their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.</para>
</sect1> </chapter>
<!-- Internals -->
-- Jani Nikula, Intel Open Source Technology Center
Hi,
I wouldn't normally nitpick like this but since I was reading it anyway and you were asking for "OCD doc style thing". :-)
This is a proofread of the force-pushed v2 in drm-intel-nightly (9a8730ddfe1d).
<sect1>
<title>Style Guidelines</title>
<para>
For consistency this documentation use American English. Abbreviations
^ s/use/uses/
are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading documentations make full use of the markup
^ insert comma
characters kerneldoc provides: @parameter for function parameters, @memberfor structure members, &structure to reference structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referenced objects exists. When referencing entries infunction vtables please use -<vfunc(). Note that kerneldoc does
^ >
not support referencing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the chances that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases and their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionnames are "Notes" with information for dangerous or tricky corner cases,and "FIXME" where the interface could be cleaned up.</para>
</sect1>
Otherwise looks nice, thank you!
Best regards,
Lukas
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
v3: More spelling fixes from Lukas.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Lukas Wunner lukas@wunner.de Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai... --- Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..c66d6412f573 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para> + <sect1> + <title>Style Guidelines</title> + <para> + For consistency this documentation uses American English. Abbreviations + are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and so + on. To aid in reading, documentations make full use of the markup + characters kerneldoc provides: @parameter for function parameters, @member + for structure members, &structure to reference structures and + function() for functions. These all get automatically hyperlinked if + kerneldoc for the referenced objects exists. When referencing entries in + function vtables please use ->vfunc(). Note that kerneldoc does + not support referencing struct members directly, so please add a reference + to the vtable struct somewhere in the same paragraph or at least section. + </para> + <para> + Except in special situations (to separate locked from unlocked variants) + locking requirements for functions aren't documented in the kerneldoc. + Instead locking should be check at runtime using e.g. + <code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier to + ignore documentation than runtime noise this provides more value. And on + top of that runtime checks do need to be updated when the locking rules + change, increasing the chances that they're correct. Within the + documentation the locking rules should be explained in the relevant + structures: Either in the comment for the lock explaining what it + protects, or data fields need a note about which lock protects them, or + both. + </para> + <para> + Functions which have a non-<code>void</code> return value should have a + section called "Returns" explaining the expected return values in + different cases and their meanings. Currently there's no consensus whether + that section name should be all upper-case or not, and whether it should + end in a colon or not. Go with the file-local style. Other common section + names are "Notes" with information for dangerous or tricky corner cases, + and "FIXME" where the interface could be cleaned up. + </para> + </sect1> </chapter>
<!-- Internals -->
On Wed, Dec 09, 2015 at 05:08:02PM +0100, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
v3: More spelling fixes from Lukas.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Lukas Wunner lukas@wunner.de Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..c66d6412f573 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency this documentation uses American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading, documentations make full use of the markup
"..., the documentation makes full use of..." and perhaps "makes use of the full set of markup characters that kerneldoc provides".
characters kerneldoc provides: @parameter for function parameters, @memberfor structure members, &structure to reference structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referenced objects exists. When referencing entries infunction vtables please use ->vfunc(). Note that kerneldoc doesnot support referencing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g.
"should be checked"
<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the chances that they're correct. Within thedocumentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.
I think you're supposed to have the "or" only in the final subsentence:
"either ... protects, data fields need ..., or both."
</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases and their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common section
I thought the colon was necessary for kerneldoc to turn it into a section?
Overall, long overdue, so thanks for writing it up:
Acked-by: Thierry Reding treding@nvidia.com
On 14/12/15 15:39, Thierry Reding wrote:
On Wed, Dec 09, 2015 at 05:08:02PM +0100, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
v3: More spelling fixes from Lukas.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Lukas Wunner lukas@wunner.de Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..c66d6412f573 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency this documentation uses American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading, documentations make full use of the markup"..., the documentation makes full use of..." and perhaps "makes use of the full set of markup characters that kerneldoc provides".
characters kerneldoc provides: @parameter for function parameters, @memberfor structure members, &structure to reference structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referenced objects exists. When referencing entries infunction vtables please use ->vfunc(). Note that kerneldoc doesnot support referencing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g."should be checked"
<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the chances that they're correct. Within the
A few commas to delimit subclauses would make this more readable:
Since it's much easier to ignore documentation than runtime noise, this provides more value. And on top of that, runtime checks have to be updated when the locking rules change, thus increasing the chances that they're correct.
documentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.I think you're supposed to have the "or" only in the final subsentence:
"either ... protects, data fields need ..., or both."
Within the documentation, the locking rules should be explained in comments on the relevant structures; these comments may be with the lock, explaining what it protects, or with the data, noting which lock protects it, or both -- in which case they should agree!
</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases and their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionI thought the colon was necessary for kerneldoc to turn it into a section?
Overall, long overdue, so thanks for writing it up:
Acked-by: Thierry Reding treding@nvidia.com
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Dec 15, 2015 at 02:56:10PM +0000, Dave Gordon wrote:
On 14/12/15 15:39, Thierry Reding wrote:
On Wed, Dec 09, 2015 at 05:08:02PM +0100, Daniel Vetter wrote:
Every time I type or review docs this seems a bit different. Try to document the common style so we can try to unify at least new docs.
v2: Spelling fixes from Pierre, Laurent and Jani.
v3: More spelling fixes from Lukas.
Cc: Pierre Moreau pierre.morrow@free.fr Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Lukas Wunner lukas@wunner.de Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Link: http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-emai...
Documentation/DocBook/gpu.tmpl | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 749b8e2f2113..c66d6412f573 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -124,6 +124,43 @@ <para> [Insert diagram of typical DRM stack here] </para>
<sect1>
<title>Style Guidelines</title>
<para>
For consistency this documentation uses American English. Abbreviationsare written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and soon. To aid in reading, documentations make full use of the markup"..., the documentation makes full use of..." and perhaps "makes use of the full set of markup characters that kerneldoc provides".
characters kerneldoc provides: @parameter for function parameters, @memberfor structure members, &structure to reference structures andfunction() for functions. These all get automatically hyperlinked ifkerneldoc for the referenced objects exists. When referencing entries infunction vtables please use ->vfunc(). Note that kerneldoc doesnot support referencing struct members directly, so please add a referenceto the vtable struct somewhere in the same paragraph or at least section.</para>
<para>
Except in special situations (to separate locked from unlocked variants)locking requirements for functions aren't documented in the kerneldoc.Instead locking should be check at runtime using e.g."should be checked"
<code>WARN_ON(!mutex_is_locked(...));</code>. Since it's much easier toignore documentation than runtime noise this provides more value. And ontop of that runtime checks do need to be updated when the locking ruleschange, increasing the chances that they're correct. Within theA few commas to delimit subclauses would make this more readable:
Since it's much easier to ignore documentation than runtime noise, this provides more value. And on top of that, runtime checks have to be updated when the locking rules change, thus increasing the chances that they're correct.
documentation the locking rules should be explained in the relevantstructures: Either in the comment for the lock explaining what itprotects, or data fields need a note about which lock protects them, orboth.I think you're supposed to have the "or" only in the final subsentence:
"either ... protects, data fields need ..., or both."
Within the documentation, the locking rules should be explained in comments on the relevant structures; these comments may be with the lock, explaining what it protects, or with the data, noting which lock protects it, or both -- in which case they should agree!
</para>
<para>
Functions which have a non-<code>void</code> return value should have asection called "Returns" explaining the expected return values indifferent cases and their meanings. Currently there's no consensus whetherthat section name should be all upper-case or not, and whether it shouldend in a colon or not. Go with the file-local style. Other common sectionI thought the colon was necessary for kerneldoc to turn it into a section?
Overall, long overdue, so thanks for writing it up:
Acked-by: Thierry Reding treding@nvidia.com
Unfortunately pull request with this already went to Dave before I could take your feedback into account. Anyone up for a quick follow-up patch that I could vacuum up in time for 4.5 (i.e. until Thu latest)?
Thanks, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Vetter -
Daniel Vetter -
Dave Gordon -
Emil Velikov -
Ilia Mirkin -
Jani Nikula -
Laurent Pinchart -
Lukas Wunner -
Pierre Moreau -
Rodrigo Vivi -
Russell King - ARM Linux -
Thierry Reding