[Mesa-dev] [PATCH 1/2] mesa: add R10G10B10{A, X}2 to MESA <-> DRI format translations

[Chad Versace]

Thanks for the patches. The code looks good. All my suggestions are
merely nitpicks to make the patches follow Mesa conventions.

In general, if you have questions about commit message style, examine
the git log for previous patches that touched the same files and
directories as yours. Sometimes, different directories in Mesa can have
very different code style as well as commit style.

First, when a patch touches src/mesa/drivers/dri/common and/or
include/GL/internal/dri_interface.h, and touches nothing else, the patch
subject should probably have the prefix "dri:". For patches that touch
only dri_util.c, like yours, there is also a precedent for using the
"dri_util:" prefix.

On Wed 02 May 2018, Miguel Casas wrote:
> This patch adds R10G10B10{A,X}2 MESA <-> DRI translation entries
> in the appropriate places for dri2 functions to accept them.

"DRI translation entries in the appropriate places for dri2 functions to
accept them" is quite vague but a lot of text. Dense, precise git logs
are the best. Please omit the phrase, or replace it with a precise one.

At risk of over-laboring the point, short-and-sweet-and-precise like any
of the following:

    * Add R10G10B10{A,X}2 translation between mesa_format and DRI format.

    * Add R10G10B10{A,X}2 translation between mesa_format and DRI format
      to driGLFormatToImageFormat() and driImageFormatToGLFormat().

    * Teach driGLFormatToImageFormat() and driImageFormatToGLFormat() to
      translate __DRI_IMAGE_FORMAT_ABGR2101010 and
      __DRI_IMAGE_FORMAT_XBGR2101010.

    * Teach dri_util.c to translate R10G10B10{A,X}2 between mesa_format
      and DRI format.

> BUG=https://crbug.com/776093
> TEST=Compile and deploy mesa+this patch, then playback
> a VP9 Profile 2 video with sw decoder using crrev.com/c/897894.

The Chromium-specific taglines BUG= and TEST= appear nowhere in the Mesa
git log.

The BUG line should be converted to any of the following trailer lines:

    Bug: https://crbug.com/776903
        (This is my favorite).
    Fixes: https://crbug.com/776903
        (But only use Fixes if it fully fixes the bug).
    Reference: https://crbug.com/776903
    References: https://crbug.com/776903
        (Some people use singular Reference, others plural. Shrug).

The TEST line doesn't have a clear translation. Some people would simply
add a paragraph to the commit message like this:

    Tested by playing a VP9 Profile 2 video with sw
    decoder using foo.

Other people try to put in a trailer line, like below. If you use
a trailer, then *please* indent wrapped lines with at least two spaces,
just like RFC 822. Read man:git-interpret-trailers(1) if want to learn
more about trailers.

    Test: Play a VP9 Profile 2 video with sw
      decoder using foo.

Regardless, in the test description:

    * Don't say you built and deployed the patch, *then* ran a test. If
      you ran the test, then we trust you ran it with the patch applied :-)
      Dense git log == good.

    * For a test like this, it's critical to mention what GPU you
      used. If you used Eve, then saying "on Kabylake" would be
      sufficient for this patch.

    * How is the VP9 video related to DRI images? Did you import each
      frame as a dma_buf into a single EGLImage? Into multiple
      EGLImages, one per plane? I don't understand how VP9 is related to
      this patch without more description.

    * Whose software decoder? I don't believe the source of the VP9
      video is important to this patch. You could probably
      s/video with sw decoder/video/ without losing significant
      information. But if you think it's important to mention that the
      video was sw-decoded, then please tell us what decoder you used.

Woo... that was a lot... Thanks for your first Mesa patch!

> ---
>  src/mesa/drivers/dri/common/dri_util.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
> index 7cb6248b13..78c6bbf234 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -886,6 +886,10 @@ driGLFormatToImageFormat(mesa_format format)
>        return __DRI_IMAGE_FORMAT_ARGB2101010;
>     case MESA_FORMAT_B10G10R10X2_UNORM:
>        return __DRI_IMAGE_FORMAT_XRGB2101010;
> +   case MESA_FORMAT_R10G10B10A2_UNORM:
> +      return __DRI_IMAGE_FORMAT_ABGR2101010;
> +   case MESA_FORMAT_R10G10B10X2_UNORM:
> +      return __DRI_IMAGE_FORMAT_XBGR2101010;
>     case MESA_FORMAT_B8G8R8A8_UNORM:
>        return __DRI_IMAGE_FORMAT_ARGB8888;
>     case MESA_FORMAT_R8G8B8A8_UNORM:
> @@ -923,6 +927,10 @@ driImageFormatToGLFormat(uint32_t image_format)
>        return MESA_FORMAT_B10G10R10A2_UNORM;
>     case __DRI_IMAGE_FORMAT_XRGB2101010:
>        return MESA_FORMAT_B10G10R10X2_UNORM;
> +   case __DRI_IMAGE_FORMAT_ABGR2101010:
> +      return MESA_FORMAT_R10G10B10A2_UNORM;
> +   case __DRI_IMAGE_FORMAT_XBGR2101010:
> +      return MESA_FORMAT_R10G10B10X2_UNORM;
>     case __DRI_IMAGE_FORMAT_ARGB8888:
>        return MESA_FORMAT_B8G8R8A8_UNORM;
>     case __DRI_IMAGE_FORMAT_ABGR8888:
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev