Differential D11781 Diff 38093 plugins/platforms/drm/drm_backend.cpp

Changeset View

Standalone View

View Options

plugins/platforms/drm/drm_backend.cpp

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Line(s)
44#include <KCoreAddons> 44​#include <KCoreAddons>
45#include <KLocalizedString> 45​#include <KLocalizedString>
46#include <KSharedConfig> 46​#include <KSharedConfig>
47// Qt 47​// Qt
48#include <QCryptographicHash> 48​#include <QCryptographicHash>
49#include <QSocketNotifier> 49​#include <QSocketNotifier>
50#include <QPainter> 50​#include <QPainter>
51// system 51​// system
52​#include <algorithm>
52#include <unistd.h> 53​#include <unistd.h>
53// drm 54​// drm
54#include <xf86drm.h> 55​#include <xf86drm.h>
55#include <xf86drmMode.h> 56​#include <xf86drmMode.h>
56#include <libdrm/drm_mode.h> 57​#include <libdrm/drm_mode.h>
57 58​
58#ifndef DRM_CAP_CURSOR_WIDTH 59​#ifndef DRM_CAP_CURSOR_WIDTH
59#define DRM_CAP_CURSOR_WIDTH 0x8 60​#define DRM_CAP_CURSOR_WIDTH 0x8
Show All 25 Lines85 if (m_gbmDevice) {
85 gbm_device_destroy(m_gbmDevice); 86​ gbm_device_destroy(m_gbmDevice);
86 } 87​ }
87#endif 88​#endif
88 if (m_fd >= 0) { 89​ if (m_fd >= 0) {
89 // wait for pageflips 90​ // wait for pageflips
90 while (m_pageFlipsPending != 0) { 91​ while (m_pageFlipsPending != 0) {
91 QCoreApplication::processEvents(QEventLoop::WaitForMoreEvents); 92​ QCoreApplication::processEvents(QEventLoop::WaitForMoreEvents);
92 } 93​ }
94​ // we need to first remove all outputs
93 qDeleteAll(m_outputs); 95​ qDeleteAll(m_outputs);
96​ m_outputs.clear();
97​ m_enabledOutputs.clear();
98​
94 qDeleteAll(m_planes); 99​ qDeleteAll(m_planes);
95 qDeleteAll(m_crtcs); 100​ qDeleteAll(m_crtcs);
96 qDeleteAll(m_connectors); 101​ qDeleteAll(m_connectors);
97 close(m_fd); 102​ close(m_fd);
98 } 103​ }
99} 104​}
100 105​
101void DrmBackend::init() 106​void DrmBackend::init()
Show All 9 Lines107{
111 }; 116​ };
112 if (logind->isConnected()) { 117​ if (logind->isConnected()) {
113 takeControl(); 118​ takeControl();
114 } else { 119​ } else {
115 connect(logind, &LogindIntegration::connectedChanged, this, takeControl); 120​ connect(logind, &LogindIntegration::connectedChanged, this, takeControl);
116 } 121​ }
117} 122​}
118 123​
124​Outputs DrmBackend::outputs() const
125​{
126​ return m_outputs;
127​}
128​
129​Outputs DrmBackend::enabledOutputs() const
130​{
131​ return m_enabledOutputs;
132​}
133​
119void DrmBackend::outputWentOff() 134​void DrmBackend::outputWentOff()
120{ 135​{
121 if (!m_dpmsFilter.isNull()) { 136​ if (!m_dpmsFilter.isNull()) {
122 // already another output is off 137​ // already another output is off
123 return; 138​ return;
124 } 139​ }
125 m_dpmsFilter.reset(new DpmsInputEventFilter(this)); 140​ m_dpmsFilter.reset(new DpmsInputEventFilter(this));
126 input()->prependInputEventFilter(m_dpmsFilter.data()); 141​ input()->prependInputEventFilter(m_dpmsFilter.data());
127} 142​}
128 143​
129void DrmBackend::turnOutputsOn() 144​void DrmBackend::turnOutputsOn()
130{ 145​{
131 m_dpmsFilter.reset(); 146​ m_dpmsFilter.reset();
132 for (auto it = m_enabledOutputs.constBegin(), end = m_enabledOutputs.constEnd(); it != end; it++) { 147​ for (auto it = m_enabledOutputs.constBegin(), end = m_enabledOutputs.constEnd(); it != end; it++) {
133 (*it)->setDpms(DrmOutput::DpmsMode::On); 148​ (*it)->setDpms(DrmOutput::DpmsMode::On);
graesslinUnsubmitted
Done

static_cast? That sounds wrong. I doubt you need it. Just do:

auto o = *it;
graesslin: static_cast? That sounds wrong. I doubt you need it. Just do: ``` auto o = *it; ```
134 } 149​ }
135} 150​}
136 151​
137void DrmBackend::checkOutputsAreOn() 152​void DrmBackend::checkOutputsAreOn()
138{ 153​{
139 if (m_dpmsFilter.isNull()) { 154​ if (m_dpmsFilter.isNull()) {
140 // already disabled, all outputs are on 155​ // already disabled, all outputs are on
141 return; 156​ return;
142 } 157​ }
143 for (auto it = m_enabledOutputs.constBegin(), end = m_enabledOutputs.constEnd(); it != end; it++) { 158​ for (auto it = m_enabledOutputs.constBegin(), end = m_enabledOutputs.constEnd(); it != end; it++) {
144 if (!(*it)->isDpmsEnabled()) { 159​ if (!(*it)->isDpmsEnabled()) {
graesslinUnsubmitted
Done

same here, static_cast looks wrong

graesslin: same here, static_cast looks wrong
145 // dpms still disabled, need to keep the filter 160​ // dpms still disabled, need to keep the filter
146 return; 161​ return;
147 } 162​ }
148 } 163​ }
149 // all outputs are on, disable the filter 164​ // all outputs are on, disable the filter
150 m_dpmsFilter.reset(); 165​ m_dpmsFilter.reset();
151} 166​}
152 167​
Show All 12 Lines
165{ 180​{
166 if (m_active) { 181​ if (m_active) {
167 return; 182​ return;
168 } 183​ }
169 m_active = true; 184​ m_active = true;
170 if (!usesSoftwareCursor()) { 185​ if (!usesSoftwareCursor()) {
171 const QPoint cp = Cursor::pos() - softwareCursorHotspot(); 186​ const QPoint cp = Cursor::pos() - softwareCursorHotspot();
172 for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) { 187​ for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) {
173 DrmOutput *o = *it; 188​ DrmOutput *o = *it;
zzagUnsubmitted
Done

You could use auto. ;-)

zzag: You could use `auto`. ;-)
graesslinUnsubmitted
Done

here as well the static cast looks like not needed

graesslin: here as well the static cast looks like not needed
174 // only relevant in atomic mode 189​ // only relevant in atomic mode
175 o->m_modesetRequested = true; 190​ o->m_modesetRequested = true;
176 o->pageFlipped(); // TODO: Do we really need this? 191​ o->pageFlipped(); // TODO: Do we really need this?
177 o->m_crtc->blank(); 192​ o->m_crtc->blank();
178 o->showCursor(); 193​ o->showCursor();
179 o->moveCursor(cp); 194​ o->moveCursor(cp);
180 } 195​ }
181 } 196​ }
Show All 11 Lines207 if (!m_active) {
193 return; 208​ return;
194 } 209​ }
195 // block compositor 210​ // block compositor
196 if (m_pageFlipsPending == 0 && Compositor::self()) { 211​ if (m_pageFlipsPending == 0 && Compositor::self()) {
197 Compositor::self()->aboutToSwapBuffers(); 212​ Compositor::self()->aboutToSwapBuffers();
198 } 213​ }
199 // hide cursor and disable 214​ // hide cursor and disable
200 for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) { 215​ for (auto it = m_outputs.constBegin(); it != m_outputs.constEnd(); ++it) {
201 DrmOutput *o = *it; 216​ DrmOutput *o = *it;
graesslinUnsubmitted
Done

and here as well

graesslin: and here as well
202 o->hideCursor(); 217​ o->hideCursor();
203 } 218​ }
204 m_active = false; 219​ m_active = false;
205} 220​}
206 221​
207void DrmBackend::pageFlipHandler(int fd, unsigned int frame, unsigned int sec, unsigned int usec, void *data) 222​void DrmBackend::pageFlipHandler(int fd, unsigned int frame, unsigned int sec, unsigned int usec, void *data)
208{ 223​{
209 Q_UNUSED(fd) 224​ Q_UNUSED(fd)
▲ Show 20 Lines • Show All 158 Lines • ▼ Show 20 Line(s)380{
368 } 383​ }
369 384​
370 ScopedDrmPointer<_drmModeRes, &drmModeFreeResources> resources(drmModeGetResources(m_fd)); 385​ ScopedDrmPointer<_drmModeRes, &drmModeFreeResources> resources(drmModeGetResources(m_fd));
371 if (!resources) { 386​ if (!resources) {
372 qCWarning(KWIN_DRM) << "drmModeGetResources failed"; 387​ qCWarning(KWIN_DRM) << "drmModeGetResources failed";
373 return; 388​ return;
374 } 389​ }
375 390​
376 QVector<DrmOutput*> connectedOutputs; 391​ QVector<DrmOutput*> connectedOutputs;
graesslinUnsubmitted
Done

I don't understand why you changed from QVector<DrmOutput*> to QVector<Output*>. The list still gets filled with DrmOutput and afterwards you make your life hard by having to static_cast back to DrmOutput.

graesslin: I don't understand why you changed from QVector<DrmOutput*> to QVector<Output*>. The list still…
377 QVector<DrmConnector*> pendingConnectors; 392​ QVector<DrmConnector*> pendingConnectors;
378 393​
379 // split up connected connectors in already or not yet assigned ones 394​ // split up connected connectors in already or not yet assigned ones
380 for (DrmConnector *con : qAsConst(m_connectors)) { 395​ for (DrmConnector *con : qAsConst(m_connectors)) {
381 if (!con->isConnected()) { 396​ if (!con->isConnected()) {
382 continue; 397​ continue;
383 } 398​ }
384 399​
Show All 11 Lines410 if (connectedOutputs.contains(*it)) {
396 it++; 411​ it++;
397 continue; 412​ continue;
398 } 413​ }
399 DrmOutput *removed = *it; 414​ DrmOutput *removed = *it;
400 it = m_outputs.erase(it); 415​ it = m_outputs.erase(it);
401 m_enabledOutputs.removeOne(removed); 416​ m_enabledOutputs.removeOne(removed);
402 emit outputRemoved(removed); 417​ emit outputRemoved(removed);
403 removed->teardown(); 418​ removed->teardown();
404 } 419​ }
graesslinUnsubmitted
Done

Please no static_casts. Either change the signature of outputRemoved signal or do a safe cast like qobject_cast or dynamic_cast. Static_cast is evil :-)

graesslin: Please no static_casts. Either change the signature of outputRemoved signal or do a safe cast…
405 420​
406 // now check new connections 421​ // now check new connections
407 for (DrmConnector *con : qAsConst(pendingConnectors)) { 422​ for (DrmConnector *con : qAsConst(pendingConnectors)) {
408 ScopedDrmPointer<_drmModeConnector, &drmModeFreeConnector> connector(drmModeGetConnector(m_fd, con->id())); 423​ ScopedDrmPointer<_drmModeConnector, &drmModeFreeConnector> connector(drmModeGetConnector(m_fd, con->id()));
409 if (!connector) { 424​ if (!connector) {
410 continue; 425​ continue;
411 } 426​ }
412 if (connector->count_modes == 0) { 427​ if (connector->count_modes == 0) {
Show All 11 Lines438 for (DrmCrtc *crtc : qAsConst(m_crtcs)) {
424 if (!(encoder->possible_crtcs & (1 << crtc->resIndex()))) { 439​ if (!(encoder->possible_crtcs & (1 << crtc->resIndex()))) {
425 continue; 440​ continue;
426 } 441​ }
427 442​
428 // check if crtc isn't used yet -- currently we don't allow multiple outputs on one crtc (cloned mode) 443​ // check if crtc isn't used yet -- currently we don't allow multiple outputs on one crtc (cloned mode)
429 auto it = std::find_if(connectedOutputs.constBegin(), connectedOutputs.constEnd(), 444​ auto it = std::find_if(connectedOutputs.constBegin(), connectedOutputs.constEnd(),
430 [crtc] (DrmOutput *o) { 445​ [crtc] (DrmOutput *o) {
431 return o->m_crtc == crtc; 446​ return o->m_crtc == crtc;
432 } 447​ }
graesslinUnsubmitted
Done

If connectedOutputs would still be QVector<DrmOutput*> you would not need the cast here.

graesslin: If connectedOutputs would still be QVector<DrmOutput*> you would not need the cast here.
433 ); 448​ );
434 if (it != connectedOutputs.constEnd()) { 449​ if (it != connectedOutputs.constEnd()) {
435 continue; 450​ continue;
436 } 451​ }
437 452​
438 // we found a suitable encoder+crtc 453​ // we found a suitable encoder+crtc
439 // TODO: we could avoid these lib drm calls if we store all struct data in DrmCrtc and DrmConnector in the beginning 454​ // TODO: we could avoid these lib drm calls if we store all struct data in DrmCrtc and DrmConnector in the beginning
440 ScopedDrmPointer<_drmModeCrtc, &drmModeFreeCrtc> modeCrtc(drmModeGetCrtc(m_fd, crtc->id())); 455​ ScopedDrmPointer<_drmModeCrtc, &drmModeFreeCrtc> modeCrtc(drmModeGetCrtc(m_fd, crtc->id()));
Show All 30 Lines
471 outputDone = true; 486​ outputDone = true;
472 break; 487​ break;
473 } 488​ }
474 if (outputDone) { 489​ if (outputDone) {
475 break; 490​ break;
476 } 491​ }
477 } 492​ }
478 } 493​ }
479 std::sort(connectedOutputs.begin(), connectedOutputs.end(), [] (DrmOutput *a, DrmOutput *b) { return a->m_conn->id() < b->m_conn->id(); }); 494​ std::sort(connectedOutputs.begin(), connectedOutputs.end(), [] (DrmOutput *a, DrmOutput *b) { return a->m_conn->id() < b->m_conn->id(); });
480 m_outputs = connectedOutputs; 495​ m_outputs = connectedOutputs;
graesslinUnsubmitted
Done

same here. The change would not be needed if the signature were not changed.

graesslin: same here. The change would not be needed if the signature were not changed.
481 m_enabledOutputs = connectedOutputs; 496​ m_enabledOutputs = connectedOutputs;
482 readOutputsConfiguration(); 497​ readOutputsConfiguration();
483 if (!m_outputs.isEmpty()) { 498​ if (!m_outputs.isEmpty()) {
484 emit screensQueried(); 499​ emit screensQueried();
485 } 500​ }
486} 501​}
487 502​
488void DrmBackend::readOutputsConfiguration() 503​void DrmBackend::readOutputsConfiguration()
489{ 504​{
490 if (m_outputs.isEmpty()) { 505​ if (m_outputs.isEmpty()) {
491 return; 506​ return;
492 } 507​ }
493 const QByteArray uuid = generateOutputConfigurationUuid(); 508​ const QByteArray uuid = generateOutputConfigurationUuid();
494 const auto outputGroup = kwinApp()->config()->group("DrmOutputs"); 509​ const auto outputGroup = kwinApp()->config()->group("DrmOutputs");
495 const auto configGroup = outputGroup.group(uuid); 510​ const auto configGroup = outputGroup.group(uuid);
496 // default position goes from left to right 511​ // default position goes from left to right
497 QPoint pos(0, 0); 512​ QPoint pos(0, 0);
498 for (auto it = m_outputs.begin(); it != m_outputs.end(); ++it) { 513​ for (auto it = m_outputs.begin(); it != m_outputs.end(); ++it) {
499 qCDebug(KWIN_DRM) << "Reading output configuration for [" << uuid << "] ["<< (*it)->uuid() << "]"; 514​ qCDebug(KWIN_DRM) << "Reading output configuration for [" << uuid << "] ["<< (*it)->uuid() << "]";
500 const auto outputConfig = configGroup.group((*it)->uuid()); 515​ const auto outputConfig = configGroup.group((*it)->uuid());
501 (*it)->setGlobalPos(outputConfig.readEntry<QPoint>("Position", pos)); 516​ (*it)->setGlobalPos(outputConfig.readEntry<QPoint>("Position", pos));
zzagUnsubmitted
Done

Please don't hide pointer under auto. Make it explicit, e.g.

auto *o = qobject_cast<DrmOutput*>(*it);

That will improve readability a little bit.

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Do-not-use-auto-to-deduce-a-raw-pointer
https://lists.webkit.org/pipermail/webkit-dev/2017-January/028651.html

zzag: Please don't hide pointer under `auto`. Make it explicit, e.g. ```lang=cpp auto *o =…
graesslinUnsubmitted
Done

please a safe cast like qobject_cast or dynamic_cast

graesslin: please a safe cast like qobject_cast or dynamic_cast
502 // TODO: add mode 517​ // TODO: add mode
503 (*it)->setScale(outputConfig.readEntry("Scale", 1.0)); 518​ (*it)->setScale(outputConfig.readEntry("Scale", 1.0));
504 pos.setX(pos.x() + (*it)->geometry().width()); 519​ pos.setX(pos.x() + (*it)->geometry().width());
505 } 520​ }
506} 521​}
507 522​
508QByteArray DrmBackend::generateOutputConfigurationUuid() const 523​QByteArray DrmBackend::generateOutputConfigurationUuid() const
509{ 524​{
510 auto it = m_outputs.constBegin(); 525​ auto it = m_outputs.constBegin();
511 if (m_outputs.size() == 1) { 526​ if (m_outputs.size() == 1) {
512 // special case: one output 527​ // special case: one output
513 return (*it)->uuid(); 528​ return (*it)->uuid();
graesslinUnsubmitted
Done

same here

graesslin: same here
514 } 529​ }
515 QCryptographicHash hash(QCryptographicHash::Md5); 530​ QCryptographicHash hash(QCryptographicHash::Md5);
516 for (; it != m_outputs.constEnd(); ++it) { 531​ for (; it != m_outputs.constEnd(); ++it) {
517 hash.addData((*it)->uuid()); 532​ hash.addData((*it)->uuid());
graesslinUnsubmitted
Done

and here

graesslin: and here
518 } 533​ }
519 return hash.result().toHex().left(10); 534​ return hash.result().toHex().left(10);
520} 535​}
521 536​
522void DrmBackend::configurationChangeRequested(KWayland::Server::OutputConfigurationInterface *config) 537​void DrmBackend::configurationChangeRequested(KWayland::Server::OutputConfigurationInterface *config)
523{ 538​{
524 const auto changes = config->changes(); 539​ const auto changes = config->changes();
525 bool countChanged = false; 540​ bool countChanged = false;
▲ Show 20 Lines • Show All 270 Lines • Show Last 20 Lines