GrVkGpu initialization cleanup.

GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

Committed: https://skia.googlesource.com/skia/+/633b35657c964c32e7010b14bb2d396b4a764c52

[jvanverth1, 2016-03-22 19:32:22]

Description was changed from

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

to

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

[jvanverth1, 2016-03-22 20:08:01]

jvanverth@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[jvanverth1, 2016-03-22 20:08:33]

Description was changed from

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

to

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

[jvanverth1, 2016-03-23 13:54:32]

In looking at this further, I realized that the extension setup is not quite
correct: after the instance and device are created, we have no way to query
which extensions are enabled. The client will have to pass us this information.
However, that's going to be a significant change, so I'd rather implement that
as a separate patch unless there are other major issues with this one.

Similarly, the client will need to pass us the version information. I can
probably incorporate that into this one, if that's desired.

[egdaniel, 2016-03-23 14:51:57]

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
File include/gpu/vk/GrVkBackendContext.h (right):

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
include/gpu/vk/GrVkBackendContext.h:25: struct GrVkBackendContext : public
SkRefCnt {
Does the comment about more than one queue add anything here besides confusion?
We will never accept more than one queue, and seems like that is purely a client
specific detail

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
include/gpu/vk/GrVkBackendContext.h:30: uint32_t                         
fQueueFamilyIndex;
The fact that we pass in this queue family index has made me think of a slight
issue. When a client passes us in a wrapped texture/render target, we either
need to require they've put a barrier to switch it to the queue they are giving
us here (that's if they created it with sharing mode flag and happen to use a
different queue family when creating the object). Another option would be that
they we add to the GrVkBackendObject a flag for sharing mode and current queue.
Then we would be responsible for switching to our queue family (if different)
and making sure it goes back to their queue family when we are done. I do not
like this second approach.

Just to be clear different queues does not mean they are are in different
families. The typical use case will be that all graphics queues are in the same
family but this is driver implementation dependent.

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkExtensions.cpp
File src/gpu/vk/GrVkExtensions.cpp (right):

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkExtensions.c...
src/gpu/vk/GrVkExtensions.cpp:69: // instance layers
So most of all this work here will actually be deleted and we will simply be
setting the strings from passed in values right?

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp#newc...
src/gpu/vk/GrVkGpu.cpp:76: if (!vkBackendContext) {
any reason not to put this into the above if block where we create our own?

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp#newc...
src/gpu/vk/GrVkGpu.cpp:124: 0,                                          //
CmdPoolCreateFlags
I wonder if we should be using VK_COMMAND_POOL_CREATE_TRANSIENT_BIT here. We are
already using the one time usage flag when beginning a command buffer and delete
after it completes.

[jvanverth1, 2016-03-23 15:44:13]

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
File include/gpu/vk/GrVkBackendContext.h (right):

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
include/gpu/vk/GrVkBackendContext.h:25: struct GrVkBackendContext : public
SkRefCnt {
On 2016/03/23 14:51:57, egdaniel wrote:
> Does the comment about more than one queue add anything here besides
confusion?
> We will never accept more than one queue, and seems like that is purely a
client
> specific detail

Done.

https://codereview.chromium.org/1825593002/diff/1/include/gpu/vk/GrVkBackendC...
include/gpu/vk/GrVkBackendContext.h:30: uint32_t                         
fQueueFamilyIndex;
On 2016/03/23 14:51:57, egdaniel wrote:
> The fact that we pass in this queue family index has made me think of a slight
> issue. When a client passes us in a wrapped texture/render target, we either
> need to require they've put a barrier to switch it to the queue they are
giving
> us here (that's if they created it with sharing mode flag and happen to use a
> different queue family when creating the object). Another option would be that
> they we add to the GrVkBackendObject a flag for sharing mode and current
queue.
> Then we would be responsible for switching to our queue family (if different)
> and making sure it goes back to their queue family when we are done. I do not
> like this second approach.
> 
> Just to be clear different queues does not mean they are are in different
> families. The typical use case will be that all graphics queues are in the
same
> family but this is driver implementation dependent.

As discussed, the contract with the client will be that they will give us
backend objects in the same family as this index, whether they create them that
way or transition them to it. Noted in the comment above.

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkExtensions.cpp
File src/gpu/vk/GrVkExtensions.cpp (right):

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkExtensions.c...
src/gpu/vk/GrVkExtensions.cpp:69: // instance layers
On 2016/03/23 14:51:57, egdaniel wrote:
> So most of all this work here will actually be deleted and we will simply be
> setting the strings from passed in values right? 

Yes, and I'll be using a version of it in the default setup code in
GrVkBackendObject.

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp#newc...
src/gpu/vk/GrVkGpu.cpp:76: if (!vkBackendContext) {
On 2016/03/23 14:51:57, egdaniel wrote:
> any reason not to put this into the above if block where we create our own?

Done.

https://codereview.chromium.org/1825593002/diff/1/src/gpu/vk/GrVkGpu.cpp#newc...
src/gpu/vk/GrVkGpu.cpp:124: 0,                                          //
CmdPoolCreateFlags
On 2016/03/23 14:51:57, egdaniel wrote:
> I wonder if we should be using VK_COMMAND_POOL_CREATE_TRANSIENT_BIT here. We
are
> already using the one time usage flag when beginning a command buffer and
delete
> after it completes.

Done.

[egdaniel, 2016-03-23 17:04:29]

lgtm. not sure if Brian has any comments based on the work he is doing

[bsalomon, 2016-03-23 17:22:45]

On 2016/03/23 17:04:29, egdaniel wrote:
> lgtm. not sure if Brian has any comments based on the work he is doing

My plan is to move all the SkGLContext and GrContextFactory stuff into our tools
and not expose it or build it in the Skia lib. It seems like we'll have a
different model for GrVkBackendContext. It will be used both to wrap a client's
provided info as well as cons up a context for testing, right?

I'm wondering whether this and GrVkInteface should be integrated, or if the
small amount of non-function stuff in GrVkInterface should be pulled into this
new object.

[jvanverth1, 2016-03-23 17:38:39]

On 2016/03/23 17:22:45, bsalomon wrote:
> On 2016/03/23 17:04:29, egdaniel wrote:
> > lgtm. not sure if Brian has any comments based on the work he is doing
> 
> My plan is to move all the SkGLContext and GrContextFactory stuff into our
tools
> and not expose it or build it in the Skia lib. It seems like we'll have a
> different model for GrVkBackendContext. It will be used both to wrap a
client's
> provided info as well as cons up a context for testing, right?

That's correct. The assumption is that Chrome will want to initialize Vulkan,
and then pass this to us so we can run with their setup. Our internal code can
do the same thing (or just pass NULL to use the default setup).

> I'm wondering whether this and GrVkInteface should be integrated, or if the
> small amount of non-function stuff in GrVkInterface should be pulled into this
> new object.

The only other part I see are the layers/extensions checks. My current idea is
that client would specify the enabled extensions as a set of flags. Then to
check for an extension you 'AND' that field with the specific flag. If I did
that then GrVkExtensions would go away.

[bsalomon, 2016-03-23 17:39:48]

On 2016/03/23 17:38:39, jvanverth1 wrote:
> On 2016/03/23 17:22:45, bsalomon wrote:
> > On 2016/03/23 17:04:29, egdaniel wrote:
> > > lgtm. not sure if Brian has any comments based on the work he is doing
> > 
> > My plan is to move all the SkGLContext and GrContextFactory stuff into our
> tools
> > and not expose it or build it in the Skia lib. It seems like we'll have a
> > different model for GrVkBackendContext. It will be used both to wrap a
> client's
> > provided info as well as cons up a context for testing, right?
> 
> That's correct. The assumption is that Chrome will want to initialize Vulkan,
> and then pass this to us so we can run with their setup. Our internal code can
> do the same thing (or just pass NULL to use the default setup).
> 
> > I'm wondering whether this and GrVkInteface should be integrated, or if the
> > small amount of non-function stuff in GrVkInterface should be pulled into
this
> > new object.
> 
> The only other part I see are the layers/extensions checks. My current idea is
> that client would specify the enabled extensions as a set of flags. Then to
> check for an extension you 'AND' that field with the specific flag. If I did
> that then GrVkExtensions would go away.

sgtm, lgtm

[jvanverth1, 2016-03-23 17:42:54]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2016-03-23 17:42:59]

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

[commit-bot: I haz the power, 2016-03-23 18:01:24]

Description was changed from

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

to

==========
GrVkGpu initialization cleanup.

Add GrVkBackendContext for GrVkGpu initialization
Add missing extension initialization
Use device function pointers rather than instance

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

Committed:
https://skia.googlesource.com/skia/+/633b35657c964c32e7010b14bb2d396b4a764c52
==========

[commit-bot: I haz the power, 2016-03-23 18:01:25]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/633b35657c964c32e7010b14bb2d396b4a764c52