Details

Reviewers

Brecht Van Lommel (brecht)
William Leeson (leesonw)

Maniphest Tasks

T92212: Cycles Metal device

Commits

rC27052fbbbabd: Metal host-side code
rB9558fa519603: Cycles: Metal host-side code

Summary

This patch adds the Metal host-side code:

Add all core host-side Metal backend files (device_impl, queue, etc)
Add MetalRT BVH setup files
Integrate with Cycles device enumeration code
Revive path_source_replace_includes in util/path (required for MSL compilation)

This patch also includes a couple of small kernel-side fixes:

Add an implementation of lgammaf for Metal Nemes, Gergő (2010), "New asymptotic expansion for the Gamma function", Archiv der Mathematik
include "work_stealing.h" inside the Metal context class because it accesses state now

Ref T92212

Diff Detail

Repository: rB Blender

Event Timeline

Michael Jones (michael_jones) requested review of this revision.Nov 30 2021, 12:32 PM

Michael Jones (michael_jones) created this revision.

Harbormaster completed remote builds in B19094: Diff 45481.Nov 30 2021, 12:32 PM

Eugen (mycoconut) awarded a token.Nov 30 2021, 12:47 PM

Marnix Berger (MXBBB) awarded a token.Nov 30 2021, 1:05 PM

Wayne (osakawayne) awarded a token.Nov 30 2021, 3:13 PM

Ari Sea (PhlixFer) awarded a token.Nov 30 2021, 3:59 PM

Found a minor thing while checking code.

intern/cycles/blender/addon/version_update.py
92 ↗	(On Diff #45481)	That's not needed.

Christian Baars (Staars) awarded a token.Nov 30 2021, 5:25 PM

William Reynish (billreynish) awarded a token.Nov 30 2021, 8:32 PM

Ethan (zxleeethan) awarded a token.Dec 1 2021, 10:20 AM

Remove bogus legacy fixup

Harbormaster completed remote builds in B19122: Diff 45523.Dec 1 2021, 1:42 PM

Michael Jones (michael_jones) marked an inline comment as done.Dec 1 2021, 1:45 PM

Seems generally fine, but I have only looked at the integration with the rest of the code, not the implementation of the BVH and device yet.

intern/cycles/bvh/CMakeLists.txt
62	It was not immediately obvious to me why this was used rather than using the `.mm` file extension. But I guess it is because the header file contains objective c++ code and is included in `.cpp` files shared with other platforms. I think I would prefer to use `.mm` extensions anyway and then move the class declaration into the source file, with a `BVH *bvh_metal_create(..);` in the header. And similar for the device code. Mainly for consistency with the rest of Blender, and to avoid potential issues with syntax highlighting or code formatting.
intern/cycles/bvh/metal.cpp
33 ↗	(On Diff #45523)	Remove the debugging printf, or change it to VLOG if it's useful for logging.
60 ↗	(On Diff #45523)	Can we change this to `if (!@available(..)) { return false; }` to avoid the extra indentation for the rest of the code, here and in other functions?
intern/cycles/device/CMakeLists.txt
188	Could you move this `FIND_LIBRARY` into `cmake/external_libs.cmake`? For consistency with CUDA and HIP.
intern/cycles/device/metal/device.h
35–38	I'd use a `METAL_GPU` prefix, or make it an `enum class` instead and drop the `GPU_` prefix.
intern/cycles/device/metal/queue.cpp
47 ↗	(On Diff #45523)	Change `com.blender` to `com.cycles` since Cycles can be integrated in other applications than Blender.
intern/cycles/device/metal/queue.h
29	For debugging logs, you can use `VLOG(4)`.
intern/cycles/util/math.h
723	Add `ccl_device_inline` even if it is unnecessary for Metal.

This revision now requires changes to proceed.Dec 1 2021, 4:52 PM

xueke pei (yuzukyo) awarded a token.Dec 1 2021, 6:41 PM

Nikita Sirgienko (nsirgien) added a subscriber: Nikita Sirgienko (nsirgien).Dec 2 2021, 11:09 AM

Use .mm file extension and other minor tweaks:
- Use enum class for MetalGPUVendor
- Use ccl_device_inline for lgammaf function
- Use VLOG instead of printf
- Move Metal library search to external_libs.cmake

Harbormaster completed remote builds in B19174: Diff 45601.Dec 2 2021, 10:15 PM

Ray Molenkamp (LazyDodo) requested changes to this revision.Dec 2 2021, 10:17 PM

Ray Molenkamp (LazyDodo) added a subscriber: Ray Molenkamp (LazyDodo).

Ray Molenkamp (LazyDodo) added inline comments.

intern/cycles/cmake/external_libs.cmake
563	disabling whatnow? :)

This revision now requires changes to proceed.Dec 2 2021, 10:17 PM

Fix copy-pasta

Harbormaster completed remote builds in B19175: Diff 45602.Dec 2 2021, 10:29 PM

Ray Molenkamp (LazyDodo) removed a reviewer: Ray Molenkamp (LazyDodo).Dec 2 2021, 10:31 PM

Michael Jones (michael_jones) marked 7 inline comments as done.Dec 2 2021, 10:52 PM

Michael Jones (michael_jones) added inline comments.

intern/cycles/bvh/CMakeLists.txt
62	I have changed from .cpp to .mm files and moved some internal stuff out of metal/device.h. I still need to try that `bvh_metal_create` suggestion which will probably involve a slight change to the encapsulation approach.
intern/cycles/bvh/metal.cpp
60 ↗	(On Diff #45523)	Unfortunately the `if (@available(...))` checks require the guarded code to be enclosed in that scope to suppress compiler warnings. We will be requiring at least os version 12.0 as a min version, so we may be able to relax these checks in future with appropriate global min versioning elsewhere, but this is still in flux.
intern/cycles/cmake/external_libs.cmake
563	Ugh, thanks! :D
intern/cycles/device/metal/device.h
35–38	I have moved some of these internal types into metal/util.h since they aren't needed for the API, and I changed to `enum class` as per your suggestion.
intern/cycles/device/metal/queue.cpp
47 ↗	(On Diff #45523)	Amended (now in queue.mm, not queue.cpp)
intern/cycles/device/metal/queue.h
29	Not sure if this is what you had in mind, or whether you meant to use VLOG(4) directly instead of metal_printf, but having a specialised metal_printf to hand is useful for isolating and intercepting logging.

I read more closely through the implementation now, and couldn't spot bugs. There's of course some work-in-progress things in there, but this seems ready to land to continue development in master.

I also tested this on a Macbook Air M1, and basic final and viewport rendering worked for me.

Only waiting for the implementation of bvh_metal_create now I think.

intern/cycles/bvh/metal.cpp
60 ↗	(On Diff #45523)	Ok, no big deal.
intern/cycles/device/metal/queue.h
29	This is fine. The main thing is not to print debugging info to the console by default, which this accomplishes.

This revision now requires changes to proceed.Dec 3 2021, 2:26 PM

Philipp Remy (philippremy) awarded a token.Dec 3 2021, 3:20 PM

Move BVHMetal from "bvh/metal.h" to "device/metal/bvh.h" to stop Objective-C spilling into headers
Use METAL_GPU_ enum prefix to avoid clash with external #define INTEL ...

Harbormaster completed remote builds in B19184: Diff 45613.Dec 3 2021, 5:35 PM

In D13423#355547, @Brecht Van Lommel (brecht) wrote:

I read more closely through the implementation now, and couldn't spot bugs. There's of course some work-in-progress things in there, but this seems ready to land to continue development in master.

I also tested this on a Macbook Air M1, and basic final and viewport rendering worked for me.

I'm really pleased to hear that you were able to get this running :)

Only waiting for the implementation of bvh_metal_create now I think.

Moving the BVHMetal internals into the device/metal folder proved to be an easy way to stop the Objective-C overspill.

Running make format

Harbormaster completed remote builds in B19185: Diff 45614.Dec 3 2021, 5:47 PM

Stefan Werner (swerner) added a subscriber: Stefan Werner (swerner).Dec 6 2021, 10:06 AM

Steve Hanff (Tiwaz) awarded a token.Dec 6 2021, 2:56 PM

Steve Hanff (Tiwaz) added a subscriber: Steve Hanff (Tiwaz).

Prefix Metal entrypoints with cycles_metal

Harbormaster completed remote builds in B19266: Diff 45758.Dec 7 2021, 2:03 PM

There is a build error on Windows / Linux trying to build .mm files, this fixes it:

diff --git a/intern/cycles/device/CMakeLists.txt b/intern/cycles/device/CMakeLists.txt
index a900802..7f1e9ff 100644
--- a/intern/cycles/device/CMakeLists.txt
+++ b/intern/cycles/device/CMakeLists.txt
@@ -43,7 +43,7 @@ if(WITH_CYCLES_DEVICE_HIP AND WITH_HIP_DYNLOAD)
   add_definitions(-DWITH_HIP_DYNLOAD)
 endif()
 
-set(SRC
+set(SRC_BASE
   device.cpp
   denoise.cpp
   graphics_interop.cpp
@@ -138,6 +138,17 @@ set(SRC_HEADERS
   queue.h
 )
 
+set(SRC
+  ${SRC_BASE}
+  ${SRC_CPU}
+  ${SRC_CUDA}
+  ${SRC_HIP}
+  ${SRC_DUMMY}
+  ${SRC_MULTI}
+  ${SRC_OPTIX}
+  ${SRC_HEADERS}
+)
+
 set(LIB
   cycles_kernel
   cycles_util
@@ -178,6 +189,9 @@ if(WITH_CYCLES_DEVICE_METAL)
     ${METAL_LIBRARY}
   )
   add_definitions(-DWITH_METAL)
+  list(APPEND SRC
+    ${SRC_METAL}
+  )
 endif()
 
 if(WITH_OPENIMAGEDENOISE)
@@ -189,17 +203,7 @@ endif()
 include_directories(${INC})
 include_directories(SYSTEM ${INC_SYS})
 
-cycles_add_library(cycles_device "${LIB}"
-  ${SRC}
-  ${SRC_CPU}
-  ${SRC_CUDA}
-  ${SRC_HIP}
-  ${SRC_DUMMY}
-  ${SRC_MULTI}
-  ${SRC_METAL}
-  ${SRC_OPTIX}
-  ${SRC_HEADERS}
-)
+cycles_add_library(cycles_device "${LIB}" ${SRC})
 
 source_group("cpu" FILES ${SRC_CPU})
 source_group("cuda" FILES ${SRC_CUDA})

Other than that it looks good to commit.

This revision now requires changes to proceed.Dec 7 2021, 3:15 PM

Fix build error on Windows / Linux trying to build .mm file

Harbormaster completed remote builds in B19273: Diff 45768.Dec 7 2021, 4:12 PM

Brecht Van Lommel (brecht) accepted this revision.Dec 7 2021, 4:35 PM

This revision is now accepted and ready to land.Dec 7 2021, 4:35 PM

Closed by commit rB9558fa519603: Cycles: Metal host-side code (authored by Michael Jones (michael_jones)). · Explain WhyDec 7 2021, 4:52 PM

This revision was automatically updated to reflect the committed changes.

Michael Jones (michael_jones) added a commit: rB9558fa519603: Cycles: Metal host-side code.

Anton Raves (Memento) added a subscriber: Anton Raves (Memento).Dec 7 2021, 6:51 PM

Brecht Van Lommel (brecht) added a commit: rC27052fbbbabd: Metal host-side code.Jan 27 2022, 11:37 PM

Cycles: Metal host-side code
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 45772

intern/cycles/blender/CMakeLists.txt

intern/cycles/blender/addon/engine.py

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/device.cpp

intern/cycles/blender/python.cpp

intern/cycles/bvh/CMakeLists.txt

intern/cycles/bvh/bvh.cpp

intern/cycles/bvh/metal.h

intern/cycles/bvh/metal.mm

intern/cycles/cmake/external_libs.cmake

intern/cycles/device/CMakeLists.txt

intern/cycles/device/device.h

intern/cycles/device/device.cpp

intern/cycles/device/memory.h

intern/cycles/device/metal/bvh.h

intern/cycles/device/metal/bvh.mm

intern/cycles/device/metal/device.h

intern/cycles/device/metal/device.mm

intern/cycles/device/metal/device_impl.h

intern/cycles/device/metal/device_impl.mm

intern/cycles/device/metal/kernel.h

intern/cycles/device/metal/kernel.mm

intern/cycles/device/metal/queue.h

intern/cycles/device/metal/queue.mm

intern/cycles/device/metal/util.h

intern/cycles/device/metal/util.mm

intern/cycles/device/multi/device.cpp

intern/cycles/kernel/device/gpu/kernel.h

intern/cycles/kernel/device/metal/compat.h

intern/cycles/kernel/device/metal/kernel.metal

intern/cycles/util/math.h

intern/cycles/util/path.h

intern/cycles/util/path.cpp

Cycles: Metal host-side codeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 45772

intern/cycles/blender/CMakeLists.txt

intern/cycles/blender/addon/engine.py

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/device.cpp

intern/cycles/blender/python.cpp

intern/cycles/bvh/CMakeLists.txt

intern/cycles/bvh/bvh.cpp

intern/cycles/bvh/metal.h

intern/cycles/bvh/metal.mm

intern/cycles/cmake/external_libs.cmake

intern/cycles/device/CMakeLists.txt

intern/cycles/device/device.h

intern/cycles/device/device.cpp

intern/cycles/device/memory.h

intern/cycles/device/metal/bvh.h

intern/cycles/device/metal/bvh.mm

intern/cycles/device/metal/device.h

intern/cycles/device/metal/device.mm

intern/cycles/device/metal/device_impl.h

intern/cycles/device/metal/device_impl.mm

intern/cycles/device/metal/kernel.h

intern/cycles/device/metal/kernel.mm

intern/cycles/device/metal/queue.h

intern/cycles/device/metal/queue.mm

intern/cycles/device/metal/util.h

intern/cycles/device/metal/util.mm

intern/cycles/device/multi/device.cpp

intern/cycles/kernel/device/gpu/kernel.h

intern/cycles/kernel/device/metal/compat.h

intern/cycles/kernel/device/metal/kernel.metal

intern/cycles/util/math.h

intern/cycles/util/path.h

intern/cycles/util/path.cpp

Cycles: Metal host-side code
ClosedPublic
Actions

Revision Contents
Changeset List