From eb1daa0aadcbae3f4be8ca7450f648040a52013c Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Mon, 27 Sep 2021 21:43:26 +0300 Subject: [PATCH] platforms/drm: Avoid re-using blobs Blobs are not reference counted if used by other drm master, if kwin re-uses a deleted blob in an atomic commit, it will fail. For example, on my computer, this happens when kwin starts after xorg. Besides that, kwin may try to destroy blobs that it doesn't own, which is not fatal but it's strange to do so. CCBUG: 442603 CCBUG: 439873 --- src/plugins/platforms/drm/drm_object.cpp | 68 ++++--------------- src/plugins/platforms/drm/drm_object.h | 4 +- .../platforms/drm/drm_object_connector.cpp | 13 ++-- .../platforms/drm/drm_object_plane.cpp | 35 +++++----- 4 files changed, 40 insertions(+), 80 deletions(-) diff --git a/src/plugins/platforms/drm/drm_object.cpp b/src/plugins/platforms/drm/drm_object.cpp index 2002352ca5..09901d4e55 100644 --- a/src/plugins/platforms/drm/drm_object.cpp +++ b/src/plugins/platforms/drm/drm_object.cpp @@ -153,18 +153,10 @@ bool DrmObject::updateProperties() continue; } if (def.name == prop->name) { - drmModePropertyBlobRes *blob = nullptr; - if (prop->flags & DRM_MODE_PROP_BLOB) { - blob = drmModeGetPropertyBlob(m_gpu->fd(), properties->prop_values[drmPropIndex]); - } if (m_props[propIndex]) { - if (prop->flags & DRM_MODE_PROP_BLOB) { - m_props[propIndex]->setCurrentBlob(blob); - } else { - m_props[propIndex]->setCurrent(properties->prop_values[drmPropIndex]); - } + m_props[propIndex]->setCurrent(properties->prop_values[drmPropIndex]); } else { - m_props[propIndex] = new Property(this, prop.data(), properties->prop_values[drmPropIndex], def.enumNames, blob); + m_props[propIndex] = new Property(this, prop.data(), properties->prop_values[drmPropIndex], def.enumNames); } found = true; break; @@ -189,15 +181,12 @@ bool DrmObject::updateProperties() * Definitions for DrmObject::Property */ -DrmObject::Property::Property(DrmObject *obj, drmModePropertyRes *prop, uint64_t val, const QVector &enumNames, drmModePropertyBlobRes *blob) +DrmObject::Property::Property(DrmObject *obj, drmModePropertyRes *prop, uint64_t val, const QVector &enumNames) : m_propId(prop->prop_id) , m_propName(prop->name) , m_pending(val) - , m_pendingBlob(blob) , m_next(val) - , m_nextBlob(blob) , m_current(val) - , m_currentBlob(blob) , m_immutable(prop->flags & DRM_MODE_PROP_IMMUTABLE) , m_obj(obj) { @@ -234,8 +223,8 @@ bool DrmObject::Property::setPendingBlob(void *blob, size_t length) return false; } } - if (m_pending && m_pending != m_current && m_pending != m_next) { - drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_pending); + if (m_pendingBlob && m_pendingBlob != m_currentBlob && m_pendingBlob != m_nextBlob) { + drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_pendingBlob->id); drmModeFreePropertyBlob(m_pendingBlob); } m_pending = id; @@ -243,11 +232,6 @@ bool DrmObject::Property::setPendingBlob(void *blob, size_t length) return true; } -drmModePropertyBlobRes *DrmObject::Property::pendingBlob() const -{ - return m_pendingBlob; -} - void DrmObject::Property::setCurrent(uint64_t value) { m_current = value; @@ -260,40 +244,12 @@ uint64_t DrmObject::Property::current() const void DrmObject::Property::setCurrentBlob(drmModePropertyBlobRes *blob) { - if (!blob && !m_currentBlob) { - return; - } - if (!blob && m_pending == m_current) { - // the current blob might have been created by the prior drm master - // if it is, then the kernel has destroyed the blob and we must not continue using it - // -> simply create a copy - bool commit = m_pending == m_next; - setPendingBlob(m_currentBlob->data, m_currentBlob->length); - if (commit) { - commitPending(); - } - } - if (!blob && m_next == m_current) { - // same as above, for the "next" blob - uint32_t id = 0; - if (drmModeCreatePropertyBlob(m_obj->m_gpu->fd(), m_currentBlob->data, m_currentBlob->length, &id) != 0) { - qCWarning(KWIN_DRM) << "Creating property blob failed!" << strerror(errno); - } - m_next = id; - m_nextBlob = drmModeGetPropertyBlob(m_obj->m_gpu->fd(), id); - } - uint32_t blobId = blob ? blob->id : 0; - if (m_current && m_current != m_pending && m_current != m_next && m_current != blobId) { - drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_current); + if (m_currentBlob && m_currentBlob != m_pendingBlob && m_currentBlob != m_nextBlob && m_currentBlob != blob) { + drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_currentBlob->id); drmModeFreePropertyBlob(m_currentBlob); } m_currentBlob = blob; - m_current = blobId; -} - -drmModePropertyBlobRes *DrmObject::Property::currentBlob() const -{ - return m_currentBlob; + m_current = blob ? blob->id : 0; } void DrmObject::Property::commit() @@ -314,8 +270,8 @@ void DrmObject::Property::commitPending() return; } if (m_pendingBlob || m_nextBlob) { - if (m_next && m_next != m_current) { - drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_next); + if (m_nextBlob && m_nextBlob != m_currentBlob) { + drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_nextBlob->id); drmModeFreePropertyBlob(m_nextBlob); } m_next = m_pending; @@ -331,8 +287,8 @@ void DrmObject::Property::rollbackPending() return; } if (m_pendingBlob || m_nextBlob) { - if (m_pending && m_pending != m_current) { - drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_pending); + if (m_pendingBlob && m_pendingBlob != m_currentBlob) { + drmModeDestroyPropertyBlob(m_obj->m_gpu->fd(), m_pendingBlob->id); drmModeFreePropertyBlob(m_pendingBlob); } m_pending = m_next; diff --git a/src/plugins/platforms/drm/drm_object.h b/src/plugins/platforms/drm/drm_object.h index f8eb95ac00..889279dbfa 100644 --- a/src/plugins/platforms/drm/drm_object.h +++ b/src/plugins/platforms/drm/drm_object.h @@ -79,7 +79,7 @@ public: class Property { public: - Property(DrmObject *obj, drmModePropertyRes *prop, uint64_t val, const QVector &enumNames, drmModePropertyBlobRes *blob); + Property(DrmObject *obj, drmModePropertyRes *prop, uint64_t val, const QVector &enumNames); virtual ~Property(); void initEnumMap(drmModePropertyRes *prop); @@ -128,12 +128,10 @@ public: void setPending(uint64_t value); uint64_t pending() const; bool setPendingBlob(void *blob, size_t length); - drmModePropertyBlobRes *pendingBlob() const; void setCurrent(uint64_t value); uint64_t current() const; void setCurrentBlob(drmModePropertyBlobRes *blob); - drmModePropertyBlobRes *currentBlob() const; void commit(); void commitPending(); diff --git a/src/plugins/platforms/drm/drm_object_connector.cpp b/src/plugins/platforms/drm/drm_object_connector.cpp index dfa6ae46a9..f52da65bf9 100644 --- a/src/plugins/platforms/drm/drm_object_connector.cpp +++ b/src/plugins/platforms/drm/drm_object_connector.cpp @@ -100,15 +100,18 @@ bool DrmConnector::init() // parse edid auto edidProp = getProp(PropertyIndex::Edid); - if (edidProp && edidProp->currentBlob() && edidProp->currentBlob()->data) { - m_edid = Edid(edidProp->currentBlob()->data, edidProp->currentBlob()->length); - if (!m_edid.isValid()) { - qCWarning(KWIN_DRM, "Couldn't parse EDID for connector with id %d", id()); + if (edidProp) { + DrmScopedPointer blob(drmModeGetPropertyBlob(gpu()->fd(), edidProp->current())); + if (blob && blob->data) { + m_edid = Edid(blob->data, blob->length); + if (!m_edid.isValid()) { + qCWarning(KWIN_DRM, "Couldn't parse EDID for connector with id %d", id()); + } } + deleteProp(PropertyIndex::Edid); } else { qCDebug(KWIN_DRM) << "Could not find edid for connector" << this; } - deleteProp(PropertyIndex::Edid); // check the physical size if (m_edid.physicalSize().isEmpty()) { diff --git a/src/plugins/platforms/drm/drm_object_plane.cpp b/src/plugins/platforms/drm/drm_object_plane.cpp index d0b25881a1..000b28ef5a 100644 --- a/src/plugins/platforms/drm/drm_object_plane.cpp +++ b/src/plugins/platforms/drm/drm_object_plane.cpp @@ -74,25 +74,28 @@ bool DrmPlane::init() // read formats from blob if available and if modifiers are supported, and from the plane object if not if (auto formatProp = getProp(PropertyIndex::In_Formats); formatProp && gpu()->addFB2ModifiersSupported() && qEnvironmentVariableIntValue("KWIN_DRM_USE_MODIFIERS") == 1) { - auto blob = static_cast(formatProp->currentBlob()->data); - auto modifiers = reinterpret_cast(reinterpret_cast(blob) + blob->modifiers_offset); - uint32_t *formatarr = reinterpret_cast(reinterpret_cast(blob) + blob->formats_offset); + DrmScopedPointer propertyBlob(drmModeGetPropertyBlob(gpu()->fd(), formatProp->current())); + if (propertyBlob && propertyBlob->data) { + auto blob = static_cast(propertyBlob->data); + auto modifiers = reinterpret_cast(reinterpret_cast(blob) + blob->modifiers_offset); + uint32_t *formatarr = reinterpret_cast(reinterpret_cast(blob) + blob->formats_offset); - for (uint32_t f = 0; f < blob->count_formats; f++) { - auto format = formatarr[f]; - QVector mods; - for (uint32_t m = 0; m < blob->count_modifiers; m++) { - auto modifier = &modifiers[m]; - // The modifier advertisement blob is partitioned into groups of 64 formats - if (m < modifier->offset || m > modifier->offset + 63) { - continue; + for (uint32_t f = 0; f < blob->count_formats; f++) { + auto format = formatarr[f]; + QVector mods; + for (uint32_t m = 0; m < blob->count_modifiers; m++) { + auto modifier = &modifiers[m]; + // The modifier advertisement blob is partitioned into groups of 64 formats + if (m < modifier->offset || m > modifier->offset + 63) { + continue; + } + if (!(modifier->formats & (1 << (f - modifier->offset)))) { + continue; + } + mods << modifier->modifier; } - if (!(modifier->formats & (1 << (f - modifier->offset)))) { - continue; - } - mods << modifier->modifier; + m_supportedFormats.insert(format, mods); } - m_supportedFormats.insert(format, mods); } } else { for (uint32_t i = 0; i < p->count_formats; i++) {