Fix CommandWork moving

Author: nyorain

docs/tests/allocator.cpp

@@ -57,3 +57,68 @@ TEST(memory) {
 		EXPECT(alloc.memories()[0].totalFree(), 1024u * 3);
 	}
 }
+
+
+TEST(custom) {
+	auto& dev = *globals.device;
+	auto alloc = vpp::DeviceMemoryAllocator(dev);
+	EXPECT(alloc.memories().empty(), true);
+
+	// allocate some stuff
+	{
+		vk::BufferCreateInfo bufInfo;
+		bufInfo.size = 1024;
+		bufInfo.usage = vk::BufferUsageBits::storageBuffer;
+		auto buffer = vpp::Buffer(vpp::defer, dev, bufInfo, ~0u, &alloc);
+		vpp::Buffer buffer1(vpp::defer, dev, bufInfo, ~0u, &alloc);
+		vpp::Buffer buffer2(vpp::defer, dev, bufInfo, ~0u, &alloc);
+		vpp::Buffer buffer3(vpp::defer, dev, bufInfo, ~0u, &alloc);
+		EXPECT(buffer3.memoryEntry().allocator(), &alloc);
+
+		auto alloc2 = std::move(alloc);
+
+		vpp::Buffer buffer4(vpp::defer, dev, bufInfo, ~0u, &alloc2);
+		EXPECT(alloc2.memories().empty(), true);
+		EXPECT(buffer1.memoryEntry().allocated(), false);
+		EXPECT(buffer2.memoryEntry().allocator(), &alloc2);
+		EXPECT(buffer3.memoryEntry().allocator(), &alloc2);
+
+		alloc = std::move(alloc2);
+		alloc.allocate();
+		EXPECT(alloc.memories().size(), 1u);
+
+		EXPECT(buffer2.memoryEntry().allocated(), true);
+		EXPECT(buffer3.memoryEntry().allocated(), true);
+		EXPECT(buffer3.memoryEntry().memory(), &alloc.memories()[0]);
+
+		EXPECT(buffer1.memorySize(), 1024u);
+		EXPECT(buffer2.memorySize(), 1024u);
+		EXPECT(buffer3.memorySize(), 1024u);
+		EXPECT(buffer4.memorySize(), 1024u);
+	}
+
+	EXPECT(alloc.memories().size(), 1u);
+
+	// XXX: might fail due to alignment requirements etc (allocater 
+	// allocated larger memory)
+	EXPECT(alloc.memories()[0].totalFree(), 1024u * 5);
+
+	// make sure same memory is used again
+	{
+		vk::BufferCreateInfo bufInfo;
+		bufInfo.size = 1024;
+		bufInfo.usage = vk::BufferUsageBits::storageBuffer;
+		auto buffer = vpp::Buffer(dev, bufInfo, ~0u, &alloc);
+		EXPECT(alloc.memories().size(), 1u);
+		EXPECT(alloc.memories()[0].totalFree(), 1024u * 4);
+
+		vpp::Buffer buffer1(vpp::defer, dev, bufInfo, ~0u, &alloc);
+		EXPECT(alloc.memories()[0].totalFree(), 1024u * 4);
+
+		auto alloc2 = std::move(alloc);
+		buffer1.init();
+		EXPECT(alloc2.memories()[0].totalFree(), 1024u * 3);
+
+		alloc = std::move(alloc2);
+	}
+}

docs/tests/bufferOps.cpp

@@ -210,13 +210,19 @@ vpp::BufferRange readWrite(bool mappable, vk::BufferUsageFlags usage,
 	std::int32_t c {};
 
 	if(!direct && !mappable) {
-		vpp::writeStaging140(buf, 42.f, Vec3f {1.f, 2.f, 3.f}, (std::int32_t) -420);
+		auto work = vpp::writeStaging140(buf, 42.f, Vec3f {1.f, 2.f, 3.f}, (std::int32_t) -420);
+		auto work2 = std::move(work);
+		work2.finish();
+
 		vpp::readStaging140(buf, a, b, c);
 	} else if(!direct && mappable) {
 		vpp::writeMap140(buf, 42.f, Vec3f {1.f, 2.f, 3.f}, (std::int32_t) -420);
 		vpp::readMap140(buf, a, b, c);
 	} else {
-		vpp::writeDirect140(buf, 42.f, Vec3f {1.f, 2.f, 3.f}, (std::int32_t) -420);
+		auto work = vpp::writeDirect140(buf, 42.f, Vec3f {1.f, 2.f, 3.f}, (std::int32_t) -420);
+		auto work2 = std::move(work);
+		work2.finish();
+		
 		vpp::readStaging140(buf, a, b, c);
 	}
 
@@ -243,7 +249,9 @@ TEST(buffer_range) {
 	auto buf6 = readWrite(false, usage, true);
 
 	for(auto i = 0u; i < 100u; ++i) {
-		readWrite(true, usage);
+		auto range = readWrite(true, usage);
+		auto range2 = std::move(range);
+
 		readWrite(false, usage);
 		readWrite(true, usage, true);
 		readWrite(false, usage, true);

docs/tests/framebuffer.cpp

@@ -61,4 +61,5 @@ TEST(framebuffer) {
 		{}, renderPass, 2, fbAttachments.begin(), size.width, size.height, 1
 	};
 	auto fb = vpp::Framebuffer(dev, info);
+	auto moved = std::move(fb);
 }

docs/tests/image.cpp

@@ -21,6 +21,8 @@ TEST(image) {
 	};
 
 	vpp::Image img1 = {dev, imgInfo};
+	auto moved = std::move(img1);
+	img1 = std::move(moved);
 
 	imgInfo.usage = vk::ImageUsageBits::sampled;
 	imgInfo.format = vk::Format::r8Uint;
@@ -43,26 +45,33 @@ TEST(image) {
 		vk::AccessBits::transferWrite,
 		{vk::ImageAspectBits::color, 0, 1, 0, 1}, dev.queueSubmitter());
 
-	vpp::fillStaging(img1, vk::Format::r8g8b8a8Unorm,
+	auto work = vpp::fillStaging(img1, vk::Format::r8g8b8a8Unorm,
 		vk::ImageLayout::transferDstOptimal, size, data,
 		{vk::ImageAspectBits::color});
+	auto workm = std::move(work);
+	workm.wait();
+	workm.submit();
+	workm.finish();
 
 	vpp::fillMap(img2, vk::Format::r8Unorm, size, 
 		{data.data(), size.width * size.height}, 
 		{vk::ImageAspectBits::color});
 
-	vpp::changeLayout(img1,
+	auto work1 = vpp::changeLayout(img1,
 		vk::ImageLayout::transferDstOptimal, vk::PipelineStageBits::transfer, 
 		vk::AccessBits::transferWrite,
 		vk::ImageLayout::transferSrcOptimal, vk::PipelineStageBits::transfer, 
 		vk::AccessBits::transferRead,
 		{vk::ImageAspectBits::color, 0, 1, 0, 1}, dev.queueSubmitter());
+	auto work2 = std::move(work1);
+	work2.finish();
 
-	auto dataWork = vpp::retrieveStaging(img1, 
+	auto dataWork1 = vpp::retrieveStaging(img1, 
 		vk::Format::r8g8b8a8Unorm, vk::ImageLayout::transferSrcOptimal,
 		size, {vk::ImageAspectBits::color, 0, 0});
+	auto dataWork2 = std::move(dataWork1);
 
-	auto data1 = dataWork.data();
+	auto data1 = dataWork2.data();
 	EXPECT(data1.size(), size.width * size.height * 4u);
 	EXPECT((unsigned int) data1[0], 0x00u);
 	EXPECT((unsigned int) data1[1], 0x01u);

docs/tests/memory.cpp

@@ -61,14 +61,17 @@ TEST(map) {
 
 	EXPECT(memory.mappable(), true);
 	auto map = memory.map({0u, 1024u});
+	auto map2 = std::move(map);
+	map = std::move(map2);
+
 	EXPECT(map.offset(), 0u);
 	EXPECT(map.size(), 1024u);
 	EXPECT(&map.memory(), &memory);
 	EXPECT(map.ptr() != nullptr, true);
 	EXPECT(memory.mapped() != nullptr, true);
 
 	// this will have no effect, just return another view
-	auto map2 = memory.map({0, 256});
+	map2 = memory.map({0, 256});
 	EXPECT(map2.offset(), 0u);
 	EXPECT(map2.size(), 256u);
 	EXPECT(map2.ptr(), map.ptr());
@@ -77,6 +80,8 @@ TEST(map) {
 	// just another view
 	auto map3 = memory.map({256, 256});
 	EXPECT(map3.ptr(), map.ptr() + 256);
+
+	map = std::move(map3);
 }
 
 TEST(remap) {

docs/tests/objects.cpp

@@ -20,13 +20,15 @@ TEST(mem) {
 	auto rawMem = vk::allocateMemory(dev, {2048, 0u});
 	auto memHandle1 = vpp::DeviceMemoryHandle(dev, {256u, 0u});
 	auto memHandle2 = vpp::DeviceMemoryHandle(dev, rawMem);
+	auto memHandle3 = std::move(memHandle2);
 
 	auto rawMem2 = vk::allocateMemory(dev, {14u, 0u});
 	auto mem1 = vpp::DeviceMemory(dev, {256u, 0u});
 	auto mem2 = vpp::DeviceMemory(dev, rawMem2, 14u, 0u);
 
 	auto allocator = vpp::DeviceMemoryAllocator(dev);
 	auto memEntry = vpp::MemoryEntry(mem1, mem1.alloc(10u, 0u, vpp::AllocationType::linear));
+	auto memEntry2 = std::move(memEntry);
 }
 
 TEST(cmd) {
@@ -44,6 +46,7 @@ TEST(cmd) {
 	auto cmdAllocator = vpp::CommandAllocator(dev);
 	auto cmdBuf5 = cmdAllocator.get(0);
 	auto cmdBufs6 = cmdAllocator.get(0, 3);
+	auto cmdBuf7 = std::move(cmdBuf5);
 }
 
 // returns the index of the first set bit of an uint
@@ -100,6 +103,7 @@ TEST(buf) {
 	vpp::Buffer buf8(dev, rawBuf3, std::move(entry));
 
 	vpp::Buffer buf9(dev, info, mem);
+	auto buf10 = std::move(buf9);
 }
 
 TEST(sync) {

docs/tests/sharedBuffer.cpp

@@ -39,6 +39,11 @@ TEST(sharedBuf) {
 	EXPECT(bufRange.size(), 100u);
 	EXPECT(bufRange.offset(), 0u);
 
+	auto bufRange2 = std::move(bufRange);
+	EXPECT(&bufRange2.buffer(), &buf);
+	EXPECT(bufRange2.size(), 100u);
+	EXPECT(bufRange2.offset(), 0u);
+
 	ERROR(vpp::BufferRange(buf, 1000u), std::runtime_error);
 
 	// allocator
@@ -119,6 +124,8 @@ TEST(nonCoherentAtomAlign) {
 	vpp::BufferRange range3(buf, buf.alloc(100u));
 	offset = vpp::align<vk::DeviceSize>(range2.allocation().end(), atomAlign);
 	EXPECT(range3.allocation(), (Alloc{offset, 100u}));
+
+	auto range4 = std::move(range3);
 }
 
 TEST(mappable) {
@@ -148,6 +155,10 @@ TEST(mappable) {
 	EXPECT((coherentBits & (1 << type)) != 0, true);
 	EXPECT(buf2.buffer().nonCoherentAtomAlign, false);
 
+	auto mapView1 = buf2.memoryMap();
+	auto mapView2 = buf2.memoryMap();
+	auto mapView3 = buf2.memoryMap();
+
 	// allocate mappable buffer on non-coherent memory
 	// there might be vulkan implementations where are all hostVisible
 	// types are also hostCoherent, we cannot do this test there

docs/todo.md

@@ -22,7 +22,8 @@ Todo list vor vpp
 		- image constructors (simply copy from buffer constructors in objects.cpp)
 		- etc...
 	- also test coherent atom handling in SharedBuffer
-- fix config for vkpp
+- fix config.hpp for vkpp (messed up atm)
+- memoryMap: remap when MemoryMapView is destroyed? for guarantees?
 - imageOps: really allow extent with depth == 0? also handle it for height == 0?
 - make codestyle consistent everywhere
 - device: cache supported extensions (see e.g. defaults.cpp: could change
@@ -35,6 +36,7 @@ Todo list vor vpp
 - implement BufferAllocator optimize/shrink
 - add more assertions everywhere where things are assumed
 	- don't overdo, only if potentially useful when debugging
+	- see work.inl (something like tryFinish probably best)
 
 low prio / general / ideas
 --------------------------

include/vpp/bits/work.inl

@@ -4,17 +4,20 @@
 
 #pragma once
 
+void commandWork_assert(bool, const char* msg);
+void commandWork_updateMoved(QueueSubmitter*, uint64_t, unsigned int, 
+	const vk::CommandBuffer&);
+
 template<typename R>
 CommandWork<R>::CommandWork(QueueSubmitter& submitter, CommandBuffer&& cmd) :
 	cmdBuffer_(std::move(cmd))
 {
+	commandWork_assert(cmdBuffer_, "invalid commandBuffer passed");
 	init(submitter, {{}, {}, {}, 1, &cmdBuffer_.vkHandle(), {}, {}});
 }
 
 template<typename R>
-CommandWork<R>::CommandWork(QueueSubmitter& submitter, 
-	const vk::SubmitInfo& info, CommandBuffer&& cmd) :
-		cmdBuffer_(std::move(cmd))
+CommandWork<R>::CommandWork(QueueSubmitter& submitter, const vk::SubmitInfo& info)
 {
 	init(submitter, info);
 }
@@ -30,30 +33,44 @@ CommandWork<R>::~CommandWork()
 template<typename R>
 void CommandWork<R>::init(QueueSubmitter& submitter, const vk::SubmitInfo& info)
 {
+	commandWork_assert(state_ == WorkBase::State::none, 
+		"already initialized");
+
 	submitter_ = &submitter;
-	submitID_ = submitter.add(info);
+	submitID_ = submitter.add(info, &infoID_);
 	state_ = WorkBase::State::pending;
 }
 
 template<typename R>
 void CommandWork<R>::submit()
 {
-	// dlg_assert(state_ != WorkBase::State::none);
-	// dlg_assert(submitter_ && submitID_);
+	commandWork_assert(state_ != WorkBase::State::none && submitter_ && submitID_, 
+		"submit: invalid");
 
-	submitter_->submit(submitID_);
-	state_ = WorkBase::State::submitted;
+	if(this->pending()) {
+		submitter_->submit(submitID_);
+		state_ = WorkBase::State::submitted;
+	}
 }
 
 template<typename R>
 void CommandWork<R>::wait()
 {
-	// dlg_assert(state_ != WorkBase::State::none);
-	// dlg_assert(submitter_ && submitID_);
+	commandWork_assert(state_ != WorkBase::State::none && submitter_ && submitID_, 
+		"wait: invalid");
+
+	if(!this->executed()) {		
+		submitter_->wait(submitID_); // will automatically submit if needed
+	}
 
-	submitter_->wait(submitID_); // will automatically submit if needed
 	cmdBuffer_ = {}; // free the commandBuffer it is no longer needed
-	state_ = WorkBase::State::executed;
+	state_ = WorkBase::State::finished;
+}
+
+template<typename R>
+void CommandWork<R>::finish()
+{
+	wait();
 }
 
 template<typename R>
@@ -64,15 +81,12 @@ WorkBase::State CommandWork<R>::state()
 	}
 
 	// query state
-	// dlg_assert(submitID_);
-	// dlg_assert(submitter_);
-
 	if(state_ == WorkBase::State::pending && submitter_->submitted(submitID_)) {
 		state_ = WorkBase::State::submitted;
 	}
 
 	if(state_ == WorkBase::State::submitted && submitter_->completed(submitID_)) {
-		state_ = WorkBase::State::executed;
+		state_ = WorkBase::State::finished;
 	}
 
 	return state_;
@@ -81,11 +95,14 @@ WorkBase::State CommandWork<R>::state()
 template<typename R>
 CommandWork<R>::CommandWork(CommandWork&& rhs) noexcept :
 	cmdBuffer_(std::move(rhs.cmdBuffer_)), submitter_(rhs.submitter_),
-	submitID_(rhs.submitID_), state_(rhs.state_)
+	submitID_(rhs.submitID_), infoID_(rhs.infoID_), state_(rhs.state_)
 {
 	rhs.submitter_ = {};
 	rhs.submitID_ = {};
 	rhs.state_ = WorkBase::State::none;
+	rhs.infoID_ = {};
+
+	commandWork_updateMoved(submitter_, submitID_, infoID_, cmdBuffer_.vkHandle());
 }
 
 template<typename R>
@@ -98,11 +115,15 @@ CommandWork<R>& CommandWork<R>::operator=(CommandWork&& rhs) noexcept
 	cmdBuffer_ = std::move(rhs.cmdBuffer_);
 	submitter_ = rhs.submitter_;
 	submitID_ = rhs.submitID_;
+	infoID_ = rhs.infoID_;
 	state_ = rhs.state_;
 
 	rhs.submitter_ = {};
 	rhs.submitID_ = {};
+	rhs.infoID_ = {};
 	rhs.state_ = WorkBase::State::none;
 
+	commandWork_updateMoved(submitter_, submitID_, infoID_, cmdBuffer_.vkHandle());
+
 	return *this;
 }