Mir

Merge lp:~vanvugt/mir/nonprime-server-support into lp:mir

Proposed by Daniel van Vugt on 2016-11-09
Status: Rejected
Rejected by: Daniel van Vugt on 2017-03-09
Proposed branch: lp:~vanvugt/mir/nonprime-server-support
Merge into: lp:mir
Diff against target: 221 lines (+126/-17)
3 files modified
src/platforms/mesa/server/gbm_buffer.cpp (+37/-8)
src/platforms/mesa/server/gbm_buffer.h (+2/-0)
tests/unit-tests/platforms/mesa/kms/test_gbm_buffer.cpp (+87/-9)
To merge this branch: bzr merge lp:~vanvugt/mir/nonprime-server-support
Reviewer Review Type Date Requested Status
Kevin DuBois (community) Abstain on 2017-02-10
Chris Halse Rogers 2016-11-09 Disapprove on 2016-11-09
Mir CI Bot continuous-integration 2016-11-09 Approve on 2016-11-09
Review via email: mp+310391@code.launchpad.net

Commit message

Add server-side support for non-PRIME DRM drivers like VirtualBox.

This is enough to get mir_demo_standalone_render_surfaces working
(very nicely) but more work would be required to support clients
(Mesa patch) and nesting.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3811
https://mir-jenkins.ubuntu.com/job/mir-ci/2131/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2755/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2818
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2810
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2810
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2810
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2784
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2784/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2784
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2784/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2784/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2784
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2784/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2784
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2784/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2784
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2784/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Daniel van Vugt (vanvugt) wrote :
3812. By Daniel van Vugt on 2016-11-09

Try again

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

PASSED: Continuous integration, rev:3812
https://mir-jenkins.ubuntu.com/job/mir-ci/2133/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2759
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2822
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2814
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2814
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2814
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2788/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2788/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2788/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2788/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2788/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2788
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2788/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Chris Halse Rogers (raof) wrote :

The code itself looks fine.

I'm mildly against this support being implemented in Mir at all, however. I'm at 0.5 disapproves.

This bifurcates our mesa platform into essentially two platforms, where one is susceptible to framebuffer snooping and imposes other constraints (notably, cannot use rendernodes).

The flink buffer naming method is long-deprecated; since the proximal cause for this support is that VirtualBox doesn't support dma-buf, the vbox driver is in-tree, and I'm aware of no other driver that we care about that doesn't support dma-buf, it would seem the sensible approach would be to implement dma-buf sharing in vbox.

While it's possible we'll run into other drivers not supporting dma-buf, said drivers would have to be out-of-tree - I would not expect a new driver to be integrated into kernel drm without dma-buf support (as it's almost entirely handled by the drm helpers).

review: Disapprove
Daniel van Vugt (vanvugt) wrote :

I get that argument but it's also a little too arrogant to say we won't support older drivers when we can.

Wayland has said yes to supporting such older drivers. We're really shooting ourselves in the foot if we say no. It's then our fault that VirtualBox (and other drivers?) then can't be supported for an indefinite period of time, without users also waiting on newer kernels.

We have a choice to support everybody right now and write the code once, or to continue not supporting some drivers and wait indefinitely for those drivers to all get fixed separately. It's just not sensible to disapprove a pragmatic solution like this.

Kevin DuBois (kdub) wrote :

The mgm::GBMBuffer switches behavior based on supports_prime. It would seem more natural to have a different implementation of mg::Buffer for prime support vs GEM_FLINK.

Seems that the legacy method is insecure (based on Chris's comment), so it seems like the question is whether we're allowed to do this, as per the security policy. Might be as simple as getting an answer that fits with the security policy.

review: Needs Information
Daniel van Vugt (vanvugt) wrote :

It's not a security question. It's a question of having no choice other than hacking the kernel driver (and then waiting for all existing users to then get a new kernel before we can support them). But given the more secure feature support, that is still used as first preference here.

I already pondered the idea of having a different implementation of Buffer. Decided that would be inappropriate because even the flink code path uses GBM. So both code paths are GBM code paths. If you were to separate them then even the existing one would need renaming from GBMBuffer to PrimeGBMBuffer. But I don't like the idea of having duplicated code. I guess it would have to be prototyped to see if it's just an acceptable level of duplication.

Personally I would still like to keep a single class "GBMBuffer". That's short and sweet and the most accurate representation of what it is.

Kevin DuBois (kdub) wrote :

> It's not a security question. It's a question of having no choice other than
> hacking the kernel driver (and then waiting for all existing users to then get
> a new kernel before we can support them). But given the more secure feature
> support, that is still used as first preference here.

Well, its a tradeoff/question of security vs support. We can support the flink-gem methods, but they do expose a framesnooping problem. [1] Seems like a judgement call of priorities, so probably need some more input. "Needs info" still seems like the appropriate option overall, although approve if the tradeoff is something the team is comfortable with.

> I already pondered the idea of having a different implementation of Buffer.
> Decided that would be inappropriate because even the flink code path uses GBM.
> So both code paths are GBM code paths. If you were to separate them then even
> the existing one would need renaming from GBMBuffer to PrimeGBMBuffer. But I
> don't like the idea of having duplicated code. I guess it would have to be
> prototyped to see if it's just an acceptable level of duplication.
>
> Personally I would still like to keep a single class "GBMBuffer". That's short
> and sweet and the most accurate representation of what it is.

The choice is reasonable, but we could avoid sharing by having a base class that needs the flink or prime specific functions implemented. IMO, One object behaving in two different ways lends itself to two objects in C++. Abstain on this point.

[1]
Not being an area I know a whole lot about, had to do some googling, but that turned up:
https://en.wikipedia.org/wiki/Direct_Rendering_Manager#cite_note-Peres_Ravier_2013-12

Daniel van Vugt (vanvugt) wrote :

Try not overthink the problem, it's just an if statement. I think we'd need a more compelling reason to replace one or two if statements with new classes. New classes carry a greater maintenance burden.

Andreas Pokorny (andreas-pokorny) wrote :

> The code itself looks fine.
>
> I'm mildly against this support being implemented in Mir at all, however. I'm
> at 0.5 disapproves.
>
> This bifurcates our mesa platform into essentially two platforms, where one is
> susceptible to framebuffer snooping and imposes other constraints (notably,
> cannot use rendernodes).

You mean in general? Or just on systems that rely on the fallback?

> The flink buffer naming method is long-deprecated; since the proximal cause
> for this support is that VirtualBox doesn't support dma-buf, the vbox driver
> is in-tree, and I'm aware of no other driver that we care about that doesn't
> support dma-buf, it would seem the sensible approach would be to implement
> dma-buf sharing in vbox.

I think it is still an external module. I.e. I am looking at:
https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Additions/linux/drm/vbox_drv.c

But adding support is essentially wiring two helper functions to the drv object and implementing a bunch of empty functions that deny attempts to convert dmabufd from other drm drivers.

Daniel van Vugt (vanvugt) wrote :

I wonder if I/we should just jump straight to the final step of resolving the generalised VM case (as well as the generalised software rendering case): bug 1118903

It might perform worse than this, and it might take a lot more effort than this, but it's something we have to do anyway. Once that's in place we could make mesa-kms fail to load if PRIME is unsupported and then Mir will fall back to the generalised software driver (which doesn't exist yet).

Kevin DuBois (kdub) :
review: Abstain

Unmerged revisions

3812. By Daniel van Vugt on 2016-11-09

Try again

3811. By Daniel van Vugt on 2016-11-08

Prototype

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/platforms/mesa/server/gbm_buffer.cpp'
2--- src/platforms/mesa/server/gbm_buffer.cpp 2016-10-11 07:12:33 +0000
3+++ src/platforms/mesa/server/gbm_buffer.cpp 2016-11-09 05:29:41 +0000
4@@ -41,18 +41,39 @@
5 : gbm_handle{handle},
6 bo_flags{bo_flags},
7 texture_binder{std::move(texture_binder)},
8- prime_fd{-1}
9+ prime_fd{-1},
10+ gem_flink_name{0}
11 {
12 auto device = gbm_bo_get_device(gbm_handle.get());
13 auto gem_handle = gbm_bo_get_handle(gbm_handle.get()).u32;
14 auto drm_fd = gbm_device_get_fd(device);
15
16- auto ret = drmPrimeHandleToFD(drm_fd, gem_handle, DRM_CLOEXEC, &prime_fd);
17+ uint64_t has_prime = 0;
18+ if (0 != drmGetCap(drm_fd, DRM_CAP_PRIME, &has_prime))
19+ has_prime = 0;
20+ supports_prime = !!has_prime;
21
22- if (ret)
23- {
24- std::string const msg("Failed to get PRIME fd from gbm bo");
25- BOOST_THROW_EXCEPTION((std::system_error{errno, std::system_category(), msg}));
26+ // If we have PRIME support, use it exclusively (greatest security)
27+ if (supports_prime)
28+ {
29+ auto ret = drmPrimeHandleToFD(drm_fd, gem_handle, DRM_CLOEXEC, &prime_fd);
30+ if (ret)
31+ {
32+ std::string const msg("Failed to get PRIME fd from gbm bo");
33+ BOOST_THROW_EXCEPTION((std::system_error{errno, std::system_category(), msg}));
34+ }
35+ }
36+ else // use what works, in the absence of PRIME support
37+ {
38+ struct drm_gem_flink flink;
39+ flink.handle = gem_handle;
40+ auto ret = drmIoctl(drm_fd, DRM_IOCTL_GEM_FLINK, &flink);
41+ if (ret)
42+ {
43+ std::string const msg("Failed to get GEM flink name");
44+ BOOST_THROW_EXCEPTION((std::system_error{errno, std::system_category(), msg}));
45+ }
46+ gem_flink_name = flink.name;
47 }
48 }
49
50@@ -86,8 +107,16 @@
51 {
52 auto temp = std::make_shared<NativeBuffer>();
53
54- temp->fd_items = 1;
55- temp->fd[0] = prime_fd;
56+ if (supports_prime)
57+ {
58+ temp->fd_items = 1;
59+ temp->fd[0] = prime_fd;
60+ }
61+ else
62+ {
63+ temp->data_items = 1;
64+ temp->data[0] = gem_flink_name;
65+ }
66 temp->stride = stride().as_uint32_t();
67 temp->flags = (bo_flags & GBM_BO_USE_SCANOUT) ? mir_buffer_flag_can_scanout : 0;
68 temp->bo = gbm_handle.get();
69
70=== modified file 'src/platforms/mesa/server/gbm_buffer.h'
71--- src/platforms/mesa/server/gbm_buffer.h 2016-10-12 13:31:04 +0000
72+++ src/platforms/mesa/server/gbm_buffer.h 2016-11-09 05:29:41 +0000
73@@ -70,7 +70,9 @@
74 std::shared_ptr<gbm_bo> const gbm_handle;
75 uint32_t bo_flags;
76 std::unique_ptr<common::BufferTextureBinder> const texture_binder;
77+ bool supports_prime;
78 int prime_fd;
79+ uint32_t gem_flink_name;
80 };
81
82 }
83
84=== modified file 'tests/unit-tests/platforms/mesa/kms/test_gbm_buffer.cpp'
85--- tests/unit-tests/platforms/mesa/kms/test_gbm_buffer.cpp 2016-10-12 13:31:04 +0000
86+++ tests/unit-tests/platforms/mesa/kms/test_gbm_buffer.cpp 2016-11-09 05:29:41 +0000
87@@ -61,6 +61,9 @@
88 usage = mg::BufferUsage::hardware;
89 buffer_properties = mg::BufferProperties{size, pf, usage};
90
91+ ON_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
92+ .WillByDefault(DoAll(SetArgPointee<2>(1),Return(0)));
93+
94 ON_CALL(mock_gbm, gbm_bo_get_width(_))
95 .WillByDefault(Return(size.width.as_uint32_t()));
96
97@@ -144,7 +147,7 @@
98 ASSERT_LE(minimum, geom::Stride{native->stride});
99 }
100
101-TEST_F(GBMBufferTest, buffer_native_handle_has_correct_size)
102+TEST_F(GBMBufferTest, prime_native_buffer_handle_has_correct_size)
103 {
104 using namespace testing;
105
106@@ -155,6 +158,20 @@
107 EXPECT_EQ(0, native_handle->data_items);
108 }
109
110+TEST_F(GBMBufferTest, nonprime_native_buffer_handle_has_correct_size)
111+{
112+ using namespace testing;
113+
114+ EXPECT_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
115+ .WillOnce(DoAll(SetArgPointee<2>(0),Return(0)));
116+
117+ auto buffer = allocator->alloc_buffer(buffer_properties);
118+ auto native_handle = std::dynamic_pointer_cast<mgm::NativeBuffer>(buffer->native_buffer_handle());
119+ ASSERT_THAT(native_handle, Ne(nullptr));
120+ EXPECT_EQ(0, native_handle->fd_items);
121+ EXPECT_EQ(1, native_handle->data_items);
122+}
123+
124 MATCHER_P(GEMFlinkHandleIs, value, "")
125 {
126 auto flink = reinterpret_cast<struct drm_gem_flink*>(arg);
127@@ -167,7 +184,7 @@
128 flink->name = value;
129 }
130
131-TEST_F(GBMBufferTest, buffer_native_handle_contains_correct_data)
132+TEST_F(GBMBufferTest, prime_native_buffer_handle_contains_correct_data)
133 {
134 using namespace testing;
135
136@@ -190,17 +207,78 @@
137 EXPECT_EQ(stride.as_uint32_t(), static_cast<unsigned int>(handle->stride));
138 }
139
140+TEST_F(GBMBufferTest, nonprime_native_buffer_handle_contains_correct_data)
141+{
142+ using namespace testing;
143+
144+ EXPECT_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
145+ .WillOnce(DoAll(SetArgPointee<2>(0),Return(0)));
146+
147+ uint32_t const flink_name{0xf000baaa};
148+
149+ EXPECT_CALL(mock_drm, drmIoctl(_,DRM_IOCTL_GEM_FLINK,_))
150+ .Times(Exactly(1))
151+ .WillOnce(DoAll(SetGEMFlinkName(flink_name), Return(0)));
152+
153+ auto buffer = allocator->alloc_buffer(buffer_properties);
154+ auto handle = std::dynamic_pointer_cast<mgm::NativeBuffer>(buffer->native_buffer_handle());
155+ ASSERT_THAT(handle, Ne(nullptr));
156+ EXPECT_EQ(0, handle->fd_items);
157+ EXPECT_EQ(1, handle->data_items);
158+ EXPECT_EQ(flink_name, static_cast<uint32_t>(handle->data[0]));
159+ EXPECT_EQ(stride.as_uint32_t(), static_cast<unsigned int>(handle->stride));
160+}
161+
162 TEST_F(GBMBufferTest, buffer_creation_throws_on_prime_fd_failure)
163 {
164 using namespace testing;
165
166- EXPECT_CALL(mock_drm, drmPrimeHandleToFD(_,_,_,_))
167- .Times(Exactly(1))
168- .WillOnce(Return(-1));
169-
170- EXPECT_THROW({
171- auto buffer = allocator->alloc_buffer(buffer_properties);
172- }, std::runtime_error);
173+ EXPECT_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
174+ .Times(Exactly(1))
175+ .WillOnce(DoAll(SetArgPointee<2>(1),Return(0)));
176+ EXPECT_CALL(mock_drm, drmPrimeHandleToFD(_,_,_,_))
177+ .Times(Exactly(1))
178+ .WillOnce(Return(-1));
179+
180+ EXPECT_THROW({
181+ auto buffer = allocator->alloc_buffer(buffer_properties);
182+ }, std::runtime_error);
183+}
184+
185+TEST_F(GBMBufferTest, buffer_creation_throws_on_flink_failure)
186+{
187+ using namespace testing;
188+
189+ EXPECT_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
190+ .Times(Exactly(1))
191+ .WillOnce(DoAll(SetArgPointee<2>(0),Return(0)));
192+ EXPECT_CALL(mock_drm, drmIoctl(_,DRM_IOCTL_GEM_FLINK,_))
193+ .Times(Exactly(1))
194+ .WillOnce(Return(-1));
195+ EXPECT_CALL(mock_drm, drmPrimeHandleToFD(_,_,_,_))
196+ .Times(0);
197+
198+ EXPECT_THROW({
199+ auto buffer = allocator->alloc_buffer(buffer_properties);
200+ }, std::runtime_error);
201+}
202+
203+TEST_F(GBMBufferTest, buffer_creation_falls_back_without_throwing)
204+{
205+ using namespace testing;
206+
207+ EXPECT_CALL(mock_drm, drmGetCap(_,DRM_CAP_PRIME,_))
208+ .Times(Exactly(1))
209+ .WillOnce(DoAll(SetArgPointee<2>(0),Return(0)));
210+ EXPECT_CALL(mock_drm, drmIoctl(_,DRM_IOCTL_GEM_FLINK,_))
211+ .Times(Exactly(1))
212+ .WillOnce(Return(0));
213+ EXPECT_CALL(mock_drm, drmPrimeHandleToFD(_,_,_,_))
214+ .Times(0);
215+
216+ EXPECT_NO_THROW({
217+ auto buffer = allocator->alloc_buffer(buffer_properties);
218+ });
219 }
220
221 TEST_F(GBMBufferTest, gl_bind_to_texture_egl_image_creation_failed)

Subscribers

People subscribed via source and target branches