make opengl module support multiple dri devices
ClosedPublic

Authored by sitter on Mar 11 2020, 1:17 PM.

Details

Summary

previously the code would only ever look at dri0 when that may not actually
be the device that is used. instead list all possible devices (I am
assuming in nvidia/intel combos the actual computations may happen on any
of them depending on configuration).
as a side effect we can no longer list a single kernel module but instead
list the module per-device.
the devices use cardN names on linux.
for the /proc based fallback we do not use cardN but simply N (to be
improved in master with localized string).

the legacy /proc/dri/ support was entirely removed. it's not a thing since
linux 3.12, which is from 2013.

on freebsd we continue to only support one device, in part because I have
no way to test multi-device support. the info is woefully underwhelming and
could as well not be there anyway TBH.

BUG: 417986
FIXED-IN: 5.18.4

Test Plan

vm with 3 cards show all of them with suitable info

Diff Detail

Repository
R102 KInfoCenter
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Mar 11 2020, 1:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 11 2020, 1:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sitter requested review of this revision.Mar 11 2020, 1:17 PM
apol added subscribers: adridg, apol.Mar 11 2020, 2:44 PM

+1 overall

Modules/opengl/opengl.cpp
247

I'd make it QVector, less to worry in the future.

249

Probably only want to list files?

271

Use QStandardPaths::findExecutable?

283–284

Indentation looks off

284–289

Probably want to list them all on BSD too, right @adridg?

zzag added a subscriber: zzag.Mar 11 2020, 3:01 PM

I'm not sure whether this is useful, but libdrm has an API to enumerate devices.

sitter added inline comments.Mar 11 2020, 3:35 PM
Modules/opengl/opengl.cpp
249

Dirs actually, e.g. /proc/dri/0/name
I honestly do not know what they are so I figured best to just list everything. They could be symlinks for all I know. /proc/dri/ isn't a thing on any of my VMs :|

271

I agree. Not really related though.

In D27980#625840, @zzag wrote:

I'm not sure whether this is useful, but libdrm has an API to enumerate devices.

Had a quick look, using libdrm seems a bit excessive for what we want to do here. The current /sys digging also doesn't exactly fill me with joy though :(

Modules/opengl/opengl.cpp
249

As it turns out the /proc/dri stuff was supposedly removed here https://github.com/torvalds/linux/commit/cb6458f97b53d7f73043206c18014b3ca63ac345 making this code entirely irrelevant since kernel 3.12.
Any objections to removing it altogether?

zzag added a comment.Mar 12 2020, 3:53 PM

Had a quick look, using libdrm seems a bit excessive for what we want to do here.

Well, it was worth a try.

sitter updated this revision to Diff 77716.Mar 16 2020, 12:11 PM

use qvector instead of list

fix device detection by doing readlink resolution on the correct directory (subdir appending got lost in the code rejiggering)

/proc/dri support is now gone entirely as it is only relevant for aaaaaaaancient kernels

sitter edited the summary of this revision. (Show Details)Mar 16 2020, 12:11 PM
pino added a subscriber: pino.Mar 19 2020, 11:41 AM
pino added inline comments.
Modules/opengl/opengl.cpp
169

const

207–215

qPrintable is wrong when passing paths to native C functions; use QFile::encodeName instead

230–233

why not just use QDirIterator instead, so it combines dir listing and filtering?

sitter added inline comments.Mar 19 2020, 12:04 PM
Modules/opengl/opengl.cpp
207–215

Is that also true for paths that are only ascii? Specifically path is a /dev node.

230–233

Hadn't even occured to me. It's a good thought though, I'll get on it.

pino added inline comments.Mar 19 2020, 1:37 PM
Modules/opengl/opengl.cpp
207–215

sure, i get this specific path is ascii

the general recommendation is to use QFile::encodeName for QString -> C paths conversions, no matter what (and viceversa QFile::decodeName for C -> QString); i personally always use these two avoid any possible future mistake

sitter updated this revision to Diff 78003.Mar 19 2020, 2:24 PM
  • isValid is const now
  • use qdiriterator
  • use QFile::encodeName for the stat call
adridg added inline comments.Apr 2 2020, 12:07 PM
Modules/opengl/opengl.cpp
284–289

Possibly. I don't have any machines with multiple cards, so I can't tell -- I'd have to ask Niclas about this. I don't know if it's possible for the numbering to start not-at-zero, for instance. In general it might be nice to do this via sysctl(3) , since that saves us pipes and ugliness. But let's leave that for a different time.

apol added inline comments.Apr 2 2020, 1:21 PM
Modules/opengl/opengl.cpp
249

Go for it.

sitter added inline comments.Apr 2 2020, 2:18 PM
Modules/opengl/opengl.cpp
249

Already is in latest diff ;)

apol added inline comments.Apr 2 2020, 2:46 PM
Modules/opengl/opengl.cpp
285

the {} isn't necessary here.

294

return {}

629

setting twice in a row? I'd skip setting a nullptr.

sitter added inline comments.Apr 3 2020, 10:51 AM
Modules/opengl/opengl.cpp
629

Mh. I think that's to ensure l3 (which is passed into newItem) is null on >1 iteration. Indeed garbage though, I'll just pass in nullptr instead.

sitter updated this revision to Diff 79208.Apr 3 2020, 11:40 AM
  • do however init the stat struct, its members are not initialized otherwise
  • streamline get_dri_device return
  • do not init l3 to nullptr inside the info loop. l3 is the entry for the infos, so simply leave it as-is (nullptr initially, preceding entry after that)
  • also sort infos by their device id. it'd be weird to have a list of card2 then 1 then 0
apol accepted this revision.Apr 3 2020, 1:07 PM

LGTM

This revision is now accepted and ready to land.Apr 3 2020, 1:07 PM
This revision was automatically updated to reflect the committed changes.