Paths

Table of Contentst

Differential D2338

Cycles: Refactor Device selection to allow individual GPU compute device selection
ClosedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Nov 6 2016, 3:09 AM.

Details

Reviewers

Brecht Van Lommel (brecht)

Group Reviewers

Cycles

Commits

rC42c5ef2fa662: Refactor Device selection to allow individual GPU compute device selection
rBf840239ee043: Cycles: Refactor Device selection to allow individual GPU compute device…
rBSdd921238d922: Cycles: Refactor Device selection to allow individual GPU compute device…
rCe3ea3bd82b08: Refactor Device selection to allow individual GPU compute device selection
rBdd921238d922: Cycles: Refactor Device selection to allow individual GPU compute device…

Summary

Previously, it was only possible to choose a single GPU or all of that type (CUDA or OpenCL).
Now, a toggle button is displayed for every device.
These settings are tied to the PCI Bus ID of the devices, so they're consistent across hardware addition and removal (but not when swapping/moving cards).

From the code perspective, the more important change is that now, the compute device properties are stored in the Addon preferences of the Cycles addon, instead of directly in the User Preferences.
This allows for a cleaner implementation, removing the Cycles C API functions that were called by the RNA code to specify the enum items.

Note that this change is neither backwards- nor forwards-compatible, but since it's only a User Preference no existing files are broken.

Also, I wasn't able to test this completely since I don't have access to any CUDA card right now - I tested with the CPU OpenCL devices of both the AMD and Intel framework, and at least for those two the MultiDevice code worked.

Diff Detail

Repository

rB Blender

Branch

cycles_device_select (branched from master)

Build Status

Buildable 263
Build 263: arc lint + arc unit

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 7743.Nov 6 2016, 3:09 AM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Refactor Device selection to allow individual GPU compute device selection.

Lukas Stockner (lukasstockner97) added a reviewer: Cycles.

Lukas Stockner (lukasstockner97) updated this object.

Aaron Carlisle (Blendify) awarded a token.Nov 6 2016, 4:22 AM

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).

Derek Barker (lordodin) awarded a token.Nov 6 2016, 8:05 AM

Hi, checked on:

Opensuse Leap 42.1 x86_64
Intel i5 3570K
RAM 16 GB
GTX 760 4 GB /Display card
GTX 670 2 GB
Driver 367.57
opencl_runtime_16.1_x64_sles_5.2.0.10002

With Cuda and OpenCL.
All work fine except error messages with enabled Auto Tile addon.

File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 177, in on_scene_update
    threads = get_threads(context, device)
  File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 208, in get_threads
    if engine_is_gpu(engine, device, userpref):
  File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 149, in engine_is_gpu
    return engine == 'CYCLES' and device == 'GPU' and userpref.system.compute_device_type != 'NONE'
AttributeError: 'UserPreferencesSystem' object has no attribute 'compute_device_type'

In D2338#54191, @Wolfgang Faehnle (mib2berlin) wrote:

All work fine except error messages with enabled Auto Tile addon.

File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 177, in on_scene_update
    threads = get_threads(context, device)
  File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 208, in get_threads
    if engine_is_gpu(engine, device, userpref):
  File "/home/xxx/blender_build/build/bin/2.78/scripts/addons/render_auto_tile_size.py", line 149, in engine_is_gpu
    return engine == 'CYCLES' and device == 'GPU' and userpref.system.compute_device_type != 'NONE'
AttributeError: 'UserPreferencesSystem' object has no attribute 'compute_device_type'

That is to be expected.

Ill test this on a 3 GPU machine where i now have to use CUDA_VISIBLE_DEVICES to get a 2 out of 3 selection.

Great to see this long standing limitation tackled!

For the UI, I'm thinking perhaps we should leave this out of the System tab in the user preferences. To make it easy to find/edit these settings we could add a button to the Render panel in the scene properties to pop up the Cycles user preferences to configure the devices?

We can add a generic operator for that in startup/bl_operators/wm.py:

class WM_OT_addon_userpref_show(Operator):
    "Show add-on user preferences"
    bl_idname = "wm.addon_userpref_show"
    bl_label = ""
    bl_options = {'INTERNAL'}

    module = StringProperty(
            name="Module",
            description="Module name of the add-on to expand",
            )

    def execute(self, context):
        import addon_utils

        module_name = self.module

        modules = addon_utils.modules(refresh=False)
        mod = addon_utils.addons_fake_modules.get(module_name)
        if mod is not None:
            info = addon_utils.module_bl_info(mod)
            info["show_expanded"] = True

            bpy.context.user_preferences.active_section = 'ADDONS'
            context.window_manager.addon_filter = 'All'
            context.window_manager.addon_search = info["name"]
            bpy.ops.screen.userpref_show('INVOKE_DEFAULT')

        return {'FINISHED'}

intern/cycles/blender/addon/properties.py
127–129	Use uppercase identifiers?
1190	I would prefer to break this line up into a `for` loop, is a bit long and difficult to mentally parse.
1195	Any specific reason to use `toggle=True`? Leaving it out to me seems more consistent with existing UI conventions.
intern/cycles/device/device.cpp
366	Building in debug mode I get `error: use of undeclared identifier 'persistent_ids`.
403	The purpose of hashing a string rather than using the original string is not clear to me.
intern/cycles/device/device.h
52–53	Do we need both an `id` and `persistent_id`? To me it seems like `id` could be the persistent id.
intern/cycles/device/opencl/opencl_util.cpp
669	Maybe use `sizeof(topology)` instead of `24*sizeof(cl_char)`.
681–682	Can we fall back to a device number for this case, as before? Seems better than nothing still.

This revision now requires changes to proceed.Nov 6 2016, 1:53 PM

Thanks for the quick review!
I like the idea of having a button to bring up the settings, that would also make it more obvious that the settings are now working differently. I'll update the patch.

intern/cycles/blender/addon/properties.py
127–129	I went for lowercase identifiers so that they match Device::type_to_string, but just changing that to uppercase works as well.
1195	Not really, it just seemed to look nicer to me, especially along with the device type buttons above.
intern/cycles/device/device.cpp
403	I originally added that because I expected to use the persistent_id directly as a property name, for which the general string (which will contain spaces from the name, and might also have weirder symbols). But now, where the persistent_id is only used as a regular string, the hash is indeed useless.
intern/cycles/device/device.h
52–53	Yes, it seems that the original id isn't even needed anymore.
intern/cycles/device/opencl/opencl_util.cpp
681–682	The main reason I considered was that the IDs are supposed to be consistent across hardware changes, and the device number might change with things like driver updates. But, using a running counter for that would probably be more robust, yes.

Hi, some ideas about UI.
We may can get rid of naming devices as OpenCL and/or Cuda devices for user.
If you check Cuda it is clear these are Cuda devices, same for OpenCL.
Brecht mention in IRC:

we'll need to handle case where computer contains both OpenCL and CUDA devices, warn about it or actually support it at some point

If network devices are working in Cycles we need UI for 20 or more devices, just to keep it in mind UI wise.

Cheers, mib

Addressed review.

@Wolfgang Faehnle (mib2berlin): I'm not sure what you mean with "get rid of naming devices as"...

Hi, hard for me to explain better in Englisch so small mockup:

Ah, I thought @Wolfgang Faehnle (mib2berlin) was proposing to basically get rid of the Compute Device setting entirely, listing both OpenCL and CUDA devices always. But ideally we would then also actually support using all of them at the same timing (including CPU), and that's a bigger project. If it's just about the labels, I don't have a strong opinion either way.

Accepting the patch assuming you agree with the Render panel UI tweak I proposed.

intern/cycles/blender/addon/ui.py

1611–1617

I'd like this UI to be a bit more compact:

split = layout.split(percentage=1/3)
split.label("Device:")
row = split.row(align=True)
sub = row.split(align=True)
sub.active = get_device_type(context) in {'CUDA', 'OPENCL', 'NETWORK'}
sub.prop(cscene, "device", text="")
row.operator("wm.addon_userpref_show", text="Preferences", icon='PREFERENCES').module = __package__

And then also change the enum:

     ('CPU', "CPU", "Use CPU for rendering"),
-    ('GPU', "GPU Compute", "Use GPU compute device for rendering, configured in user preferences"),
+    ('GPU', "GPU", "Use GPU compute device(s) for rendering"),

This revision is now accepted and ready to land.Nov 6 2016, 11:47 PM

Yes, that render panel change looks good. I did just notice one more thing, though:
When the user starts with default user preferences, no GPUs will be selected. Then, even if the type is set to CUDA or OpenCL, Cycles will fall back to the CPU device. This might not be obvious at first.

So, to fix that, I'd suggest to only make the CPU/GPU selection in the render tab active when CUDA or OpenCL is enabled and at least one device of the selected type is enabled. That at least makes it obvious that with the current settings, GPU rendering isn't possible.

@Wolfgang Faehnle (mib2berlin) Okay, so the idea is to only display devices of the type that's currently selected. Hm, that's possible of course, but I'd favor showing them all - there is enough space for it and they're clearly labeled, so I see no reason to hide some devices.

In D2338#54281, @Lukas Stockner (lukasstockner97) wrote:

Yes, that render panel change looks good. I did just notice one more thing, though:
When the user starts with default user preferences, no GPUs will be selected. Then, even if the type is set to CUDA or OpenCL, Cycles will fall back to the CPU device. This might not be obvious at first.

So, to fix that, I'd suggest to only make the CPU/GPU selection in the render tab active when CUDA or OpenCL is enabled and at least one device of the selected type is enabled. That at least makes it obvious that with the current settings, GPU rendering isn't possible.

That makes sense. Additionally we can also enable all devices by default? I can't think of a good reason not to.

Closed by commit rBdd921238d922: Cycles: Refactor Device selection to allow individual GPU compute device… (authored by Lukas Stockner (lukasstockner97)). · Explain WhyNov 7 2016, 3:25 AM

This revision was automatically updated to reflect the committed changes.

Duarte Farrajota Ramos (duarteframos) awarded a token.Nov 7 2016, 7:55 AM

Duarte Farrajota Ramos (duarteframos) added a subscriber: Duarte Farrajota Ramos (duarteframos).