Add support for 2x2 matrices

Add support for 2x2 matrices

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1755483002

Committed: https://skia.googlesource.com/skia/+/8d988b3a73a75e062bc139f19cc6966888c722e2

[Chris Dalton, 2016-03-01 06:34:34]

Description was changed from

==========
Add support for 2x2 matrices

BUG=skia:
==========

to

==========
Add support for 2x2 matrices

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-03-01 06:49:04]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[Chris Dalton, 2016-03-01 06:49:05]

Greg, can you recommend a bot to verify the changes to GrVkProgramDataManager?

[bsalomon, 2016-03-01 14:18:09]

lgtm other than one nit. We don't have a Vk bot yet (hopefully soon). I'll let
Greg decide between landing with fingers crossed or applying and running it
locally before landing.

https://codereview.chromium.org/1755483002/diff/20001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDataManager.h (right):

https://codereview.chromium.org/1755483002/diff/20001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDataManager.h:64: void setMatrix2f(UniformHandle u, const
float matrix[]) const override;
Why this change? Seems like it just repeats the typename without adding any
additional documentation about what the param means.

[egdaniel, 2016-03-01 14:22:12]

Currently we do not have vulkan running on any of our bots (but work is underway
to get there). The vulkan stuff here all looks good minus one missing part. We
need to update GrVkUniformHandler.cpp get_ubo_aligned_offset(...) and its two
helpers.

So matrices always are viewed in vulkan as an array of column vec<n>s. In
general they are aligned and have array stride equal to the size of the column
type. However, when used in a uniform buffer we must round up these sizes to 16
byte multiples. Things do get a little more complicated with 2x2 since with 3x3
(and trivially 4x4) we could rely on the fact that a vec3 was the same as a vec4
in terms of alignment and size. Vec2's have alignment of 8 bytes.

[Chris Dalton, 2016-03-07 18:35:38]

This should address the comments.

Greg, still shooting in the dark, but does this Vulkan change look right?

https://codereview.chromium.org/1755483002/diff/20001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDataManager.h (right):

https://codereview.chromium.org/1755483002/diff/20001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDataManager.h:64: void setMatrix2f(UniformHandle u, const
float matrix[]) const override;
On 2016/03/01 14:18:08, bsalomon wrote:
> Why this change? Seems like it just repeats the typename without adding any
> additional documentation about what the param means.

Oops, that snuck its way in by accident. Done.

[egdaniel, 2016-03-07 18:51:26]

So there are still some vulkan issues here. I've tried to explain them in the
comments to clarify what the issue is.

Basically it all comes down to the following form the spec for std140 (uniform
buffers) "If the member is an array of scalars or vectors, the base alignment
and array
stride are set to match the base alignment of a single array element, and
rounded up to the base alignment of a vec4.".

Std340 (push buffers) don't have the extra "rounded up to the base alignment of
a vec4".

Vec3 have a base alignment of a vec4

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
File src/gpu/vk/GrVkUniformHandler.cpp (right):

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:25: 0xF, // kMat22f_GrSLType
so this function technically returns things in std430 layout. Which means for
arrays (which a matrix is) it is the alignment of the column vecs, so the
alignment of a vec2. Thus the alignment for a mat22f would be 8 bytes and thus a
mask of 0x7.

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:55: 8 * sizeof(float),        //
kMat22f_GrSLType
This function may need some reworking since we have decide what we mean by size.
The size of a mat2x2 in a uniform buffer, std140 layout, will be 2*
sizeof(vec4). In a push constant buffer, std430 layout, it will be 2 *
sizeof(vec2). This is one of the reasons I was saying adding a 2x2 matrix makes
things more complicated here. For a Mat3x3 (and thus a column of vec3), always
acts like a vec4.

So either we need to make 2 different size helper functions, or we need a way to
pass in some variable to this function so it knows what the size of a matrix
column should be.

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:94: if (arrayCount) {
we could make this if (arrayCount || Mat22 == type). Not the cleanest but would
give use the correct alignments.

[Chris Dalton, 2016-03-07 18:59:43]

On 2016/03/07 18:51:26, egdaniel wrote:
> So there are still some vulkan issues here. I've tried to explain them in the
> comments to clarify what the issue is.
> 
> Basically it all comes down to the following form the spec for std140 (uniform
> buffers) "If the member is an array of scalars or vectors, the base alignment
> and array
> stride are set to match the base alignment of a single array element, and
> rounded up to the base alignment of a vec4.".
> 
> Std340 (push buffers) don't have the extra "rounded up to the base alignment
of
> a vec4".
> 
> Vec3 have a base alignment of a vec4
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> File src/gpu/vk/GrVkUniformHandler.cpp (right):
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:25: 0xF, // kMat22f_GrSLType
> so this function technically returns things in std430 layout. Which means for
> arrays (which a matrix is) it is the alignment of the column vecs, so the
> alignment of a vec2. Thus the alignment for a mat22f would be 8 bytes and thus
a
> mask of 0x7.
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:55: 8 * sizeof(float),        //
> kMat22f_GrSLType
> This function may need some reworking since we have decide what we mean by
size.
> The size of a mat2x2 in a uniform buffer, std140 layout, will be 2*
> sizeof(vec4). In a push constant buffer, std430 layout, it will be 2 *
> sizeof(vec2). This is one of the reasons I was saying adding a 2x2 matrix
makes
> things more complicated here. For a Mat3x3 (and thus a column of vec3), always
> acts like a vec4.
> 
> So either we need to make 2 different size helper functions, or we need a way
to
> pass in some variable to this function so it knows what the size of a matrix
> column should be.
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:94: if (arrayCount) {
> we could make this if (arrayCount || Mat22 == type). Not the cleanest but
would
> give use the correct alignments.

This keeps getting more complex. I don't think it makes sense for me to do these
changes without the ability to build or test them.

Can we either have you take over this patch, or else push without vulkan changes
and let you fix it up once it lands?

[egdaniel, 2016-03-07 21:20:03]

If you just fix the couple things I just added then it should be fine to land
(assuming currently vulkan won't be seeing 2x2 matrices). I'll figure out an
actual fix once this lands.

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
File src/gpu/vk/GrVkUniformHandler.cpp (right):

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:25: 0xF, // kMat22f_GrSLType
On 2016/03/07 18:51:26, egdaniel wrote:
> so this function technically returns things in std430 layout. Which means for
> arrays (which a matrix is) it is the alignment of the column vecs, so the
> alignment of a vec2. Thus the alignment for a mat22f would be 8 bytes and thus
a
> mask of 0x7.

Can you just make this change 0x7

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:55: 8 * sizeof(float),        //
kMat22f_GrSLType
On 2016/03/07 18:51:26, egdaniel wrote:
> This function may need some reworking since we have decide what we mean by
size.
> The size of a mat2x2 in a uniform buffer, std140 layout, will be 2*
> sizeof(vec4). In a push constant buffer, std430 layout, it will be 2 *
> sizeof(vec2). This is one of the reasons I was saying adding a 2x2 matrix
makes
> things more complicated here. For a Mat3x3 (and thus a column of vec3), always
> acts like a vec4.
> 
> So either we need to make 2 different size helper functions, or we need a way
to
> pass in some variable to this function so it knows what the size of a matrix
> column should be.

Just add a comment after Mat22f_GrSLType saying something along the lines that
this needs to be fixed

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:94: if (arrayCount) {
On 2016/03/07 18:51:26, egdaniel wrote:
> we could make this if (arrayCount || Mat22 == type). Not the cleanest but
would
> give use the correct alignments.

Add assert in this function the type != Mat22

[Chris Dalton, 2016-03-07 22:17:12]

Thanks for the suggestions Greg. Does this look ok now?

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
File src/gpu/vk/GrVkUniformHandler.cpp (right):

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:25: 0xF, // kMat22f_GrSLType
On 2016/03/07 21:20:03, egdaniel wrote:
> On 2016/03/07 18:51:26, egdaniel wrote:
> > so this function technically returns things in std430 layout. Which means
for
> > arrays (which a matrix is) it is the alignment of the column vecs, so the
> > alignment of a vec2. Thus the alignment for a mat22f would be 8 bytes and
thus
> a
> > mask of 0x7.
> 
> Can you just make this change 0x7

Done.

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:55: 8 * sizeof(float),        //
kMat22f_GrSLType
On 2016/03/07 21:20:03, egdaniel wrote:
> On 2016/03/07 18:51:26, egdaniel wrote:
> > This function may need some reworking since we have decide what we mean by
> size.
> > The size of a mat2x2 in a uniform buffer, std140 layout, will be 2*
> > sizeof(vec4). In a push constant buffer, std430 layout, it will be 2 *
> > sizeof(vec2). This is one of the reasons I was saying adding a 2x2 matrix
> makes
> > things more complicated here. For a Mat3x3 (and thus a column of vec3),
always
> > acts like a vec4.
> > 
> > So either we need to make 2 different size helper functions, or we need a
way
> to
> > pass in some variable to this function so it knows what the size of a matrix
> > column should be.
> 
> Just add a comment after Mat22f_GrSLType saying something along the lines that
> this needs to be fixed

Done.

https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
src/gpu/vk/GrVkUniformHandler.cpp:94: if (arrayCount) {
On 2016/03/07 21:20:03, egdaniel wrote:
> On 2016/03/07 18:51:26, egdaniel wrote:
> > we could make this if (arrayCount || Mat22 == type). Not the cleanest but
> would
> > give use the correct alignments.
> 
> Add assert in this function the type != Mat22

Done.

[egdaniel, 2016-03-07 23:08:14]

On 2016/03/07 22:17:12, Chris Dalton wrote:
> Thanks for the suggestions Greg. Does this look ok now?
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> File src/gpu/vk/GrVkUniformHandler.cpp (right):
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:25: 0xF, // kMat22f_GrSLType
> On 2016/03/07 21:20:03, egdaniel wrote:
> > On 2016/03/07 18:51:26, egdaniel wrote:
> > > so this function technically returns things in std430 layout. Which means
> for
> > > arrays (which a matrix is) it is the alignment of the column vecs, so the
> > > alignment of a vec2. Thus the alignment for a mat22f would be 8 bytes and
> thus
> > a
> > > mask of 0x7.
> > 
> > Can you just make this change 0x7
> 
> Done.
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:55: 8 * sizeof(float),        //
> kMat22f_GrSLType
> On 2016/03/07 21:20:03, egdaniel wrote:
> > On 2016/03/07 18:51:26, egdaniel wrote:
> > > This function may need some reworking since we have decide what we mean by
> > size.
> > > The size of a mat2x2 in a uniform buffer, std140 layout, will be 2*
> > > sizeof(vec4). In a push constant buffer, std430 layout, it will be 2 *
> > > sizeof(vec2). This is one of the reasons I was saying adding a 2x2 matrix
> > makes
> > > things more complicated here. For a Mat3x3 (and thus a column of vec3),
> always
> > > acts like a vec4.
> > > 
> > > So either we need to make 2 different size helper functions, or we need a
> way
> > to
> > > pass in some variable to this function so it knows what the size of a
matrix
> > > column should be.
> > 
> > Just add a comment after Mat22f_GrSLType saying something along the lines
that
> > this needs to be fixed
> 
> Done.
> 
>
https://codereview.chromium.org/1755483002/diff/40001/src/gpu/vk/GrVkUniformH...
> src/gpu/vk/GrVkUniformHandler.cpp:94: if (arrayCount) {
> On 2016/03/07 21:20:03, egdaniel wrote:
> > On 2016/03/07 18:51:26, egdaniel wrote:
> > > we could make this if (arrayCount || Mat22 == type). Not the cleanest but
> > would
> > > give use the correct alignments.
> > 
> > Add assert in this function the type != Mat22
> 
> Done.

lgtm

[Chris Dalton, 2016-03-07 23:29:19]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-03-07 23:29:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1755483002/#ps60001
(title: "vk updates")

[commit-bot: I haz the power, 2016-03-07 23:29:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755483002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755483002/60001

[commit-bot: I haz the power, 2016-03-07 23:39:12]

Description was changed from

==========
Add support for 2x2 matrices

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add support for 2x2 matrices

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8d988b3a73a75e062bc139f19cc6966888c722e2
==========

[commit-bot: I haz the power, 2016-03-07 23:39:13]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/8d988b3a73a75e062bc139f19cc6966888c722e2