From 941bae2810a13e3483b217133e475da12b0949f2 Mon Sep 17 00:00:00 2001 From: Xaver Hugl Date: Thu, 21 Apr 2022 17:11:24 +0200 Subject: [PATCH] backends/drm: fail atomic tests gracefully when buffer allocations fail This should fix the crash. However, it's still unclear to me why allocations fail in the first place CCBUG: 452572 --- src/backends/drm/drm_layer.h | 7 +-- src/backends/drm/drm_lease_egl_gbm_layer.cpp | 4 +- src/backends/drm/drm_lease_egl_gbm_layer.h | 2 +- src/backends/drm/drm_pipeline.cpp | 2 +- src/backends/drm/drm_pipeline_legacy.cpp | 2 +- src/backends/drm/drm_qpainter_layer.cpp | 8 +-- src/backends/drm/drm_qpainter_layer.h | 4 +- src/backends/drm/egl_gbm_layer.cpp | 31 +++--------- src/backends/drm/egl_gbm_layer.h | 6 +-- src/backends/drm/egl_gbm_layer_surface.cpp | 53 ++++++++++++++------ src/backends/drm/egl_gbm_layer_surface.h | 2 + 11 files changed, 61 insertions(+), 60 deletions(-) diff --git a/src/backends/drm/drm_layer.h b/src/backends/drm/drm_layer.h index 285889266e..592d4eaefc 100644 --- a/src/backends/drm/drm_layer.h +++ b/src/backends/drm/drm_layer.h @@ -36,12 +36,7 @@ class DrmPipelineLayer : public DrmOutputLayer public: DrmPipelineLayer(DrmPipeline *pipeline); - /** - * @returns a buffer for atomic test commits - * If no fitting buffer is available, a new current buffer is created - */ - virtual QSharedPointer testBuffer() = 0; - + virtual bool checkTestBuffer() = 0; virtual QSharedPointer currentBuffer() const = 0; virtual bool hasDirectScanoutBuffer() const; diff --git a/src/backends/drm/drm_lease_egl_gbm_layer.cpp b/src/backends/drm/drm_lease_egl_gbm_layer.cpp index 112fb4ba16..433fc1a0b9 100644 --- a/src/backends/drm/drm_lease_egl_gbm_layer.cpp +++ b/src/backends/drm/drm_lease_egl_gbm_layer.cpp @@ -27,7 +27,7 @@ DrmLeaseEglGbmLayer::DrmLeaseEglGbmLayer(EglGbmBackend *backend, DrmPipeline *pi }); } -QSharedPointer DrmLeaseEglGbmLayer::testBuffer() +bool DrmLeaseEglGbmLayer::checkTestBuffer() { const auto mods = m_pipeline->formats().value(DRM_FORMAT_XRGB8888); const auto size = m_pipeline->bufferSize(); @@ -49,7 +49,7 @@ QSharedPointer DrmLeaseEglGbmLayer::testBuffer() qCWarning(KWIN_DRM) << "Failed to create gbm_bo for lease output"; } } - return m_buffer; + return m_buffer && m_buffer->bufferId() != 0; } QSharedPointer DrmLeaseEglGbmLayer::currentBuffer() const diff --git a/src/backends/drm/drm_lease_egl_gbm_layer.h b/src/backends/drm/drm_lease_egl_gbm_layer.h index 5a090f0c50..9b83adff3d 100644 --- a/src/backends/drm/drm_lease_egl_gbm_layer.h +++ b/src/backends/drm/drm_lease_egl_gbm_layer.h @@ -24,7 +24,7 @@ public: OutputLayerBeginFrameInfo beginFrame() override; void endFrame(const QRegion &damagedRegion, const QRegion &renderedRegion) override; - QSharedPointer testBuffer() override; + bool checkTestBuffer() override; QSharedPointer currentBuffer() const override; private: diff --git a/src/backends/drm/drm_pipeline.cpp b/src/backends/drm/drm_pipeline.cpp index b431dabca3..53078d2817 100644 --- a/src/backends/drm/drm_pipeline.cpp +++ b/src/backends/drm/drm_pipeline.cpp @@ -113,7 +113,7 @@ bool DrmPipeline::commitPipelinesAtomic(const QVector &pipelines, return false; }; for (const auto &pipeline : pipelines) { - if (!pipeline->pending.layer->testBuffer()) { + if (!pipeline->pending.layer->checkTestBuffer()) { qCWarning(KWIN_DRM) << "Checking test buffer failed for" << mode; return failed(); } diff --git a/src/backends/drm/drm_pipeline_legacy.cpp b/src/backends/drm/drm_pipeline_legacy.cpp index 3d6f851689..62579e0690 100644 --- a/src/backends/drm/drm_pipeline_legacy.cpp +++ b/src/backends/drm/drm_pipeline_legacy.cpp @@ -38,7 +38,7 @@ bool DrmPipeline::presentLegacy() bool DrmPipeline::legacyModeset() { uint32_t connId = m_connector->id(); - if (!pending.layer->testBuffer() || drmModeSetCrtc(gpu()->fd(), pending.crtc->id(), pending.layer->currentBuffer()->bufferId(), 0, 0, &connId, 1, pending.mode->nativeMode()) != 0) { + if (!pending.layer->checkTestBuffer() || drmModeSetCrtc(gpu()->fd(), pending.crtc->id(), pending.layer->currentBuffer()->bufferId(), 0, 0, &connId, 1, pending.mode->nativeMode()) != 0) { qCWarning(KWIN_DRM) << "Modeset failed!" << strerror(errno); pending = m_next; return false; diff --git a/src/backends/drm/drm_qpainter_layer.cpp b/src/backends/drm/drm_qpainter_layer.cpp index 55da331f3b..766ec9cad0 100644 --- a/src/backends/drm/drm_qpainter_layer.cpp +++ b/src/backends/drm/drm_qpainter_layer.cpp @@ -52,12 +52,12 @@ void DrmQPainterLayer::endFrame(const QRegion &renderedRegion, const QRegion &da m_swapchain->releaseBuffer(m_swapchain->currentBuffer(), damagedRegion); } -QSharedPointer DrmQPainterLayer::testBuffer() +bool DrmQPainterLayer::checkTestBuffer() { if (!doesSwapchainFit()) { m_swapchain = QSharedPointer::create(m_pipeline->gpu(), m_pipeline->bufferSize(), DRM_FORMAT_XRGB8888); } - return m_swapchain->currentBuffer(); + return !m_swapchain->isEmpty(); } bool DrmQPainterLayer::doesSwapchainFit() const @@ -110,13 +110,13 @@ DrmLeaseQPainterLayer::DrmLeaseQPainterLayer(DrmQPainterBackend *backend, DrmPip }); } -QSharedPointer DrmLeaseQPainterLayer::testBuffer() +bool DrmLeaseQPainterLayer::checkTestBuffer() { const auto size = m_pipeline->bufferSize(); if (!m_buffer || m_buffer->size() != size) { m_buffer = QSharedPointer::create(m_pipeline->gpu(), size, DRM_FORMAT_XRGB8888); } - return m_buffer; + return m_buffer->bufferId() != 0; } QSharedPointer DrmLeaseQPainterLayer::currentBuffer() const diff --git a/src/backends/drm/drm_qpainter_layer.h b/src/backends/drm/drm_qpainter_layer.h index 8089b61de5..c148e9bcaf 100644 --- a/src/backends/drm/drm_qpainter_layer.h +++ b/src/backends/drm/drm_qpainter_layer.h @@ -27,7 +27,7 @@ public: OutputLayerBeginFrameInfo beginFrame() override; void endFrame(const QRegion &damagedRegion, const QRegion &renderedRegion) override; - QSharedPointer testBuffer() override; + bool checkTestBuffer() override; QSharedPointer currentBuffer() const override; QRegion currentDamage() const override; @@ -62,7 +62,7 @@ public: OutputLayerBeginFrameInfo beginFrame() override; void endFrame(const QRegion &damagedRegion, const QRegion &renderedRegion) override; - QSharedPointer testBuffer() override; + bool checkTestBuffer() override; QSharedPointer currentBuffer() const override; private: diff --git a/src/backends/drm/egl_gbm_layer.cpp b/src/backends/drm/egl_gbm_layer.cpp index 5bd1d93b35..aa77f7f9a1 100644 --- a/src/backends/drm/egl_gbm_layer.cpp +++ b/src/backends/drm/egl_gbm_layer.cpp @@ -34,17 +34,6 @@ EglGbmLayer::EglGbmLayer(EglGbmBackend *eglBackend, DrmPipeline *pipeline) , m_surface(pipeline->gpu(), eglBackend) , m_dmabufFeedback(pipeline->gpu(), eglBackend) { - connect(eglBackend, &EglGbmBackend::aboutToBeDestroyed, this, &EglGbmLayer::destroyResources); -} - -EglGbmLayer::~EglGbmLayer() -{ - destroyResources(); -} - -void EglGbmLayer::destroyResources() -{ - m_surface.destroyResources(); } OutputLayerBeginFrameInfo EglGbmLayer::beginFrame() @@ -74,21 +63,17 @@ QRegion EglGbmLayer::currentDamage() const return m_currentDamage; } -QSharedPointer EglGbmLayer::testBuffer() +bool EglGbmLayer::checkTestBuffer() { if (!m_surface.doesSurfaceFit(m_pipeline->bufferSize(), m_pipeline->formats())) { - renderTestBuffer(); + const auto buffer = m_surface.renderTestBuffer(m_pipeline->bufferSize(), m_pipeline->formats()); + if (!buffer) { + return false; + } else { + m_currentBuffer = buffer; + } } - return m_currentBuffer; -} - -bool EglGbmLayer::renderTestBuffer() -{ - const auto oldBuffer = m_currentBuffer; - beginFrame(); - glClear(GL_COLOR_BUFFER_BIT); - endFrame(QRegion(), infiniteRegion()); - return m_currentBuffer != oldBuffer; + return true; } QSharedPointer EglGbmLayer::texture() const diff --git a/src/backends/drm/egl_gbm_layer.h b/src/backends/drm/egl_gbm_layer.h index 4c6b9e56a7..3415206c51 100644 --- a/src/backends/drm/egl_gbm_layer.h +++ b/src/backends/drm/egl_gbm_layer.h @@ -35,22 +35,18 @@ class EglGbmLayer : public DrmPipelineLayer { public: EglGbmLayer(EglGbmBackend *eglBackend, DrmPipeline *pipeline); - ~EglGbmLayer(); OutputLayerBeginFrameInfo beginFrame() override; void aboutToStartPainting(const QRegion &damagedRegion) override; void endFrame(const QRegion &renderedRegion, const QRegion &damagedRegion) override; bool scanout(SurfaceItem *surfaceItem) override; - QSharedPointer testBuffer() override; + bool checkTestBuffer() override; QSharedPointer currentBuffer() const override; bool hasDirectScanoutBuffer() const override; QRegion currentDamage() const override; QSharedPointer texture() const override; private: - bool renderTestBuffer(); - void destroyResources(); - QSharedPointer m_scanoutBuffer; QSharedPointer m_currentBuffer; QRegion m_currentDamage; diff --git a/src/backends/drm/egl_gbm_layer_surface.cpp b/src/backends/drm/egl_gbm_layer_surface.cpp index abb2a3dc13..ff339b74ca 100644 --- a/src/backends/drm/egl_gbm_layer_surface.cpp +++ b/src/backends/drm/egl_gbm_layer_surface.cpp @@ -54,21 +54,8 @@ void EglGbmLayerSurface::destroyResources() OutputLayerBeginFrameInfo EglGbmLayerSurface::startRendering(const QSize &bufferSize, DrmPlane::Transformations renderOrientation, DrmPlane::Transformations bufferOrientation, const QMap> &formats) { - // gbm surface - if (doesGbmSurfaceFit(m_gbmSurface.data(), bufferSize, formats)) { - m_oldGbmSurface.reset(); - } else { - if (doesGbmSurfaceFit(m_oldGbmSurface.data(), bufferSize, formats)) { - m_gbmSurface = m_oldGbmSurface; - } else { - if (!createGbmSurface(bufferSize, formats)) { - return {}; - } - // dmabuf might work with the new surface - m_importMode = MultiGpuImportMode::Dmabuf; - m_importSwapchain.reset(); - m_oldImportSwapchain.reset(); - } + if (!checkGbmSurface(bufferSize, formats)) { + return {}; } if (!m_gbmSurface->makeContextCurrent()) { return {}; @@ -147,6 +134,26 @@ std::optional, QRegion>> EglGbmLayerSurface } } +bool EglGbmLayerSurface::checkGbmSurface(const QSize &bufferSize, const QMap> &formats) +{ + if (doesGbmSurfaceFit(m_gbmSurface.data(), bufferSize, formats)) { + m_oldGbmSurface.reset(); + } else { + if (doesGbmSurfaceFit(m_oldGbmSurface.data(), bufferSize, formats)) { + m_gbmSurface = m_oldGbmSurface; + } else { + if (!createGbmSurface(bufferSize, formats)) { + return false; + } + // dmabuf might work with the new surface + m_importMode = MultiGpuImportMode::Dmabuf; + m_importSwapchain.reset(); + m_oldImportSwapchain.reset(); + } + } + return m_gbmSurface != nullptr; +} + bool EglGbmLayerSurface::createGbmSurface(const QSize &size, uint32_t format, const QVector &modifiers) { static bool modifiersEnvSet = false; @@ -374,4 +381,20 @@ QSharedPointer EglGbmLayerSurface::texture() const } return gbmBuffer->createTexture(m_eglBackend->eglDisplay()); } + +QSharedPointer EglGbmLayerSurface::renderTestBuffer(const QSize &bufferSize, const QMap> &formats) +{ + if (!checkGbmSurface(bufferSize, formats) || !m_gbmSurface->makeContextCurrent()) { + return nullptr; + } + glClear(GL_COLOR_BUFFER_BIT); + if (m_gpu == m_eglBackend->gpu()) { + return m_gbmSurface->swapBuffersForDrm(infiniteRegion()); + } else { + if (m_gbmSurface->swapBuffers(infiniteRegion())) { + return importBuffer(); + } + } + return nullptr; +} } diff --git a/src/backends/drm/egl_gbm_layer_surface.h b/src/backends/drm/egl_gbm_layer_surface.h index a8d055f646..317bf851d1 100644 --- a/src/backends/drm/egl_gbm_layer_surface.h +++ b/src/backends/drm/egl_gbm_layer_surface.h @@ -49,8 +49,10 @@ public: QSharedPointer texture() const; void destroyResources(); EglGbmBackend *eglBackend() const; + QSharedPointer renderTestBuffer(const QSize &bufferSize, const QMap> &formats); private: + bool checkGbmSurface(const QSize &size, const QMap> &formats); bool createGbmSurface(const QSize &size, uint32_t format, const QVector &modifiers); bool createGbmSurface(const QSize &size, const QMap> &formats); bool doesGbmSurfaceFit(GbmSurface *surf, const QSize &size, const QMap> &formats) const;