Merge pull request #778 from shuffle2/fix-memcard-flush

Rewrite raw memcard threading code. Fixes issue 7484.

Source/Core/Common/Common.vcxproj

@@ -50,6 +50,7 @@
     <ClInclude Include="CommonTypes.h" />
     <ClInclude Include="CPUDetect.h" />
     <ClInclude Include="DebugInterface.h" />
+    <ClInclude Include="Event.h" />
     <ClInclude Include="ExtendedTrace.h" />
     <ClInclude Include="FifoQueue.h" />
     <ClInclude Include="FileSearch.h" />

Source/Core/Common/Common.vcxproj.filters

@@ -68,6 +68,7 @@
       <Filter>Crypto</Filter>
     </ClInclude>
     <ClInclude Include="GekkoDisassembler.h" />
+    <ClInclude Include="Event.h" />
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="BreakPoints.cpp" />

Source/Core/Common/Event.h

@@ -16,6 +16,7 @@
 #include <concrt.h>
 #endif
 
+#include <chrono>
 #include <condition_variable>
 #include <mutex>
 
@@ -48,6 +49,20 @@ public:
 		m_flag.Clear();
 	}
 
+	template<class Rep, class Period>
+	bool WaitFor(const std::chrono::duration<Rep, Period>& rel_time)
+	{
+		if (m_flag.TestAndClear())
+			return true;
+
+		std::unique_lock<std::mutex> lk(m_mutex);
+		bool signaled = m_condvar.wait_for(lk, rel_time,
+			[&]{ return m_flag.IsSet(); });
+		m_flag.Clear();
+
+		return signaled;
+	}
+
 	void Reset()
 	{
 		// no other action required, since wait loops on
@@ -65,9 +80,31 @@ private:
 class Event final
 {
 public:
-	void Set() { m_event.set(); }
+	void Set()
-	void Wait() { m_event.wait(); m_event.reset(); }
+	{
-	void Reset() { m_event.reset(); }
+		m_event.set();
+	}
+
+	void Wait()
+	{
+		m_event.wait();
+		m_event.reset();
+	}
+
+	template<class Rep, class Period>
+	bool WaitFor(const std::chrono::duration<Rep, Period>& rel_time)
+	{
+		bool signaled = m_event.wait(
+			(u32)std::chrono::duration_cast<std::chrono::milliseconds>(rel_time).count()
+			) == 0;
+		m_event.reset();
+		return signaled;
+	}
+
+	void Reset()
+	{
+		m_event.reset();
+	}
 
 private:
 	concurrency::event m_event;

Source/Core/Core/HW/EXI_Channel.cpp

@@ -159,7 +159,7 @@ void CEXIChannel::SendTransferComplete()
 void CEXIChannel::RemoveDevices()
 {
 	for (auto& device : m_pDevices)
-		device.reset();
+		device.reset(nullptr);
 }
 
 void CEXIChannel::AddDevice(const TEXIDevices device_type, const int device_num)

Source/Core/Core/HW/EXI_DeviceMemoryCard.cpp

@@ -33,45 +33,62 @@
 static const u32 MC_TRANSFER_RATE_READ = 512 * 1024;
 static const u32 MC_TRANSFER_RATE_WRITE = (u32)(96.125f * 1024.0f);
 
-void CEXIMemoryCard::FlushCallback(u64 userdata, int cyclesLate)
+// Takes care of the nasty recovery of the 'this' pointer from card_index,
+// stored in the userdata parameter of the CoreTiming event.
+void CEXIMemoryCard::EventCompleteFindInstance(u64 userdata, std::function<void(CEXIMemoryCard*)> callback)
 {
-	// note that userdata is forbidden to be a pointer, due to the implementation of EventDoState
 	int card_index = (int)userdata;
-	CEXIMemoryCard* pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARD, card_index);
+	CEXIMemoryCard* pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(
+		EXIDEVICE_MEMORYCARD, card_index);
 	if (pThis == nullptr)
-		pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARDFOLDER, card_index);
+	{
-	if (pThis && pThis->memorycard)
+		pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(
-		pThis->memorycard->Flush();
+			EXIDEVICE_MEMORYCARDFOLDER, card_index);
+	}
+	if (pThis)
+	{
+		callback(pThis);
+	}
 }
 
 void CEXIMemoryCard::CmdDoneCallback(u64 userdata, int cyclesLate)
 {
-	int card_index = (int)userdata;
+	EventCompleteFindInstance(userdata, [](CEXIMemoryCard* instance)
-	CEXIMemoryCard* pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARD, card_index);
+	{
-	if (pThis == nullptr)
+		instance->CmdDone();
-		pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARDFOLDER, card_index);
+	});
-	if (pThis)
-		pThis->CmdDone();
 }
 
 void CEXIMemoryCard::TransferCompleteCallback(u64 userdata, int cyclesLate)
 {
-	int card_index = (int)userdata;
+	EventCompleteFindInstance(userdata, [](CEXIMemoryCard* instance)
-	CEXIMemoryCard* pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARD, card_index);
+	{
-	if (pThis == nullptr)
+		instance->TransferComplete();
-		pThis = (CEXIMemoryCard*)ExpansionInterface::FindDevice(EXIDEVICE_MEMORYCARDFOLDER, card_index);
+	});
-	if (pThis)
-		pThis->TransferComplete();
 }
 
 CEXIMemoryCard::CEXIMemoryCard(const int index, bool gciFolder)
 	: card_index(index)
-	, m_bDirty(false)
 {
-	// we're potentially leaking events here, since there's no UnregisterEvent until emu shutdown, but I guess it's inconsequential
+	struct
-	et_this_card = CoreTiming::RegisterEvent((index == 0) ? "memcardFlushA" : "memcardFlushB", FlushCallback);
+	{
-	et_cmd_done = CoreTiming::RegisterEvent((index == 0) ? "memcardDoneA" : "memcardDoneB", CmdDoneCallback);
+		const char *done;
-	et_transfer_complete = CoreTiming::RegisterEvent((index == 0) ? "memcardTransferCompleteA" : "memcardTransferCompleteB", TransferCompleteCallback);
+		const char *transfer_complete;
+	} const event_names[] = {
+		{ "memcardDoneA", "memcardTransferCompleteA" },
+		{ "memcardDoneB", "memcardTransferCompleteB" },
+	};
+
+	if ((size_t)index >= ArraySize(event_names))
+	{
+		PanicAlertT("Trying to create invalid memory card index.");
+	}
+	// we're potentially leaking events here, since there's no RemoveEvent
+	// until emu shutdown, but I guess it's inconsequential
+	et_cmd_done = CoreTiming::RegisterEvent(event_names[index].done,
+		CmdDoneCallback);
+	et_transfer_complete = CoreTiming::RegisterEvent(
+		event_names[index].transfer_complete, TransferCompleteCallback);
 
 	interruptSwitch = 0;
 	m_bInterruptSet = 0;
@@ -226,9 +243,8 @@ void CEXIMemoryCard::SetupRawMemcard(u16 sizeMb)
 
 CEXIMemoryCard::~CEXIMemoryCard()
 {
-	CoreTiming::RemoveEvent(et_this_card);
+	CoreTiming::RemoveEvent(et_cmd_done);
-	memorycard->Flush(true);
+	CoreTiming::RemoveEvent(et_transfer_complete);
-	memorycard.reset();
 }
 
 bool CEXIMemoryCard::UseDelayedTransferCompletion()
@@ -247,7 +263,6 @@ void CEXIMemoryCard::CmdDone()
 	status &= ~MC_STATUS_BUSY;
 
 	m_bInterruptSet = 1;
-	m_bDirty = true;
 }
 
 void CEXIMemoryCard::TransferComplete()
@@ -264,9 +279,6 @@ void CEXIMemoryCard::CmdDoneLater(u64 cycles)
 
 void CEXIMemoryCard::SetCS(int cs)
 {
-	// So that memory card won't be invalidated during flushing
-	memorycard->JoinThread();
-
 	if (cs)  // not-selected to selected
 	{
 		m_uPosition = 0;
@@ -291,10 +303,10 @@ void CEXIMemoryCard::SetCS(int cs)
 		case cmdChipErase:
 			if (m_uPosition > 2)
 			{
-				// TODO: Investigate on HW, I (LPFaint99) believe that this only erases the system area (Blocks 0-4)
+				// TODO: Investigate on HW, I (LPFaint99) believe that this only
+				// erases the system area (Blocks 0-4)
 				memorycard->ClearAll();
 				status &= ~MC_STATUS_BUSY;
-				m_bDirty = true;
 			}
 			break;
 
@@ -483,16 +495,6 @@ void CEXIMemoryCard::TransferByte(u8 &byte)
 	DEBUG_LOG(EXPANSIONINTERFACE, "EXI MEMCARD: < %02x", byte);
 }
 
-void CEXIMemoryCard::PauseAndLock(bool doLock, bool unpauseOnUnlock)
-{
-	if (doLock)
-	{
-		// we don't exactly have anything to pause,
-		// but let's make sure the flush thread isn't running.
-		memorycard->JoinThread();
-	}
-}
-
 void CEXIMemoryCard::DoState(PointerWrap &p)
 {
 	// for movie sync, we need to save/load memory card contents (and other data) in savestates.
@@ -509,7 +511,6 @@ void CEXIMemoryCard::DoState(PointerWrap &p)
 		p.Do(status);
 		p.Do(m_uPosition);
 		p.Do(programming_buffer);
-		p.Do(m_bDirty);
 		p.Do(address);
 		memorycard->DoState(p);
 		p.Do(card_index);
@@ -531,13 +532,16 @@ void CEXIMemoryCard::DMARead(u32 _uAddr, u32 _uSize)
 {
 	memorycard->Read(address, _uSize, Memory::GetPointer(_uAddr));
 
-#ifdef _DEBUG
 	if ((address + _uSize) % BLOCK_SIZE == 0)
-		INFO_LOG(EXPANSIONINTERFACE, "reading from block: %x", address / BLOCK_SIZE);
+	{
-#endif
+		DEBUG_LOG(EXPANSIONINTERFACE, "reading from block: %x",
+			address / BLOCK_SIZE);
+	}
 
 	// Schedule transfer complete later based on read speed
-	CoreTiming::ScheduleEvent(_uSize * (SystemTimers::GetTicksPerSecond() / MC_TRANSFER_RATE_READ), et_transfer_complete, (u64)card_index);
+	CoreTiming::ScheduleEvent(
+		_uSize * (SystemTimers::GetTicksPerSecond() / MC_TRANSFER_RATE_READ),
+		et_transfer_complete, (u64)card_index);
 }
 
 // DMA write are preceded by all of the necessary setup via IMMWrite
@@ -546,21 +550,14 @@ void CEXIMemoryCard::DMAWrite(u32 _uAddr, u32 _uSize)
 {
 	memorycard->Write(address, _uSize, Memory::GetPointer(_uAddr));
 
-	// At the end of writing to a block flush to disk
-	// memory card blocks are always(?) written as a whole,
-	// but the dma calls are by page size (0x200) at a time
-	// just in case this is the last block that the game will be writing for a while
 	if (((address + _uSize) % BLOCK_SIZE) == 0)
 	{
-		INFO_LOG(EXPANSIONINTERFACE, "writing to block: %x", address / BLOCK_SIZE);
+		DEBUG_LOG(EXPANSIONINTERFACE, "writing to block: %x",
-		// Page written to memory card, not just to buffer - let's schedule a flush 0.5b cycles into the future (1 sec)
+			address / BLOCK_SIZE);
-		// But first we unschedule already scheduled flushes - no point in flushing once per page for a large write
-		// Scheduling event is mainly for raw memory cards as the flush the whole 16MB to disk
-		// Flushing the gci folder is free in comparison
-		CoreTiming::RemoveEvent(et_this_card);
-		CoreTiming::ScheduleEvent(500000000, et_this_card, (u64)card_index);
 	}
 
 	// Schedule transfer complete later based on write speed
-	CoreTiming::ScheduleEvent(_uSize * (SystemTimers::GetTicksPerSecond() / MC_TRANSFER_RATE_WRITE), et_transfer_complete, (u64)card_index);
+	CoreTiming::ScheduleEvent(
+		_uSize * (SystemTimers::GetTicksPerSecond() / MC_TRANSFER_RATE_WRITE),
+		et_transfer_complete, (u64)card_index);
 }

Source/Core/Core/HW/EXI_DeviceMemoryCard.h

@@ -16,7 +16,6 @@ public:
 	bool UseDelayedTransferCompletion() override;
 	bool IsPresent() override;
 	void DoState(PointerWrap &p) override;
-	void PauseAndLock(bool doLock, bool unpauseOnUnlock=true) override;
 	IEXIDevice* FindDevice(TEXIDevices device_type, int customIndex=-1) override;
 	void DMARead(u32 _uAddr, u32 _uSize) override;
 	void DMAWrite(u32 _uAddr, u32 _uSize) override;
@@ -24,9 +23,7 @@ public:
 private:
 	void SetupGciFolder(u16 sizeMb);
 	void SetupRawMemcard(u16 sizeMb);
-	// This is scheduled whenever a page write is issued. The this pointer is passed
+	static void EventCompleteFindInstance(u64 userdata, std::function<void(CEXIMemoryCard*)> callback);
-	// through the userdata parameter, so that it can then call Flush on the right card.
-	static void FlushCallback(u64 userdata, int cyclesLate);
 
 	// Scheduled when a command that required delayed end signaling is done.
 	static void CmdDoneCallback(u64 userdata, int cyclesLate);
@@ -34,9 +31,6 @@ private:
 	// Scheduled when memory card is done transferring data
 	static void TransferCompleteCallback(u64 userdata, int cyclesLate);
 
-	// Flushes the memory card contents to disk.
-	void Flush(bool exiting = false);
-
 	// Signals that the command that was previously executed is now done.
 	void CmdDone();
 
@@ -66,7 +60,7 @@ private:
 	};
 
 	int card_index;
-	int et_this_card, et_cmd_done, et_transfer_complete;
+	int et_cmd_done, et_transfer_complete;
 	//! memory card state
 
 	// STATE_TO_SAVE
@@ -76,7 +70,6 @@ private:
 	int status;
 	u32 m_uPosition;
 	u8 programming_buffer[128];
-	bool m_bDirty;
 	//! memory card parameters
 	unsigned int card_id;
 	unsigned int address;

Source/Core/Core/HW/GCMemcard.h

@@ -70,14 +70,16 @@ public:
 	{
 	}
 	virtual ~MemoryCardBase() {}
-	virtual void Flush(bool exiting = false) = 0;
 	virtual s32 Read(u32 address, s32 length, u8 *destaddress) = 0;
 	virtual s32 Write(u32 destaddress, s32 length, u8 *srcaddress) = 0;
 	virtual void ClearBlock(u32 address) = 0;
 	virtual void ClearAll() = 0;
 	virtual void DoState(PointerWrap &p) = 0;
-	virtual void JoinThread() {};
 	u32 GetCardId() { return nintendo_card_id; }
+	bool IsAddressInBounds(u32 address) const
+	{
+		return address <= (memory_card_size - 1);
+	}
 
 protected:
 	int card_index;

Source/Core/Core/HW/GCMemcardDirectory.cpp

@@ -157,6 +157,11 @@ GCMemcardDirectory::GCMemcardDirectory(std::string directory, int slot, u16 size
 	m_bat2 = m_bat1;
 }
 
+GCMemcardDirectory::~GCMemcardDirectory()
+{
+	FlushToFile();
+}
+
 s32 GCMemcardDirectory::Read(u32 address, s32 length, u8 *destaddress)
 {
 
@@ -468,7 +473,7 @@ bool GCMemcardDirectory::SetUsedBlocks(int saveIndex)
 	return true;
 }
 
-void GCMemcardDirectory::Flush(bool exiting)
+void GCMemcardDirectory::FlushToFile()
 {
 	int errors = 0;
 	DEntry invalid;
@@ -500,21 +505,18 @@ void GCMemcardDirectory::Flush(bool exiting)
 					GCI.WriteBytes(&m_saves[i].m_gci_header, DENTRY_SIZE);
 					GCI.WriteBytes(m_saves[i].m_save_data.data(), BLOCK_SIZE * m_saves[i].m_save_data.size());
 
-					if (!exiting)
+					if (GCI.IsGood())
 					{
-						if (GCI.IsGood())
+						Core::DisplayMessage(
-						{
+							StringFromFormat("Wrote save contents to %s", m_saves[i].m_filename.c_str()), 4000);
-							Core::DisplayMessage(
+					}
-								StringFromFormat("Wrote save contents to %s", m_saves[i].m_filename.c_str()), 4000);
+					else
-						}
+					{
-						else
+						++errors;
-						{
+						Core::DisplayMessage(
-							++errors;
+							StringFromFormat("Failed to write save contents to %s", m_saves[i].m_filename.c_str()),
-							Core::DisplayMessage(
+							4000);
-								StringFromFormat("Failed to write save contents to %s", m_saves[i].m_filename.c_str()),
+						ERROR_LOG(EXPANSIONINTERFACE, "Failed to save data to %s", m_saves[i].m_filename.c_str());
-								4000);
-							ERROR_LOG(EXPANSIONINTERFACE, "Failed to save data to %s", m_saves[i].m_filename.c_str());
-						}
 					}
 				}
 			}

Source/Core/Core/HW/GCMemcardDirectory.h

@@ -15,13 +15,13 @@ class GCMemcardDirectory : public MemoryCardBase, NonCopyable
 public:
 	GCMemcardDirectory(std::string directory, int slot = 0, u16 sizeMb = MemCard2043Mb, bool ascii = true,
 		DiscIO::IVolume::ECountry  card_region = DiscIO::IVolume::COUNTRY_EUROPE, int gameId = 0);
-	~GCMemcardDirectory() { Flush(true); }
+	~GCMemcardDirectory();
-	void Flush(bool exiting = false) override;
+	void FlushToFile();
 
 	s32 Read(u32 address, s32 length, u8 *destaddress) override;
 	s32 Write(u32 destaddress, s32 length, u8 *srcaddress) override;
 	void ClearBlock(u32 address) override;
-	void ClearAll() override { ; }
+	void ClearAll() override {}
 	void DoState(PointerWrap &p) override;
 
 private:

Source/Core/Core/HW/GCMemcardRaw.cpp

@@ -2,7 +2,9 @@
 // Licensed under GPLv2
 // Refer to the license.txt file included.
 
+#include <chrono>
 #include "Common/ChunkFile.h"
+#include "Common/StdMakeUnique.h"
 #include "Core/Core.h"
 #include "Core/HW/GCMemcard.h"
 #include "Core/HW/GCMemcardRaw.h"
@@ -10,53 +12,21 @@
 #define SIZE_TO_Mb (1024 * 8 * 16)
 #define MC_HDR_SIZE 0xA000
 
-static void innerFlush(FlushData *data)
-{
-	File::IOFile pFile(data->filename, "r+b");
-	if (!pFile)
-	{
-		std::string dir;
-		SplitPath(data->filename, &dir, nullptr, nullptr);
-		if (!File::IsDirectory(dir))
-			File::CreateFullPath(dir);
-		pFile.Open(data->filename, "wb");
-	}
-
-	if (!pFile) // Note - pFile changed inside above if
-	{
-		PanicAlertT("Could not write memory card file %s.\n\n"
-					"Are you running Dolphin from a CD/DVD, or is the save file maybe write protected?\n\n"
-					"Are you receiving this after moving the emulator directory?\nIf so, then you may "
-					"need to re-specify your memory card location in the options.",
-					data->filename.c_str());
-		return;
-	}
-
-	pFile.WriteBytes(data->memcardContent, data->memcardSize);
-
-	if (!data->bExiting)
-		Core::DisplayMessage(StringFromFormat("Wrote memory card %c contents to %s", data->memcardIndex ? 'B' : 'A',
-											  data->filename.c_str()).c_str(),
-							 4000);
-	return;
-}
-
 MemoryCard::MemoryCard(std::string filename, int _card_index, u16 sizeMb)
 	: MemoryCardBase(_card_index, sizeMb)
-	, m_bDirty(false)
+	, m_filename(filename)
-	, m_strFilename(filename)
 {
-	File::IOFile pFile(m_strFilename, "rb");
+	File::IOFile pFile(m_filename, "rb");
 	if (pFile)
 	{
-		// Measure size of the memcard file.
+		// Measure size of the existing memcard file.
-		memory_card_size = (int)pFile.GetSize();
+		memory_card_size = (u32)pFile.GetSize();
 		nintendo_card_id = memory_card_size / SIZE_TO_Mb;
-		memory_card_content = new u8[memory_card_size];
+		m_memcard_data = std::make_unique<u8[]>(memory_card_size);
-		memset(memory_card_content, 0xFF, memory_card_size);
+		memset(&m_memcard_data[0], 0xFF, memory_card_size);
 
-		INFO_LOG(EXPANSIONINTERFACE, "Reading memory card %s", m_strFilename.c_str());
+		INFO_LOG(EXPANSIONINTERFACE, "Reading memory card %s", m_filename.c_str());
-		pFile.ReadBytes(memory_card_content, memory_card_size);
+		pFile.ReadBytes(&m_memcard_data[0], memory_card_size);
 	}
 	else
 	{
@@ -64,108 +34,186 @@ MemoryCard::MemoryCard(std::string filename, int _card_index, u16 sizeMb)
 		nintendo_card_id = sizeMb;
 		memory_card_size = sizeMb * SIZE_TO_Mb;
 
-		memory_card_content = new u8[memory_card_size];
+		m_memcard_data = std::make_unique<u8[]>(memory_card_size);
-		GCMemcard::Format(memory_card_content, m_strFilename.find(".JAP.raw") != std::string::npos, sizeMb);
+		// Fills in MC_HDR_SIZE bytes
-		memset(memory_card_content + MC_HDR_SIZE, 0xFF, memory_card_size - MC_HDR_SIZE);
+		GCMemcard::Format(&m_memcard_data[0], m_filename.find(".JAP.raw") != std::string::npos, sizeMb);
+		memset(&m_memcard_data[MC_HDR_SIZE], 0xFF, memory_card_size - MC_HDR_SIZE);
 
-		WARN_LOG(EXPANSIONINTERFACE, "No memory card found. Will create a new one.");
+		INFO_LOG(EXPANSIONINTERFACE, "No memory card found - a new one was created.");
 	}
+
+	// Class members (including inherited ones) have now been initialized, so
+	// it's safe to startup the flush thread (which reads them).
+	m_flush_buffer = std::make_unique<u8[]>(memory_card_size);
+	m_flush_thread = std::thread(&MemoryCard::FlushThread, this);
 }
 
 MemoryCard::~MemoryCard()
 {
-	Flush(true);
+	if (m_flush_thread.joinable())
-	delete[] memory_card_content;
-}
-
-void MemoryCard::JoinThread()
-{
-	if (flushThread.joinable())
 	{
-		flushThread.join();
+		// Update the flush buffer one last time, flush, and join.
+		{
+			std::unique_lock<std::mutex> l(m_flush_mutex);
+			memcpy(&m_flush_buffer[0], &m_memcard_data[0], memory_card_size);
+		}
+
+		m_is_exiting.Set();
+		m_flush_trigger.Set();
+
+		m_flush_thread.join();
 	}
 }
 
-// Flush memory card contents to disc
+void MemoryCard::FlushThread()
-void MemoryCard::Flush(bool exiting)
 {
-	if (!m_bDirty)
-		return;
-
 	if (!Core::g_CoreStartupParameter.bEnableMemcardSaving)
-		return;
-
-	if (flushThread.joinable())
 	{
-		flushThread.join();
+		return;
 	}
 
-	if (!exiting)
+	Common::SetCurrentThreadName(
-		Core::DisplayMessage(StringFromFormat("Writing to memory card %c", card_index ? 'B' : 'A'), 1000);
+		StringFromFormat("Memcard%x-Flush", card_index).c_str());
 
-	flushData.filename = m_strFilename;
+	const auto flush_interval = std::chrono::seconds(15);
-	flushData.memcardContent = memory_card_content;
+	auto last_flush = std::chrono::steady_clock::now();
-	flushData.memcardIndex = card_index;
+	bool dirty = false;
-	flushData.memcardSize = memory_card_size;
-	flushData.bExiting = exiting;
 
-	flushThread = std::thread(innerFlush, &flushData);
+	for (;;)
-	if (exiting)
+	{
-		flushThread.join();
+		bool triggered = m_flush_trigger.WaitFor(flush_interval);
+		bool do_exit = m_is_exiting.IsSet();
+		if (triggered)
+		{
+			dirty = true;
+		}
+		// Delay the flush if we're not exiting or if the event timed out and
+		// the state isn't dirty.
+		if (!do_exit)
+		{
+			auto now = std::chrono::steady_clock::now();
+			if (now - last_flush < flush_interval || !dirty)
+			{
+				continue;
+			}
+			last_flush = now;
+		}
 
-	m_bDirty = false;
+		// Opening the file is purposefully done each iteration to ensure the
+		// file doesn't disappear out from under us after the first check.
+		File::IOFile pFile(m_filename, "r+b");
+
+		if (!pFile)
+		{
+			std::string dir;
+			SplitPath(m_filename, &dir, nullptr, nullptr);
+			if (!File::IsDirectory(dir))
+			{
+				File::CreateFullPath(dir);
+			}
+			pFile.Open(m_filename, "wb");
+		}
+
+		// Note - pFile may have changed above, after ctor
+		if (!pFile)
+		{
+			PanicAlertT(
+				"Could not write memory card file %s.\n\n"
+				"Are you running Dolphin from a CD/DVD, or is the save file maybe write protected?\n\n"
+				"Are you receiving this after moving the emulator directory?\nIf so, then you may "
+				"need to re-specify your memory card location in the options.",
+				m_filename.c_str());
+
+			// Exit the flushing thread - further flushes will be ignored unless
+			// the thread is recreated.
+			return;
+		}
+
+		{
+			std::unique_lock<std::mutex> l(m_flush_mutex);
+			pFile.WriteBytes(&m_flush_buffer[0], memory_card_size);
+		}
+
+		dirty = false;
+
+		if (!do_exit)
+		{
+			Core::DisplayMessage(
+				StringFromFormat("Wrote memory card %c contents to %s",
+				card_index ? 'B' : 'A', m_filename.c_str()).c_str(),
+				4000);
+		}
+		else
+		{
+			return;
+		}
+	}
+}
+
+// Attempt to update the flush buffer and trigger a flush. If we can't get a
+// lock in order to update the flush buffer, a write is in progress and a future
+// write will take care of any changes to the memcard data, so nothing needs to
+// be done now.
+void MemoryCard::TryFlush()
+{
+	if (m_flush_mutex.try_lock())
+	{
+		memcpy(&m_flush_buffer[0], &m_memcard_data[0], memory_card_size);
+		m_flush_mutex.unlock();
+		m_flush_trigger.Set();
+	}
 }
 
 s32 MemoryCard::Read(u32 srcaddress, s32 length, u8 *destaddress)
 {
-	if (!memory_card_content)
+	if (!IsAddressInBounds(srcaddress))
-		return -1;
-	if (srcaddress > (memory_card_size - 1))
 	{
-		PanicAlertT("MemoryCard: Read called with invalid source address, %x", srcaddress);
+		PanicAlertT("MemoryCard: Read called with invalid source address, %x",
+					srcaddress);
 		return -1;
 	}
 
-	memcpy(destaddress, &(memory_card_content[srcaddress]), length);
+	memcpy(destaddress, &m_memcard_data[srcaddress], length);
 	return length;
 }
 
 s32 MemoryCard::Write(u32 destaddress, s32 length, u8 *srcaddress)
 {
-	if (!memory_card_content)
+	if (!IsAddressInBounds(destaddress))
-		return -1;
-
-	if (destaddress > (memory_card_size - 1))
 	{
-		PanicAlertT("MemoryCard: Write called with invalid destination address, %x", destaddress);
+		PanicAlertT("MemoryCard: Write called with invalid destination address, %x",
+					destaddress);
 		return -1;
 	}
 
-	m_bDirty = true;
+	memcpy(&m_memcard_data[destaddress], srcaddress, length);
-	memcpy(&(memory_card_content[destaddress]), srcaddress, length);
+	TryFlush();
 	return length;
 }
 
 void MemoryCard::ClearBlock(u32 address)
 {
-	if (address & (BLOCK_SIZE - 1) || address > (memory_card_size - 1))
+	if (address & (BLOCK_SIZE - 1) || !IsAddressInBounds(address))
-		PanicAlertT("MemoryCard: ClearBlock called on invalid address %x", address);
+	{
+		PanicAlertT("MemoryCard: ClearBlock called on invalid address %x",
+					address);
+	}
 	else
 	{
-		m_bDirty = true;
+		memset(&m_memcard_data[address], 0xFF, BLOCK_SIZE);
-		memset(memory_card_content + address, 0xFF, BLOCK_SIZE);
+		TryFlush();
 	}
 }
 
 void MemoryCard::ClearAll()
 {
-	m_bDirty = true;
+	memset(&m_memcard_data[0], 0xFF, memory_card_size);
-	memset(memory_card_content, 0xFF, memory_card_size);
+	TryFlush();
 }
 
 void MemoryCard::DoState(PointerWrap &p)
 {
 	p.Do(card_index);
 	p.Do(memory_card_size);
-	p.DoArray(memory_card_content, memory_card_size);
+	p.DoArray(&m_memcard_data[0], memory_card_size);
 }

Source/Core/Core/HW/GCMemcardRaw.h

@@ -4,39 +4,34 @@
 
 #pragma once
 
+#include <memory>
+#include "Common/Event.h"
+#include "Common/Flag.h"
 #include "Common/Thread.h"
 #include "Core/HW/GCMemcard.h"
 
 class PointerWrap;
 
-// Data structure to be passed to the flushing thread.
-struct FlushData
-{
-	bool bExiting;
-	std::string filename;
-	u8 *memcardContent;
-	int memcardSize, memcardIndex;
-};
-
 class MemoryCard : public MemoryCardBase
 {
 public:
 	MemoryCard(std::string filename, int _card_index, u16 sizeMb = MemCard2043Mb);
 	~MemoryCard();
-	void Flush(bool exiting = false) override;
+	void FlushThread();
+	void TryFlush();
 
 	s32 Read(u32 address, s32 length, u8 *destaddress) override;
 	s32 Write(u32 destaddress, s32 length, u8 *srcaddress) override;
 	void ClearBlock(u32 address) override;
 	void ClearAll() override;
 	void DoState(PointerWrap &p) override;
-	void JoinThread() override;
 
 private:
-	u8 *memory_card_content;
+	std::string m_filename;
-	bool m_bDirty;
+	std::unique_ptr<u8[]> m_memcard_data;
-	std::string m_strFilename;
+	std::thread m_flush_thread;
-
+	std::mutex m_flush_mutex;
-	FlushData flushData;
+	Common::Event m_flush_trigger;
-	std::thread flushThread;
+	Common::Flag m_is_exiting;
+	std::unique_ptr<u8[]> m_flush_buffer;
 };

Source/Core/Core/State.cpp

@@ -63,7 +63,7 @@ static Common::Event g_compressAndDumpStateSyncEvent;
 static std::thread g_save_thread;
 
 // Don't forget to increase this after doing changes on the savestate system
-static const u32 STATE_VERSION = 30;
+static const u32 STATE_VERSION = 31;
 
 enum
 {