From e6f41f5574dff8999153cf8747ee439a30ecd585 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 5 Jan 2026 18:20:39 -0600 Subject: [PATCH] Improve DRM renderer plane compatibility testing For atomic drivers, use a test-only commit to fully exercise the path that we will be using when rendering. For legacy drivers, mask the variable portion of Broadcom modifiers before matching with IN_FORMATS. --- app/streaming/video/ffmpeg-renderers/drm.cpp | 79 +++++++++++++++++++- app/streaming/video/ffmpeg-renderers/drm.h | 53 +++++++++++++ 2 files changed, 128 insertions(+), 4 deletions(-) diff --git a/app/streaming/video/ffmpeg-renderers/drm.cpp b/app/streaming/video/ffmpeg-renderers/drm.cpp index 92d3ae95..79a96f8f 100644 --- a/app/streaming/video/ffmpeg-renderers/drm.cpp +++ b/app/streaming/video/ffmpeg-renderers/drm.cpp @@ -1203,12 +1203,20 @@ bool DrmRenderer::addFbForFrame(AVFrame *frame, uint32_t* newFbId, bool testMode drmFrame = (AVDRMFrameDescriptor*)frame->data[0]; } - if (testMode) { - // Check if plane can actually be imported + // For non-atomic drivers, check the IN_FORMATS property or legacy plane formats + if (testMode && !m_PropSetter.isAtomic()) { bool formatMatch = false; + uint64_t maskedModifier = drmFrame->objects[0].format_modifier; + if (fourcc_mod_is_vendor(maskedModifier, BROADCOM)) { + // Broadcom has modifiers that contain variable data, so we need to mask + // off the variable data because the IN_FORMATS blob contains the just + // the base modifier alone + maskedModifier = fourcc_mod_broadcom_mod(maskedModifier); + } + // If we have an IN_FORMATS property and the frame has DRM modifiers, use that since it supports modifiers too - if (auto prop = m_VideoPlane.property("IN_FORMATS"); prop && drmFrame->objects[0].format_modifier != DRM_FORMAT_MOD_INVALID) { + if (auto prop = m_VideoPlane.property("IN_FORMATS"); prop && maskedModifier != DRM_FORMAT_MOD_INVALID) { drmModePropertyBlobPtr blob = drmModeGetPropertyBlob(m_DrmFd, prop->initialValue()); if (blob) { auto *header = (struct drm_format_modifier_blob *)blob->data; @@ -1216,7 +1224,7 @@ bool DrmRenderer::addFbForFrame(AVFrame *frame, uint32_t* newFbId, bool testMode uint32_t *formats = (uint32_t *)((char *)header + header->formats_offset); for (uint32_t i = 0; i < header->count_modifiers; i++) { - if (modifiers[i].modifier == drmFrame->objects[0].format_modifier) { + if (modifiers[i].modifier == maskedModifier) { for (uint32_t j = 0; j < header->count_formats && j < sizeof(modifiers[i].formats) * 8; j++) { if (modifiers[i].formats & (1ULL << j)) { if (formats[modifiers[i].offset + j] == drmFrame->layers[0].format) { @@ -1327,6 +1335,69 @@ bool DrmRenderer::addFbForFrame(AVFrame *frame, uint32_t* newFbId, bool testMode return false; } + // For atomic drivers, we'll use a test-only commit to confirm this plane+FB works + if (testMode && m_PropSetter.isAtomic()) { + SDL_Rect src, dst; + src.x = src.y = 0; + src.w = frame->width; + src.h = frame->height; + + // This isn't a completely accurate test since SDL hasn't modeset to the new + // display resolution that we'll actually be using for streaming (since we're + // still not committed to even using DrmRenderer for rendering yet). Hopefully, + // it will be good enough though. + dst.x = dst.y = 0; + drmModeCrtc* crtc = drmModeGetCrtc(m_DrmFd, m_Crtc.objectId()); + if (crtc != nullptr) { + dst.w = crtc->width; + dst.h = crtc->height; + drmModeFreeCrtc(crtc); + } + else { + SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, + "drmModeGetCrtc() failed: %d", + errno); + + // We'll just hope for the best here + dst.w = frame->width; + dst.h = frame->height; + } + + StreamUtils::scaleSourceToDestinationSurface(&src, &dst); + + // Temporarily take DRM master if we dropped it after initialization + if (!m_DrmStateModified) { + drmSetMaster(m_DrmFd); + } + bool testResult = m_PropSetter.testPlane(m_VideoPlane, + m_Crtc.objectId(), + *newFbId, + dst.x, dst.y, + dst.w, dst.h, + 0, 0, + frame->width << 16, + frame->height << 16); + if (!m_DrmStateModified) { + drmDropMaster(m_DrmFd); + } + + if (testResult) { + SDL_LogInfo(SDL_LOG_CATEGORY_APPLICATION, + "Selected DRM plane supports chosen decoding format and modifier: " FOURCC_FMT " %016" PRIx64, + FOURCC_FMT_ARGS(drmFrame->layers[0].format), + drmFrame->objects[0].format_modifier); + } + else { + SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, + "Selected DRM plane doesn't support chosen decoding format and modifier: " FOURCC_FMT " %016" PRIx64, + FOURCC_FMT_ARGS(drmFrame->layers[0].format), + drmFrame->objects[0].format_modifier); + drmModeRmFB(m_DrmFd, *newFbId); + *newFbId = 0; + return false; + } + } + return true; } diff --git a/app/streaming/video/ffmpeg-renderers/drm.h b/app/streaming/video/ffmpeg-renderers/drm.h index 38dab113..a27eb390 100644 --- a/app/streaming/video/ffmpeg-renderers/drm.h +++ b/app/streaming/video/ffmpeg-renderers/drm.h @@ -439,6 +439,59 @@ class DrmRenderer : public IFFmpegRenderer { } } + bool testPlane(const DrmPropertyMap& plane, + uint32_t crtcId, uint32_t fbId, + int32_t crtcX, int32_t crtcY, + uint32_t crtcW, uint32_t crtcH, + uint32_t srcX, uint32_t srcY, + uint32_t srcW, uint32_t srcH) + { + // Normally we wouldn't want to hold this lock across a blocking atomic commit, + // but we do in this case because this operation is infrequent and it significantly + // simplifies our state management to avoid racing atomic commits or modifications + // to our atomic request object. It's also only a test commit, so it should be fast. + std::lock_guard lg { m_Lock }; + + SDL_assert(m_Atomic); + + // Store the old atomic request to restore after testing + drmModeAtomicReqPtr oldReq = nullptr; + std::swap(oldReq, m_AtomicReq); + + bool ret = true; + ret = ret && set(*plane.property("CRTC_ID"), crtcId, false); + ret = ret && set(*plane.property("FB_ID"), fbId, false); + ret = ret && set(*plane.property("CRTC_X"), crtcX, false); + ret = ret && set(*plane.property("CRTC_Y"), crtcY, false); + ret = ret && set(*plane.property("CRTC_W"), crtcW, false); + ret = ret && set(*plane.property("CRTC_H"), crtcH, false); + ret = ret && set(*plane.property("SRC_X"), srcX, false); + ret = ret && set(*plane.property("SRC_Y"), srcY, false); + ret = ret && set(*plane.property("SRC_W"), srcW, false); + ret = ret && set(*plane.property("SRC_H"), srcH, false); + + if (ret) { + SDL_assert(m_AtomicReq); + int err = drmModeAtomicCommit(m_Fd, m_AtomicReq, + DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, + nullptr); + if (err < 0) { + SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, + "drmModeAtomicCommit(DRM_MODE_ATOMIC_TEST_ONLY) failed: %d", + err); + ret = false; + } + } + + // Swap the old atomic request back and free the test one + std::swap(oldReq, m_AtomicReq); + if (oldReq) { + drmModeAtomicFree(oldReq); + } + + return ret; + } + bool apply() { if (!m_Atomic) { return 0;