Issue 2018933004: Add offset to memory allocations

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+97 lines, -60 lines)			Patch
M	include/gpu/vk/GrVkTypes.h	View	1 2 3	2 chunks	+7 lines, -1 line	0 comments	Download
M	src/gpu/vk/GrVkBuffer.h	View	1 2 3 4	3 chunks	+8 lines, -5 lines	0 comments	Download
M	src/gpu/vk/GrVkBuffer.cpp	View	1 2 3	7 chunks	+12 lines, -8 lines	1 comment	Download
M	src/gpu/vk/GrVkGpu.cpp	View	1 2 3 4	9 chunks	+13 lines, -11 lines	0 comments	Download
M	src/gpu/vk/GrVkImage.h	View	1 2 3 4	4 chunks	+11 lines, -8 lines	0 comments	Download
M	src/gpu/vk/GrVkImage.cpp	View	1 2 3	3 chunks	+4 lines, -4 lines	0 comments	Download
M	src/gpu/vk/GrVkMemory.h	View	1 2 3	2 chunks	+5 lines, -2 lines	0 comments	Download
M	src/gpu/vk/GrVkMemory.cpp	View	1 2 3	1 chunk	+26 lines, -10 lines	0 comments	Download
M	src/gpu/vk/GrVkRenderTarget.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	src/gpu/vk/GrVkStencilAttachment.cpp	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	src/gpu/vk/GrVkTexture.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	src/gpu/vk/GrVkTextureRenderTarget.cpp	View	1 2 3	1 chunk	+1 line, -2 lines	0 comments	Download
M	tests/VkWrapTests.cpp	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	tools/viewer/Viewer.cpp	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	tools/viewer/sk_app/VulkanWindowContext.cpp	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 16 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

egdaniel

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkBuffer.h File src/gpu/vk/GrVkBuffer.h (right): https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkBuffer.h#newcode61 src/gpu/vk/GrVkBuffer.h:61: VkDeviceMemory fAlloc; I haven't though deeply into this, but ...

4 years, 6 months ago (2016-05-31 13:22:14 UTC) #4

jvanverth1

4 years, 6 months ago (2016-05-31 22:05:57 UTC) #5

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkBuffer.h
File src/gpu/vk/GrVkBuffer.h (right):

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkBuffer.h...
src/gpu/vk/GrVkBuffer.h:61: VkDeviceMemory fAlloc;
On 2016/05/31 13:22:14, egdaniel wrote:
> I haven't though deeply into this, but do you think there would be any
advantage
> to having like a GrVkDeviceMemory class that would wrap the VkDeviceMemory and
> offsets so the images/buffers don't need to know about them?

I added a GrVkAlloc struct to GrVkTypes that wraps both the VkDeviceMemory and
the offset. I considered using a class, but it looked like the members would end
up being effectively public, so went for the simpler approach.

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

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:334: VkDeviceSize offset = tex->offset() +
texTop*layout.rowPitch + left*bpp;
On 2016/05/31 13:22:14, egdaniel wrote:
> is the offset actually needed here? If the image is bound to memory with some
> offset, then do you reference the start of the image as 0 or the offset?

There's no reference to the image in MapMemory, so it must be relative to the
start of the memory allocation.

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkMemory.cpp
File src/gpu/vk/GrVkMemory.cpp (right):

https://codereview.chromium.org/2018933004/diff/20001/src/gpu/vk/GrVkMemory.c...
src/gpu/vk/GrVkMemory.cpp:87: void GrVkMemory::FreeBufferMemory(const GrVkGpu*
gpu, VkDeviceMemory memory,
On 2016/05/31 13:22:14, egdaniel wrote:
> Is the plan to treat buffer and image memory/freeing different with the sub
> allocations?

At the very least they'll have different heaps. Buffers will eventually be
suballocated within a VkBuffer, rather than suballocating the VkDeviceMemory. We
can't do that with VkImages.

egdaniel

lgtm https://codereview.chromium.org/2018933004/diff/100001/src/gpu/vk/GrVkBuffer.cpp File src/gpu/vk/GrVkBuffer.cpp (right): https://codereview.chromium.org/2018933004/diff/100001/src/gpu/vk/GrVkBuffer.cpp#newcode146 src/gpu/vk/GrVkBuffer.cpp:146: VK_WHOLE_SIZE, 0, &fMapPtr)); Does whole size here just ...

4 years, 6 months ago (2016-06-01 16:02:47 UTC) #6

jvanverth1

On 2016/06/01 16:02:47, egdaniel wrote: > lgtm > > https://codereview.chromium.org/2018933004/diff/100001/src/gpu/vk/GrVkBuffer.cpp > File src/gpu/vk/GrVkBuffer.cpp (right): > ...

4 years, 6 months ago (2016-06-01 16:09:26 UTC) #7

commit-bot: I haz the power

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

4 years, 6 months ago (2016-06-01 16:16:48 UTC) #9

commit-bot: I haz the power

Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/builds/10089)

4 years, 6 months ago (2016-06-01 16:18:19 UTC) #11

commit-bot: I haz the power

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

4 years, 6 months ago (2016-06-01 16:29:39 UTC) #14

commit-bot: I haz the power

4 years, 6 months ago (2016-06-01 16:39:19 UTC) #16

Message was sent while issue was closed.

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/1e305ba0d6a4237020d36234e9e286d3b0489401

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages