Mir

Merge lp:~raof/mir/fix-drm-buffer-multiple-attach into lp:mir

Proposed by Chris Halse Rogers on 2017-09-21
Status: Merged
Approved by: Gerry Boland on 2017-09-21
Approved revision: 4258
Merged at revision: 4261
Proposed branch: lp:~raof/mir/fix-drm-buffer-multiple-attach
Merge into: lp:mir
Diff against target: 224 lines (+82/-56)
3 files modified
include/platform/mir/graphics/wayland_allocator.h (+1/-1)
src/platforms/mesa/server/buffer_allocator.cpp (+80/-54)
src/platforms/mesa/server/buffer_allocator.h (+1/-1)
To merge this branch: bzr merge lp:~raof/mir/fix-drm-buffer-multiple-attach
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve on 2017-09-22
Gerry Boland Approve on 2017-09-21
Alan Griffiths 2017-09-21 Approve on 2017-09-21
Review via email: mp+331123@code.launchpad.net

Commit message

mgm::BufferAllocator: Handle a wl_buffer being attached multiple times.

As with the WlShmBuffer, it's valid for a wl_buffer to be wl_surface.attach()ed to multiple wl_surface-s at the same time. If a buffer is attached twice then the buffer.release() event needs to be sent exactly once, when the compositor is finished with *all* submissions.

So, do what we do with WlShmBuffer - associate a shared_ptr<WaylandBuffer> with each wl_buffer, and hand out references to it rather than creating a new WaylandBuffer on each (committed) attach().

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

Nice

review: Approve
Gerry Boland (gerboland) wrote :

lgtm

review: Approve
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4258
https://mir-jenkins.ubuntu.com/job/mir-ci/3674/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/5033/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5269
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5257
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/5076/artifact/output/*zip*/output.zip
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/5076/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/5076/artifact/output/*zip*/output.zip
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/5076/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5076
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/5076/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3674/rebuild

review: Needs Fixing (continuous-integration)
Mir CI Bot (mir-ci-bot) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/platform/mir/graphics/wayland_allocator.h'
2--- include/platform/mir/graphics/wayland_allocator.h 2017-09-01 07:00:55 +0000
3+++ include/platform/mir/graphics/wayland_allocator.h 2017-09-21 08:03:22 +0000
4@@ -37,7 +37,7 @@
5 virtual ~WaylandAllocator() = default;
6
7 virtual void bind_display(wl_display* display) = 0;
8- virtual std::unique_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) = 0;
9+ virtual std::shared_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) = 0;
10 };
11 }
12 }
13
14=== modified file 'src/platforms/mesa/server/buffer_allocator.cpp'
15--- src/platforms/mesa/server/buffer_allocator.cpp 2017-09-01 07:00:55 +0000
16+++ src/platforms/mesa/server/buffer_allocator.cpp 2017-09-21 08:03:22 +0000
17@@ -332,66 +332,54 @@
18 public mir::renderer::gl::TextureSource
19 {
20 public:
21- WaylandBuffer(
22+ static std::shared_ptr<mg::Buffer> mir_buffer_from_wl_buffer(
23 EGLDisplay dpy,
24 wl_resource* buffer,
25 std::shared_ptr<mg::EGLExtensions> const& extensions,
26 std::function<void()>&& on_consumed)
27- : buffer{buffer},
28- dpy{dpy},
29- egl_image{EGL_NO_IMAGE_KHR},
30- extensions{extensions},
31- on_consumed{std::move(on_consumed)}
32 {
33+ std::shared_ptr<WaylandBuffer> mir_buffer;
34+ DestructionShim* shim;
35+
36 if (auto notifier = wl_resource_get_destroy_listener(buffer, &on_buffer_destroyed))
37 {
38- DestructionShim* shim;
39-
40+ // We've already constructed a shim for this buffer, update it.
41 shim = wl_container_of(notifier, shim, destruction_listener);
42
43- if (shim->associated_buffer)
44- BOOST_THROW_EXCEPTION(std::logic_error("Attempt to associate a single wl_buffer with multiple WaylandBuffer wrappers"));
45-
46- shim->associated_buffer = this;
47- buffer_mutex = shim->mutex;
48+ if (!(mir_buffer = shim->associated_buffer.lock()))
49+ {
50+ /*
51+ * We've seen this wl_buffer before, but all the WaylandBuffers associated with it
52+ * have been destroyed.
53+ *
54+ * Recreate a new WaylandBuffer to track the new compositor lifetime.
55+ */
56+ mir_buffer = std::shared_ptr<WaylandBuffer>{
57+ new WaylandBuffer{
58+ dpy,
59+ buffer,
60+ extensions,
61+ std::move(on_consumed)}};
62+ shim->associated_buffer = mir_buffer;
63+ }
64 }
65 else
66 {
67- auto shim = new DestructionShim;
68+ mir_buffer = std::shared_ptr<WaylandBuffer>{
69+ new WaylandBuffer{
70+ dpy,
71+ buffer,
72+ extensions,
73+ std::move(on_consumed)}};
74+ shim = new DestructionShim;
75 shim->destruction_listener.notify = &on_buffer_destroyed;
76- shim->associated_buffer = this;
77- buffer_mutex = shim->mutex;
78+ shim->associated_buffer = mir_buffer;
79
80 wl_resource_add_destroy_listener(buffer, &shim->destruction_listener);
81 }
82
83- if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_WIDTH, &width) == EGL_FALSE)
84- {
85- BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer width"));
86- }
87- if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_HEIGHT, &height) == EGL_FALSE)
88- {
89- BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer height"));
90- }
91-
92- EGLint texture_format;
93- if (!extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_TEXTURE_FORMAT, &texture_format))
94- {
95- BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WL buffer format"));
96- }
97-
98- if (texture_format == EGL_TEXTURE_RGB)
99- {
100- format = mir_pixel_format_xrgb_8888;
101- }
102- else if (texture_format == EGL_TEXTURE_RGBA)
103- {
104- format = mir_pixel_format_argb_8888;
105- }
106- else
107- {
108- BOOST_THROW_EXCEPTION((std::invalid_argument{"YUV buffers are unimplemented"}));
109- }
110+ mir_buffer->buffer_mutex = shim->mutex;
111+ return mir_buffer;
112 }
113
114 ~WaylandBuffer()
115@@ -403,12 +391,6 @@
116 if (buffer)
117 {
118 wl_resource_queue_event(buffer, WL_BUFFER_RELEASE);
119- auto notifier = wl_resource_get_destroy_listener(buffer, &on_buffer_destroyed);
120- DestructionShim* shim;
121-
122- shim = wl_container_of(notifier, shim, destruction_listener);
123-
124- shim->associated_buffer = nullptr;
125 }
126 }
127
128@@ -477,6 +459,46 @@
129 }
130
131 private:
132+ WaylandBuffer(
133+ EGLDisplay dpy,
134+ wl_resource* buffer,
135+ std::shared_ptr<mg::EGLExtensions> const& extensions,
136+ std::function<void()>&& on_consumed)
137+ : buffer{buffer},
138+ dpy{dpy},
139+ egl_image{EGL_NO_IMAGE_KHR},
140+ extensions{extensions},
141+ on_consumed{std::move(on_consumed)}
142+ {
143+ if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_WIDTH, &width) == EGL_FALSE)
144+ {
145+ BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer width"));
146+ }
147+ if (extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_HEIGHT, &height) == EGL_FALSE)
148+ {
149+ BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WaylandAllocator buffer height"));
150+ }
151+
152+ EGLint texture_format;
153+ if (!extensions->wayland->eglQueryWaylandBufferWL(dpy, buffer, EGL_TEXTURE_FORMAT, &texture_format))
154+ {
155+ BOOST_THROW_EXCEPTION(mg::egl_error("Failed to query WL buffer format"));
156+ }
157+
158+ if (texture_format == EGL_TEXTURE_RGB)
159+ {
160+ format = mir_pixel_format_xrgb_8888;
161+ }
162+ else if (texture_format == EGL_TEXTURE_RGBA)
163+ {
164+ format = mir_pixel_format_argb_8888;
165+ }
166+ else
167+ {
168+ BOOST_THROW_EXCEPTION((std::invalid_argument{"YUV buffers are unimplemented"}));
169+ }
170+ }
171+
172 static void on_buffer_destroyed(wl_listener* listener, void*)
173 {
174 static_assert(
175@@ -488,9 +510,9 @@
176
177 {
178 std::lock_guard<std::mutex> lock{*shim->mutex};
179- if (shim->associated_buffer)
180+ if (auto mir_buffer = shim->associated_buffer.lock())
181 {
182- shim->associated_buffer->buffer = nullptr;
183+ mir_buffer->buffer = nullptr;
184 }
185 }
186
187@@ -500,7 +522,7 @@
188 struct DestructionShim
189 {
190 std::shared_ptr<std::mutex> const mutex = std::make_shared<std::mutex>();
191- WaylandBuffer* associated_buffer;
192+ std::weak_ptr<WaylandBuffer> associated_buffer;
193 wl_listener destruction_listener;
194 };
195
196@@ -542,9 +564,13 @@
197 }
198 }
199
200-std::unique_ptr<mg::Buffer> mgm::BufferAllocator::buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed)
201+std::shared_ptr<mg::Buffer> mgm::BufferAllocator::buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed)
202 {
203 if (egl_extensions->wayland)
204- return std::make_unique<WaylandBuffer>(dpy, buffer, egl_extensions, std::move(on_consumed));
205+ return WaylandBuffer::mir_buffer_from_wl_buffer(
206+ dpy,
207+ buffer,
208+ egl_extensions,
209+ std::move(on_consumed));
210 return nullptr;
211 }
212
213=== modified file 'src/platforms/mesa/server/buffer_allocator.h'
214--- src/platforms/mesa/server/buffer_allocator.h 2017-08-31 07:40:41 +0000
215+++ src/platforms/mesa/server/buffer_allocator.h 2017-09-21 08:03:22 +0000
216@@ -63,7 +63,7 @@
217 std::vector<MirPixelFormat> supported_pixel_formats() override;
218
219 void bind_display(wl_display* display) override;
220- std::unique_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) override;
221+ std::shared_ptr<Buffer> buffer_from_resource (wl_resource* buffer, std::function<void ()>&& on_consumed) override;
222 private:
223 std::shared_ptr<Buffer> alloc_hardware_buffer(
224 graphics::BufferProperties const& buffer_properties);

Subscribers

People subscribed via source and target branches