Re: [PATCH] wayland-drm: add a description for every item.

[Pekka Paalanen <ppaalanen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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