From 32d8f5310bffdeae30a500e2c0d6bbb59d795f0c Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Thu, 7 Nov 2019 17:03:34 +0100 Subject: eglfs: kms: Allow overriding plane ids per crtc With atomic enabled (QT_QPA_EGLFS_KMS_ATOMIC=1) the plane chosen for a crtc becomes important. The current logic is pretty broken when there are multiple planes available with many of them marked as being supported by multiple CRTCs. Choosing the same plane for multiple crtcs results in failing the atomic commit. This happens with a RPi4 with two screens connected, for example. (because there are 2*3 planes: one primary, one overlay, one cursor for each screen, but Qt is trying to use the same primary plane with both CRTCs) The issue does not surface in special environments where there is one (dedicated) plane per crtc exposed by drm, but becomes apparent on a PC or on the RPi where the setup described above is the common case. As a temporary solution allow doing the following: export QT_QPA_EGLFS_KMS_PLANES_FOR_CRTCS="49,28:78,57" This would then force using plane id 28 for crtc id 49, and plane id 57 for crtc id 78. (the ids can be discovered either from the eglfs debug logs, or by checking /sys/kernel/debug/dri//state) This is to be complemented with a fix for picking a unique primary plane for all CRTCs but that's going to be a separate patch. Allowing a manual override is important regardless since it helps troubleshooting. Task-number: QTBUG-74953 Change-Id: Ie03f80dac31813f2c4489235d435769dd3d3883e Reviewed-by: Johan Helsing --- src/platformsupport/kmsconvenience/qkmsdevice.cpp | 26 +++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) (limited to 'src/platformsupport/kmsconvenience/qkmsdevice.cpp') diff --git a/src/platformsupport/kmsconvenience/qkmsdevice.cpp b/src/platformsupport/kmsconvenience/qkmsdevice.cpp index 6121faf362..263c02682d 100644 --- a/src/platformsupport/kmsconvenience/qkmsdevice.cpp +++ b/src/platformsupport/kmsconvenience/qkmsdevice.cpp @@ -490,8 +490,30 @@ QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, } } - if (output.eglfs_plane) - qCDebug(qLcKmsDebug, "Output eglfs plane is: %d", output.eglfs_plane->id); + // A more useful version: allows specifying "crtc_id,plane_id:crtc_id,plane_id:..." + // in order to allow overriding the plane used for a given crtc. + if (qEnvironmentVariableIsSet("QT_QPA_EGLFS_KMS_PLANES_FOR_CRTCS")) { + const QString val = qEnvironmentVariable("QT_QPA_EGLFS_KMS_PLANES_FOR_CRTCS"); + qCDebug(qLcKmsDebug, "crtc_id:plane_id override list: %s", qPrintable(val)); + const QStringList crtcPlanePairs = val.split(QLatin1Char(':')); + for (const QString &crtcPlanePair : crtcPlanePairs) { + const QStringList values = crtcPlanePair.split(QLatin1Char(',')); + if (values.count() == 2 && uint(values[0].toInt()) == output.crtc_id) { + uint planeId = values[1].toInt(); + for (const QKmsPlane &kmsplane : qAsConst(m_planes)) { + if (kmsplane.id == planeId) { + output.eglfs_plane = (QKmsPlane*)&kmsplane; + break; + } + } + } + } + } + + if (output.eglfs_plane) { + qCDebug(qLcKmsDebug, "Chose plane %u for output %s (crtc id %u) (may not be applicable)", + output.eglfs_plane->id, connectorName.constData(), output.crtc_id); + } m_crtc_allocator |= (1 << output.crtc_index); -- cgit v1.2.3 From 2f81ad9d746a4949d7fd76e2aa26c2281fac5d53 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Fri, 8 Nov 2019 10:18:10 +0100 Subject: eglfs: kms: Choose unique primary planes for each crtc Otherwise we end up with using the same plane for multiple crtcs, which fails (with atomic - planes have no importance when atomic is not enabled). This fixes systems where we run with atomic enabled and there are multiple primary planes with their possible_crtcs matching multiple crtcs. Task-number: QTBUG-74953 Change-Id: I8bcbdd389265d09f8851187881102fb5b9a83b5c Reviewed-by: Johan Helsing --- src/platformsupport/kmsconvenience/qkmsdevice.cpp | 37 ++++++++++++++++++----- 1 file changed, 29 insertions(+), 8 deletions(-) (limited to 'src/platformsupport/kmsconvenience/qkmsdevice.cpp') diff --git a/src/platformsupport/kmsconvenience/qkmsdevice.cpp b/src/platformsupport/kmsconvenience/qkmsdevice.cpp index 263c02682d..90437117aa 100644 --- a/src/platformsupport/kmsconvenience/qkmsdevice.cpp +++ b/src/platformsupport/kmsconvenience/qkmsdevice.cpp @@ -176,6 +176,15 @@ static bool parseModeline(const QByteArray &text, drmModeModeInfoPtr mode) return true; } +static inline void assignPlane(QKmsOutput *output, QKmsPlane *plane) +{ + if (output->eglfs_plane) + output->eglfs_plane->activeCrtcId = 0; + + plane->activeCrtcId = output->crtc_id; + output->eglfs_plane = plane; +} + QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, drmModeConnectorPtr connector, ScreenInfo *vinfo) @@ -449,13 +458,16 @@ QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, #endif QString planeListStr; - for (const QKmsPlane &plane : qAsConst(m_planes)) { + for (QKmsPlane &plane : m_planes) { if (plane.possibleCrtcs & (1 << output.crtc_index)) { output.available_planes.append(plane); planeListStr.append(QString::number(plane.id)); planeListStr.append(QLatin1Char(' ')); - if (plane.type == QKmsPlane::PrimaryPlane) - output.eglfs_plane = (QKmsPlane*)&plane; + + // Choose the first primary plane that is not already assigned to + // another screen's associated crtc. + if (!output.eglfs_plane && plane.type == QKmsPlane::PrimaryPlane && !plane.activeCrtcId) + assignPlane(&output, &plane); } } qCDebug(qLcKmsDebug, "Output %s can use %d planes: %s", @@ -477,9 +489,11 @@ QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, qCDebug(qLcKmsDebug, "Forcing plane index %d, plane id %u (belongs to crtc id %u)", idx, plane->plane_id, plane->crtc_id); - for (const QKmsPlane &kmsplane : qAsConst(m_planes)) { - if (kmsplane.id == output.forced_plane_id) - output.eglfs_plane = (QKmsPlane*)&kmsplane; + for (QKmsPlane &kmsplane : m_planes) { + if (kmsplane.id == output.forced_plane_id) { + assignPlane(&output, &kmsplane); + break; + } } drmModeFreePlane(plane); @@ -500,9 +514,9 @@ QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, const QStringList values = crtcPlanePair.split(QLatin1Char(',')); if (values.count() == 2 && uint(values[0].toInt()) == output.crtc_id) { uint planeId = values[1].toInt(); - for (const QKmsPlane &kmsplane : qAsConst(m_planes)) { + for (QKmsPlane &kmsplane : m_planes) { if (kmsplane.id == planeId) { - output.eglfs_plane = (QKmsPlane*)&kmsplane; + assignPlane(&output, &kmsplane); break; } } @@ -515,6 +529,13 @@ QPlatformScreen *QKmsDevice::createScreenForConnector(drmModeResPtr resources, output.eglfs_plane->id, connectorName.constData(), output.crtc_id); } +#if QT_CONFIG(drm_atomic) + if (hasAtomicSupport() && !output.eglfs_plane) { + qCDebug(qLcKmsDebug, "No plane associated with output %s (crtc id %u) and atomic modesetting is enabled. This is bad.", + connectorName.constData(), output.crtc_id); + } +#endif + m_crtc_allocator |= (1 << output.crtc_index); vinfo->output = output; -- cgit v1.2.3 From 5bd48047de4abecc47187d938c5e6ed8b8304aaf Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Fri, 8 Nov 2019 11:41:16 +0100 Subject: eglfs: kms: Make threaded atomic drm work The atomic modesetting support was not prepared for page flips being issued from different (per-screen) threads. This could be seen with the threaded render loop of Qt Quick: having a QQuickWindow per screen means having a dedicated render thread for each screen. QKmsDevice used simply instance variables to keep track of the request. This leads to the commit failing with EBUSY sooner or later. Make the atomic request and related variables thread local. This prevents failing drmModeAtomicCommit() with 2 or more screens and the threaded render loop. It does not fix other potential issues when waiting for page flips to complete, that is to be tackled separately. Task-number: QTBUG-74953 Change-Id: I2dac10d5e9bdc0cb556ac78c9643c96d40d692e4 Reviewed-by: Johan Helsing --- src/platformsupport/kmsconvenience/qkmsdevice.cpp | 58 +++++++++++++---------- 1 file changed, 33 insertions(+), 25 deletions(-) (limited to 'src/platformsupport/kmsconvenience/qkmsdevice.cpp') diff --git a/src/platformsupport/kmsconvenience/qkmsdevice.cpp b/src/platformsupport/kmsconvenience/qkmsdevice.cpp index 90437117aa..92ba9fbcd9 100644 --- a/src/platformsupport/kmsconvenience/qkmsdevice.cpp +++ b/src/platformsupport/kmsconvenience/qkmsdevice.cpp @@ -581,10 +581,6 @@ QKmsDevice::QKmsDevice(QKmsScreenConfig *screenConfig, const QString &path) , m_path(path) , m_dri_fd(-1) , m_has_atomic_support(false) -#if QT_CONFIG(drm_atomic) - , m_atomic_request(nullptr) - , m_previous_request(nullptr) -#endif , m_crtc_allocator(0) { if (m_path.isEmpty()) { @@ -600,7 +596,7 @@ QKmsDevice::QKmsDevice(QKmsScreenConfig *screenConfig, const QString &path) QKmsDevice::~QKmsDevice() { #if QT_CONFIG(drm_atomic) - atomicReset(); + threadLocalAtomicReset(); #endif } @@ -940,39 +936,51 @@ bool QKmsDevice::hasAtomicSupport() } #if QT_CONFIG(drm_atomic) -drmModeAtomicReq * QKmsDevice::atomic_request() +drmModeAtomicReq *QKmsDevice::threadLocalAtomicRequest() { - if (!m_atomic_request && m_has_atomic_support) - m_atomic_request = drmModeAtomicAlloc(); + if (!m_has_atomic_support) + return nullptr; - return m_atomic_request; + AtomicReqs &a(m_atomicReqs.localData()); + if (!a.request) + a.request = drmModeAtomicAlloc(); + + return a.request; } -bool QKmsDevice::atomicCommit(void *user_data) +bool QKmsDevice::threadLocalAtomicCommit(void *user_data) { - if (m_atomic_request) { - int ret = drmModeAtomicCommit(m_dri_fd, m_atomic_request, - DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_ALLOW_MODESET, user_data); + if (!m_has_atomic_support) + return false; - if (ret) { - qWarning("Failed to commit atomic request (code=%d)", ret); - return false; - } + AtomicReqs &a(m_atomicReqs.localData()); + if (!a.request) + return false; - m_previous_request = m_atomic_request; - m_atomic_request = nullptr; + int ret = drmModeAtomicCommit(m_dri_fd, a.request, + DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_ALLOW_MODESET, + user_data); - return true; + if (ret) { + qWarning("Failed to commit atomic request (code=%d)", ret); + return false; } - return false; + a.previous_request = a.request; + a.request = nullptr; + + return true; } -void QKmsDevice::atomicReset() +void QKmsDevice::threadLocalAtomicReset() { - if (m_previous_request) { - drmModeAtomicFree(m_previous_request); - m_previous_request = nullptr; + if (!m_has_atomic_support) + return; + + AtomicReqs &a(m_atomicReqs.localData()); + if (a.previous_request) { + drmModeAtomicFree(a.previous_request); + a.previous_request = nullptr; } } #endif -- cgit v1.2.3 From af2daafde72db02454d24b7d691aa6861525ab99 Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Mon, 18 Nov 2019 17:01:26 +0100 Subject: Deprecate constructing QFlags from a pointer This was used to support QFlags f = 0 initialization, but with 0 used as a pointer literal now considered bad form, it had been changed many places to QFlags f = nullptr, which is meaningless and confusing. Change-Id: I4bc592151c255dc5cab1a232615caecc520f02e8 Reviewed-by: Thiago Macieira --- src/platformsupport/kmsconvenience/qkmsdevice.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/platformsupport/kmsconvenience/qkmsdevice.cpp') diff --git a/src/platformsupport/kmsconvenience/qkmsdevice.cpp b/src/platformsupport/kmsconvenience/qkmsdevice.cpp index 92ba9fbcd9..b820fafd50 100644 --- a/src/platformsupport/kmsconvenience/qkmsdevice.cpp +++ b/src/platformsupport/kmsconvenience/qkmsdevice.cpp @@ -873,7 +873,7 @@ void QKmsDevice::discoverPlanes() plane.type = QKmsPlane::Type(value); } else if (!strcmp(prop->name, "rotation")) { plane.initialRotation = QKmsPlane::Rotations(int(value)); - plane.availableRotations = 0; + plane.availableRotations = { }; if (propTypeIs(prop, DRM_MODE_PROP_BITMASK)) { for (int i = 0; i < prop->count_enums; ++i) plane.availableRotations |= QKmsPlane::Rotation(1 << prop->enums[i].value); -- cgit v1.2.3