On May 4, 2018 5:44:25 PM PDT, Chad Versace <chadvers...@chromium.org> wrote:
>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).
Please only use Fixes: to declare that this patch fixed another patch, as noted
in the documentation.
> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>_______________________________________________
>mesa-dev mailing list
>mesa-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
