Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318603002:
Update Vulkan docs (Closed)
DescriptionAdd Vulkan docs
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2318603002
NOTRY=true
DOCS_PREVIEW= https://skia.org/?cl=2318603002
Committed: https://skia.googlesource.com/skia/+/54dd2e244c4f49be8815611f4c10f6c7d1b700ab
Patch Set 1 #Patch Set 2 : more #Patch Set 3 : replace #Patch Set 4 : more #Patch Set 5 : more #
Total comments: 22
Patch Set 6 : Address comments #Patch Set 7 : info info #Patch Set 8 : add comment about setting SDK env var #Patch Set 9 : Add sync comment #
Total comments: 1
Patch Set 10 : issues->issuing #Patch Set 11 : regyp #Patch Set 12 : more #Patch Set 13 : fix var name #Messages
Total messages: 26 (11 generated)
Description was changed from ========== Add Vulkan docs BUG=skia: ========== to ========== Add Vulkan docs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2318603002 NOTRY=true DOCS_PREVIEW= https://skia.org/?cl=2318603002 ==========
bsalomon@google.com changed reviewers: + egdaniel@google.com, jvanverth@google.com
You can see what this will look like at: https://skia.org/user/special/vulkan?cl=2318603002
https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); Should/do we require the client to create this GrVkInterface? It seems like everything passed in here is already on the GrVkBackendContext and we could just as easily call GrVkCreateInterface.
https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:573: * Vulkan: GrVkImage* GrVkImageInfo https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:608: * Vulkan: GrVkImage* ImageInfo https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:13: The Vulkan SDK must be installed and the VULKAN_SDK environment variable must point to the installation location. Not sure if its worth saying, but windows will set VULKAN_SDK (and VK_SDK_PATH, which was the old variable used for windows and what we check for) automatically when you install the sdk. For linux you must manually set VULKAN_SDK https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); On 2016/09/06 13:52:42, bsalomon wrote: > Should/do we require the client to create this GrVkInterface? It seems like > everything passed in here is already on the GrVkBackendContext and we could just > as easily call GrVkCreateInterface. I guess it is left like this so that it could be used in a command buffer type thing where they want to have their own function pointers for the interface. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:38: When using the Vulkan backend the GrBackendObject field in GrBackendRenderTargetDesc and GrBackendTextureDesc is interpeted as a pointer to a GrVkImageInfo object. GrVkImage specifies a VkImage and associated state (tiling, layout, format, etc). This allows the client to import externally created Vulkan images as destinations for Skia rendering via SkSurface factory functions or for to composite Skia rendered content using SkImage::getTextureHandle(). GrVkImageInfo https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:39: Should we add the following to the doc or just leave it in the comments for the code: "When getting a GrVkImageInfo* via getTextureHandle() or getRenderTargetHandle(), the client should check the fImageLayout field to know what layout Skia left the VkImage in before using the VkImage. If the client changes the layout of the VkImage, they should call GrVkImageInfo::updateImageLayout(VkImageLayout layout) before issuing any future commands to Skia with that VkImage."
https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:11: export GYP_DEFINES="$GYP_DEFINES skia_vulkan=1" Don't we need skia_arch_width=64 skia_arch_type=x86_64 (on Windows and Linux)? https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:26: To create a GrContext that is backend by Vulkan the client creates a Vulkan device and queue, initializes a GrVkBackendContext to describe the context, and then calls GrContext::Create: backend -> backed https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); fInterface.reset(). And I'm not sure on the need for creation, I'll check. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:38: When using the Vulkan backend the GrBackendObject field in GrBackendRenderTargetDesc and GrBackendTextureDesc is interpeted as a pointer to a GrVkImageInfo object. GrVkImage specifies a VkImage and associated state (tiling, layout, format, etc). This allows the client to import externally created Vulkan images as destinations for Skia rendering via SkSurface factory functions or for to composite Skia rendered content using SkImage::getTextureHandle(). GrVkImageInfo specifies a VkImage [...]
https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:39: On 2016/09/06 14:07:39, egdaniel wrote: > Should we add the following to the doc or just leave it in the comments for the > code: > > "When getting a GrVkImageInfo* via getTextureHandle() or > getRenderTargetHandle(), the client should check the fImageLayout field to know > what layout Skia left the VkImage in before using the VkImage. If the client > changes the layout of the VkImage, they should call > GrVkImageInfo::updateImageLayout(VkImageLayout layout) before issuing any future > commands to Skia with that VkImage." Also should we add something along the lines of, "The client is responsible for any synchronization or barriers needed before handing Skia an GrVkImageInfo. Skia will assume it can start issues commands with the corresponding VkImage without the need for adding additional synchronization at the start."
https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h File include/gpu/GrTypes.h (right): https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:573: * Vulkan: GrVkImage* On 2016/09/06 14:07:38, egdaniel wrote: > GrVkImageInfo Done. https://codereview.chromium.org/2318603002/diff/80001/include/gpu/GrTypes.h#n... include/gpu/GrTypes.h:608: * Vulkan: GrVkImage* On 2016/09/06 14:07:38, egdaniel wrote: > ImageInfo Done. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:11: export GYP_DEFINES="$GYP_DEFINES skia_vulkan=1" On 2016/09/06 14:09:00, jvanverth1 wrote: > Don't we need skia_arch_width=64 skia_arch_type=x86_64 (on Windows and Linux)? Do we only support vulkan for 64bit builds? https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:13: The Vulkan SDK must be installed and the VULKAN_SDK environment variable must point to the installation location. On 2016/09/06 14:07:38, egdaniel wrote: > Not sure if its worth saying, but windows will set VULKAN_SDK (and VK_SDK_PATH, > which was the old variable used for windows and what we check for) automatically > when you install the sdk. For linux you must manually set VULKAN_SDK I'll add something about windows setting it up automatically and doing it manually on linux. I'll put up a CL to update the windows build to use the new variable before landing this. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:26: To create a GrContext that is backend by Vulkan the client creates a Vulkan device and queue, initializes a GrVkBackendContext to describe the context, and then calls GrContext::Create: On 2016/09/06 14:09:00, jvanverth1 wrote: > backend -> backed Done. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); On 2016/09/06 14:09:00, jvanverth1 wrote: > fInterface.reset(). And I'm not sure on the need for creation, I'll check. Fixed the spelling. I see Greg's point about custom bindings. But perhaps we could say if this this null we'll call GrVkCreateInterface. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:38: When using the Vulkan backend the GrBackendObject field in GrBackendRenderTargetDesc and GrBackendTextureDesc is interpeted as a pointer to a GrVkImageInfo object. GrVkImage specifies a VkImage and associated state (tiling, layout, format, etc). This allows the client to import externally created Vulkan images as destinations for Skia rendering via SkSurface factory functions or for to composite Skia rendered content using SkImage::getTextureHandle(). On 2016/09/06 14:09:00, jvanverth1 wrote: > GrVkImageInfo specifies a VkImage [...] Done. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:39: On 2016/09/06 14:07:39, egdaniel wrote: > Should we add the following to the doc or just leave it in the comments for the > code: > > "When getting a GrVkImageInfo* via getTextureHandle() or > getRenderTargetHandle(), the client should check the fImageLayout field to know > what layout Skia left the VkImage in before using the VkImage. If the client > changes the layout of the VkImage, they should call > GrVkImageInfo::updateImageLayout(VkImageLayout layout) before issuing any future > commands to Skia with that VkImage." Done.
https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); On 2016/09/06 14:09:00, jvanverth1 wrote: > fInterface.reset(). And I'm not sure on the need for creation, I'll check. The client is filling in the rest of the struct -- I don't think it's too much to ask them to create the interface. And I'm not sure where we'd create it -- in GrContext::Create? That seems like an odd side effect.
I updated the text to say that the env var must be set after installing the SDK on Linux. I think the only thing left is the 64 bit build question. https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/80001/site/user/special/vulka... site/user/special/vulkan.md:33: vkBackendContext.fInteface.reset(GrVkCreateInterface(instance, vkPhysDevice, extensionFlags); On 2016/09/06 14:32:53, jvanverth1 wrote: > On 2016/09/06 14:09:00, jvanverth1 wrote: > > fInterface.reset(). And I'm not sure on the need for creation, I'll check. > > The client is filling in the rest of the struct -- I don't think it's too much > to ask them to create the interface. And I'm not sure where we'd create it -- in > GrContext::Create? That seems like an odd side effect. I was thinking we'd store a GrVkInterface pointer in GrVkGpu separately from GrVkBackendContext. But I don't feel strongly about this. We can leave as is.
how about the synchronization comment?
On 2016/09/06 16:47:33, egdaniel wrote: > how about the synchronization comment? Sorry, missed that. Done. Do you think this is something that is likely to evolve? I could image that we'd support taking a sync primitive for an input GrVkImage and provide one when we return a GrVkImage to the client.
lgtm https://codereview.chromium.org/2318603002/diff/160001/site/user/special/vulk... File site/user/special/vulkan.md (right): https://codereview.chromium.org/2318603002/diff/160001/site/user/special/vulk... site/user/special/vulkan.md:43: Skia will assume it can start issues commands referencing the VkImage without the need for additional synchronization. issuing
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2318603002/#ps180001 (title: "issues->issuing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bsalomon@google.com
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2318603002/#ps220001 (title: "more")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bsalomon@google.com
The CQ bit was checked by bsalomon@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com Link to the patchset: https://codereview.chromium.org/2318603002/#ps240001 (title: "fix var name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Vulkan docs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2318603002 NOTRY=true DOCS_PREVIEW= https://skia.org/?cl=2318603002 ========== to ========== Add Vulkan docs BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2318603002 NOTRY=true DOCS_PREVIEW= https://skia.org/?cl=2318603002 Committed: https://skia.googlesource.com/skia/+/54dd2e244c4f49be8815611f4c10f6c7d1b700ab ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://skia.googlesource.com/skia/+/54dd2e244c4f49be8815611f4c10f6c7d1b700ab |
