Differential D18570 Diff 50397 plugins/platforms/drm/drm_backend.cpp

Changeset View

Standalone View

View Options

plugins/platforms/drm/drm_backend.cpp

Context not available.
35#include "egl_gbm_backend.h" 35​#include "egl_gbm_backend.h"
36#include <gbm.h> 36​#include <gbm.h>
37#endif 37​#endif
38​#include "egl_stream_backend.h"
38// KWayland 39​// KWayland
39#include <KWayland/Server/seat_interface.h> 40​#include <KWayland/Server/seat_interface.h>
40#include <KWayland/Server/outputconfiguration_interface.h> 41​#include <KWayland/Server/outputconfiguration_interface.h>
Context not available.
74 , m_udevMonitor(m_udev->monitor()) 75​ , m_udevMonitor(m_udev->monitor())
75 , m_dpmsFilter() 76​ , m_dpmsFilter()
76{ 77​{
78​ if (qEnvironmentVariableIsSet("KWIN_DRM_USE_EGLDEVICE")) {
79​ m_useEglDevice = true;
80​ }
77 setSupportsGammaControl(true); 81​ setSupportsGammaControl(true);
78 handleOutputs(); 82​ handleOutputs();
79} 83​}
Context not available.
250 qCWarning(KWIN_DRM) << "Did not find a GPU"; 254​ qCWarning(KWIN_DRM) << "Did not find a GPU";
251 return; 255​ return;
252 } 256​ }
253 int fd = LogindIntegration::self()->takeDevice(device->devNode()); 257​ m_devNode = device->devNode();
258​ int fd = LogindIntegration::self()->takeDevice(m_devNode.constData());
254 if (fd < 0) { 259​ if (fd < 0) {
255 qCWarning(KWIN_DRM) << "failed to open drm device at" << device->devNode(); 260​ qCWarning(KWIN_DRM) << "failed to open drm device at" << m_devNode;
256 return; 261​ return;
257 } 262​ }
258 m_fd = fd; 263​ m_fd = fd;
Context not available.
273 m_drmId = device->sysNum(); 278​ m_drmId = device->sysNum();
274 279​
275 // trying to activate Atomic Mode Setting (this means also Universal Planes) 280​ // trying to activate Atomic Mode Setting (this means also Universal Planes)
276 if (!qEnvironmentVariableIsSet("KWIN_DRM_NO_AMS")) { 281​ // note: Atomic Mode Setting is not currently supported with EGLStream backend
282​ if (!qEnvironmentVariableIsSet("KWIN_DRM_NO_AMS") && !m_useEglDevice) {
277 if (drmSetClientCap(m_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0) { 283​ if (drmSetClientCap(m_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0) {
278 qCDebug(KWIN_DRM) << "Using Atomic Mode Setting."; 284​ qCDebug(KWIN_DRM) << "Using Atomic Mode Setting.";
279 m_atomicModeSetting = true; 285​ m_atomicModeSetting = true;
davidedmundsonUnsubmitted
Not Done

I know this isn't related to your patch, and I don't want to take it off-topic, but when I run on my nvidia machine I get a null value from drmResources.

I get the same failure with a simple self-contained libDRM test.

I have a valid FD from logind.
/dev/card0 exists, drmGetVersion works fine.

I'm running Nvidia driver version 415.27

Any ideas?

davidedmundson: I know this isn't related to your patch, and I don't want to take it off-topic, but when I run…
ekurzingerAuthorUnsubmitted
Not Done

Hmm, interesting. Could you confirm that the nvidia_drm module is loaded with the option "modeset=1"? We currently don't enable the DRM driver by default (although that will probably change eventually).

# cat /sys/module/nvidia_drm/parameters/modeset

should print "Y"

ekurzinger: Hmm, interesting. Could you confirm that the nvidia_drm module is loaded with the option…
Context not available.
605 return nullptr; 611​ return nullptr;
606} 612​}
607 613​
614​void DrmBackend::queuePageFlip() {
zzagUnsubmitted
Done

Functions have opening braces on the start of a line.

zzag: Functions have opening braces on the start of a line.
615​ ++m_pageFlipsPending;
616​ if (m_pageFlipsPending == 1 && Compositor::self()) {
617​ Compositor::self()->aboutToSwapBuffers();
618​ }
619​}
620​
608void DrmBackend::present(DrmBuffer *buffer, DrmOutput *output) 621​void DrmBackend::present(DrmBuffer *buffer, DrmOutput *output)
609{ 622​{
610 if (!buffer || buffer->bufferId() == 0) { 623​ if (!buffer || buffer->bufferId() == 0) {
Context not available.
615 } 628​ }
616 629​
617 if (output->present(buffer)) { 630​ if (output->present(buffer)) {
618 m_pageFlipsPending++; 631​ queuePageFlip();
619 if (m_pageFlipsPending == 1 && Compositor::self()) { ​
620 Compositor::self()->aboutToSwapBuffers(); ​
621 } ​
622 } else if (m_deleteBufferAfterPageFlip) { 632​ } else if (m_deleteBufferAfterPageFlip) {
623 delete buffer; 633​ delete buffer;
624 } 634​ }
Context not available.
626 636​
627void DrmBackend::initCursor() 637​void DrmBackend::initCursor()
628{ 638​{
639​ // Hardware cursors aren't currently supported with EGLStream backend
640​ if (m_useEglDevice) {
641​ setSoftWareCursor(true);
642​ }
cfeckUnsubmitted
Not Done

A was searching for the definition of this function, because it is named wrongly (should be setSoftwareCursor), but it seems to be a mistake in the base class. If it's private API, it might be worth to rename it.

cfeck: A was searching for the definition of this function, because it is named wrongly (should be…
romanggUnsubmitted
Not Done

Not in this diff though. Same holds for other suggestions below in this file, which are not directly related to this diff.

romangg: Not in this diff though. Same holds for other suggestions below in this file, which are not…
ekurzingerAuthorUnsubmitted
Not Done

yeah, while I agree with these suggestions I think it would be best to limit this commit to code related to the new backend as opposed to general cleanup. I'll address the style issues you noticed in egl_stream_backend.cpp, though (thanks for catching those).

ekurzinger: yeah, while I agree with these suggestions I think it would be best to limit this commit to…
643​
629 m_cursorEnabled = waylandServer()->seat()->hasPointer(); 644​ m_cursorEnabled = waylandServer()->seat()->hasPointer();
630 connect(waylandServer()->seat(), &KWayland::Server::SeatInterface::hasPointerChanged, this, 645​ connect(waylandServer()->seat(), &KWayland::Server::SeatInterface::hasPointerChanged, this,
631 [this] { 646​ [this] {
cfeckUnsubmitted
Not Done

For implicitely shared Qt classes, it might be dangerous to use a reference, because that won't increase the shared-ref counter. Please just omit the &.

cfeck: For implicitely shared Qt classes, it might be dangerous to use a reference, because that won't…
cfeckUnsubmitted
Not Done

If for (const &output : m_outputs) also works here (and at other places), you could improve readability.

cfeck: If `for (const &output : m_outputs)` also works here (and at other places), you could improve…
Context not available.
733 748​
734OpenGLBackend *DrmBackend::createOpenGLBackend() 749​OpenGLBackend *DrmBackend::createOpenGLBackend()
735{ 750​{
751​ if (m_useEglDevice) {
zzagUnsubmitted
Not Done

It would be great if EGLStreams were optional.

zzag: It would be great if EGLStreams were optional.
graesslinUnsubmitted
Not Done

Wouldn't it make more sense to try the available backends? E.g first create EglGbmBackend, if that fails create EglStreamBackend - comparable to how we load the compositor.

graesslin: Wouldn't it make more sense to try the available backends? E.g first create EglGbmBackend, if…
ekurzingerAuthorUnsubmitted
Not Done

If Mesa is available, creating an EglGbmBackend should always succeed, I believe. On systems running the NVIDIA driver it will just fall back to using llvmpipe-based software rendering (of course, with abysmal performance).

We could attempt to create the EglStreamBackend first, which should fail on any non-NVIDIA systems, and then fall back to GBM (and then QPainter). Alternatively, I suppose we would need some way to detect if the NVIDIA driver is present. There are probably a few ways to do this but they might be a little hacky.

And re: zzag I can totally make the EGLStream stuff compile-time optional if that would be preferred, I'll work on an amended version of the change and get it up shortly.

ekurzinger: If Mesa is available, creating an EglGbmBackend should always succeed, I believe. On systems…
752​ m_deleteBufferAfterPageFlip = false;
753​ return new EglStreamBackend(this);
754​ } else {
736#if HAVE_GBM 755​#if HAVE_GBM
737 m_deleteBufferAfterPageFlip = true; 756​ m_deleteBufferAfterPageFlip = true;
738 return new EglGbmBackend(this); 757​ return new EglGbmBackend(this);
739#else 758​#else
740 return Platform::createOpenGLBackend(); 759​ return Platform::createOpenGLBackend();
741#endif 760​#endif
761​ }
742} 762​}
743 763​
744DrmDumbBuffer *DrmBackend::createBuffer(const QSize &size) 764​DrmDumbBuffer *DrmBackend::createBuffer(const QSize &size)
romanggUnsubmitted
Done

else keyword and curly braces are unnecessary.

romangg: `else` keyword and curly braces are unnecessary.
Context not available.
784 s << "Name: " << "DRM" << endl; 804​ s << "Name: " << "DRM" << endl;
785 s << "Active: " << m_active << endl; 805​ s << "Active: " << m_active << endl;
786 s << "Atomic Mode Setting: " << m_atomicModeSetting << endl; 806​ s << "Atomic Mode Setting: " << m_atomicModeSetting << endl;
807​ s << "Using EGL Device: " << m_useEglDevice << endl;
romanggUnsubmitted
Done

In case of HAVE_EGL_STREAMS not if m_useEglStreams is false.

romangg: In case of `HAVE_EGL_STREAMS` not if `m_useEglStreams` is false.
787 return supportInfo; 808​ return supportInfo;
788} 809​}
789 810​
Context not available.