From fc55f26f4209309702c46b49051270759cdf7b2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sawicz?= Date: Mon, 23 Sep 2024 18:24:30 +0200 Subject: [PATCH 01/14] debian: package atomic-kms --- debian/control | 15 +++++++++++++++ debian/mir-platform-graphics-atomic-kms22.install | 1 + debian/rules | 2 +- snap/snapcraft.yaml | 2 +- 4 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 debian/mir-platform-graphics-atomic-kms22.install diff --git a/debian/control b/debian/control index 6b1b2841e48..d69b46de8a1 100644 --- a/debian/control +++ b/debian/control @@ -297,6 +297,20 @@ Description: Display server for Ubuntu - platform library for X11 Contains the shared libraries required for the Mir server to interact with the X11 platform. +Package: mir-platform-graphics-atomic-kms22 +Section: libs +Architecture: linux-any +Multi-Arch: same +Pre-Depends: ${misc:Pre-Depends} +Depends: ${misc:Depends}, + ${shlibs:Depends}, +Description: Display server for Ubuntu - platform library for Atomic KMS + Mir is a display server running on linux systems, with a focus on efficiency, + robust operation and a well-defined driver model. + . + Contains the shared libraries required for the Mir server to interact with + the hardware platform using the Mesa drivers and Atomic KMS API. + Package: mir-platform-graphics-gbm-kms22 Section: libs Architecture: linux-any @@ -404,6 +418,7 @@ Architecture: linux-any Multi-Arch: same Pre-Depends: ${misc:Pre-Depends} Depends: ${misc:Depends}, + mir-platform-graphics-atomic-kms, mir-platform-graphics-gbm-kms, mir-platform-graphics-x, mir-platform-graphics-wayland, diff --git a/debian/mir-platform-graphics-atomic-kms22.install b/debian/mir-platform-graphics-atomic-kms22.install new file mode 100644 index 00000000000..be383231799 --- /dev/null +++ b/debian/mir-platform-graphics-atomic-kms22.install @@ -0,0 +1 @@ +usr/lib/*/mir/server-platform/graphics-atomic-kms.so.22 diff --git a/debian/rules b/debian/rules index 7d0e2472924..54340461efd 100755 --- a/debian/rules +++ b/debian/rules @@ -65,7 +65,7 @@ export DEB_BUILD_MAINT_OPTIONS $(info COMMON_CONFIGURE_OPTIONS: ${COMMON_CONFIGURE_OPTIONS}) $(info DEB_BUILD_MAINT_OPTIONS: ${DEB_BUILD_MAINT_OPTIONS}) -AVAILABLE_PLATFORMS=gbm-kms\;x11\;wayland\;eglstream-kms +AVAILABLE_PLATFORMS=atomic-kms\;gbm-kms\;x11\;wayland\;eglstream-kms override_dh_auto_configure: ifneq ($(filter armhf,$(DEB_HOST_ARCH)),) diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index 416ce428d94..d654b6ecf91 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -18,7 +18,7 @@ parts: cmake-parameters: - -DCMAKE_INSTALL_PREFIX=/usr - -DMIR_ENABLE_WLCS_TESTS=OFF - - -DMIR_PLATFORM='gbm-kms;eglstream-kms;x11;wayland' + - -DMIR_PLATFORM='atomic-kms;gbm-kms;eglstream-kms;x11;wayland' build-packages: - build-essential - eglexternalplatform-dev From 3e8e482a64b2bbf1f3d88e70c71177308e275ef2 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Wed, 11 Sep 2024 17:36:43 +0300 Subject: [PATCH 02/14] Add the plumbing needed to enable bypass `drmModeAddFB2` returns -2, which is `ENOENT` for some reason... --- include/platform/mir/graphics/platform.h | 4 +- .../atomic-kms/server/kms/display_buffer.cpp | 86 ++++++++++++++++++- .../atomic-kms/server/kms/display_sink.h | 26 +++++- .../gbm-kms/server/buffer_allocator.cpp | 29 ++++++- .../default_display_buffer_compositor.cpp | 7 ++ .../compositor/multi_threaded_compositor.cpp | 2 +- 6 files changed, 147 insertions(+), 7 deletions(-) diff --git a/include/platform/mir/graphics/platform.h b/include/platform/mir/graphics/platform.h index 4f8373bbc58..193e3988dcd 100644 --- a/include/platform/mir/graphics/platform.h +++ b/include/platform/mir/graphics/platform.h @@ -387,7 +387,7 @@ class GBMDisplayAllocator : public DisplayAllocator virtual auto make_surface(DRMFormat format, std::span modifiers) -> std::unique_ptr = 0; }; -class DmaBufBuffer; +class DMABufBuffer; class DmaBufDisplayAllocator : public DisplayAllocator { @@ -396,7 +396,7 @@ class DmaBufDisplayAllocator : public DisplayAllocator { }; - virtual auto framebuffer_for(std::shared_ptr buffer) + virtual auto framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr = 0; }; diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 4c5c17777df..e188226466f 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -37,10 +37,13 @@ #include #include #include +#include #include +#include #include #include +#include namespace mg = mir::graphics; namespace mga = mir::graphics::atomic; @@ -56,7 +59,8 @@ mga::DisplaySink::DisplaySink( std::vector> const& outputs, geom::Rectangle const& area, glm::mat2 const& transformation) - : gbm{std::move(gbm)}, + : DmaBufDisplayAllocator(drm_fd), + gbm{std::move(gbm)}, listener(listener), outputs(outputs), event_handler{std::move(event_handler)}, @@ -335,6 +339,78 @@ void mga::DisplaySink::set_next_image(std::unique_ptr content) } } +auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr +{ + auto plane_descriptors = buffer->planes(); + + assert(plane_descriptors.size() <= 4); + + auto width = buffer->size().width; + auto height = buffer->size().height; + auto pixel_format = buffer->format(); + + uint32_t bo_handles[4] = {0}; + uint32_t pitches[4] = {0}; + uint32_t offsets[4] = {0}; + + for (std::size_t i = 0; i < std::min(4zu, plane_descriptors.size()); i++) + { + bo_handles[i] = plane_descriptors[i].dma_buf; + pitches[i] = plane_descriptors[i].stride; + offsets[i] = plane_descriptors[i].offset; + } + + auto fb_id = std::shared_ptr{ + new uint32_t{0}, + [drm_fd = drm_fd()](uint32_t* fb_id) + { + if (*fb_id) + { + drmModeRmFB(drm_fd, *fb_id); + } + delete fb_id; + }}; + + int ret = drmModeAddFB2( + drm_fd(), + width.as_uint32_t(), + height.as_uint32_t(), + pixel_format, + bo_handles, + pitches, + offsets, + fb_id.get(), + 0); + + if (ret) + return {}; + + struct AtomicKmsFbHandle : public mg::FBHandle + { + AtomicKmsFbHandle(std::shared_ptr fb_handle, geometry::Size size) : + kms_fb_id{fb_handle}, + size_{size} + { + } + + virtual auto size() const -> geometry::Size override + { + return size_; + } + + virtual operator uint32_t() const override + { + return *kms_fb_id; + } + + private: + std::shared_ptr kms_fb_id; + geometry::Size size_; + }; + + return std::make_unique(fb_id, buffer->size()); +} + auto mga::DisplaySink::maybe_create_allocator(DisplayAllocator::Tag const& type_tag) -> DisplayAllocator* { @@ -354,5 +430,13 @@ auto mga::DisplaySink::maybe_create_allocator(DisplayAllocator::Tag const& type_ } return gbm_allocator.get(); } + if(dynamic_cast(&type_tag)) + { + if (!bypass_allocator) + { + bypass_allocator = std::make_shared(drm_fd()); + } + return bypass_allocator.get(); + } return nullptr; } diff --git a/src/platforms/atomic-kms/server/kms/display_sink.h b/src/platforms/atomic-kms/server/kms/display_sink.h index 7257ce5263b..8f7ce8a2d34 100644 --- a/src/platforms/atomic-kms/server/kms/display_sink.h +++ b/src/platforms/atomic-kms/server/kms/display_sink.h @@ -26,6 +26,7 @@ #include "platform_common.h" #include "kms_framebuffer.h" +#include #include #include #include @@ -50,8 +51,28 @@ namespace atomic class Platform; class KMSOutput; -class DisplaySink : public graphics::DisplaySink, - public graphics::DisplaySyncGroup +class DmaBufDisplayAllocator : public graphics::DmaBufDisplayAllocator +{ + public: + DmaBufDisplayAllocator(mir::Fd drm_fd) : drm_fd_{drm_fd} + { + } + + virtual auto framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr override; + + auto drm_fd() -> mir::Fd const + { + return drm_fd_; + } + + private: + mir::Fd const drm_fd_; +}; + +class DisplaySink : + public graphics::DisplaySink, + public graphics::DisplaySyncGroup, + public DmaBufDisplayAllocator { public: DisplaySink( @@ -103,6 +124,7 @@ class DisplaySink : public graphics::DisplaySink, std::shared_ptr const event_handler; + std::shared_ptr bypass_allocator; std::shared_ptr kms_allocator; std::unique_ptr gbm_allocator; diff --git a/src/platforms/gbm-kms/server/buffer_allocator.cpp b/src/platforms/gbm-kms/server/buffer_allocator.cpp index be849f41205..cbb7f7c3152 100644 --- a/src/platforms/gbm-kms/server/buffer_allocator.cpp +++ b/src/platforms/gbm-kms/server/buffer_allocator.cpp @@ -47,6 +47,7 @@ #include #include +#include #include #include #include @@ -552,9 +553,35 @@ auto mgg::GLRenderingProvider::surface_for_sink( config); } -auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& /*sink*/) +auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) -> std::unique_ptr { + if(auto* allocator = sink.acquire_compatible_allocator()) + { + struct FooFramebufferProvider: public FramebufferProvider + { + public: + FooFramebufferProvider(DmaBufDisplayAllocator* allocator) : allocator{allocator} + { + } + + auto buffer_to_framebuffer(std::shared_ptr buffer) -> std::unique_ptr override + { + if(auto dma_buf = std::dynamic_pointer_cast(buffer)) + { + return allocator->framebuffer_for(dma_buf); + } + + return {}; + } + + private: + DmaBufDisplayAllocator* allocator; + }; + + return std::make_unique(allocator); + } + // TODO: Make this not a null implementation, so bypass/overlays can work again class NullFramebufferProvider : public FramebufferProvider { diff --git a/src/server/compositor/default_display_buffer_compositor.cpp b/src/server/compositor/default_display_buffer_compositor.cpp index 84415efb628..cfffb43bb99 100644 --- a/src/server/compositor/default_display_buffer_compositor.cpp +++ b/src/server/compositor/default_display_buffer_compositor.cpp @@ -27,9 +27,13 @@ #include "mir/renderer/renderer.h" #include "occlusion.h" +#define MIR_LOG_COMPONENT "compositor" +#include "mir/log.h" + namespace mc = mir::compositor; namespace mg = mir::graphics; + mc::DefaultDisplayBufferCompositor::DefaultDisplayBufferCompositor( mg::DisplaySink& display_sink, graphics::GLRenderingProvider& gl_provider, @@ -110,11 +114,14 @@ bool mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& sc if (framebuffers.size() == renderable_list.size() && display_sink.overlay(framebuffers)) { + mir::log_debug("Bypass path\n"); report->renderables_in_frame(this, renderable_list); renderer->suspend(); } else { + mir::log_debug("composite path\n"); + renderer->set_output_transform(display_sink.transformation()); renderer->set_viewport(view_area); diff --git a/src/server/compositor/multi_threaded_compositor.cpp b/src/server/compositor/multi_threaded_compositor.cpp index 7c7d5960c19..6c6a075113a 100644 --- a/src/server/compositor/multi_threaded_compositor.cpp +++ b/src/server/compositor/multi_threaded_compositor.cpp @@ -197,7 +197,7 @@ class CompositingFunctor void wait_until_stopped() { stop(); - if (stopped_future.wait_for(10s) != std::future_status::ready) + if (stopped_future.wait_for(10h) != std::future_status::ready) BOOST_THROW_EXCEPTION(std::runtime_error("Compositor thread failed to stop")); stopped_future.get(); From 492219d558a66dce37857e8db669bd3382a18079 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Thu, 12 Sep 2024 13:11:26 +0300 Subject: [PATCH 03/14] Use GEM handles instead of DMABuf FDs --- .../atomic-kms/server/kms/display_buffer.cpp | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index e188226466f..d600abbd030 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -21,6 +21,7 @@ #include "gbm_display_allocator.h" #include "mir/fd.h" #include "mir/graphics/display_report.h" +#include "mir/graphics/drm_formats.h" #include "mir/graphics/platform.h" #include "mir/graphics/transformation.h" #include "bypass.h" @@ -40,9 +41,12 @@ #include #include +#include +#include #include #include #include +#include #include namespace mg = mir::graphics; @@ -349,13 +353,32 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr auto height = buffer->size().height; auto pixel_format = buffer->format(); - uint32_t bo_handles[4] = {0}; + uint32_t gem_handles[4] = {0}; uint32_t pitches[4] = {0}; uint32_t offsets[4] = {0}; - for (std::size_t i = 0; i < std::min(4zu, plane_descriptors.size()); i++) + uint64_t modifiers[4] = {}; + std::fill_n(modifiers, 4, 0); + + mir::log_debug( + "Buffer format %s", + mir::graphics::drm_format_to_string(buffer->format()) + /* mir::graphics::drm_modifier_to_string(buffer->modifier().value_or(0)).c_str(), */ + /* buffer->modifier().value_or(0) */ + ); + + for (std::size_t i = 0; i < std::min(1zu, plane_descriptors.size()); i++) { - bo_handles[i] = plane_descriptors[i].dma_buf; + uint32_t gem_handle = 0; + int ret = drmPrimeFDToHandle(drm_fd(), plane_descriptors[i].dma_buf, &gem_handle); + + if(ret) + { + mir::log_debug("Failed to convert buffer"); + return {}; + } + + gem_handles[i] = gem_handle; pitches[i] = plane_descriptors[i].stride; offsets[i] = plane_descriptors[i].offset; } @@ -371,19 +394,24 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr delete fb_id; }}; - int ret = drmModeAddFB2( + + int ret = drmModeAddFB2WithModifiers( drm_fd(), width.as_uint32_t(), height.as_uint32_t(), pixel_format, - bo_handles, + gem_handles, pitches, offsets, + modifiers, fb_id.get(), 0); if (ret) + { + mir::log_debug("drmModeAddFB2WithModifiers returned an error: %d", ret); return {}; + } struct AtomicKmsFbHandle : public mg::FBHandle { From a30c63706087faec948142609e5c66ef0ec364d7 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Thu, 12 Sep 2024 18:34:51 +0300 Subject: [PATCH 04/14] Sneak `DisplaySink` in and try to use the GBM path ( doesn't work :( ) --- include/platform/mir/graphics/platform.h | 2 +- .../atomic-kms/server/kms/display_buffer.cpp | 53 +++++++++++++------ .../atomic-kms/server/kms/display_sink.h | 2 +- .../gbm-kms/server/buffer_allocator.cpp | 7 +-- 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/include/platform/mir/graphics/platform.h b/include/platform/mir/graphics/platform.h index 193e3988dcd..5e5f709a8c9 100644 --- a/include/platform/mir/graphics/platform.h +++ b/include/platform/mir/graphics/platform.h @@ -396,7 +396,7 @@ class DmaBufDisplayAllocator : public DisplayAllocator { }; - virtual auto framebuffer_for(std::shared_ptr buffer) + virtual auto framebuffer_for(mir::graphics::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr = 0; }; diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index d600abbd030..831a79ed5ae 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -343,22 +343,27 @@ void mga::DisplaySink::set_next_image(std::unique_ptr content) } } -auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr +auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr { + DisplaySink* ds = dynamic_cast(&sink); + if(!ds) + return {}; + auto plane_descriptors = buffer->planes(); + auto const max_planes = 4zu; assert(plane_descriptors.size() <= 4); auto width = buffer->size().width; auto height = buffer->size().height; auto pixel_format = buffer->format(); - uint32_t gem_handles[4] = {0}; - uint32_t pitches[4] = {0}; - uint32_t offsets[4] = {0}; + int dmabuf_fds[4] = {0}; + int pitches[4] = {0}; + int offsets[4] = {0}; uint64_t modifiers[4] = {}; - std::fill_n(modifiers, 4, 0); + std::fill_n(modifiers, 4, buffer->modifier().value_or(DRM_FORMAT_MOD_NONE)); mir::log_debug( "Buffer format %s", @@ -367,22 +372,36 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr /* buffer->modifier().value_or(0) */ ); - for (std::size_t i = 0; i < std::min(1zu, plane_descriptors.size()); i++) + for (std::size_t i = 0; i < std::min(max_planes, plane_descriptors.size()); i++) { - uint32_t gem_handle = 0; - int ret = drmPrimeFDToHandle(drm_fd(), plane_descriptors[i].dma_buf, &gem_handle); - if(ret) - { - mir::log_debug("Failed to convert buffer"); - return {}; - } - - gem_handles[i] = gem_handle; + dmabuf_fds[i] = plane_descriptors[i].dma_buf; pitches[i] = plane_descriptors[i].stride; offsets[i] = plane_descriptors[i].offset; } + gbm_import_fd_modifier_data import_data = { + .width = buffer->size().width.as_uint32_t(), + .height = buffer->size().height.as_uint32_t(), + .format = buffer->format(), + .num_fds = static_cast(std::min(plane_descriptors.size(), max_planes)), + .fds = {dmabuf_fds[0], dmabuf_fds[1], dmabuf_fds[2], dmabuf_fds[3]}, + .strides = {pitches[0], pitches[1], pitches[2], pitches[3]}, + .offsets = {offsets[0], pitches[1], pitches[2], pitches[3]}, + .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), + }; + struct gbm_bo* gbm_bo = gbm_bo_import(ds->gbm_device().get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); + + if(!gbm_bo) + { + mir::log_debug("Failed to import buffer"); + return {}; + } + + uint32_t gem_handles[4] = {0}; + for (std::size_t i = 0; i < std::min(max_planes, plane_descriptors.size()); i++) + gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; + auto fb_id = std::shared_ptr{ new uint32_t{0}, [drm_fd = drm_fd()](uint32_t* fb_id) @@ -401,8 +420,8 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr height.as_uint32_t(), pixel_format, gem_handles, - pitches, - offsets, + (uint32_t*)pitches, + (uint32_t*)offsets, modifiers, fb_id.get(), 0); diff --git a/src/platforms/atomic-kms/server/kms/display_sink.h b/src/platforms/atomic-kms/server/kms/display_sink.h index 8f7ce8a2d34..b925dd87836 100644 --- a/src/platforms/atomic-kms/server/kms/display_sink.h +++ b/src/platforms/atomic-kms/server/kms/display_sink.h @@ -58,7 +58,7 @@ class DmaBufDisplayAllocator : public graphics::DmaBufDisplayAllocator { } - virtual auto framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr override; + virtual auto framebuffer_for(mir::graphics::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr override; auto drm_fd() -> mir::Fd const { diff --git a/src/platforms/gbm-kms/server/buffer_allocator.cpp b/src/platforms/gbm-kms/server/buffer_allocator.cpp index cbb7f7c3152..5792b216aa3 100644 --- a/src/platforms/gbm-kms/server/buffer_allocator.cpp +++ b/src/platforms/gbm-kms/server/buffer_allocator.cpp @@ -561,7 +561,7 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) struct FooFramebufferProvider: public FramebufferProvider { public: - FooFramebufferProvider(DmaBufDisplayAllocator* allocator) : allocator{allocator} + FooFramebufferProvider(DmaBufDisplayAllocator* allocator, mg::DisplaySink& sink) : allocator{allocator}, sink{sink} { } @@ -569,7 +569,7 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) { if(auto dma_buf = std::dynamic_pointer_cast(buffer)) { - return allocator->framebuffer_for(dma_buf); + return allocator->framebuffer_for(sink, dma_buf); } return {}; @@ -577,9 +577,10 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) private: DmaBufDisplayAllocator* allocator; + mg::DisplaySink& sink; }; - return std::make_unique(allocator); + return std::make_unique(allocator, sink); } // TODO: Make this not a null implementation, so bypass/overlays can work again From bd21b95edbd41d337922a479f6bceaa5c3af7666 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Fri, 13 Sep 2024 11:39:10 +0300 Subject: [PATCH 05/14] Bypass working LETS GOOOO --- src/platforms/atomic-kms/server/kms/display_buffer.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 831a79ed5ae..4674c54e5d6 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -361,9 +361,7 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::sh int dmabuf_fds[4] = {0}; int pitches[4] = {0}; int offsets[4] = {0}; - - uint64_t modifiers[4] = {}; - std::fill_n(modifiers, 4, buffer->modifier().value_or(DRM_FORMAT_MOD_NONE)); + uint64_t modifiers[4] = {0}; mir::log_debug( "Buffer format %s", @@ -378,6 +376,7 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::sh dmabuf_fds[i] = plane_descriptors[i].dma_buf; pitches[i] = plane_descriptors[i].stride; offsets[i] = plane_descriptors[i].offset; + modifiers[i] = buffer->modifier().value_or(DRM_FORMAT_MOD_INVALID); } gbm_import_fd_modifier_data import_data = { @@ -424,7 +423,7 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::sh (uint32_t*)offsets, modifiers, fb_id.get(), - 0); + DRM_MODE_FB_MODIFIERS); if (ret) { From 27205d8231e30bd2dc1fc5d8cfa38fb211984c88 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Fri, 13 Sep 2024 14:06:09 +0300 Subject: [PATCH 06/14] Call `DmabufTexBuffer::on_consume` in bypass path. --- include/platform/mir/graphics/dmabuf_buffer.h | 2 ++ src/platform/graphics/linux_dmabuf.cpp | 11 ++++++++--- .../compositor/default_display_buffer_compositor.cpp | 7 +++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/include/platform/mir/graphics/dmabuf_buffer.h b/include/platform/mir/graphics/dmabuf_buffer.h index 347f64aa859..c0daea68528 100644 --- a/include/platform/mir/graphics/dmabuf_buffer.h +++ b/include/platform/mir/graphics/dmabuf_buffer.h @@ -62,6 +62,8 @@ class DMABufBuffer : public NativeBufferBase virtual auto layout() const -> gl::Texture::Layout = 0; virtual auto size() const -> geometry::Size = 0; + + virtual void on_consumed() {}; }; } } diff --git a/src/platform/graphics/linux_dmabuf.cpp b/src/platform/graphics/linux_dmabuf.cpp index 9eea3a8bb4a..ca31506c239 100644 --- a/src/platform/graphics/linux_dmabuf.cpp +++ b/src/platform/graphics/linux_dmabuf.cpp @@ -970,7 +970,7 @@ class DmabufTexBuffer : : dpy{dpy}, tex{dpy, extensions, dma_buf, descriptor, std::move(egl_delegate)}, provider_{std::move(provider)}, - on_consumed{std::move(on_consumed)}, + on_consumed_{std::move(on_consumed)}, on_release{std::move(on_release)}, size_{dma_buf.size()}, has_alpha{format_has_known_alpha(dma_buf.format()).value_or(true)}, // Has-alpha is the safe default for unknown formats @@ -1017,6 +1017,12 @@ class DmabufTexBuffer : return this; } + void on_consumed() override + { + on_consumed_(); + on_consumed_ = [](){}; + } + auto as_texture() -> DMABufTex* { /* We only get asked for a texture when the Renderer is about to @@ -1025,7 +1031,6 @@ class DmabufTexBuffer : */ std::lock_guard lock{consumed_mutex}; on_consumed(); - on_consumed = [](){}; return &tex; } @@ -1061,7 +1066,7 @@ class DmabufTexBuffer : std::shared_ptr const provider_; std::mutex consumed_mutex; - std::function on_consumed; + std::function on_consumed_; std::function const on_release; geom::Size const size_; diff --git a/src/server/compositor/default_display_buffer_compositor.cpp b/src/server/compositor/default_display_buffer_compositor.cpp index cfffb43bb99..6fbd8072b23 100644 --- a/src/server/compositor/default_display_buffer_compositor.cpp +++ b/src/server/compositor/default_display_buffer_compositor.cpp @@ -19,6 +19,7 @@ #include "mir/compositor/compositor_report.h" #include "mir/compositor/scene.h" #include "mir/compositor/scene_element.h" +#include "mir/graphics/dmabuf_buffer.h" #include "mir/graphics/renderable.h" #include "mir/graphics/display_sink.h" #include "mir/graphics/buffer.h" @@ -26,6 +27,7 @@ #include "mir/compositor/buffer_stream.h" #include "mir/renderer/renderer.h" #include "occlusion.h" +#include #define MIR_LOG_COMPONENT "compositor" #include "mir/log.h" @@ -116,6 +118,11 @@ bool mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& sc { mir::log_debug("Bypass path\n"); report->renderables_in_frame(this, renderable_list); + + // TODO: This is ugly, but its here for the purpose of showing more than one or two frames + if (auto dmabuf = std::dynamic_pointer_cast(renderable_list[0]->buffer())) + dmabuf->on_consumed(); + renderer->suspend(); } else From 3879115dda4aaef7b8d81e56195fb8a8e8b60f0a Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Fri, 13 Sep 2024 14:07:49 +0300 Subject: [PATCH 07/14] split `mga::DmaBufDisplayAllocator::framebuffer_for` into more managable pieces --- .../atomic-kms/server/kms/display_buffer.cpp | 170 ++++++++++-------- 1 file changed, 96 insertions(+), 74 deletions(-) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 4674c54e5d6..6a6d5439b41 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -33,6 +33,7 @@ #include "mir/graphics/gl_config.h" #include "mir/graphics/dmabuf_buffer.h" +#include #include #include #include @@ -343,93 +344,114 @@ void mga::DisplaySink::set_next_image(std::unique_ptr content) } } -auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr -{ - DisplaySink* ds = dynamic_cast(&sink); - if(!ds) - return {}; - - auto plane_descriptors = buffer->planes(); +namespace { + auto const MAX_PLANES = 4zu; - auto const max_planes = 4zu; - assert(plane_descriptors.size() <= 4); - - auto width = buffer->size().width; - auto height = buffer->size().height; - auto pixel_format = buffer->format(); - - int dmabuf_fds[4] = {0}; - int pitches[4] = {0}; - int offsets[4] = {0}; - uint64_t modifiers[4] = {0}; + auto get_import_buffers(std::shared_ptr buffer) + -> std::tuple, std::array, std::array, std::array> + { + auto const plane_descriptors = buffer->planes(); + assert(plane_descriptors.size() <= MAX_PLANES); - mir::log_debug( - "Buffer format %s", - mir::graphics::drm_format_to_string(buffer->format()) - /* mir::graphics::drm_modifier_to_string(buffer->modifier().value_or(0)).c_str(), */ - /* buffer->modifier().value_or(0) */ - ); + // std::array becuase we can't really return int[4] + std::array dmabuf_fds = {0}; + std::array pitches = {0}; + std::array offsets = {0}; + std::array modifiers = {0}; - for (std::size_t i = 0; i < std::min(max_planes, plane_descriptors.size()); i++) - { + for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) + { + dmabuf_fds[i] = plane_descriptors[i].dma_buf; + pitches[i] = plane_descriptors[i].stride; + offsets[i] = plane_descriptors[i].offset; + modifiers[i] = buffer->modifier().value_or(DRM_FORMAT_MOD_INVALID); + } - dmabuf_fds[i] = plane_descriptors[i].dma_buf; - pitches[i] = plane_descriptors[i].stride; - offsets[i] = plane_descriptors[i].offset; - modifiers[i] = buffer->modifier().value_or(DRM_FORMAT_MOD_INVALID); + return std::make_tuple(dmabuf_fds, pitches, offsets, modifiers); } - gbm_import_fd_modifier_data import_data = { - .width = buffer->size().width.as_uint32_t(), - .height = buffer->size().height.as_uint32_t(), - .format = buffer->format(), - .num_fds = static_cast(std::min(plane_descriptors.size(), max_planes)), - .fds = {dmabuf_fds[0], dmabuf_fds[1], dmabuf_fds[2], dmabuf_fds[3]}, - .strides = {pitches[0], pitches[1], pitches[2], pitches[3]}, - .offsets = {offsets[0], pitches[1], pitches[2], pitches[3]}, - .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), - }; - struct gbm_bo* gbm_bo = gbm_bo_import(ds->gbm_device().get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); - - if(!gbm_bo) + auto import_gbm_bo( + mg::DisplaySink const& sink, + std::shared_ptr buffer, + std::array dmabuf_fds, + std::array pitches, + std::array offsets) -> struct gbm_bo* { - mir::log_debug("Failed to import buffer"); - return {}; + auto* ds = dynamic_cast(&sink); + if (!ds) + return nullptr; + + auto const plane_descriptors = buffer->planes(); + + gbm_import_fd_modifier_data import_data = { + .width = buffer->size().width.as_uint32_t(), + .height = buffer->size().height.as_uint32_t(), + .format = buffer->format(), + .num_fds = static_cast(std::min(plane_descriptors.size(), MAX_PLANES)), + .fds = {dmabuf_fds[0], dmabuf_fds[1], dmabuf_fds[2], dmabuf_fds[3]}, + .strides = {pitches[0], pitches[1], pitches[2], pitches[3]}, + .offsets = {offsets[0], offsets[1], offsets[2], offsets[3]}, + .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), + }; + + return gbm_bo_import( + ds->gbm_device().get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); } - uint32_t gem_handles[4] = {0}; - for (std::size_t i = 0; i < std::min(max_planes, plane_descriptors.size()); i++) - gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; + auto drm_fb_id_from_dma_buffer(mir::Fd drm_fd, mg::DisplaySink const& sink, std::shared_ptr buffer) + -> std::shared_ptr + { - auto fb_id = std::shared_ptr{ - new uint32_t{0}, - [drm_fd = drm_fd()](uint32_t* fb_id) + auto [dmabuf_fds, pitches, offsets, modifiers] = get_import_buffers(buffer); + auto* gbm_bo = import_gbm_bo(sink, buffer, dmabuf_fds, pitches, offsets); + if (!gbm_bo) { - if (*fb_id) + mir::log_warning("Failed to import buffer"); + return {}; + } + + auto const plane_descriptors = buffer->planes(); + uint32_t gem_handles[MAX_PLANES] = {0}; + for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) + gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; + + auto fb_id = std::shared_ptr{ + new uint32_t{0}, + [drm_fd](uint32_t* fb_id) { - drmModeRmFB(drm_fd, *fb_id); - } - delete fb_id; - }}; - - - int ret = drmModeAddFB2WithModifiers( - drm_fd(), - width.as_uint32_t(), - height.as_uint32_t(), - pixel_format, - gem_handles, - (uint32_t*)pitches, - (uint32_t*)offsets, - modifiers, - fb_id.get(), - DRM_MODE_FB_MODIFIERS); - - if (ret) - { - mir::log_debug("drmModeAddFB2WithModifiers returned an error: %d", ret); - return {}; + if (*fb_id) + { + drmModeRmFB(drm_fd, *fb_id); + } + delete fb_id; + }}; + + auto [width, height] = buffer->size(); + int ret = drmModeAddFB2WithModifiers( + drm_fd, + width.as_uint32_t(), + height.as_uint32_t(), + buffer->format(), + gem_handles, + reinterpret_cast(pitches.data()), + reinterpret_cast(offsets.data()), + modifiers.data(), + fb_id.get(), + DRM_MODE_FB_MODIFIERS); + + if (ret) + { + mir::log_warning("drmModeAddFB2WithModifiers returned an error: %d", ret); + return {}; + } + + return fb_id; } +} + +auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr +{ + auto fb_id = drm_fb_id_from_dma_buffer(drm_fd(), sink, buffer); struct AtomicKmsFbHandle : public mg::FBHandle { From 7ffb745bc6d1b0fbbc313fc7d0259a7fb0c32685 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Fri, 13 Sep 2024 14:43:50 +0300 Subject: [PATCH 08/14] Remove now unnecessary debug logs --- src/server/compositor/default_display_buffer_compositor.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/server/compositor/default_display_buffer_compositor.cpp b/src/server/compositor/default_display_buffer_compositor.cpp index 6fbd8072b23..2df77a0c386 100644 --- a/src/server/compositor/default_display_buffer_compositor.cpp +++ b/src/server/compositor/default_display_buffer_compositor.cpp @@ -116,7 +116,6 @@ bool mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& sc if (framebuffers.size() == renderable_list.size() && display_sink.overlay(framebuffers)) { - mir::log_debug("Bypass path\n"); report->renderables_in_frame(this, renderable_list); // TODO: This is ugly, but its here for the purpose of showing more than one or two frames @@ -127,8 +126,6 @@ bool mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& sc } else { - mir::log_debug("composite path\n"); - renderer->set_output_transform(display_sink.transformation()); renderer->set_viewport(view_area); From ff8bf02eea9043f7da2ace77ba0b1e59b4d7e8f4 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Mon, 23 Sep 2024 18:34:51 +0300 Subject: [PATCH 09/14] Pass `gbm_device` directly instead of smuggling it through `DisplaySink` --- include/platform/mir/graphics/platform.h | 2 +- .../atomic-kms/server/kms/display_buffer.cpp | 21 +++++++------------ .../atomic-kms/server/kms/display_sink.h | 7 +++++-- .../gbm-kms/server/buffer_allocator.cpp | 7 +++---- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/include/platform/mir/graphics/platform.h b/include/platform/mir/graphics/platform.h index 5e5f709a8c9..193e3988dcd 100644 --- a/include/platform/mir/graphics/platform.h +++ b/include/platform/mir/graphics/platform.h @@ -396,7 +396,7 @@ class DmaBufDisplayAllocator : public DisplayAllocator { }; - virtual auto framebuffer_for(mir::graphics::DisplaySink& sink, std::shared_ptr buffer) + virtual auto framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr = 0; }; diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 6a6d5439b41..66cab58630b 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -64,7 +64,7 @@ mga::DisplaySink::DisplaySink( std::vector> const& outputs, geom::Rectangle const& area, glm::mat2 const& transformation) - : DmaBufDisplayAllocator(drm_fd), + : DmaBufDisplayAllocator(gbm, drm_fd), gbm{std::move(gbm)}, listener(listener), outputs(outputs), @@ -371,16 +371,12 @@ namespace { } auto import_gbm_bo( - mg::DisplaySink const& sink, + std::shared_ptr gbm, std::shared_ptr buffer, std::array dmabuf_fds, std::array pitches, std::array offsets) -> struct gbm_bo* { - auto* ds = dynamic_cast(&sink); - if (!ds) - return nullptr; - auto const plane_descriptors = buffer->planes(); gbm_import_fd_modifier_data import_data = { @@ -394,16 +390,15 @@ namespace { .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), }; - return gbm_bo_import( - ds->gbm_device().get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); + return gbm_bo_import(gbm.get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); } - auto drm_fb_id_from_dma_buffer(mir::Fd drm_fd, mg::DisplaySink const& sink, std::shared_ptr buffer) + auto drm_fb_id_from_dma_buffer(mir::Fd drm_fd, std::shared_ptr const gbm, std::shared_ptr buffer) -> std::shared_ptr { auto [dmabuf_fds, pitches, offsets, modifiers] = get_import_buffers(buffer); - auto* gbm_bo = import_gbm_bo(sink, buffer, dmabuf_fds, pitches, offsets); + auto* gbm_bo = import_gbm_bo(gbm, buffer, dmabuf_fds, pitches, offsets); if (!gbm_bo) { mir::log_warning("Failed to import buffer"); @@ -449,9 +444,9 @@ namespace { } } -auto mga::DmaBufDisplayAllocator::framebuffer_for(mg::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr +auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr { - auto fb_id = drm_fb_id_from_dma_buffer(drm_fd(), sink, buffer); + auto fb_id = drm_fb_id_from_dma_buffer(drm_fd(), gbm, buffer); struct AtomicKmsFbHandle : public mg::FBHandle { @@ -502,7 +497,7 @@ auto mga::DisplaySink::maybe_create_allocator(DisplayAllocator::Tag const& type_ { if (!bypass_allocator) { - bypass_allocator = std::make_shared(drm_fd()); + bypass_allocator = std::make_shared(gbm, drm_fd()); } return bypass_allocator.get(); } diff --git a/src/platforms/atomic-kms/server/kms/display_sink.h b/src/platforms/atomic-kms/server/kms/display_sink.h index b925dd87836..45ccefe664d 100644 --- a/src/platforms/atomic-kms/server/kms/display_sink.h +++ b/src/platforms/atomic-kms/server/kms/display_sink.h @@ -54,11 +54,13 @@ class KMSOutput; class DmaBufDisplayAllocator : public graphics::DmaBufDisplayAllocator { public: - DmaBufDisplayAllocator(mir::Fd drm_fd) : drm_fd_{drm_fd} + DmaBufDisplayAllocator(std::shared_ptr const gbm, mir::Fd drm_fd) : + drm_fd_{drm_fd}, + gbm{gbm} { } - virtual auto framebuffer_for(mir::graphics::DisplaySink& sink, std::shared_ptr buffer) -> std::unique_ptr override; + virtual auto framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr override; auto drm_fd() -> mir::Fd const { @@ -67,6 +69,7 @@ class DmaBufDisplayAllocator : public graphics::DmaBufDisplayAllocator private: mir::Fd const drm_fd_; + std::shared_ptr const gbm; }; class DisplaySink : diff --git a/src/platforms/gbm-kms/server/buffer_allocator.cpp b/src/platforms/gbm-kms/server/buffer_allocator.cpp index 5792b216aa3..cbb7f7c3152 100644 --- a/src/platforms/gbm-kms/server/buffer_allocator.cpp +++ b/src/platforms/gbm-kms/server/buffer_allocator.cpp @@ -561,7 +561,7 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) struct FooFramebufferProvider: public FramebufferProvider { public: - FooFramebufferProvider(DmaBufDisplayAllocator* allocator, mg::DisplaySink& sink) : allocator{allocator}, sink{sink} + FooFramebufferProvider(DmaBufDisplayAllocator* allocator) : allocator{allocator} { } @@ -569,7 +569,7 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) { if(auto dma_buf = std::dynamic_pointer_cast(buffer)) { - return allocator->framebuffer_for(sink, dma_buf); + return allocator->framebuffer_for(dma_buf); } return {}; @@ -577,10 +577,9 @@ auto mgg::GLRenderingProvider::make_framebuffer_provider(DisplaySink& sink) private: DmaBufDisplayAllocator* allocator; - mg::DisplaySink& sink; }; - return std::make_unique(allocator, sink); + return std::make_unique(allocator); } // TODO: Make this not a null implementation, so bypass/overlays can work again From 96ccdf1d9caa74659bcc8936ef264a31bd91a97b Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Tue, 8 Oct 2024 14:15:24 +0300 Subject: [PATCH 10/14] Remove unneedeed `DMABufBuffer::on_consumed` --- src/server/compositor/default_display_buffer_compositor.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/server/compositor/default_display_buffer_compositor.cpp b/src/server/compositor/default_display_buffer_compositor.cpp index 2df77a0c386..a38401e68a4 100644 --- a/src/server/compositor/default_display_buffer_compositor.cpp +++ b/src/server/compositor/default_display_buffer_compositor.cpp @@ -117,11 +117,6 @@ bool mc::DefaultDisplayBufferCompositor::composite(mc::SceneElementSequence&& sc if (framebuffers.size() == renderable_list.size() && display_sink.overlay(framebuffers)) { report->renderables_in_frame(this, renderable_list); - - // TODO: This is ugly, but its here for the purpose of showing more than one or two frames - if (auto dmabuf = std::dynamic_pointer_cast(renderable_list[0]->buffer())) - dmabuf->on_consumed(); - renderer->suspend(); } else From 65cab50cbbd082862ad45290e166b737dc80e4a6 Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Mon, 14 Oct 2024 13:16:23 +0300 Subject: [PATCH 11/14] Fix atomic-kms `display_buffer.cpp` anonymous namespace indentation Removed some unused headers as well :) --- .../atomic-kms/server/kms/display_buffer.cpp | 175 +++++++++--------- 1 file changed, 84 insertions(+), 91 deletions(-) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 66cab58630b..ab9b2bd52a7 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -23,14 +23,7 @@ #include "mir/graphics/display_report.h" #include "mir/graphics/drm_formats.h" #include "mir/graphics/platform.h" -#include "mir/graphics/transformation.h" -#include "bypass.h" -#include "mir/fatal.h" #include "mir/log.h" -#include "display_helpers.h" -#include "egl_helper.h" -#include "mir/graphics/egl_error.h" -#include "mir/graphics/gl_config.h" #include "mir/graphics/dmabuf_buffer.h" #include @@ -345,103 +338,103 @@ void mga::DisplaySink::set_next_image(std::unique_ptr content) } namespace { - auto const MAX_PLANES = 4zu; +auto const MAX_PLANES = 4zu; - auto get_import_buffers(std::shared_ptr buffer) - -> std::tuple, std::array, std::array, std::array> +auto get_import_buffers(std::shared_ptr buffer) + -> std::tuple, std::array, std::array, std::array> +{ + auto const plane_descriptors = buffer->planes(); + assert(plane_descriptors.size() <= MAX_PLANES); + + // std::array becuase we can't really return int[4] + std::array dmabuf_fds = {0}; + std::array pitches = {0}; + std::array offsets = {0}; + std::array modifiers = {0}; + + for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) { - auto const plane_descriptors = buffer->planes(); - assert(plane_descriptors.size() <= MAX_PLANES); + dmabuf_fds[i] = plane_descriptors[i].dma_buf; + pitches[i] = plane_descriptors[i].stride; + offsets[i] = plane_descriptors[i].offset; + modifiers[i] = buffer->modifier().value_or(DRM_FORMAT_MOD_INVALID); + } - // std::array becuase we can't really return int[4] - std::array dmabuf_fds = {0}; - std::array pitches = {0}; - std::array offsets = {0}; - std::array modifiers = {0}; + return std::make_tuple(dmabuf_fds, pitches, offsets, modifiers); +} - for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) - { - dmabuf_fds[i] = plane_descriptors[i].dma_buf; - pitches[i] = plane_descriptors[i].stride; - offsets[i] = plane_descriptors[i].offset; - modifiers[i] = buffer->modifier().value_or(DRM_FORMAT_MOD_INVALID); - } +auto import_gbm_bo( + std::shared_ptr gbm, + std::shared_ptr buffer, + std::array dmabuf_fds, + std::array pitches, + std::array offsets) -> struct gbm_bo* +{ + auto const plane_descriptors = buffer->planes(); + + gbm_import_fd_modifier_data import_data = { + .width = buffer->size().width.as_uint32_t(), + .height = buffer->size().height.as_uint32_t(), + .format = buffer->format(), + .num_fds = static_cast(std::min(plane_descriptors.size(), MAX_PLANES)), + .fds = {dmabuf_fds[0], dmabuf_fds[1], dmabuf_fds[2], dmabuf_fds[3]}, + .strides = {pitches[0], pitches[1], pitches[2], pitches[3]}, + .offsets = {offsets[0], offsets[1], offsets[2], offsets[3]}, + .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), + }; - return std::make_tuple(dmabuf_fds, pitches, offsets, modifiers); - } + return gbm_bo_import(gbm.get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); +} - auto import_gbm_bo( - std::shared_ptr gbm, - std::shared_ptr buffer, - std::array dmabuf_fds, - std::array pitches, - std::array offsets) -> struct gbm_bo* +auto drm_fb_id_from_dma_buffer( + mir::Fd drm_fd, std::shared_ptr const gbm, std::shared_ptr buffer) + -> std::shared_ptr +{ + auto [dmabuf_fds, pitches, offsets, modifiers] = get_import_buffers(buffer); + auto* gbm_bo = import_gbm_bo(gbm, buffer, dmabuf_fds, pitches, offsets); + if (!gbm_bo) { - auto const plane_descriptors = buffer->planes(); - - gbm_import_fd_modifier_data import_data = { - .width = buffer->size().width.as_uint32_t(), - .height = buffer->size().height.as_uint32_t(), - .format = buffer->format(), - .num_fds = static_cast(std::min(plane_descriptors.size(), MAX_PLANES)), - .fds = {dmabuf_fds[0], dmabuf_fds[1], dmabuf_fds[2], dmabuf_fds[3]}, - .strides = {pitches[0], pitches[1], pitches[2], pitches[3]}, - .offsets = {offsets[0], offsets[1], offsets[2], offsets[3]}, - .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), - }; - - return gbm_bo_import(gbm.get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); + mir::log_warning("Failed to import buffer"); + return {}; } - auto drm_fb_id_from_dma_buffer(mir::Fd drm_fd, std::shared_ptr const gbm, std::shared_ptr buffer) - -> std::shared_ptr - { + auto const plane_descriptors = buffer->planes(); + uint32_t gem_handles[MAX_PLANES] = {0}; + for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) + gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; - auto [dmabuf_fds, pitches, offsets, modifiers] = get_import_buffers(buffer); - auto* gbm_bo = import_gbm_bo(gbm, buffer, dmabuf_fds, pitches, offsets); - if (!gbm_bo) + auto fb_id = std::shared_ptr{ + new uint32_t{0}, + [drm_fd](uint32_t* fb_id) { - mir::log_warning("Failed to import buffer"); - return {}; - } - - auto const plane_descriptors = buffer->planes(); - uint32_t gem_handles[MAX_PLANES] = {0}; - for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) - gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; - - auto fb_id = std::shared_ptr{ - new uint32_t{0}, - [drm_fd](uint32_t* fb_id) + if (*fb_id) { - if (*fb_id) - { - drmModeRmFB(drm_fd, *fb_id); - } - delete fb_id; - }}; - - auto [width, height] = buffer->size(); - int ret = drmModeAddFB2WithModifiers( - drm_fd, - width.as_uint32_t(), - height.as_uint32_t(), - buffer->format(), - gem_handles, - reinterpret_cast(pitches.data()), - reinterpret_cast(offsets.data()), - modifiers.data(), - fb_id.get(), - DRM_MODE_FB_MODIFIERS); - - if (ret) - { - mir::log_warning("drmModeAddFB2WithModifiers returned an error: %d", ret); - return {}; - } - - return fb_id; + drmModeRmFB(drm_fd, *fb_id); + } + delete fb_id; + }}; + + auto [width, height] = buffer->size(); + int ret = drmModeAddFB2WithModifiers( + drm_fd, + width.as_uint32_t(), + height.as_uint32_t(), + buffer->format(), + gem_handles, + reinterpret_cast(pitches.data()), + reinterpret_cast(offsets.data()), + modifiers.data(), + fb_id.get(), + DRM_MODE_FB_MODIFIERS); + + if (ret) + { + mir::log_warning("drmModeAddFB2WithModifiers returned an error: %d", ret); + return {}; } + + return fb_id; +} } auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr buffer) -> std::unique_ptr From c16bc1ffe74e667f5a7a8f6ff445e54f09ef387e Mon Sep 17 00:00:00 2001 From: Tarek Ismail Date: Mon, 14 Oct 2024 13:41:33 +0300 Subject: [PATCH 12/14] Release `gbm_bo` used for bypass when framebuffer is released --- .../atomic-kms/server/kms/display_buffer.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index ab9b2bd52a7..0b7371d3e18 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -368,7 +368,7 @@ auto import_gbm_bo( std::shared_ptr buffer, std::array dmabuf_fds, std::array pitches, - std::array offsets) -> struct gbm_bo* + std::array offsets) -> std::shared_ptr { auto const plane_descriptors = buffer->planes(); @@ -383,7 +383,8 @@ auto import_gbm_bo( .modifier = buffer->modifier().value_or(DRM_FORMAT_MOD_NONE), }; - return gbm_bo_import(gbm.get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); + auto* gbm_bo = gbm_bo_import(gbm.get(), GBM_BO_IMPORT_FD_MODIFIER, (void*)&import_data, GBM_BO_USE_SCANOUT); + return std::shared_ptr(gbm_bo, &gbm_bo_destroy); } auto drm_fb_id_from_dma_buffer( @@ -391,7 +392,7 @@ auto drm_fb_id_from_dma_buffer( -> std::shared_ptr { auto [dmabuf_fds, pitches, offsets, modifiers] = get_import_buffers(buffer); - auto* gbm_bo = import_gbm_bo(gbm, buffer, dmabuf_fds, pitches, offsets); + auto gbm_bo = import_gbm_bo(gbm, buffer, dmabuf_fds, pitches, offsets); if (!gbm_bo) { mir::log_warning("Failed to import buffer"); @@ -401,16 +402,16 @@ auto drm_fb_id_from_dma_buffer( auto const plane_descriptors = buffer->planes(); uint32_t gem_handles[MAX_PLANES] = {0}; for (std::size_t i = 0; i < std::min(MAX_PLANES, plane_descriptors.size()); i++) - gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo, i).u32; + gem_handles[i] = gbm_bo_get_handle_for_plane(gbm_bo.get(), i).u32; + // Note that we capture `gbm_bo` so that its lifetime matches that of the fb_id auto fb_id = std::shared_ptr{ new uint32_t{0}, - [drm_fd](uint32_t* fb_id) + [drm_fd, gbm_bo](uint32_t* fb_id) { if (*fb_id) - { drmModeRmFB(drm_fd, *fb_id); - } + delete fb_id; }}; From f46056bcf19750e6df5e20d2bdbf91cc03b7e3cc Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Tue, 15 Oct 2024 18:49:08 +1100 Subject: [PATCH 13/14] platforms/atomic-kms: Fail gracefully when bypass buffer can't be imported for scanout. The compositor code is expecting `framebuffer_for` to sometimes return `nullptr`; this is fine. What's *not* fine is passing a null `fb_id` to `AtomicKmsFbHandle` and then returning a *non*-null `Framebuffer` that's broken :) --- src/platforms/atomic-kms/server/kms/display_buffer.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 0b7371d3e18..902c8c71ee9 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -442,6 +442,11 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr { auto fb_id = drm_fb_id_from_dma_buffer(drm_fd(), gbm, buffer); + if (!fb_id) + { + return {}; + } + struct AtomicKmsFbHandle : public mg::FBHandle { AtomicKmsFbHandle(std::shared_ptr fb_handle, geometry::Size size) : From 82cdf9c0cbd6ec8ffd0abb37f40a555fe5ae1d7c Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Tue, 15 Oct 2024 18:50:13 +1100 Subject: [PATCH 14/14] platforms/atomic-kms: Trigger `on_consumed` when successfully importing a buffer for bypass --- src/platforms/atomic-kms/server/kms/display_buffer.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/platforms/atomic-kms/server/kms/display_buffer.cpp b/src/platforms/atomic-kms/server/kms/display_buffer.cpp index 902c8c71ee9..9218dddab2c 100644 --- a/src/platforms/atomic-kms/server/kms/display_buffer.cpp +++ b/src/platforms/atomic-kms/server/kms/display_buffer.cpp @@ -470,6 +470,8 @@ auto mga::DmaBufDisplayAllocator::framebuffer_for(std::shared_ptr geometry::Size size_; }; + buffer->on_consumed(); + return std::make_unique(fb_id, buffer->size()); }