diff mbox

wayland-drm: add a description for every item.

Message ID 1427285424-12452-1-git-send-email-linkmauve@linkmauve.fr (mailing list archive)
State New, archived
Headers show

Commit Message

Emmanuel Gil Peyrot March 25, 2015, 12:10 p.m. UTC
This makes the generated protocol headers a lot more readable.
---
 src/egl/wayland/wayland-drm/wayland-drm.xml | 159 +++++++++++++++++-----------
 1 file changed, 100 insertions(+), 59 deletions(-)

Comments

Pekka Paalanen April 6, 2015, 7:37 a.m. UTC | #1
On Wed, 25 Mar 2015 13:10:24 +0100
Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> wrote:

> This makes the generated protocol headers a lot more readable.
> ---
>  src/egl/wayland/wayland-drm/wayland-drm.xml | 159 +++++++++++++++++-----------
>  1 file changed, 100 insertions(+), 59 deletions(-)
> 
> diff --git a/src/egl/wayland/wayland-drm/wayland-drm.xml b/src/egl/wayland/wayland-drm/wayland-drm.xml
> index 5e64622..7cf06d9 100644
> --- a/src/egl/wayland/wayland-drm/wayland-drm.xml
> +++ b/src/egl/wayland/wayland-drm/wayland-drm.xml
> @@ -27,19 +27,31 @@
>      THIS SOFTWARE.
>    </copyright>
>  
> -  <!-- drm support. This object is created by the server and published
> -       using the display's global event. -->
>    <interface name="wl_drm" version="2">
> +    <description summary="drm support">
> +      This object is created by the server and published using the
> +      display's global event.

Hi,

you can say that a lot shorter: "This is a global object
interface." But you could also say more:
- is this a singleton?
- this is a detail of an EGL implementation
- this is specific to Mesa, an internal implementation detail that
  no-one outside Mesa is supposed to access.

> +    </description>
> +
>      <enum name="error">
> -      <entry name="authenticate_fail" value="0"/>
> -      <entry name="invalid_format" value="1"/>
> -      <entry name="invalid_name" value="2"/>
> +      <description summary="error values">
> +        These errors can be emitted in response to wl_drm requests.

That's obvious, isn't it?

> +      </description>
> +      <entry name="authenticate_fail" value="0"
> +             summary="authentication failure"/>
> +      <entry name="invalid_format" value="1"
> +             summary="buffer format is not supported"/>
> +      <entry name="invalid_name" value="2"
> +             summary="invalid name for buffer creation"/>

Ok.

>      </enum>
>  
>      <enum name="format">
> -      <!-- The drm format codes match the #defines in drm_fourcc.h.
> -           The formats actually supported by the compositor will be
> -           reported by the format event. -->
> +      <description summary="pixel formats">
> +        The drm format codes match the #defines in drm_fourcc.h.
> +
> +        The formats actually supported by the compositor will be
> +        reported by the format event.
> +      </description>

Ok.

>        <entry name="c8" value="0x20203843"/>
>        <entry name="rgb332" value="0x38424752"/>
>        <entry name="bgr233" value="0x38524742"/>
> @@ -100,84 +112,113 @@
>        <entry name="yvu444" value="0x34325659"/>
>      </enum>
>  
> -    <!-- Call this request with the magic received from drmGetMagic().
> -         It will be passed on to the drmAuthMagic() or
> -         DRIAuthConnection() call.  This authentication must be
> -         completed before create_buffer could be used. -->
>      <request name="authenticate">
> -      <arg name="id" type="uint"/>
> +      <description summary="authentication request">
> +        Call this request with the magic received from drmGetMagic().
> +
> +        It will be passed on to the drmAuthMagic() or
> +        DRIAuthConnection() call.  This authentication must be
> +        completed before create_buffer could be used.
> +      </description>
> +      <arg name="id" type="uint" summary="drm magic identifier"/>

Ok.

>      </request>
>  
> -    <!-- Create a wayland buffer for the named DRM buffer.  The DRM
> -         surface must have a name using the flink ioctl -->
>      <request name="create_buffer">
> -      <arg name="id" type="new_id" interface="wl_buffer"/>
> -      <arg name="name" type="uint"/>
> -      <arg name="width" type="int"/>
> -      <arg name="height" type="int"/>
> -      <arg name="stride" type="uint"/>
> -      <arg name="format" type="uint"/>
> +      <description summary="create drm buffer">
> +        Create a wayland buffer for the named DRM buffer.
> +
> +        The DRM surface must have a name using the flink ioctl.
> +      </description>
> +      <arg name="id" type="new_id" interface="wl_buffer"
> +           summary="wl_buffer assigned to this drm buffer"/>
> +      <arg name="name" type="uint" summary="unique buffer name"/>

Would be much more explicit to say that is a flink name, not just
any freely chosen unique number.

> +      <arg name="width" type="int" summary="buffer width"/>
> +      <arg name="height" type="int" summary="buffer height"/>

Units?

> +      <arg name="stride" type="uint" summary="stride of a line"/>

Units?

Without specifying the units, you are not really adding any
information here.

> +      <arg name="format" type="uint" summary="see wl_drm_format"/>
>      </request>
>  
> -    <!-- Create a wayland buffer for the named DRM buffer.  The DRM
> -         surface must have a name using the flink ioctl -->
>      <request name="create_planar_buffer">
> -      <arg name="id" type="new_id" interface="wl_buffer"/>
> -      <arg name="name" type="uint"/>
> -      <arg name="width" type="int"/>
> -      <arg name="height" type="int"/>
> -      <arg name="format" type="uint"/>
> -      <arg name="offset0" type="int"/>
> -      <arg name="stride0" type="int"/>
> -      <arg name="offset1" type="int"/>
> -      <arg name="stride1" type="int"/>
> -      <arg name="offset2" type="int"/>
> -      <arg name="stride2" type="int"/>
> +      <description summary="create planar drm buffer">
> +        Create a wayland buffer for the named planar DRM buffer.
> +
> +        The DRM surface must have a name using the flink ioctl.
> +      </description>
> +      <arg name="id" type="new_id" interface="wl_buffer"
> +           summary="wl_buffer assigned to this drm buffer"/>
> +      <arg name="name" type="uint" summary="unique buffer name"/>

Flink name.

> +      <arg name="width" type="int" summary="buffer width"/>
> +      <arg name="height" type="int" summary="buffer height"/>
> +      <arg name="format" type="uint" summary="see wl_drm_format"/>
> +      <arg name="offset0" type="int" summary="first plane offset"/>
> +      <arg name="stride0" type="int" summary="first plane stride"/>
> +      <arg name="offset1" type="int" summary="second plane offset"/>
> +      <arg name="stride1" type="int" summary="second plane stride"/>
> +      <arg name="offset2" type="int" summary="third plane offset"/>
> +      <arg name="stride2" type="int" summary="third plane stride"/>

Units?

>      </request>
>  
> -    <!-- Notification of the path of the drm device which is used by
> -         the server.  The client should use this device for creating
> -         local buffers.  Only buffers created from this device should
> -         be be passed to the server using this drm object's
> -         create_buffer request. -->
>      <event name="device">
> -      <arg name="name" type="string"/>
> +      <description summary="drm device of the server">
> +        Notification of the path of the drm device which is used by the
> +        server.
> +
> +        The client should use this device for creating local buffers.
> +        Only buffers created from this device should be be passed to
> +        the server using this drm object's create_buffer request.
> +      </description>
> +      <arg name="name" type="string" summary="path of the drm device"/>

Ok.

>      </event>
>  
>      <event name="format">
> -      <arg name="format" type="uint"/>
> +      <description summary="pixel format description">
> +        Informs the client about a valid pixel format that can be used
> +        for buffers.
> +      </description>
> +      <arg name="format" type="uint" summary="pixel format"/>
>      </event>
>  
> -    <!-- Raised if the authenticate request succeeded -->
> -    <event name="authenticated"/>
> +    <event name="authenticated">
> +      <description summary="successful authentication">
> +        Raised if the authenticate request succeeded.
> +      </description>
> +    </event>

Ok.

>  
>      <enum name="capability" since="2">
> -      <description summary="wl_drm capability bitmask">
> -        Bitmask of capabilities.
> +      <description summary="capability description">
> +        Lists the available capabilities the server can expose.

You lost the most important word of the description: "bitmask". It
tells how to add new values to this enum.

>        </description>
>        <entry name="prime" value="1" summary="wl_drm prime available"/>
>      </enum>
>  
>      <event name="capabilities">
> -      <arg name="value" type="uint"/>
> +      <description summary="capabilities bitmask">
> +        Bitmask of capabilities supported by the server.
> +      </description>
> +      <arg name="value" type="uint" summary="capabilities"/>

Ok. Might add "see wl_drm_capability".

>      </event>
>  
>      <!-- Version 2 additions -->
>  
> -    <!-- Create a wayland buffer for the prime fd.  Use for regular and planar
> -         buffers.  Pass 0 for offset and stride for unused planes. -->
>      <request name="create_prime_buffer" since="2">
> -      <arg name="id" type="new_id" interface="wl_buffer"/>
> -      <arg name="name" type="fd"/>
> -      <arg name="width" type="int"/>
> -      <arg name="height" type="int"/>
> -      <arg name="format" type="uint"/>
> -      <arg name="offset0" type="int"/>
> -      <arg name="stride0" type="int"/>
> -      <arg name="offset1" type="int"/>
> -      <arg name="stride1" type="int"/>
> -      <arg name="offset2" type="int"/>
> -      <arg name="stride2" type="int"/>
> +      <description summary="create prime drm buffer">
> +        Create a wayland buffer for the prime fd.
> +
> +        Use for regular and planar buffers.  Pass 0 for offset and
> +        stride for unused planes.
> +      </description>
> +      <arg name="id" type="new_id" interface="wl_buffer"
> +           summary="wl_buffer assigned to this drm buffer"/>
> +      <arg name="name" type="fd" summary="prime fd"/>

I'm not really sure what a "prime fd" is, care to explain it in the
description? Is it a dmabuf fd? Any additional semantics or
constraints?

> +      <arg name="width" type="int" summary="buffer width"/>
> +      <arg name="height" type="int" summary="buffer height"/>
> +      <arg name="format" type="uint" summary="see wl_drm_format"/>
> +      <arg name="offset0" type="int" summary="first plane offset"/>
> +      <arg name="stride0" type="int" summary="first plane stride"/>
> +      <arg name="offset1" type="int" summary="second plane offset"/>
> +      <arg name="stride1" type="int" summary="second plane stride"/>
> +      <arg name="offset2" type="int" summary="third plane offset"/>
> +      <arg name="stride2" type="int" summary="third plane stride"/>
>      </request>
>  
>    </interface>

I don't mind if you don't add the extra information I asked for.
So, if you put the "bitmask" back, then this would be:
Revieved-by: Pekka Paalanen <ppaalanen@gmail.com>


Thanks,
pq

PS. the proper mailing lists would be mesa-dev and wayland-devel.
dri-devel is more for the kernel stuff.
diff mbox

Patch

diff --git a/src/egl/wayland/wayland-drm/wayland-drm.xml b/src/egl/wayland/wayland-drm/wayland-drm.xml
index 5e64622..7cf06d9 100644
--- a/src/egl/wayland/wayland-drm/wayland-drm.xml
+++ b/src/egl/wayland/wayland-drm/wayland-drm.xml
@@ -27,19 +27,31 @@ 
     THIS SOFTWARE.
   </copyright>
 
-  <!-- drm support. This object is created by the server and published
-       using the display's global event. -->
   <interface name="wl_drm" version="2">
+    <description summary="drm support">
+      This object is created by the server and published using the
+      display's global event.
+    </description>
+
     <enum name="error">
-      <entry name="authenticate_fail" value="0"/>
-      <entry name="invalid_format" value="1"/>
-      <entry name="invalid_name" value="2"/>
+      <description summary="error values">
+        These errors can be emitted in response to wl_drm requests.
+      </description>
+      <entry name="authenticate_fail" value="0"
+             summary="authentication failure"/>
+      <entry name="invalid_format" value="1"
+             summary="buffer format is not supported"/>
+      <entry name="invalid_name" value="2"
+             summary="invalid name for buffer creation"/>
     </enum>
 
     <enum name="format">
-      <!-- The drm format codes match the #defines in drm_fourcc.h.
-           The formats actually supported by the compositor will be
-           reported by the format event. -->
+      <description summary="pixel formats">
+        The drm format codes match the #defines in drm_fourcc.h.
+
+        The formats actually supported by the compositor will be
+        reported by the format event.
+      </description>
       <entry name="c8" value="0x20203843"/>
       <entry name="rgb332" value="0x38424752"/>
       <entry name="bgr233" value="0x38524742"/>
@@ -100,84 +112,113 @@ 
       <entry name="yvu444" value="0x34325659"/>
     </enum>
 
-    <!-- Call this request with the magic received from drmGetMagic().
-         It will be passed on to the drmAuthMagic() or
-         DRIAuthConnection() call.  This authentication must be
-         completed before create_buffer could be used. -->
     <request name="authenticate">
-      <arg name="id" type="uint"/>
+      <description summary="authentication request">
+        Call this request with the magic received from drmGetMagic().
+
+        It will be passed on to the drmAuthMagic() or
+        DRIAuthConnection() call.  This authentication must be
+        completed before create_buffer could be used.
+      </description>
+      <arg name="id" type="uint" summary="drm magic identifier"/>
     </request>
 
-    <!-- Create a wayland buffer for the named DRM buffer.  The DRM
-         surface must have a name using the flink ioctl -->
     <request name="create_buffer">
-      <arg name="id" type="new_id" interface="wl_buffer"/>
-      <arg name="name" type="uint"/>
-      <arg name="width" type="int"/>
-      <arg name="height" type="int"/>
-      <arg name="stride" type="uint"/>
-      <arg name="format" type="uint"/>
+      <description summary="create drm buffer">
+        Create a wayland buffer for the named DRM buffer.
+
+        The DRM surface must have a name using the flink ioctl.
+      </description>
+      <arg name="id" type="new_id" interface="wl_buffer"
+           summary="wl_buffer assigned to this drm buffer"/>
+      <arg name="name" type="uint" summary="unique buffer name"/>
+      <arg name="width" type="int" summary="buffer width"/>
+      <arg name="height" type="int" summary="buffer height"/>
+      <arg name="stride" type="uint" summary="stride of a line"/>
+      <arg name="format" type="uint" summary="see wl_drm_format"/>
     </request>
 
-    <!-- Create a wayland buffer for the named DRM buffer.  The DRM
-         surface must have a name using the flink ioctl -->
     <request name="create_planar_buffer">
-      <arg name="id" type="new_id" interface="wl_buffer"/>
-      <arg name="name" type="uint"/>
-      <arg name="width" type="int"/>
-      <arg name="height" type="int"/>
-      <arg name="format" type="uint"/>
-      <arg name="offset0" type="int"/>
-      <arg name="stride0" type="int"/>
-      <arg name="offset1" type="int"/>
-      <arg name="stride1" type="int"/>
-      <arg name="offset2" type="int"/>
-      <arg name="stride2" type="int"/>
+      <description summary="create planar drm buffer">
+        Create a wayland buffer for the named planar DRM buffer.
+
+        The DRM surface must have a name using the flink ioctl.
+      </description>
+      <arg name="id" type="new_id" interface="wl_buffer"
+           summary="wl_buffer assigned to this drm buffer"/>
+      <arg name="name" type="uint" summary="unique buffer name"/>
+      <arg name="width" type="int" summary="buffer width"/>
+      <arg name="height" type="int" summary="buffer height"/>
+      <arg name="format" type="uint" summary="see wl_drm_format"/>
+      <arg name="offset0" type="int" summary="first plane offset"/>
+      <arg name="stride0" type="int" summary="first plane stride"/>
+      <arg name="offset1" type="int" summary="second plane offset"/>
+      <arg name="stride1" type="int" summary="second plane stride"/>
+      <arg name="offset2" type="int" summary="third plane offset"/>
+      <arg name="stride2" type="int" summary="third plane stride"/>
     </request>
 
-    <!-- Notification of the path of the drm device which is used by
-         the server.  The client should use this device for creating
-         local buffers.  Only buffers created from this device should
-         be be passed to the server using this drm object's
-         create_buffer request. -->
     <event name="device">
-      <arg name="name" type="string"/>
+      <description summary="drm device of the server">
+        Notification of the path of the drm device which is used by the
+        server.
+
+        The client should use this device for creating local buffers.
+        Only buffers created from this device should be be passed to
+        the server using this drm object's create_buffer request.
+      </description>
+      <arg name="name" type="string" summary="path of the drm device"/>
     </event>
 
     <event name="format">
-      <arg name="format" type="uint"/>
+      <description summary="pixel format description">
+        Informs the client about a valid pixel format that can be used
+        for buffers.
+      </description>
+      <arg name="format" type="uint" summary="pixel format"/>
     </event>
 
-    <!-- Raised if the authenticate request succeeded -->
-    <event name="authenticated"/>
+    <event name="authenticated">
+      <description summary="successful authentication">
+        Raised if the authenticate request succeeded.
+      </description>
+    </event>
 
     <enum name="capability" since="2">
-      <description summary="wl_drm capability bitmask">
-        Bitmask of capabilities.
+      <description summary="capability description">
+        Lists the available capabilities the server can expose.
       </description>
       <entry name="prime" value="1" summary="wl_drm prime available"/>
     </enum>
 
     <event name="capabilities">
-      <arg name="value" type="uint"/>
+      <description summary="capabilities bitmask">
+        Bitmask of capabilities supported by the server.
+      </description>
+      <arg name="value" type="uint" summary="capabilities"/>
     </event>
 
     <!-- Version 2 additions -->
 
-    <!-- Create a wayland buffer for the prime fd.  Use for regular and planar
-         buffers.  Pass 0 for offset and stride for unused planes. -->
     <request name="create_prime_buffer" since="2">
-      <arg name="id" type="new_id" interface="wl_buffer"/>
-      <arg name="name" type="fd"/>
-      <arg name="width" type="int"/>
-      <arg name="height" type="int"/>
-      <arg name="format" type="uint"/>
-      <arg name="offset0" type="int"/>
-      <arg name="stride0" type="int"/>
-      <arg name="offset1" type="int"/>
-      <arg name="stride1" type="int"/>
-      <arg name="offset2" type="int"/>
-      <arg name="stride2" type="int"/>
+      <description summary="create prime drm buffer">
+        Create a wayland buffer for the prime fd.
+
+        Use for regular and planar buffers.  Pass 0 for offset and
+        stride for unused planes.
+      </description>
+      <arg name="id" type="new_id" interface="wl_buffer"
+           summary="wl_buffer assigned to this drm buffer"/>
+      <arg name="name" type="fd" summary="prime fd"/>
+      <arg name="width" type="int" summary="buffer width"/>
+      <arg name="height" type="int" summary="buffer height"/>
+      <arg name="format" type="uint" summary="see wl_drm_format"/>
+      <arg name="offset0" type="int" summary="first plane offset"/>
+      <arg name="stride0" type="int" summary="first plane stride"/>
+      <arg name="offset1" type="int" summary="second plane offset"/>
+      <arg name="stride1" type="int" summary="second plane stride"/>
+      <arg name="offset2" type="int" summary="third plane offset"/>
+      <arg name="stride2" type="int" summary="third plane stride"/>
     </request>
 
   </interface>