DVDInterface: Remove GetVolume

For thread safety reasons, the currently inserted volume must only be accessed by the DVD thread (or by the CPU thread if it calls DVDThread::WaitUntilIdle() first). After this commit, only DVDThread.cpp can access the volume, which prevents code in other files from accessing the volume in a non-threadsafe way.
2025-01-24 23:11:14 +01:00 · 2016-09-24 14:43:03 +02:00 · 2016-09-24 14:43:03 +02:00 · 2a2db16087
commit 2a2db16087
parent 5a55957741
8 changed files with 157 additions and 81 deletions
--- a/Source/Core/Core/ConfigManager.cpp
+++ b/Source/Core/Core/ConfigManager.cpp
@ -761,20 +761,11 @@ void SConfig::SetRunningGameMetadata(const IOS::ES::TMDReader& tmd)
  // the disc header instead of the TMD. They can differ.
  // (IOS HLE ES calls us with a TMDReader rather than a volume when launching
  // a disc game, because ES has no reason to be accessing the disc directly.)
-  if (DVDInterface::IsDiscInside())
+  if (!DVDThread::UpdateRunningGameMetadata(tmd_title_id))
  {
-    DVDThread::WaitUntilIdle();
-    const DiscIO::IVolume& volume = DVDInterface::GetVolume();
-    u64 volume_title_id;
-    if (volume.GetTitleID(&volume_title_id) && volume_title_id == tmd_title_id)
-    {
-      SetRunningGameMetadata(volume.GetGameID(), volume_title_id, volume.GetRevision());
-      return;
-    }
+    // If not launching a disc game, just read everything from the TMD.
+    SetRunningGameMetadata(tmd.GetGameID(), tmd_title_id, tmd.GetTitleVersion());
  }
-
-  // If not launching a disc game, just read everything from the TMD.
-  SetRunningGameMetadata(tmd.GetGameID(), tmd_title_id, tmd.GetTitleVersion());
 }

 void SConfig::SetRunningGameMetadata(const std::string& game_id, u64 title_id, u16 revision)
--- a/Source/Core/Core/HW/DVD/DVDInterface.cpp
+++ b/Source/Core/Core/HW/DVD/DVDInterface.cpp
@ -21,7 +21,6 @@
 #include "Core/HW/DVD/DVDInterface.h"
 #include "Core/HW/DVD/DVDMath.h"
 #include "Core/HW/DVD/DVDThread.h"
-#include "Core/HW/DVD/FileMonitor.h"
 #include "Core/HW/MMIO.h"
 #include "Core/HW/Memmap.h"
 #include "Core/HW/ProcessorInterface.h"
@ -195,8 +194,6 @@ union UDICFG
  UDICFG(u32 _hex) { Hex = _hex; }
 };

-static std::unique_ptr<DiscIO::IVolume> s_inserted_volume;
-
 // STATE_TO_SAVE

 // Hardware registers
@ -255,8 +252,6 @@ void ScheduleReads(u64 offset, u32 length, bool decrypt, u32 output_address, Rep

 void DoState(PointerWrap& p)
 {
-  bool disc_inside = IsDiscInside();
-
  p.DoPOD(s_DISR);
  p.DoPOD(s_DICVR);
  p.DoArray(s_DICMDBUF);
@ -276,7 +271,6 @@ void DoState(PointerWrap& p)
  p.Do(s_pending_samples);

  p.Do(s_error_code);
-  p.Do(disc_inside);

  p.Do(s_read_buffer_start_time);
  p.Do(s_read_buffer_end_time);
@ -286,24 +280,6 @@ void DoState(PointerWrap& p)
  p.Do(s_disc_path_to_insert);

  DVDThread::DoState(p);
-
-  // s_inserted_volume isn't savestated (because it points to
-  // files on the local system). Instead, we check that the
-  // savestated disc_inside matches our IsDiscInside(). This
-  // won't catch cases of having the wrong disc inserted, though.
-  // TODO: Check the game ID, disc number, revision?
-  if (disc_inside != IsDiscInside())
-  {
-    if (disc_inside)
-    {
-      PanicAlertT("An inserted disc was expected but not found.");
-    }
-    else
-    {
-      s_inserted_volume.reset();
-      FileMonitor::SetFileSystem(nullptr);
-    }
-  }
 }

 static size_t ProcessDTKSamples(std::vector<s16>* temp_pcm, const std::vector<u8>& audio_data)
@ -452,27 +428,17 @@ void Reset()
 void Shutdown()
 {
  DVDThread::Stop();
-  s_inserted_volume.reset();
-  FileMonitor::SetFileSystem(nullptr);
 }

 void SetDisc(std::unique_ptr<DiscIO::IVolume> disc)
 {
-  DVDThread::WaitUntilIdle();
-  s_inserted_volume = std::move(disc);
-  FileMonitor::SetFileSystem(s_inserted_volume.get());
+  DVDThread::SetDisc(std::move(disc));
  SetLidOpen();
 }

-const DiscIO::IVolume& GetVolume()
-{
-  _assert_(IsDiscInside());
-  return *s_inserted_volume;
-}
-
 bool IsDiscInside()
 {
-  return s_inserted_volume != nullptr;
+  return DVDThread::HasDisc();
 }

 // Take care of all logic of "swapping discs"
@ -481,10 +447,7 @@ bool IsDiscInside()
 // that the userdata string exists when called
 static void EjectDiscCallback(u64 userdata, s64 cyclesLate)
 {
-  DVDThread::WaitUntilIdle();
-  s_inserted_volume.reset();
-  FileMonitor::SetFileSystem(s_inserted_volume.get());
-  SetLidOpen();
+  SetDisc(nullptr);
 }

 static void InsertDiscCallback(u64 userdata, s64 cyclesLate)
@ -535,10 +498,7 @@ void SetLidOpen()

 bool ChangePartition(u64 offset)
 {
-  DVDThread::WaitUntilIdle();
-  const bool success = s_inserted_volume->ChangePartition(offset);
-  FileMonitor::SetFileSystem(s_inserted_volume.get());
-  return success;
+  return DVDThread::ChangePartition(offset);
 }

 void RegisterMMIO(MMIO::Mapping* mmio, u32 base)
@ -1150,7 +1110,7 @@ void ScheduleReads(u64 offset, u32 length, bool decrypt, u32 output_address, Rep

  const u64 current_time = CoreTiming::GetTicks();
  const u32 ticks_per_second = SystemTimers::GetTicksPerSecond();
-  const bool wii_disc = s_inserted_volume->GetVolumeType() == DiscIO::Platform::WII_DISC;
+  const bool wii_disc = DVDThread::GetDiscType() == DiscIO::Platform::WII_DISC;

  // Where the DVD read head is (usually parked at the end of the buffer,
  // unless we've interrupted it mid-buffer-read).
@ -1166,7 +1126,7 @@ void ScheduleReads(u64 offset, u32 length, bool decrypt, u32 output_address, Rep
  // It's rounded to a whole ECC block and never uses Wii partition addressing.
  u64 dvd_offset = offset;
  if (decrypt)
-    dvd_offset = s_inserted_volume->PartitionOffsetToRawOffset(offset);
+    dvd_offset = DVDThread::PartitionOffsetToRawOffset(offset);
  dvd_offset = Common::AlignDown(dvd_offset, DVD_ECC_BLOCK_SIZE);

  if (SConfig::GetInstance().bFastDiscSpeed)
--- a/Source/Core/Core/HW/DVD/DVDInterface.h
+++ b/Source/Core/Core/HW/DVD/DVDInterface.h
@ -108,9 +108,7 @@ void DoState(PointerWrap& p);

 void RegisterMMIO(MMIO::Mapping* mmio, u32 base);

-// Disc access (don't call GetVolume unless you know that IsDiscInside() == true)
 void SetDisc(std::unique_ptr<DiscIO::IVolume> disc);
-const DiscIO::IVolume& GetVolume();
 bool IsDiscInside();
 void ChangeDiscAsHost(const std::string& new_path);  // Can only be called by the host thread
 void ChangeDiscAsCPU(const std::string& new_path);   // Can only be called by the CPU thread
--- a/Source/Core/Core/HW/DVD/DVDThread.cpp
+++ b/Source/Core/Core/HW/DVD/DVDThread.cpp
@ -4,6 +4,7 @@

 #include <cinttypes>
 #include <map>
+#include <memory>
 #include <mutex>
 #include <thread>
 #include <utility>
@ -19,6 +20,7 @@
 #include "Common/Thread.h"
 #include "Common/Timer.h"

+#include "Core/ConfigManager.h"
 #include "Core/Core.h"
 #include "Core/CoreTiming.h"
 #include "Core/HW/DVD/DVDInterface.h"
@ -26,7 +28,9 @@
 #include "Core/HW/DVD/FileMonitor.h"
 #include "Core/HW/Memmap.h"
 #include "Core/HW/SystemTimers.h"
+#include "Core/IOS/ES/Formats.h"

+#include "DiscIO/Enums.h"
 #include "DiscIO/Volume.h"

 namespace DVDThread
@ -61,6 +65,7 @@ static void StartDVDThread();
 static void StopDVDThread();

 static void DVDThread();
+static void WaitUntilIdle();

 static void StartReadInternal(bool copy_to_ram, u32 output_address, u64 dvd_offset, u32 length,
                              bool decrypt, DVDInterface::ReplyType reply_type,
@ -80,6 +85,8 @@ static Common::FifoQueue<ReadRequest, false> s_request_queue;
 static Common::FifoQueue<ReadResult, false> s_result_queue;
 static std::map<u64, ReadResult> s_result_map;

+static std::unique_ptr<DiscIO::IVolume> s_disc;
+
 void Start()
 {
  s_finish_read = CoreTiming::RegisterEvent("FinishReadDVDThread", FinishRead);
@ -106,6 +113,8 @@ static void StartDVDThread()
 void Stop()
 {
  StopDVDThread();
+  s_disc.reset();
+  FileMonitor::SetFileSystem(nullptr);
 }

 static void StopDVDThread()
@ -123,24 +132,42 @@ static void StopDVDThread()

 void DoState(PointerWrap& p)
 {
-  // By waiting for the DVD thread to be done working, we ensure that
-  // there are no pending requests. The DVD thread won't be touching
-  // s_result_queue, and everything we need to save will be in either
-  // s_result_queue or s_result_map (other than s_next_id).
+  // By waiting for the DVD thread to be done working, we ensure
+  // that s_request_queue will be empty and that the DVD thread
+  // won't be touching anything while this function runs.
  WaitUntilIdle();

-  // Move everything from s_result_queue to s_result_map because
+  // Move all results from s_result_queue to s_result_map because
  // PointerWrap::Do supports std::map but not Common::FifoQueue.
  // This won't affect the behavior of FinishRead.
  ReadResult result;
  while (s_result_queue.Pop(result))
    s_result_map.emplace(result.first.id, std::move(result));

-  // Everything is now in s_result_map, so we simply savestate that.
-  // We also savestate s_next_id to avoid ID collisions.
+  // Both queues are now empty, so we don't need to savestate them.
  p.Do(s_result_map);
  p.Do(s_next_id);

+  // s_disc isn't savestated (because it points to files on the
+  // local system). Instead, we check that the status of the disc
+  // is the same as when the savestate was made. This won't catch
+  // cases of having the wrong disc inserted, though.
+  // TODO: Check the game ID, disc number, revision?
+  bool had_disc = HasDisc();
+  p.Do(had_disc);
+  if (had_disc != HasDisc())
+  {
+    if (had_disc)
+    {
+      PanicAlertT("An inserted disc was expected but not found.");
+    }
+    else
+    {
+      s_disc.reset();
+      FileMonitor::SetFileSystem(nullptr);
+    }
+  }
+
  // TODO: Savestates can be smaller if the buffers of results aren't saved,
  // but instead get re-read from the disc when loading the savestate.

@ -152,6 +179,82 @@ void DoState(PointerWrap& p)
  // was made. Handling that properly may be more effort than it's worth.
 }

+void SetDisc(std::unique_ptr<DiscIO::IVolume> disc)
+{
+  WaitUntilIdle();
+  s_disc = std::move(disc);
+  FileMonitor::SetFileSystem(s_disc.get());
+}
+
+bool HasDisc()
+{
+  return s_disc != nullptr;
+}
+
+u64 PartitionOffsetToRawOffset(u64 offset)
+{
+  // This is thread-safe as long as the partition currently isn't being changed,
+  // and that isn't supposed to be happening while running this function, because both
+  // this function and ChangePartition are only supposed to be called on the CPU thread.
+  _assert_(Core::IsCPUThread());
+  return s_disc->PartitionOffsetToRawOffset(offset);
+}
+
+DiscIO::Platform GetDiscType()
+{
+  // GetVolumeType is thread-safe, so calling WaitUntilIdle isn't necessary.
+  return s_disc->GetVolumeType();
+}
+
+IOS::ES::TMDReader GetTMD()
+{
+  WaitUntilIdle();
+  return s_disc->GetTMD();
+}
+
+IOS::ES::TicketReader GetTicket()
+{
+  WaitUntilIdle();
+  return s_disc->GetTicket();
+}
+
+bool ChangePartition(u64 offset)
+{
+  WaitUntilIdle();
+  const bool success = s_disc->ChangePartition(offset);
+  FileMonitor::SetFileSystem(s_disc.get());
+  return success;
+}
+
+bool UpdateRunningGameMetadata(u64 title_id)
+{
+  if (!s_disc)
+    return false;
+
+  WaitUntilIdle();
+
+  u64 volume_title_id;
+  if (!s_disc->GetTitleID(&volume_title_id))
+    return false;
+
+  if (volume_title_id != title_id)
+    return false;
+
+  SConfig::GetInstance().SetRunningGameMetadata(*s_disc);
+  return true;
+}
+
+bool UpdateRunningGameMetadata()
+{
+  if (!s_disc)
+    return false;
+
+  DVDThread::WaitUntilIdle();
+
+  SConfig::GetInstance().SetRunningGameMetadata(*s_disc);
+  return true;
+}
+
 void WaitUntilIdle()
 {
  _assert_(Core::IsCPUThread());
@ -281,8 +384,7 @@ static void DVDThread()
      FileMonitor::Log(request.dvd_offset, request.decrypt);

      std::vector<u8> buffer(request.length);
-      const DiscIO::IVolume& volume = DVDInterface::GetVolume();
-      if (!volume.Read(request.dvd_offset, request.length, buffer.data(), request.decrypt))
+      if (!s_disc->Read(request.dvd_offset, request.length, buffer.data(), request.decrypt))
        buffer.resize(0);

      request.realtime_done_us = Common::Timer::GetTimeUs();
--- a/Source/Core/Core/HW/DVD/DVDThread.h
+++ b/Source/Core/Core/HW/DVD/DVDThread.h
@ -4,6 +4,9 @@

 #pragma once

+#include <memory>
+#include <vector>
+
 #include "Common/CommonTypes.h"

 class PointerWrap;
@ -11,6 +14,19 @@ namespace DVDInterface
 {
 enum class ReplyType : u32;
 }
+namespace DiscIO
+{
+enum class Platform;
+class IVolume;
+}
+namespace IOS
+{
+namespace ES
+{
+class TMDReader;
+class TicketReader;
+}
+}

 namespace DVDThread
 {
@ -18,7 +34,21 @@ void Start();
 void Stop();
 void DoState(PointerWrap& p);

-void WaitUntilIdle();
+void SetDisc(std::unique_ptr<DiscIO::IVolume> disc);
+bool HasDisc();
+
+u64 PartitionOffsetToRawOffset(u64 offset);
+DiscIO::Platform GetDiscType();
+IOS::ES::TMDReader GetTMD();
+IOS::ES::TicketReader GetTicket();
+bool ChangePartition(u64 offset);
+// If a disc is inserted and its title ID is equal to the title_id argument, returns true and
+// calls SConfig::SetRunningGameMetadata(IVolume&). Otherwise, returns false.
+bool UpdateRunningGameMetadata(u64 title_id);
+// If a disc is inserted, returns true and calls
+// SConfig::SetRunningGameMetadata(IVolume&). Otherwise, returns false.
+bool UpdateRunningGameMetadata();
+
 void StartRead(u64 dvd_offset, u32 length, bool decrypt, DVDInterface::ReplyType reply_type,
               s64 ticks_until_completion);
 void StartReadToEmulatedRAM(u32 output_address, u64 dvd_offset, u32 length, bool decrypt,
--- a/Source/Core/Core/IOS/DI/DI.cpp
+++ b/Source/Core/Core/IOS/DI/DI.cpp
@ -12,6 +12,7 @@
 #include "Common/Logging/Log.h"
 #include "Common/MsgHandler.h"
 #include "Core/HW/DVD/DVDInterface.h"
+#include "Core/HW/DVD/DVDThread.h"
 #include "Core/HW/Memmap.h"
 #include "Core/IOS/DI/DI.h"
 #include "Core/IOS/ES/ES.h"
@ -106,10 +107,10 @@ IPCCommandResult DI::IOCtlV(const IOCtlVRequest& request)
    INFO_LOG(IOS_DI, "DVDLowOpenPartition: partition_offset 0x%016" PRIx64, partition_offset);

    // Read TMD to the buffer
-    const IOS::ES::TMDReader tmd = DVDInterface::GetVolume().GetTMD();
+    const IOS::ES::TMDReader tmd = DVDThread::GetTMD();
    const std::vector<u8> raw_tmd = tmd.GetRawTMD();
    Memory::CopyToEmu(request.io_vectors[0].address, raw_tmd.data(), raw_tmd.size());
-    ES::DIVerify(tmd, DVDInterface::GetVolume().GetTicket());
+    ES::DIVerify(tmd, DVDThread::GetTicket());

    return_value = 1;
    break;
--- a/Source/Core/Core/IOS/MIOS.cpp
+++ b/Source/Core/Core/IOS/MIOS.cpp
@ -120,13 +120,6 @@ static void ReinitHardware()
  SystemTimers::ChangePPCClock(SystemTimers::Mode::GC);
 }

-static void UpdateRunningGame()
-{
-  DVDThread::WaitUntilIdle();
-  SConfig::GetInstance().m_BootType = SConfig::BOOT_MIOS;
-  SConfig::GetInstance().SetRunningGameMetadata(DVDInterface::GetVolume());
-}
-
 constexpr u32 ADDRESS_INIT_SEMAPHORE = 0x30f8;

 bool Load()
@ -176,7 +169,8 @@ bool Load()

  Memory::Write_U32(0x00000000, ADDRESS_INIT_SEMAPHORE);
  NOTICE_LOG(IOS, "IPL ready.");
-  UpdateRunningGame();
+  SConfig::GetInstance().m_BootType = SConfig::BOOT_MIOS;
+  DVDThread::UpdateRunningGameMetadata();
  return true;
 }
 }  // namespace MIOS
--- a/Source/Core/Core/State.cpp
+++ b/Source/Core/Core/State.cpp
@ -71,7 +71,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 = 84;  // Last changed in PR 5354
+static const u32 STATE_VERSION = 85;  // Last changed in PR 4241

 // Maps savestate versions to Dolphin versions.
 // Versions after 42 don't need to be added to this list,