backends/drm: port away from mode indices

They're error prone and don't really work for changing modes. Having
a current mode in DrmConnector also doesn't work well together with
the transactional style of how DrmPipeline operates

src/backends/drm/drm_object_connector.cpp

@@ -23,6 +23,22 @@
 namespace KWin
 {
 
+static bool checkIfEqual(const drmModeModeInfo *one, const drmModeModeInfo *two)
+{
+    return one->clock       == two->clock
+        && one->hdisplay    == two->hdisplay
+        && one->hsync_start == two->hsync_start
+        && one->hsync_end   == two->hsync_end
+        && one->htotal      == two->htotal
+        && one->hskew       == two->hskew
+        && one->vdisplay    == two->vdisplay
+        && one->vsync_start == two->vsync_start
+        && one->vsync_end   == two->vsync_end
+        && one->vtotal      == two->vtotal
+        && one->vscan       == two->vscan
+        && one->vrefresh    == two->vrefresh;
+}
+
 static quint64 refreshRateForMode(_drmModeModeInfo *m)
 {
     // Calculate higher precision (mHz) refresh rate
@@ -81,6 +97,11 @@ uint32_t DrmConnectorMode::blobId()
     return m_blobId;
 }
 
+bool DrmConnectorMode::operator==(const DrmConnectorMode &otherMode)
+{
+    return checkIfEqual(&m_nativeMode, &otherMode.m_nativeMode);
+}
+
 DrmConnector::DrmConnector(DrmGpu *gpu, uint32_t connectorId)
     : DrmObject(gpu, connectorId, {
             PropertyDefinition(QByteArrayLiteral("CRTC_ID"), Requirement::Required),
@@ -124,11 +145,6 @@ DrmConnector::DrmConnector(DrmGpu *gpu, uint32_t connectorId)
     }
 }
 
-DrmConnector::~DrmConnector()
-{
-    qDeleteAll(m_modes);
-}
-
 bool DrmConnector::init()
 {
     return m_conn && initProps();
@@ -197,51 +213,17 @@ QSize DrmConnector::physicalSize() const
     return m_physicalSize;
 }
 
-DrmConnectorMode *DrmConnector::currentMode() const
+QVector<QSharedPointer<DrmConnectorMode>> DrmConnector::modes() const
-{
-    return m_modes[m_modeIndex];
-}
-
-int DrmConnector::currentModeIndex() const
-{
-    return m_modeIndex;
-}
-
-QVector<DrmConnectorMode *> DrmConnector::modes() const
 {
     return m_modes;
 }
 
-void DrmConnector::setModeIndex(int index)
+QSharedPointer<DrmConnectorMode> DrmConnector::findMode(const drmModeModeInfo &modeInfo) const
 {
-    m_modeIndex = index;
+    const auto it = std::find_if(m_modes.constBegin(), m_modes.constEnd(), [&modeInfo](const auto &mode) {
-}
+        return checkIfEqual(mode->nativeMode(), &modeInfo);
-
+    });
-static bool checkIfEqual(const drmModeModeInfo *one, const drmModeModeInfo *two)
+    return it == m_modes.constEnd() ? nullptr : *it;
-{
-    return one->clock       == two->clock
-        && one->hdisplay    == two->hdisplay
-        && one->hsync_start == two->hsync_start
-        && one->hsync_end   == two->hsync_end
-        && one->htotal      == two->htotal
-        && one->hskew       == two->hskew
-        && one->vdisplay    == two->vdisplay
-        && one->vsync_start == two->vsync_start
-        && one->vsync_end   == two->vsync_end
-        && one->vtotal      == two->vtotal
-        && one->vscan       == two->vscan
-        && one->vrefresh    == two->vrefresh;
-}
-
-void DrmConnector::findCurrentMode(drmModeModeInfo currentMode)
-{
-    for (int i = 0; i < m_modes.count(); i++) {
-        if (checkIfEqual(m_modes[i]->nativeMode(), &currentMode)) {
-            m_modeIndex = i;
-            return;
-        }
-    }
-    m_modeIndex = 0;
 }
 
 AbstractWaylandOutput::SubPixel DrmConnector::subpixel() const
@@ -322,12 +304,11 @@ bool DrmConnector::needsModeset() const
 
 void DrmConnector::updateModes()
 {
-    qDeleteAll(m_modes);
     m_modes.clear();
 
     // reload modes
     for (int i = 0; i < m_conn->count_modes; i++) {
-        m_modes.append(new DrmConnectorMode(this, m_conn->modes[i]));
+        m_modes.append(QSharedPointer<DrmConnectorMode>::create(this, m_conn->modes[i]));
     }
 }
 
@@ -408,6 +389,9 @@ bool DrmConnector::updateProperties()
 
     // init modes
     updateModes();
+    if (!m_modes.isEmpty() && !m_pipeline->pending.mode) {
+        m_pipeline->pending.mode = m_modes.constFirst();
+    }
 
     return true;
 }

src/backends/drm/drm_object_connector.h

@@ -41,6 +41,8 @@ public:
     QSize size() const;
     uint32_t refreshRate() const;
 
+    bool operator==(const DrmConnectorMode &otherMode);
+
 private:
     DrmConnector *m_connector;
     drmModeModeInfo m_nativeMode;
@@ -53,7 +55,6 @@ class DrmConnector : public DrmObject
 {
 public:
     DrmConnector(DrmGpu *gpu, uint32_t connectorId);
-    ~DrmConnector() override;
 
     enum class PropertyIndex : uint32_t {
         CrtcId = 0,
@@ -97,11 +98,8 @@ public:
     QString modelName() const;
     QSize physicalSize() const;
 
-    DrmConnectorMode *currentMode() const;
+    QVector<QSharedPointer<DrmConnectorMode>> modes() const;
-    int currentModeIndex() const;
+    QSharedPointer<DrmConnectorMode> findMode(const drmModeModeInfo &modeInfo) const;
-    QVector<DrmConnectorMode *> modes() const;
-    void setModeIndex(int index);
-    void findCurrentMode(drmModeModeInfo currentMode);
     void updateModes();
 
     AbstractWaylandOutput::SubPixel subpixel() const;
@@ -118,8 +116,7 @@ private:
     DrmScopedPointer<drmModeConnector> m_conn;
     Edid m_edid;
     QSize m_physicalSize = QSize(-1, -1);
-    QVector<DrmConnectorMode *> m_modes;
+    QVector<QSharedPointer<DrmConnectorMode>> m_modes;
-    int m_modeIndex = 0;
     uint32_t m_possibleCrtcs = 0;
 
     friend QDebug& operator<<(QDebug& s, const KWin::DrmConnector *obj);

src/backends/drm/drm_output.cpp

@@ -46,8 +46,8 @@ DrmOutput::DrmOutput(DrmPipeline *pipeline)
     , m_connector(pipeline->connector())
 {
     m_pipeline->setOutput(this);
-    auto conn = m_pipeline->connector();
+    const auto conn = m_pipeline->connector();
-    m_renderLoop->setRefreshRate(conn->currentMode()->refreshRate());
+    m_renderLoop->setRefreshRate(m_pipeline->pending.mode->refreshRate());
     setSubPixelInternal(conn->subpixel());
     setInternal(conn->isInternal());
     setCapabilityInternal(DrmOutput::Capability::Dpms);
@@ -177,13 +177,13 @@ QVector<AbstractWaylandOutput::Mode> DrmOutput::getModes() const
 {
     bool modeFound = false;
     QVector<Mode> modes;
-    auto conn = m_pipeline->connector();
+    const auto modelist = m_pipeline->connector()->modes();
-    QVector<DrmConnectorMode *> modelist = conn->modes();
 
     modes.reserve(modelist.count());
     for (int i = 0; i < modelist.count(); ++i) {
         Mode mode;
-        if (i == conn->currentModeIndex()) {
+        // compare the actual mode objects, not the pointers!
+        if (*modelist[i] == *m_pipeline->pending.mode) {
             mode.flags |= ModeFlag::Current;
             modeFound = true;
         }
@@ -298,31 +298,21 @@ DrmPlane::Transformations outputToPlaneTransform(DrmOutput::Transform transform)
 
 void DrmOutput::updateModes()
 {
-    auto conn = m_pipeline->connector();
+    m_pipeline->connector()->updateModes();
-    conn->updateModes();
+    setModes(getModes());
-
+    if (m_pipeline->pending.crtc) {
-    const auto modes = getModes();
+        const auto currentMode = m_pipeline->connector()->findMode(m_pipeline->pending.crtc->queryCurrentMode());
-    setModes(modes);
+        if (currentMode != m_pipeline->pending.mode) {
-
+            // DrmConnector::findCurrentMode might fail
-    auto it = std::find_if(modes.constBegin(), modes.constEnd(),
+            m_pipeline->pending.mode = currentMode ? currentMode : m_pipeline->connector()->modes().constFirst();
-        [](const AbstractWaylandOutput::Mode &mode){
+            if (DrmPipeline::commitPipelines(m_gpu->pipelines(), DrmPipeline::CommitMode::Test)) {
-            return mode.flags.testFlag(ModeFlag::Current);
+                m_pipeline->applyPendingChanges();
-        }
+                setCurrentModeInternal(m_pipeline->pending.mode->size(), m_pipeline->pending.mode->refreshRate());
-    );
+                m_renderLoop->setRefreshRate(m_pipeline->pending.mode->refreshRate());
-    Q_ASSERT(it != modes.constEnd());
+            } else {
-    AbstractWaylandOutput::Mode mode = *it;
+                qCWarning(KWIN_DRM) << "Setting changed mode failed!";
-
+                m_pipeline->revertPendingChanges();
-    // mode changed
+            }
-    if (mode.size != modeSize() || mode.refreshRate != refreshRate()) {
-        m_pipeline->pending.modeIndex = mode.id;
-        if (DrmPipeline::commitPipelines({m_pipeline}, DrmPipeline::CommitMode::Test)) {
-            m_pipeline->applyPendingChanges();
-            auto mode = m_pipeline->connector()->currentMode();
-            setCurrentModeInternal(mode->size(), mode->refreshRate());
-            m_renderLoop->setRefreshRate(mode->refreshRate());
-        } else {
-            qCWarning(KWIN_DRM) << "Setting changed mode failed!";
-            m_pipeline->revertPendingChanges();
         }
     }
 }
@@ -409,21 +399,17 @@ bool DrmOutput::queueChanges(const WaylandOutputConfig &config)
     static bool valid;
     static int envOnlySoftwareRotations = qEnvironmentVariableIntValue("KWIN_DRM_SW_ROTATIONS_ONLY", &valid) == 1 || !valid;
 
-    auto props = config.constChangeSet(this);
+    const auto props = config.constChangeSet(this);
     m_pipeline->pending.active = props->enabled;
-    auto modelist = m_connector->modes();
+    const auto modelist = m_connector->modes();
-    int index = -1;
+    const auto it = std::find_if(modelist.begin(), modelist.end(), [&props](const auto &mode) {
-    for (int i = 0; i < modelist.size(); i++) {
+        return mode->size() == props->modeSize && mode->refreshRate() == props->refreshRate;
-        if (modelist[i]->size() == props->modeSize && modelist[i]->refreshRate() == props->refreshRate) {
+    });
-            index = i;
+    if (it == modelist.end()) {
-            break;
-        }
-    }
-    if (index == -1) {
         qCWarning(KWIN_DRM).nospace() << "Could not find mode " << props->modeSize << "@" << props->refreshRate << " for output " << this;
         return false;
     }
-    m_pipeline->pending.modeIndex = index;
+    m_pipeline->pending.mode = *it;
     m_pipeline->pending.overscan = props->overscan;
     m_pipeline->pending.rgbRange = props->rgbRange;
     m_pipeline->pending.sourceTransformation = outputToPlaneTransform(props->transform);
@@ -448,8 +434,7 @@ void DrmOutput::applyQueuedChanges(const WaylandOutputConfig &config)
     setScale(props->scale);
     setTransformInternal(props->transform);
 
-    m_connector->setModeIndex(m_pipeline->pending.modeIndex);
+    const auto &mode = m_pipeline->pending.mode;
-    auto mode = m_connector->currentMode();
     setCurrentModeInternal(mode->size(), mode->refreshRate());
     m_renderLoop->setRefreshRate(mode->refreshRate());
     setOverscanInternal(m_pipeline->pending.overscan);

src/backends/drm/drm_pipeline.cpp

@@ -191,7 +191,7 @@ bool DrmPipeline::populateAtomicValues(drmModeAtomicReq *req, uint32_t &flags)
     if (pending.crtc) {
         pending.crtc->setPending(DrmCrtc::PropertyIndex::VrrEnabled, pending.syncMode == RenderLoopPrivate::SyncMode::Adaptive);
         pending.crtc->setPending(DrmCrtc::PropertyIndex::Gamma_LUT, pending.gamma ? pending.gamma->blobId() : 0);
-        auto modeSize = m_connector->modes().at(pending.modeIndex)->size();
+        const auto modeSize = pending.mode->size();
         pending.crtc->primaryPlane()->set(QPoint(0, 0), m_primaryBuffer ? m_primaryBuffer->size() : bufferSize(), QPoint(0, 0), modeSize);
         pending.crtc->primaryPlane()->setBuffer(activePending() ? m_primaryBuffer.get() : nullptr);
 
@@ -224,7 +224,6 @@ void DrmPipeline::prepareAtomicModeset()
         m_connector->setPending(DrmConnector::PropertyIndex::CrtcId, 0);
         return;
     }
-    auto mode = m_connector->modes().at(pending.modeIndex);
 
     m_connector->setPending(DrmConnector::PropertyIndex::CrtcId, activePending() ? pending.crtc->id() : 0);
     if (const auto &prop = m_connector->getProp(DrmConnector::PropertyIndex::Broadcast_RGB)) {
@@ -235,7 +234,7 @@ void DrmPipeline::prepareAtomicModeset()
     }
 
     pending.crtc->setPending(DrmCrtc::PropertyIndex::Active, activePending());
-    pending.crtc->setPending(DrmCrtc::PropertyIndex::ModeId, activePending() ? mode->blobId() : 0);
+    pending.crtc->setPending(DrmCrtc::PropertyIndex::ModeId, activePending() ? pending.mode->blobId() : 0);
 
     pending.crtc->primaryPlane()->setPending(DrmPlane::PropertyIndex::CrtcId, activePending() ? pending.crtc->id() : 0);
     pending.crtc->primaryPlane()->setTransformation(pending.bufferTransformation);
@@ -400,7 +399,7 @@ void DrmPipeline::applyPendingChanges()
 
 QSize DrmPipeline::bufferSize() const
 {
-    const auto modeSize = m_connector->modes().at(pending.modeIndex)->size();
+    const auto modeSize = pending.mode->size();
     if (pending.bufferTransformation & (DrmPlane::Transformation::Rotate90 | DrmPlane::Transformation::Rotate270)) {
         return modeSize.transposed();
     }
@@ -409,7 +408,7 @@ QSize DrmPipeline::bufferSize() const
 
 QSize DrmPipeline::sourceSize() const
 {
-    const auto modeSize = m_connector->modes().at(pending.modeIndex)->size();
+    const auto modeSize = pending.mode->size();
     if (pending.sourceTransformation & (DrmPlane::Transformation::Rotate90 | DrmPlane::Transformation::Rotate270)) {
         return modeSize.transposed();
     }
@@ -418,7 +417,7 @@ QSize DrmPipeline::sourceSize() const
 
 bool DrmPipeline::isCursorVisible() const
 {
-    const QRect mode = QRect(QPoint(), m_connector->modes().at(pending.modeIndex)->size());
+    const QRect mode = QRect(QPoint(), pending.mode->size());
     return pending.cursorBo && QRect(pending.cursorPos, pending.cursorBo->size()).intersects(mode);
 }
 
@@ -500,7 +499,7 @@ bool DrmPipeline::needsModeset() const
 {
     return pending.crtc != m_current.crtc
         || pending.active != m_current.active
-        || pending.modeIndex != m_current.modeIndex
+        || pending.mode != m_current.mode
         || pending.rgbRange != m_current.rgbRange
         || pending.bufferTransformation != m_current.bufferTransformation
         || m_connector->linkStatus() == DrmConnector::LinkStatus::Bad
@@ -509,7 +508,7 @@ bool DrmPipeline::needsModeset() const
 
 bool DrmPipeline::activePending() const
 {
-    return pending.crtc && pending.active;
+    return pending.crtc && pending.mode && pending.active;
 }
 
 void DrmPipeline::revertPendingChanges()

src/backends/drm/drm_pipeline.h

@@ -30,6 +30,7 @@ class DrmCrtc;
 class DrmBuffer;
 class DrmDumbBuffer;
 class GammaRamp;
+class DrmConnectorMode;
 
 class DrmGammaRamp
 {
@@ -97,7 +98,7 @@ public:
         DrmCrtc *crtc = nullptr;
         bool active = true; // whether or not the pipeline should be currently used
         bool enabled = true;// whether or not the pipeline needs a crtc
-        int modeIndex = 0;
+        QSharedPointer<DrmConnectorMode> mode;
         uint32_t overscan = 0;
         AbstractWaylandOutput::RgbRange rgbRange = AbstractWaylandOutput::RgbRange::Automatic;
         RenderLoopPrivate::SyncMode syncMode = RenderLoopPrivate::SyncMode::Fixed;

src/backends/drm/drm_pipeline_legacy.cpp

@@ -33,9 +33,8 @@ bool DrmPipeline::presentLegacy()
 
 bool DrmPipeline::legacyModeset()
 {
-    auto mode = m_connector->modes().at(pending.modeIndex);
     uint32_t connId = m_connector->id();
-    if (!checkTestBuffer() || drmModeSetCrtc(gpu()->fd(), pending.crtc->id(), m_primaryBuffer->bufferId(), 0, 0, &connId, 1, mode->nativeMode()) != 0) {
+    if (!checkTestBuffer() || drmModeSetCrtc(gpu()->fd(), pending.crtc->id(), m_primaryBuffer->bufferId(), 0, 0, &connId, 1, pending.mode->nativeMode()) != 0) {
         qCWarning(KWIN_DRM) << "Modeset failed!" << strerror(errno);
         pending = m_next;
         m_primaryBuffer = m_oldTestBuffer;