Page MenuHome

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

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

Details

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

Event Timeline

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Refactor Device selection to allow individual GPU compute device selection.Nov 6 2016, 3:09 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'

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.

Brecht Van Lommel (brecht) requested changes to this revision.

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
Lukas Stockner (lukasstockner97) marked 3 inline comments as done.Nov 6 2016, 3:13 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:

Brecht Van Lommel (brecht) accepted this revision.

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
1618–1630

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.

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.

This revision was automatically updated to reflect the committed changes.