From 4970e589997c2f1bb3be05ce7bc7fe7a8b64044e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=97=B1=20PixelyIon?= Date: Mon, 21 Sep 2020 01:16:26 +0530 Subject: [PATCH] Address CR Comments (#102) --- app/src/main/cpp/skyline/common.h | 27 +++++++------ app/src/main/cpp/skyline/loader/loader.cpp | 4 +- .../am/controller/ISelfController.cpp | 1 - .../hosbinder/GraphicBufferProducer.cpp | 15 +++++-- .../hosbinder/GraphicBufferProducer.h | 22 +++++++++-- .../services/hosbinder/IHOSBinderDriver.h | 2 +- .../cpp/skyline/services/hosbinder/buffer.h | 12 ------ .../cpp/skyline/services/hosbinder/display.h | 39 ------------------- .../services/nvdrv/devices/nvdevice.cpp | 24 ++++++------ .../main/cpp/skyline/services/nvdrv/driver.h | 4 +- .../services/visrv/IDisplayService.cpp | 1 - .../services/visrv/IManagerDisplayService.cpp | 1 - 12 files changed, 63 insertions(+), 89 deletions(-) delete mode 100644 app/src/main/cpp/skyline/services/hosbinder/buffer.h delete mode 100644 app/src/main/cpp/skyline/services/hosbinder/display.h diff --git a/app/src/main/cpp/skyline/common.h b/app/src/main/cpp/skyline/common.h index 92524a1f..6ce0e4b0 100644 --- a/app/src/main/cpp/skyline/common.h +++ b/app/src/main/cpp/skyline/common.h @@ -115,7 +115,6 @@ namespace skyline { */ template constexpr inline TypeVal AlignUp(TypeVal value, TypeMul multiple) { - static_assert(std::is_integral() && std::is_integral()); multiple--; return (value + multiple) & ~(multiple); } @@ -130,7 +129,6 @@ namespace skyline { */ template constexpr inline TypeVal AlignDown(TypeVal value, TypeMul multiple) { - static_assert(std::is_integral() && std::is_integral()); return value & ~(multiple - 1); } @@ -138,9 +136,14 @@ namespace skyline { * @param value The value to check for alignment * @param multiple The multiple to check alignment with * @return If the address is aligned with the multiple + * @note The multiple must be divisible by 2 */ - constexpr inline bool IsAligned(u64 value, u64 multiple) { - return !(value & (multiple - 1U)); + template + constexpr inline bool IsAligned(TypeVal value, TypeMul multiple) { + if ((multiple & (multiple - 1)) == 0) + return !(value & (multiple - 1U)); + else + return (value % multiple) == 0; } /** @@ -203,17 +206,17 @@ namespace skyline { } template - constexpr Out& As(const std::span &span) { - if (span.size_bytes() < sizeof(Out)) - throw exception("Span size less than Out type size"); - return *reinterpret_cast(span.data()); + constexpr Out &As(std::span span) { + if (IsAligned(span.size_bytes(), sizeof(Out))) + return *reinterpret_cast(span.data()); + throw exception("Span size not aligned with Out type size (0x{:X}/0x{:X})", span.size_bytes(), sizeof(Out)); } template - constexpr std::span AsSpan(const std::span &span) { - if (span.size_bytes() < sizeof(Out)) - throw exception("Span size less than Out type size"); - return std::span(reinterpret_cast(span.data()), span.size_bytes() / sizeof(Out)); + constexpr std::span AsSpan(std::span span) { + if (IsAligned(span.size_bytes(), sizeof(Out))) + return std::span(reinterpret_cast(span.data()), span.size_bytes() / sizeof(Out)); + throw exception("Span size not aligned with Out type size (0x{:X}/0x{:X})", span.size_bytes(), sizeof(Out)); } } diff --git a/app/src/main/cpp/skyline/loader/loader.cpp b/app/src/main/cpp/skyline/loader/loader.cpp index c1d73705..7ba63aac 100644 --- a/app/src/main/cpp/skyline/loader/loader.cpp +++ b/app/src/main/cpp/skyline/loader/loader.cpp @@ -14,10 +14,10 @@ namespace skyline::loader { u64 roSize = executable.ro.contents.size(); u64 dataSize = executable.data.contents.size() + executable.bssSize; - if (!util::IsAligned(textSize, PAGE_SIZE) || !util::IsAligned(roSize, PAGE_SIZE) || !util::IsAligned(dataSize, PAGE_SIZE)) + if (!util::PageAligned(textSize) || !util::PageAligned(roSize) || !util::PageAligned(dataSize)) throw exception("LoadProcessData: Sections are not aligned with page size: 0x{:X}, 0x{:X}, 0x{:X}", textSize, roSize, dataSize); - if (!util::IsAligned(executable.text.offset, PAGE_SIZE) || !util::IsAligned(executable.ro.offset, PAGE_SIZE) || !util::IsAligned(executable.data.offset, PAGE_SIZE)) + if (!util::PageAligned(executable.text.offset) || !util::PageAligned(executable.ro.offset) || !util::PageAligned(executable.data.offset)) throw exception("LoadProcessData: Section offsets are not aligned with page size: 0x{:X}, 0x{:X}, 0x{:X}", executable.text.offset, executable.ro.offset, executable.data.offset); // The data section will always be the last section in memory, so put the patch section after it diff --git a/app/src/main/cpp/skyline/services/am/controller/ISelfController.cpp b/app/src/main/cpp/skyline/services/am/controller/ISelfController.cpp index 8d5192f0..d96beb6a 100644 --- a/app/src/main/cpp/skyline/services/am/controller/ISelfController.cpp +++ b/app/src/main/cpp/skyline/services/am/controller/ISelfController.cpp @@ -3,7 +3,6 @@ #include #include -#include #include "ISelfController.h" namespace skyline::service::am { diff --git a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp index 3c579ba7..3b075966 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp +++ b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp @@ -8,7 +8,6 @@ #include #include #include "GraphicBufferProducer.h" -#include "display.h" namespace skyline::service::hosbinder { Buffer::Buffer(const GbpBuffer &gbpBuffer, const std::shared_ptr &texture) : gbpBuffer(gbpBuffer), texture(texture) {} @@ -190,11 +189,21 @@ namespace skyline::service::hosbinder { } } + /** + * @brief A mapping from a display's name to it's displayType entry + */ + static frz::unordered_map DisplayTypeMap{ + {"Default", DisplayId::Default}, + {"External", DisplayId::External}, + {"Edid", DisplayId::Edid}, + {"Internal", DisplayId::Internal}, + {"Null", DisplayId::Null}, + }; + void GraphicBufferProducer::SetDisplay(const std::string &name) { try { - const auto type = DisplayTypeMap.at(name); if (displayId == DisplayId::Null) - displayId = type; + displayId = DisplayTypeMap.at(frz::string(name.data(), name.size())); else throw exception("Trying to change display type from non-null type"); } catch (const std::out_of_range &) { diff --git a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h index 337bc4be..17f5b33c 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h +++ b/app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h @@ -5,7 +5,6 @@ #include #include -#include "display.h" namespace skyline::service::hosbinder { /** @@ -51,7 +50,24 @@ namespace skyline::service::hosbinder { }; /** - * @brief nvnflinger:dispdrv or nns::hosbinder::IHOSBinderDriver is responsible for writing buffers to the display + * @brief An enumeration of all the possible display IDs (https://switchbrew.org/wiki/Display_services#DisplayName) + */ + enum class DisplayId : u64 { + Default, //!< Refers to the default display used by most applications + External, //!< Refers to an external display + Edid, //!< Refers to an external display with EDID capabilities + Internal, //!< Refers to the the internal display + Null, //!< Refers to the null display which is used for discarding data + }; + + enum class LayerStatus { + Uninitialized, //!< The layer hasn't been initialized + Stray, //!< The layer has been initialized as a stray layer + Managed, //!< The layer has been initialized as a managed layer + }; + + /** + * @brief IGraphicBufferProducer is responsible for presenting buffers to the display as well as compositing and frame pacing (https://android.googlesource.com/platform/frameworks/native/+/8dc5539/libs/gui/IGraphicBufferProducer.cpp) */ class GraphicBufferProducer { private: @@ -110,7 +126,7 @@ namespace skyline::service::hosbinder { Disconnect = 11, //!< https://android.googlesource.com/platform/frameworks/native/+/8dc5539/libs/gui/IGraphicBufferProducer.cpp#396 SetSidebandStream = 12, //!< https://android.googlesource.com/platform/frameworks/native/+/8dc5539/libs/gui/IGraphicBufferProducer.cpp#403 AllocateBuffers = 13, //!< https://android.googlesource.com/platform/frameworks/native/+/8dc5539/libs/gui/IGraphicBufferProducer.cpp#413 - SetPreallocatedBuffer = 14, //!< No source on this but it's used to set a existing buffer according to libtransistor and libNX + SetPreallocatedBuffer = 14, //!< No source on this but it's used to set a existing buffer according to libtransistor and libnx }; GraphicBufferProducer(const DeviceState &state); diff --git a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h index 441dfe8a..09468afc 100644 --- a/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h +++ b/app/src/main/cpp/skyline/services/hosbinder/IHOSBinderDriver.h @@ -10,7 +10,7 @@ namespace skyline::service::hosbinder { class GraphicBufferProducer; /** - * @brief nvnflinger:dispdrv or nns::hosbinder::IHOSBinderDriver is responsible for writing buffers to the display + * @brief nvnflinger:dispdrv or nns::hosbinder::IHOSBinderDriver is a translation layer between Android Binder IPC and HOS IPC to communicate with the Android display stack */ class IHOSBinderDriver : public BaseService { private: diff --git a/app/src/main/cpp/skyline/services/hosbinder/buffer.h b/app/src/main/cpp/skyline/services/hosbinder/buffer.h deleted file mode 100644 index bdc6d858..00000000 --- a/app/src/main/cpp/skyline/services/hosbinder/buffer.h +++ /dev/null @@ -1,12 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 -// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/) - -#pragma once - -#include -#include -#include -#include - -namespace skyline::service::hosbinder { -} diff --git a/app/src/main/cpp/skyline/services/hosbinder/display.h b/app/src/main/cpp/skyline/services/hosbinder/display.h deleted file mode 100644 index 795da260..00000000 --- a/app/src/main/cpp/skyline/services/hosbinder/display.h +++ /dev/null @@ -1,39 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 -// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/) - -#pragma once - -#include - -namespace skyline::service::hosbinder { - /** - * @brief An enumeration of all the possible display IDs (https://switchbrew.org/wiki/Display_services#DisplayName) - */ - enum class DisplayId : u64 { - Default, //!< Refers to the default display used by most applications - External, //!< Refers to an external display - Edid, //!< Refers to an external display with EDID capabilities - Internal, //!< Refers to the the internal display - Null, //!< Refers to the null display which is used for discarding data - }; - - /** - * @brief A mapping from a display's name to it's displayType entry - */ - static const std::unordered_map DisplayTypeMap{ - {"Default", DisplayId::Default}, - {"External", DisplayId::External}, - {"Edid", DisplayId::Edid}, - {"Internal", DisplayId::Internal}, - {"Null", DisplayId::Null}, - }; - - /** - * @brief The status of a display layer - */ - enum class LayerStatus { - Uninitialized, //!< The layer hasn't been initialized - Stray, //!< The layer has been initialized as a stray layer - Managed, //!< The layer has been initialized as a managed layer - }; -} diff --git a/app/src/main/cpp/skyline/services/nvdrv/devices/nvdevice.cpp b/app/src/main/cpp/skyline/services/nvdrv/devices/nvdevice.cpp index 038acc8f..1450c315 100644 --- a/app/src/main/cpp/skyline/services/nvdrv/devices/nvdevice.cpp +++ b/app/src/main/cpp/skyline/services/nvdrv/devices/nvdevice.cpp @@ -18,19 +18,20 @@ namespace skyline::service::nvdrv::device { } NvStatus NvDevice::HandleIoctl(u32 cmd, IoctlType type, std::span buffer, std::span inlineBuffer) { + std::string_view typeString{[type] { + switch (type) { + case IoctlType::Ioctl: + return "IOCTL"; + case IoctlType::Ioctl2: + return "IOCTL2"; + case IoctlType::Ioctl3: + return "IOCTL3"; + } + }()}; + std::pair, std::span)>, std::string_view> function; try { function = GetIoctlFunction(cmd); - std::string_view typeString{[type] { - switch (type) { - case IoctlType::Ioctl: - return "IOCTL"; - case IoctlType::Ioctl2: - return "IOCTL2"; - case IoctlType::Ioctl3: - return "IOCTL3"; - } - }()}; state.logger->Debug("{} @ {}: {}", typeString, GetName(), function.second); } catch (std::out_of_range &) { state.logger->Warn("Cannot find IOCTL for device '{}': 0x{:X}", GetName(), cmd); @@ -39,8 +40,7 @@ namespace skyline::service::nvdrv::device { try { return function.first(type, buffer, inlineBuffer); } catch (const std::exception &e) { - throw exception("{} (Device: {})", e.what(), GetName()); + throw exception("{} ({} @ {}: {})", e.what(), typeString, GetName(), function.second); } - exit(0); } } diff --git a/app/src/main/cpp/skyline/services/nvdrv/driver.h b/app/src/main/cpp/skyline/services/nvdrv/driver.h index 850b0ae8..880e6eb6 100644 --- a/app/src/main/cpp/skyline/services/nvdrv/driver.h +++ b/app/src/main/cpp/skyline/services/nvdrv/driver.h @@ -29,8 +29,8 @@ namespace skyline::service::nvdrv { class Driver { private: const DeviceState &state; - std::vector> devices; //!< A map from an FD to a shared pointer to it's NvDevice object - u32 fdIndex{}; //!< The index of a file descriptor + std::vector> devices; //!< A vector of shared pointers to NvDevice object that correspond to FDs + u32 fdIndex{}; //!< The next file descriptor to assign public: NvHostSyncpoint hostSyncpoint; diff --git a/app/src/main/cpp/skyline/services/visrv/IDisplayService.cpp b/app/src/main/cpp/skyline/services/visrv/IDisplayService.cpp index 88d360e8..ca287197 100644 --- a/app/src/main/cpp/skyline/services/visrv/IDisplayService.cpp +++ b/app/src/main/cpp/skyline/services/visrv/IDisplayService.cpp @@ -3,7 +3,6 @@ #include #include -#include #include "IDisplayService.h" namespace skyline::service::visrv { diff --git a/app/src/main/cpp/skyline/services/visrv/IManagerDisplayService.cpp b/app/src/main/cpp/skyline/services/visrv/IManagerDisplayService.cpp index ff1f7ab9..919c97ac 100644 --- a/app/src/main/cpp/skyline/services/visrv/IManagerDisplayService.cpp +++ b/app/src/main/cpp/skyline/services/visrv/IManagerDisplayService.cpp @@ -3,7 +3,6 @@ #include #include -#include #include "IManagerDisplayService.h" namespace skyline::service::visrv {