Added initial Vulkan build files as well as a basic implementation file.

Added initial Vulkan build files as well as a basic implementation file.

As a first step in experimenting with Vulkan in Chromium, this CL adds
GN rules to build and link with Vulkan under the GN variable
"enable_vulkan".

Currently we require users to have the Vulkan SDK installed, so the
changes will not link unless a user has properly installed the SDK.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/2ce3e05ca555d4eb8f6efc5231a2ac111e91f666
Cr-Commit-Position: refs/heads/master@{#380210}

[David Yen, 2016-02-24 01:21:01]

dyen@chromium.org changed reviewers:
+ thakis@chromium.org

[David Yen, 2016-02-24 01:21:01]

+thakis@

I thought about uploading a CL after we have an actual window drawing, but I
think it would be easier to review in smaller pieces.

The next step would be to implement a vulkan surface base class as well as
vulkan_surface_x11.

[Nico, 2016-02-24 01:28:57]

Can you also add a ui/vulkan/OWNERS? What's the plan for tests? I guess since no
bot has the sdk, bots won't be able to run tests, but it's probably a good idea
to write tests as you go and to run them locally?

Overall this looks fine.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:62: queue_index = static_cast<int>(n);
(nit: personally, i think this function could become more readable by splitting
it into several functions. guidelines i sometimes use:

1. When I do

   // Do foo.
  a();
  b();
  c();

then I sometimes extract that into

  doFoo();

2. Try to keep all things in one function at a similar level of abstraction. if
a function does "init a, init b, deeply nested loops" i try to extract the last
bit into its own function so that it's "init a, init b, do c" instead)

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h
File ui/vulkan/vulkan_surface.h (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h#...
ui/vulkan/vulkan_surface.h:13: class VULKAN_EXPORT VulkanSurface : public
base::RefCounted<VulkanSurface> {
Does this have to be refcounted? Since this is all new code, maybe you can get
away with stricter ownership semantics?

[piman, 2016-02-24 01:52:13]

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:61: if (queue_properties[n].queueFlags &
VK_QUEUE_GRAPHICS_BIT) {
We'll probably also want to pick a device that can present - maybe at first, on
the same queue, though we could extend support to other queues.

The best way to do this here is probably by using
vkGetPhysicalDeviceXlibPresentationSupportKHR (on X11) and others.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h
File ui/vulkan/vulkan_surface.h (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h#...
ui/vulkan/vulkan_surface.h:13: class VULKAN_EXPORT VulkanSurface : public
base::RefCounted<VulkanSurface> {
On 2016/02/24 01:28:57, Nico wrote:
> Does this have to be refcounted? Since this is all new code, maybe you can get
> away with stricter ownership semantics?

Agreed, let's do without at least at first, we don't have to follow the GL
semantics. In particular I suspect we'll want to bake in the synchronization /
ownership semantics which will probably look different than a simple refcount -
assuming this is intended to wrap the VkSurfaceKHR and/or the VkSwapchainKHR.

[sohanjg, 2016-02-24 07:24:08]

sohan.jyoti@samsung.com changed reviewers:
+ sohan.jyoti@samsung.com

[sohanjg, 2016-02-24 07:24:08]

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:49: 
sorry for the intrusion, but do we not want any layer or extensions here?

[David Yen, 2016-02-25 00:27:37]

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:49: 
On 2016/02/24 07:24:08, sohanjg wrote:
> sorry for the intrusion, but do we not want any layer or extensions here?

The intent of this first CL is to just get linking working. I tried to write as
little code as possible to verify that the API calls actually call into Vulkan.
There definitely will be layers enabled here, although I haven't thought through
the API for that yet.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:61: if (queue_properties[n].queueFlags &
VK_QUEUE_GRAPHICS_BIT) {
On 2016/02/24 01:52:13, piman wrote:
> We'll probably also want to pick a device that can present - maybe at first,
on
> the same queue, though we could extend support to other queues.
> 
> The best way to do this here is probably by using
> vkGetPhysicalDeviceXlibPresentationSupportKHR (on X11) and others.

That would make it seem like the output surface would have to own the device
since it's display/window specific. I can move this code over to
VulkanSurfaceX11 later and add the vkGetPhysicalDeviceXlibPresentationSupportKHR
check there.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:62: queue_index = static_cast<int>(n);
On 2016/02/24 01:28:57, Nico wrote:
> (nit: personally, i think this function could become more readable by
splitting
> it into several functions. guidelines i sometimes use:
> 
> 1. When I do
> 
>    // Do foo.
>   a();
>   b();
>   c();
> 
> then I sometimes extract that into
> 
>   doFoo();
> 
> 2. Try to keep all things in one function at a similar level of abstraction.
if
> a function does "init a, init b, deeply nested loops" i try to extract the
last
> bit into its own function so that it's "init a, init b, do c" instead)

Done.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h
File ui/vulkan/vulkan_surface.h (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_surface.h#...
ui/vulkan/vulkan_surface.h:13: class VULKAN_EXPORT VulkanSurface : public
base::RefCounted<VulkanSurface> {
On 2016/02/24 01:52:13, piman wrote:
> On 2016/02/24 01:28:57, Nico wrote:
> > Does this have to be refcounted? Since this is all new code, maybe you can
get
> > away with stricter ownership semantics?
> 
> Agreed, let's do without at least at first, we don't have to follow the GL
> semantics. In particular I suspect we'll want to bake in the synchronization /
> ownership semantics which will probably look different than a simple refcount
-
> assuming this is intended to wrap the VkSurfaceKHR and/or the VkSwapchainKHR.

Done.

[piman, 2016-02-25 06:49:44]

Oh, also, maybe let's put vulkan into gpu/, it's not really directly related to
ui. I was thinking of moving ui/gl into gpu as well (for the same reason).

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:16: static VkDevice vk_device;
It would be nice to have these wrapped into an object, that would be injected
into the OutputSurface and so on. Ideally it wouldn't be a global, though maybe
ok as a first step.

We'll need more things to be associated with the device, among others keeping
the VkPhysicalDisplay that we chose (because we need it to query things such as
the memory types/heaps, the supported features/limits, the supported formats
etc.). We'll probably want a global memory allocator associated with it, because
we'll need to be able to track our usage globally.
Anyway, a bunch of things will be "global", so it would make sense to have them
gathered into a single class.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:61: if (queue_properties[n].queueFlags &
VK_QUEUE_GRAPHICS_BIT) {
On 2016/02/25 00:27:37, David Yen wrote:
> On 2016/02/24 01:52:13, piman wrote:
> > We'll probably also want to pick a device that can present - maybe at first,
> on
> > the same queue, though we could extend support to other queues.
> > 
> > The best way to do this here is probably by using
> > vkGetPhysicalDeviceXlibPresentationSupportKHR (on X11) and others.
> 
> That would make it seem like the output surface would have to own the device
> since it's display/window specific. I can move this code over to
> VulkanSurfaceX11 later and add the
vkGetPhysicalDeviceXlibPresentationSupportKHR
> check there.

We wouldn't want a separate device per output surface, that would prevent
sharing resources.

vkGetPhysicalDeviceXlibPresentationSupportKHR only needs the Display (the X
connection) and the Visual ID (we can take the one used by the root window,
since we create child windows with CopyFromParent for the visual).

[David Yen, 2016-03-01 19:51:58]

Description was changed from

==========
Added initial Vulkan build files as well as a basic implementation file.

As a first step in experimenting with Vulkan in Chromium, this CL adds
GN rules to build and link with Vulkan under the GN variable
"enable_vulkan".

Currently we require users to have the Vulkan SDK installed, so the
changes will not link unless a user has properly installed the SDK.

R=piman@chromium.org
BUG=582558
==========

to

==========
Added initial Vulkan build files as well as a basic implementation file.

As a first step in experimenting with Vulkan in Chromium, this CL adds
GN rules to build and link with Vulkan under the GN variable
"enable_vulkan".

Currently we require users to have the Vulkan SDK installed, so the
changes will not link unless a user has properly installed the SDK.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[David Yen, 2016-03-01 19:54:10]

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
File ui/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:16: static VkDevice vk_device;
On 2016/02/25 06:49:44, piman - OOO back 2016-03-04 wrote:
> It would be nice to have these wrapped into an object, that would be injected
> into the OutputSurface and so on. Ideally it wouldn't be a global, though
maybe
> ok as a first step.
> 
> We'll need more things to be associated with the device, among others keeping
> the VkPhysicalDisplay that we chose (because we need it to query things such
as
> the memory types/heaps, the supported features/limits, the supported formats
> etc.). We'll probably want a global memory allocator associated with it,
because
> we'll need to be able to track our usage globally.
> Anyway, a bunch of things will be "global", so it would make sense to have
them
> gathered into a single class.

Done.

https://codereview.chromium.org/1726303002/diff/1/ui/vulkan/vulkan_implementa...
ui/vulkan/vulkan_implementation.cc:61: if (queue_properties[n].queueFlags &
VK_QUEUE_GRAPHICS_BIT) {
On 2016/02/25 06:49:44, piman - OOO back 2016-03-04 wrote:
> On 2016/02/25 00:27:37, David Yen wrote:
> > On 2016/02/24 01:52:13, piman wrote:
> > > We'll probably also want to pick a device that can present - maybe at
first,
> > on
> > > the same queue, though we could extend support to other queues.
> > > 
> > > The best way to do this here is probably by using
> > > vkGetPhysicalDeviceXlibPresentationSupportKHR (on X11) and others.
> > 
> > That would make it seem like the output surface would have to own the device
> > since it's display/window specific. I can move this code over to
> > VulkanSurfaceX11 later and add the
> vkGetPhysicalDeviceXlibPresentationSupportKHR
> > check there.
> 
> We wouldn't want a separate device per output surface, that would prevent
> sharing resources.
> 
> vkGetPhysicalDeviceXlibPresentationSupportKHR only needs the Display (the X
> connection) and the Visual ID (we can take the one used by the root window,
> since we create child windows with CopyFromParent for the visual).

Done (although this currently crashes on my driver...)

[piman, 2016-03-04 21:07:09]

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/OWNERS
File gpu/vulkan/OWNERS (left):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/OWNERS#oldc...
gpu/vulkan/OWNERS:1: kbr@chromium.org
Please add yourself here :)

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:39: static VulkanProcs vulkan_procs;
non-POD globals are not allowed (causes global initializers).
You can use LazyInstance, for example, or make this a pointer that would be set
up by InitializeVulkan or something.

Or, since the functions are instance-specific, they could be part of
VulkanInstance.

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:198: static VulkanInstance vulkan_instance;
Same here wrt non-POD globals.

LazyInstance seems appropriate.

[David Yen, 2016-03-04 22:12:28]

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/OWNERS
File gpu/vulkan/OWNERS (left):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/OWNERS#oldc...
gpu/vulkan/OWNERS:1: kbr@chromium.org
On 2016/03/04 21:07:09, piman - OOO back 2016-03-04 wrote:
> Please add yourself here :)

Done.

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:39: static VulkanProcs vulkan_procs;
On 2016/03/04 21:07:09, piman - OOO back 2016-03-04 wrote:
> non-POD globals are not allowed (causes global initializers).
> You can use LazyInstance, for example, or make this a pointer that would be
set
> up by InitializeVulkan or something.
> 
> Or, since the functions are instance-specific, they could be part of
> VulkanInstance.

I thought I would be able to get around the loader by using
vkGetInstanceProcAddr() thinking that these functions would be part of the
driver, but it sounds these functions are suppose to be implemented by the
loader itself? Does that mean this section is pointless?

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:198: static VulkanInstance vulkan_instance;
On 2016/03/04 21:07:09, piman - OOO back 2016-03-04 wrote:
> Same here wrt non-POD globals.
> 
> LazyInstance seems appropriate.

Done.

[piman, 2016-03-05 00:11:26]

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:39: static VulkanProcs vulkan_procs;
On 2016/03/04 22:12:28, David Yen wrote:
> On 2016/03/04 21:07:09, piman - OOO back 2016-03-04 wrote:
> > non-POD globals are not allowed (causes global initializers).
> > You can use LazyInstance, for example, or make this a pointer that would be
> set
> > up by InitializeVulkan or something.
> > 
> > Or, since the functions are instance-specific, they could be part of
> > VulkanInstance.
> 
> I thought I would be able to get around the loader by using
> vkGetInstanceProcAddr() thinking that these functions would be part of the
> driver, but it sounds these functions are suppose to be implemented by the
> loader itself? Does that mean this section is pointless?

vkGetInstanceProcAddr /could/ give you a more specific function (e.g. skips
some dispatch code wrt layers), but I think that's right, this function is
implemented by the loader anyway.

[David Yen, 2016-03-07 23:33:32]

I also added the creation of the queue just to make merging with my dependent
branch a little easier. PTAL.

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.cc (right):

https://codereview.chromium.org/1726303002/diff/100001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.cc:39: static VulkanProcs vulkan_procs;
On 2016/03/05 00:11:25, piman wrote:
> On 2016/03/04 22:12:28, David Yen wrote:
> > On 2016/03/04 21:07:09, piman - OOO back 2016-03-04 wrote:
> > > non-POD globals are not allowed (causes global initializers).
> > > You can use LazyInstance, for example, or make this a pointer that would
be
> > set
> > > up by InitializeVulkan or something.
> > > 
> > > Or, since the functions are instance-specific, they could be part of
> > > VulkanInstance.
> > 
> > I thought I would be able to get around the loader by using
> > vkGetInstanceProcAddr() thinking that these functions would be part of the
> > driver, but it sounds these functions are suppose to be implemented by the
> > loader itself? Does that mean this section is pointless?
> 
> vkGetInstanceProcAddr /could/ give you a more specific function (e.g. skips
> some dispatch code wrt layers), but I think that's right, this function is
> implemented by the loader anyway.

So after building the loader locally, there are 2 functions associated with the
xlib extension. The other function (surface creation) is implemented in the
loader but this one is implemented in the driver which is still broken as of
355.00.29. I have just disabled the function call and put it behind an ifdef for
now.

[piman, 2016-03-08 00:14:20]

lgtm

[David Yen, 2016-03-08 00:19:55]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-03-08 00:19:56]

The CQ bit was unchecked by dyen@chromium.org

[David Yen, 2016-03-08 00:20:59]

On 2016/03/08 00:14:20, piman wrote:
> lgtm

Thanks!

thakis@, do you have any more comments?

[David Yen, 2016-03-09 01:24:24]

On 2016/03/08 00:20:59, David Yen wrote:
> On 2016/03/08 00:14:20, piman wrote:
> > lgtm
> 
> Thanks!
> 
> thakis@, do you have any more comments?

ping

[Nico, 2016-03-09 02:43:33]

Sorry, I stopped paying attention when this moved out of ui. Now that I look
again: The namespace should now probably no longer be gfx::. Other than that,
lgtm (and if there's precendent for gfx:: in gpu/ then lgtm as-is but that would
be weird)

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:209: gfx::VulkanSurface::InitializeOneOff();
shouldn't this be in the gpu namespace now?

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:212: if (!gfx::GLSurface::InitializeOneOff())
do you still need this in vulkan builds? i'd have expected this to go into an
#else.

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:297: gfx::VulkanSurface::InitializeOneOff();
do you need to call this both here and in the other file?

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/BUILD.gn
File gpu/vulkan/BUILD.gn (right):

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/BUILD.gn#ne...
gpu/vulkan/BUILD.gn:27: configs += [ "//build/config:precompiled_headers" ]
(nit: atm this target is so small that using this is discouraged, as building
the pch is likely slower than building all the cc files)

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.h (right):

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.h:10: namespace gfx {
this should be a different namespace, gfx is in ui/gfx

[piman, 2016-03-09 03:08:20]

On Tue, Mar 8, 2016 at 6:43 PM, <thakis@chromium.org> wrote:

> Sorry, I stopped paying attention when this moved out of ui. Now that I
> look
> again: The namespace should now probably no longer be gfx::. Other than
> that,
> lgtm (and if there's precendent for gfx:: in gpu/ then lgtm as-is but that
> would
> be weird)
>

Right, this should be gpu, thanks!


>
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
> File content/gpu/gpu_child_thread.cc (right):
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
> content/gpu/gpu_child_thread.cc:209:
> gfx::VulkanSurface::InitializeOneOff();
> shouldn't this be in the gpu namespace now?
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
> content/gpu/gpu_child_thread.cc:212: if
> (!gfx::GLSurface::InitializeOneOff())
> do you still need this in vulkan builds? i'd have expected this to go
> into an #else.
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.cc
> File content/gpu/gpu_main.cc (right):
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.c...
> content/gpu/gpu_main.cc:297: gfx::VulkanSurface::InitializeOneOff();
> do you need to call this both here and in the other file?
>
> https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/BUILD.gn
> File gpu/vulkan/BUILD.gn (right):
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/BUILD.gn#ne...
> gpu/vulkan/BUILD.gn:27: configs += [
> "//build/config:precompiled_headers" ]
> (nit: atm this target is so small that using this is discouraged, as
> building the pch is likely slower than building all the cc files)
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
> File gpu/vulkan/vulkan_implementation.h (right):
>
>
>
https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
> gpu/vulkan/vulkan_implementation.h:10: namespace gfx {
> this should be a different namespace, gfx is in ui/gfx
>
> https://codereview.chromium.org/1726303002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Yen, 2016-03-09 18:44:49]

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:209: gfx::VulkanSurface::InitializeOneOff();
On 2016/03/09 02:43:33, Nico wrote:
> shouldn't this be in the gpu namespace now?

Done.

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_child_...
content/gpu/gpu_child_thread.cc:212: if (!gfx::GLSurface::InitializeOneOff())
On 2016/03/09 02:43:33, Nico wrote:
> do you still need this in vulkan builds? i'd have expected this to go into an
> #else.

Eventually, right now I'm just injecting the initialization to be sure it works.

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/1726303002/diff/140001/content/gpu/gpu_main.c...
content/gpu/gpu_main.cc:297: gfx::VulkanSurface::InitializeOneOff();
On 2016/03/09 02:43:33, Nico wrote:
> do you need to call this both here and in the other file?

I'm still figuring out the initialization path for these, should become more
clear as the actual surface is implemented. Right now I'm just injecting it here
to be sure Vulkan actually links/initializes properly.

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
File gpu/vulkan/vulkan_implementation.h (right):

https://codereview.chromium.org/1726303002/diff/140001/gpu/vulkan/vulkan_impl...
gpu/vulkan/vulkan_implementation.h:10: namespace gfx {
On 2016/03/09 02:43:33, Nico wrote:
> this should be a different namespace, gfx is in ui/gfx

Done.

[Nico, 2016-03-09 19:13:07]

lgtm

[David Yen, 2016-03-09 20:00:57]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-03-09 20:00:57]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1726303002/#ps160001
(title: "changed namespace to gpu::")

[commit-bot: I haz the power, 2016-03-09 20:01:41]

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

[commit-bot: I haz the power, 2016-03-09 21:04:01]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-09 21:05:22]

Description was changed from

==========
Added initial Vulkan build files as well as a basic implementation file.

As a first step in experimenting with Vulkan in Chromium, this CL adds
GN rules to build and link with Vulkan under the GN variable
"enable_vulkan".

Currently we require users to have the Vulkan SDK installed, so the
changes will not link unless a user has properly installed the SDK.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Added initial Vulkan build files as well as a basic implementation file.

As a first step in experimenting with Vulkan in Chromium, this CL adds
GN rules to build and link with Vulkan under the GN variable
"enable_vulkan".

Currently we require users to have the Vulkan SDK installed, so the
changes will not link unless a user has properly installed the SDK.

R=piman@chromium.org
BUG=582558
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/2ce3e05ca555d4eb8f6efc5231a2ac111e91f666
Cr-Commit-Position: refs/heads/master@{#380210}
==========

[commit-bot: I haz the power, 2016-03-09 21:05:23]

Patchset 9 (id:??) landed as
https://crrev.com/2ce3e05ca555d4eb8f6efc5231a2ac111e91f666
Cr-Commit-Position: refs/heads/master@{#380210}