Allow unprivileged access on console (and X session with an optional libxcb)

[Lekensteyn]

After these changes it is possible to run radeontop as unprivileged user on a desktop PC with integrated Intel graphics and an AMD GPU:

00:02.0 Display controller [0380]: Intel Corporation Xeon E3-1200 v2/3rd Gen Core processor Graphics Controller [8086:0162] (rev 09)
01:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Barts LE [Radeon HD 6790] [1002:673e]
01:00.1 Audio device [0403]: Advanced Micro Devices, Inc. [AMD/ATI] Barts HDMI Audio [Radeon HD 6800 Series] [1002:aa88]

For hybrid graphics laptops where the first PCI device is also reported as "VGA compatible controller", it would be worth checking the vendor ID in the PCI loop too... that could be done later if it is needed.

Unprivileged vram access is made possible via DRM after authenticating, see DRI2Authenticate at https://www.x.org/archive/X11R7.5/doc/dri2proto/dri2proto.txt https://xcb.freedesktop.org/manual/group__XCB__DRI2__API.html https://mail.gnome.org/archives/commits-list/2013-October/msg04520.html https://www.mail-archive.com/mesa-dev@lists.freedesktop.org/msg112417.html

[clbr]

I confirmed the use-after-free with valgrind, and pulled the two ASAN commits, thanks! I did run valgrind during development, but perhaps it was before that code was added.

On the other two commits, I see some issues. First of all, adding X or xcb dependencies is not acceptable. Running this on a headless OpenCL compute server without anything X installed is supported.

With the other patch, while automatically supporting hybrid laptops is a goal I agree with, it looks like it breaks support for multiple Radeon GPUs, or at least has a risk of doing so. I currently don't have access to such hw to test the change.

[Lekensteyn]

I have rebased the changes on the latest master. Changes since v1:

• Removed unused "drmfilter" function (amended the "Fix auto-detection of radeon device" patch)
• Added patch for xcb to be optional.

I have no multi radeon GPU setup, but the "Fix auto-detection ..." patch won't break that scenario (unless for some reason the kernel driver is not named "radeon" anymore). You can still have a manual override with the -b option.

"amdgpu" is not supported yet according to the README, so there is no regression here. (I also have no appropriate hardware to test amdgpu.)

[clbr]

Compile-time selection is not enough, it would have to be runtime. That is, a distro package for radeontop should not bring in half of X.

[clbr]

Hm, just curious, but why is the X authentication required at all? Isn't it common for the /dev/dri nodes to be 664 root:video, and users belong to the video group so that they can run 3d?

[Lekensteyn]

I could add dlopen/dlsym stuff, but then I might as well move the xcb code to a new shared library and load that at runtime (libradeontop_xcb.so). How does that sound?

File permissions are not the only thing that regulate access to the ioctls on the /dev/dri/* files. See man 7 drm for a very brief description on authentication (NOTE: this is not X authentication, but DRM authentication). If no application has claimed the DRI node (i.e. no X server running), I think that the self-authentication should succeed (as radeontop is effectively the master). (I have not verified this theory though.)

If X is running (and is the DRM master), then X must authorize radeontop. This is done by radeontop obtaining the client identifier (drmGetMagic) which is sent via the DRI2Authenticate command to a Xserver which will call drmSetMagic to grant the client access.

[clbr]

I fear distros would treat a separate shared lib the same, ie bundle it in one package and so pull in X.

If anybody can provide test results from multiple Radeons with the auto-detection patch, I can then merge that one. You mention you don't have that, perhaps you can ask on a tech forum or on #radeon?

[Lekensteyn]

The idea is to load the shared library at runtime (using dlopen) so the xcb library is still optional. Distros can then package the files together and put the xcb library as recommended/optional dependency. If distros decide to mark it as hard dependency, then it would be their choice, but the shared library is completely optional.

The auto-detection patch should still work, it internally simply iterates through /dev/dri/cardX from 0 to some maximum, but also requires a matching driver name. I'll try to ask for some testing on #radeon or other channels.

[Lekensteyn]

Added another patch to make libxcb optional at runtime... ideally documentation about this is written somewhere else than the commit message, maybe the README?

Btw, what do you expect from the multiple radeon GPU test? Previously it would pick the first available PCI video device which is now still the case. However, where it would pick /dev/dri/card0 before which could possibly belong to a different PCI device (i.e. Intel in my case), now it will only accept the DRI device matching the PCI device.

Previously the second video device would not be selected by default, that is still not the case. Again, overriding with -b X still works.

[clbr]

I'd just like to know that selecting both GPUs with -b still works, and that one of them is used by default.

I'll check the new commit later.

[Lekensteyn]

Only one GPU can be selected at a time with -b if I am not mistaken, that logic has not been changed and still works. Btw, I forgot to mention that I successfully executed unprivileged radeontop from console (with no X, but with appropriate permissions to /dev/dri/card1). This became possible due to the "self-authentication" logic.

So there it is, you do not even need X if you are running from console with appropriate permissions to /dev/dri/card? (which should not be unusual if you are running OpenCL stuff)!

[clbr]

As this is a bigger change to the detection code, I'd like to be sure it doesn't break something, rather than go blind and hope, then see a new bug half a year later, I'm sure you understand. Just making sure.

The auth code looks fairly good. As the new file is entirely your code, the copyright stamp should mention you and this year, not me @ 2012. It also seems like "make install" would break without xcb?

I'd like to see all the xcb commits squashed into one, and the README updated as well (X is not needed, but optional for running without root privs on recent kernels, or similar).

[Lekensteyn]

I have split off the autodetection patch and squashed patches. Additionally, I have split the xcb change from the drmAuth one to demonstrate that it is really optional. The copyright header is updated and the "xcb" variable is documented better in Makefile.

Instructions for distros are in the commit message.

What about installing radeontop to /usr/bin/ now that it can be run as normal user?

[clbr]

It still requires root rights when running on fglrx or amdgpu, or when running on an older kernel, before the query was added to radeon. So I'm not sure putting it in bin/ would be a good idea.

[Lekensteyn]

amdgpu support can be added but needs different macros (DRM_AMDGPU_INFO and AMDGPU_INFO_READ_MMR_REG?). Don't know about fglrx since it is closed source (maybe file permissions are sufficient).

dumpcap is installed in /usr/bin/ but tcpdump in /usr/sbin/ even though both can be run without root for say, usb monitoring (with appropriate mode changes). I'd personally install it to /usr/bin since normal desktop users do not need sudo/root for this, but you'll have the final call here.

[clbr]

Merged, thanks.