[PATCH 1/5] [RFC] drm: Documentation style guide

[PATCH 1/5] [RFC] drm: Documentation style guide

[Daniel Vetter]

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, &amp;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 -&lt;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 -->
-- 
2.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/atomic-helper: Drop unneeded argument from check_pending_encoder

[Daniel Vetter]

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);
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm: Move more framebuffer doc from docbook to kerneldoc

[Daniel Vetter]

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
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/atomic-helper: Reject legacy flips on a disabled pipe

[Daniel Vetter]

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,
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/tda998x: Remove dummy save/restore funcs

[Daniel Vetter]

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,
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/tda998x: Remove dummy save/restore funcs

[Emil Velikov]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] [RFC] drm: Documentation style guide

[Pierre Moreau]

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. 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

paramters -> parameters

> +      for structure members, &amp;structure to refernce structures and

refernce -> reference

> +      function() for functions. These all get automatically hyperlinked if
> +      kerneldoc for the referencec objects exists When referencing entries in

referencec -> referenced, missing '.' after exists

> +      function vtables please use -&lt;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 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.

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/tda998x: Remove dummy save/restore funcs

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] [RFC] drm: Documentation style guide

[Daniel Vetter]

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 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.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/tda998x: Remove dummy save/restore funcs

[Russell King - ARM Linux]

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.

-- 
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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/atomic-helper: Reject legacy flips on a disabled pipe

[Daniel Stone]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] [RFC] drm: Documentation style guide

[Laurent Pinchart]

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 markup
> > +      characters kerneldoc provides: @parameter for function paramters,
> > @member
>
> paramters -> parameters
> 
> > +      for structure members, &amp;structure to refernce structures and
> 
> refernce -> reference
> 
> > +      function() for functions. These all get automatically hyperlinked
> > if
> > +      kerneldoc for the referencec objects exists When referencing
> > entries in
>
> referencec -> referenced, missing '.' after exists
> 
> > +      function vtables please use -&lt;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 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

s/changes/chances/

> > +      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

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 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.
> 
> 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 -->

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/atomic-helper: Reject legacy flips on a disabled pipe

[Ilia Mirkin]

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(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/tda998x: Remove dummy save/restore funcs

[Rodrigo Vivi]

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



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Documentation style guide

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch
---
 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, &amp;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 -&lt;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 -->
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch

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. 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, &amp;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 -&lt;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 -->
> -- 
> 2.5.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Jani Nikula]

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-email-daniel.vetter@ffwll.ch
> ---
>  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, &amp;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 -&lt;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 -->

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

Re: [PATCH] drm: Documentation style guide

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch
> > ---
> >  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, &amp;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 -&lt;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 -->
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Laurent Pinchart]

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/

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Lukas Wunner]

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 so
> +      on. To aid in reading documentations make full use of the markup
                              ^
                              insert comma

> +      characters kerneldoc provides: @parameter for function parameters, @member
> +      for structure members, &amp;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 -&lt;vfunc(). Note that kerneldoc does
                                      ^
                                      &gt;

> +      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>

Otherwise looks nice, thank you!

Best regards,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Documentation style guide

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch
---
 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, &amp;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 -&gt;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 -->
-- 
2.5.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3806 bytes --]

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-email-daniel.vetter@ffwll.ch
> ---
>  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

"..., 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, @member
> +      for structure members, &amp;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 -&gt;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.

"should be checked"

> +      <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.

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 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

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm: Move more framebuffer doc from docbook to kerneldoc

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1320 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/atomic-helper: Drop unneeded argument from check_pending_encoder

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 637 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm: Move more framebuffer doc from docbook to kerneldoc

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Documentation style guide

[Dave Gordon]

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-email-daniel.vetter@ffwll.ch
>> ---
>>   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
>
> "..., 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, @member
>> +      for structure members, &amp;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 -&gt;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.
>
> "should be checked"
>
>> +      <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

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 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.
>
> 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 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
>
> 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>
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Documentation style guide

[Daniel Vetter]

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-email-daniel.vetter@ffwll.ch
> >>---
> >>  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
> >
> >"..., 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, @member
> >>+      for structure members, &amp;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 -&gt;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.
> >
> >"should be checked"
> >
> >>+      <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
> 
> 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 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.
> >
> >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 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
> >
> >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>

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/atomic-helper: Reject legacy flips on a disabled pipe

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel