D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

This revision was automatically updated to reflect the committed changes.
Closed by commit R102:e38d60fb23f7: kcm_opengl: Fix retrieval of DRI 
information (authored by roberts).

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12476?vs=33565=33701

REVISION DETAIL
  https://phabricator.kde.org/D12476

AFFECTED FILES
  Modules/opengl/opengl.cpp

To: roberts, #plasma, alexeymin
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Alexey Min]

alexeymin accepted this revision.
alexeymin added a comment.
This revision is now accepted and ready to land.


  In D12476#258427 , @roberts wrote:
  
  > It will fail gracefully, it checks for the presence of these /proc or /sys 
files and only acts if they exist. This code is only called if an EGL context 
is successfully created in any case, and in fact on Wayland rendering _is_ 
always direct.
  
  
  Yeah, after reading about wayland more, I think so, it always uses DRM to 
render.
  
  In D12476#257806 , @roberts wrote:
  
  > Can I ask for a reviewer for this under X11?
  
  
  Seems to work for me in X11, too 
  F5834748: kinfocenter X11 test.png 
  
  About code, I don't like all those single-line `if`s whout braces around, and 
KDE coding style 
 says:
  `Use curly braces even when the body of a conditional statement contains only 
one line.`
  But I guess the existing code already has this problem.
  I'd say +1

REPOSITORY
  R102 KInfoCenter

BRANCH
  kcm_opengl_fix_dri_info

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma, alexeymin
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts added a comment.


  It will fail gracefully, it checks for the presence of these /proc or /sys 
files and only acts if they exist. This code is only called if an EGL context 
is successfully created in any case, and in fact on Wayland rendering _is_ 
always direct.

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Alexey Min]

alexeymin added a comment.


  I'm not completely sure that IsDirect should be manually set to true in 
wayland code path (it looks like a hack just to test).
  What happens if IsDirect is set to true, but software rendering is active? 
Will get_dri_device() fail gracefully or something horrible will happen? :)

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts updated this revision to Diff 33565.
roberts added a comment.


  Update to set IsDirect under Wayland.
  
  Managed to run kwin_wayland embedded in an X11 session to confirm.

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12476?vs=32999=33565

BRANCH
  kcm_opengl_fix_dri_info

REVISION DETAIL
  https://phabricator.kde.org/D12476

AFFECTED FILES
  Modules/opengl/opengl.cpp

To: roberts, #plasma
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts marked an inline comment as done.
roberts added a comment.


  Alexey: That sounds great, but unfortunately I can't get a wayland session to 
log in no matter how hard I try, so I can't confirm your changes. I'd suggest 
creating a separate review with your additions if you want to pursue this.
  
  Can I ask for a reviewer for this under X11?

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Alexey Min]

alexeymin added a comment.


  I've tested this and it seems to work!
  
  F5821132: kinfocenter DRI info wayland.png 

  
  But in wayland I had to add some changes: set `IsDirect = true;` before 
calling `print_screen_info()` in `get_gl_info_egl_qt()` line ~ 929.
  Otherwise, `get_dri_device()` is never called in this block in 
`print_screen_info()`:
  
if (IsDirect) {
if (get_dri_device())  {
l2 = newItem(l1, i18n("3D Accelerator"));
l2->setExpanded(true);
l3 = newItem(l2, l3, i18n("Vendor"), dri_info.vendor);
l3 = newItem(l2, l3, i18n("Device"), dri_info.device);
l3 = newItem(l2, l3, i18n("Subvendor"), dri_info.subvendor);
l3 = newItem(l2, l3, i18n("Revision"), dri_info.rev);
} else {
l2 = newItem(l1, l2, i18n("3D Accelerator"), i18n("unknown"));
}
}
  
  For X11, `IsDirect` is initialized in `get_gl_info_glx()` line 767 by:
  
IsDirect = glXIsDirect(dpy, ctx);
  
  Fow wayland path, it stays false forever, I guess. Though direct rendering 
**IS** used.
  
  Without this hack I only get this:
  F5821138: kinfocenter DRI info wayland bad.png 

  
  `3D Accelerator` section is gone, and `Driver` section lacks kernel module 
information. :(

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: alexeymin, wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts marked an inline comment as done.
roberts added inline comments.

INLINE COMMENTS

> wbauer wrote in opengl.cpp:211
> Using just "/driver" (instead of "/driver/module") makes it display "radeon" 
> instead of "drm" here as I would expect.
> 
> OTOH, as it already runs lspci to get the other information, wouldn't it be 
> better to get the kernel module from lspci as well?
> Additionally specifying the '-k' parameter to lspci adds these two lines to 
> lspci's output here:
> Driver: radeon
> Module: radeon
> (the first one is the driver in use, the second one lists all available 
> modules for that device, see also "man lspci")
> That should be independent of the kernel version and also work the same in 
> the get_dri_device_proc() case.
> 
> Just a thought though.

Have changed to use `/driver` instead of `/driver/module`.

Am preferring to err on the side of not adding more numbered line reads from 
lspci output.

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts updated this revision to Diff 32999.
roberts added a comment.


  Update to use /driver over /driver/module per review.

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12476?vs=32904=32999

BRANCH
  kcm_opengl_fix_dri_info

REVISION DETAIL
  https://phabricator.kde.org/D12476

AFFECTED FILES
  Modules/opengl/opengl.cpp

To: roberts, #plasma
Cc: wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Wolfgang Bauer]

wbauer added a comment.


  I gave the patch a try (openSUSE Leap 42.3 with kernel 4.4).
  
  The 3D accelerator info is displayed again now (as with kernel < 3.12), but 
it shows "drm" as kernel module instead of "radeon" as I would expect.

INLINE COMMENTS

> opengl.cpp:211
> +dri_info.pci = get_sysfs_link_name(sysPath);
> +dri_info.module = get_sysfs_link_name(sysPath + 
> QStringLiteral("/driver/module"));
> +

Using just "/driver" (instead of "/driver/module") makes it display "radeon" 
instead of "drm" here as I would expect.

OTOH, as it already runs lspci to get the other information, wouldn't it be 
better to get the kernel module from lspci as well?
Additionally specifying the '-k' parameter to lspci adds these two lines to 
lspci's output here:
Driver: radeon
Module: radeon
(the first one is the driver in use, the second one lists all available modules 
for that device, see also "man lspci")
That should be independent of the kernel version and also work the same in the 
get_dri_device_proc() case.

Just a thought though.

REPOSITORY
  R102 KInfoCenter

REVISION DETAIL
  https://phabricator.kde.org/D12476

To: roberts, #plasma
Cc: wbauer, plasma-devel, #plasma, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12476: kcm_opengl: Fix retrieval of DRI information

[Lindsay Roberts]

roberts created this revision.
roberts added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
roberts requested review of this revision.

REVISION SUMMARY
  Currently DRI information retrieval is attempted via /proc/dri, which
  was removed from the linux kernel in v3.12 (2013). This information
  includes the kernel driver in use as well as card identification sourced
  from pci.ids database.
  
  This adds support for retrieving this information in the same vein via
  /dev and /sys for modern kernels.

TEST PLAN
  kcmshell5 opengl should correctly display kernel driver.

REPOSITORY
  R102 KInfoCenter

BRANCH
  kcm_opengl_fix_dri_info

REVISION DETAIL
  https://phabricator.kde.org/D12476

AFFECTED FILES
  Modules/opengl/opengl.cpp

To: roberts, #plasma
Cc: plasma-devel, #plasma, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart