Re: [PATCH v3 1/1] Documentation: drm: describing plane alpha and color blending property

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Thu, Mar 27, 2014 at 10:50 AM,  <sagar.a.kamble@intel.com> wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
>
> v2: Added description for "src-color" and "constant-alpha" property.
>     [Review by Laurent Pinchart]
>
> v3: Fixed typos. [Review by David Herrmann]
>
> Cc: Rob Landley <rob at landley.net>
> Cc: Dave Airlie <airlied at redhat.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: "Ville Syrjälä" <ville.syrjala at linux.intel.com>
> Cc: Sagar Kamble <sagar.a.kamble at intel.com>
> Cc: "Purushothaman, Vijay A" <vijay.a.purushothaman at intel.com>
> Cc: linux-doc at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 104402a..66386d0 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2253,6 +2253,14 @@ void intel_crt_init(struct drm_device *dev)
>              enumerated bits defined by the property.</para></listitem>
>          </varlistentry>
>          <varlistentry>
> +          <term>DRM_MODE_PROP_32BIT_PAIR</term>
> +          <listitem><para>This flag restricts all enumerated values of Bitmask properties
> +           to the 0..31 range.
> +            'get_property' operation will get combination of instance values of one or more of the
> +            enumerated bits defined by the property. With 'set_property' operation, user can specify
> +           {value (MSB 32 bits), type (LSB 32 bits)} pair with 64 bit value for that property.</para></listitem>

This is a type description, right? I think this is overly complex (and
I actually don't understand it). How about this:

  "Pair-properties store two packed 32 bit values as 64bit unsigned
integer (often used as key-value pair). The lower 32bits contain the
first value (also: key), while the upper 32bits contain the second
value. Written as: ({key (LSB 32 bits), value (MSB 32 bits)})."

That should be enough for anyone to understand it. Please note that I
inverted 'type' and 'value' here. It's unnatural to list the value
first.

> +        </varlistentry>
> +        <varlistentry>
>            <term>DRM_MODE_PROP_BLOB</term>
>            <listitem><para>Blob properties store a binary blob without any format
>              restriction. The binary blobs are created as KMS standalone objects,
> @@ -2336,7 +2344,7 @@ void intel_crt_init(struct drm_device *dev)
>         </tr>
>         <tr>
>         <td rowspan="19" valign="top" >DRM</td>
> -       <td rowspan="2" valign="top" >Generic</td>
> +       <td rowspan="3" valign="top" >Generic</td>
>         <td valign="top" >"EDID"</td>
>         <td valign="top" >BLOB | IMMUTABLE</td>
>         <td valign="top" >0</td>
> @@ -2351,6 +2359,19 @@ void intel_crt_init(struct drm_device *dev)
>         <td valign="top" >Contains DPMS operation mode value.</td>
>         </tr>
>         <tr>
> +       <td valign="top" >"blend"</td>
> +       <td valign="top" >BITMASK | 32BIT_PAIR</td>

Ugh? This is not a bitmask, is it? It's just a 32BIT_PAIR, right?

> +       <td valign="top" >{ {0, "zero"}, {1, "one"}, {2, "src-color"}, {3, "one-minus-src-color"}
> +       , {4, "dst-color"}, {5, "one-minus-dst-color"}, {6, "src-alpha"}, {7, "one-minus-src-alpha"}, {8, "dst-alpha"}
> +       , {9, "one-minus-dst-alpha"}, {10, "constant-color"}, {11, "one-minus-constant-color"}, {12, "constant-alpha"}
> +       , {13, "one-minus-constant-alpha"}, {14, "alpha-saturate"} }</td>

 I think brackets are weird to describe enums, a table would be nicer.
People might misread this as 32-bit pairs, which this isn't. If you
want to avoid writing yet another table, how about:

ZERO = 0, ONE = 1, SRC_COLOR = 2, .. and so on?

> +       <td valign="top" >Plane</td>
> +       <td valign="top" >Contains plane alpha/color blending operation values. These are modeled after glBlendFunc API

Remove 'API' (or add 'the', like 'the glBlendFunc API').

> +       in OpenGL. Currently only "src-color" and "constant-alpha" are supported. <para>"src-color" will consider supplied fb

This is a driver-dependent statement. I don't think it belongs here.
I'd rather write it as "Not all modes are supported by all drivers.".

Also, please do line-breaks in front of <para>. They start
new-paragraphs, so we should do the same in the code (same below).

> +       with plane's pixel format as input without any additional color/alpha changes.</para><para>"constant-alpha" will apply constant
> +       transparency to all pixels in addition to source color.</para></td>

I would just drop the description of each of the modes. They're
described in the man-pages of glBlendFunc and everyone knows them (and
they're fairly obvious).

However, this doc doesn't describe the second value at all?
glBlendFunc() takes two of these enums, one for the source, one for
the target. So please add some statement like:

  "The first value of the 32BIT_PAIR describes the blending factors of
the source image, the second value describes the blending factors of
the destination image."

Thanks
David

> +       </tr>
> +       <tr>
>         <td rowspan="2" valign="top" >DVI-I</td>
>         <td valign="top" >"subconnector"</td>
>         <td valign="top" >ENUM</td>
> --
> 1.8.5
>