Address CR Comments (#102)

app/src/main/cpp/skyline/common.h

@@ -115,7 +115,6 @@ namespace skyline {
          */
         template<typename TypeVal, typename TypeMul>
         constexpr inline TypeVal AlignUp(TypeVal value, TypeMul multiple) {
-            static_assert(std::is_integral<TypeVal>() && std::is_integral<TypeMul>());
             multiple--;
             return (value + multiple) & ~(multiple);
         }
@@ -130,7 +129,6 @@ namespace skyline {
          */
         template<typename TypeVal, typename TypeMul>
         constexpr inline TypeVal AlignDown(TypeVal value, TypeMul multiple) {
-            static_assert(std::is_integral<TypeVal>() && std::is_integral<TypeMul>());
             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) {
+        template<typename TypeVal, typename TypeMul>
-            return !(value & (multiple - 1U));
+        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<typename Out, typename In>
-        constexpr Out& As(const std::span<In> &span) {
+        constexpr Out &As(std::span<In> span) {
-            if (span.size_bytes() < sizeof(Out))
+            if (IsAligned(span.size_bytes(), sizeof(Out)))
-                throw exception("Span size less than Out type size");
+                return *reinterpret_cast<Out *>(span.data());
-            return *reinterpret_cast<Out*>(span.data());
+            throw exception("Span size not aligned with Out type size (0x{:X}/0x{:X})", span.size_bytes(), sizeof(Out));
         }
 
         template<typename Out, typename In>
-        constexpr std::span<Out> AsSpan(const std::span<In> &span) {
+        constexpr std::span<Out> AsSpan(std::span<In> span) {
-            if (span.size_bytes() < sizeof(Out))
+            if (IsAligned(span.size_bytes(), sizeof(Out)))
-                throw exception("Span size less than Out type size");
+                return std::span(reinterpret_cast<Out *>(span.data()), span.size_bytes() / sizeof(Out));
-            return std::span(reinterpret_cast<Out*>(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));
         }
     }
 

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

app/src/main/cpp/skyline/services/am/controller/ISelfController.cpp

@@ -3,7 +3,6 @@
 
 #include <os.h>
 #include <services/hosbinder/GraphicBufferProducer.h>
-#include <services/hosbinder/display.h>
 #include "ISelfController.h"
 
 namespace skyline::service::am {

app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.cpp

@@ -8,7 +8,6 @@
 #include <services/common/fence.h>
 #include <gpu/format.h>
 #include "GraphicBufferProducer.h"
-#include "display.h"
 
 namespace skyline::service::hosbinder {
     Buffer::Buffer(const GbpBuffer &gbpBuffer, const std::shared_ptr<gpu::PresentationTexture> &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<frz::string, DisplayId, 5> 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 &) {

app/src/main/cpp/skyline/services/hosbinder/GraphicBufferProducer.h

@@ -5,7 +5,6 @@
 
 #include <gpu.h>
 #include <services/common/parcel.h>
-#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);

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:

app/src/main/cpp/skyline/services/hosbinder/buffer.h

@@ -1,12 +0,0 @@
-// SPDX-License-Identifier: MPL-2.0
-// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)
-
-#pragma once
-
-#include <common.h>
-#include <services/common/parcel.h>
-#include <services/nvdrv/devices/nvmap.h>
-#include <gpu.h>
-
-namespace skyline::service::hosbinder {
-}

app/src/main/cpp/skyline/services/hosbinder/display.h

@@ -1,39 +0,0 @@
-// SPDX-License-Identifier: MPL-2.0
-// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)
-
-#pragma once
-
-#include <common.h>
-
-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<std::string, DisplayId> 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
-    };
-}

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<u8> buffer, std::span<u8> 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::function<NvStatus(IoctlType, std::span<u8>, std::span<u8>)>, 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);
     }
 }

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<std::shared_ptr<device::NvDevice>> devices; //!< A map from an FD to a shared pointer to it's NvDevice object
+        std::vector<std::shared_ptr<device::NvDevice>> devices; //!< A vector of shared pointers to NvDevice object that correspond to FDs
-        u32 fdIndex{}; //!< The index of a file descriptor
+        u32 fdIndex{}; //!< The next file descriptor to assign
 
       public:
         NvHostSyncpoint hostSyncpoint;

app/src/main/cpp/skyline/services/visrv/IDisplayService.cpp

@@ -3,7 +3,6 @@
 
 #include <os.h>
 #include <services/hosbinder/GraphicBufferProducer.h>
-#include <services/hosbinder/display.h>
 #include "IDisplayService.h"
 
 namespace skyline::service::visrv {

app/src/main/cpp/skyline/services/visrv/IManagerDisplayService.cpp

@@ -3,7 +3,6 @@
 
 #include <os.h>
 #include <services/hosbinder/GraphicBufferProducer.h>
-#include <services/hosbinder/display.h>
 #include "IManagerDisplayService.h"
 
 namespace skyline::service::visrv {