Mir

Merge lp:~robertcarr/mir/nested-internal-egl-on-mesa-without-caveat into lp:mir

nested-internal-egl-on-mesa-without-caveat
Merge into development-branch

Proposed by Robert Carr on 2014-03-11

Status:

Merged

Approved by:

Daniel van Vugt on 2014-03-13

Approved revision:

no longer in the source branch.

Merged at revision:

1471

Proposed branch:

lp:~robertcarr/mir/nested-internal-egl-on-mesa-without-caveat

Merge into:

lp:mir

Diff against target:

139 lines (+65/-3)

3 files modified

src/platform/graphics/mesa/native_platform.cpp (+46/-1)
src/platform/graphics/mesa/native_platform.h (+10/-0)
src/platform/graphics/mesa/platform.cpp (+9/-2)

To merge this branch:

bzr merge lp:~robertcarr/mir/nested-internal-egl-on-mesa-without-caveat

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-03-11	Approve on 2014-03-13
Alan Griffiths		2014-03-11	Approve on 2014-03-12
Daniel van Vugt		2014-03-11	Approve on 2014-03-12
Kevin DuBois		2014-03-11	Pending
Review via email: mp+210514@code.launchpad.net

This proposal supersedes a proposal from 2014-01-02.

Commit message

Implement internal clients on nested Mesa platform (LP: #1279092)

This is to unblock the Unity8 on desktop preview.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-17: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/553/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2013-12-18: Posted in a previous version of this proposal

Most of this diff is not yours :)

Please resubmit with the appropriate prerequisite defined:
lp:~raof/mir/check-for-surfaceless

review: Needs Resubmitting

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2013-12-18: Posted in a previous version of this proposal

> Most of this diff is not yours :)
>
> Please resubmit with the appropriate prerequisite defined:
> lp:~raof/mir/check-for-surfaceless

review: Needs Resubmitting

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2013-12-27: Posted in a previous version of this proposal

The prereq branch interfering with this diff has now landed. Please merge development-branch and resubmit.

review: Needs Resubmitting

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-02: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/600/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-01-03: Posted in a previous version of this proposal

(1) Could the freeze in swap buffers be related to bug 1236256...?

(2) printf is sometimes slower than rendering, and should at least not be done on every frame:
22 + printf("Swapped\n");
Are you sure you want to keep that?

(3) There are logic changes in src/ without any changes in tests/. Are these changes already covered by existing tests?

Mostly Needs Information, but (2) needs fixing.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-01-03: Posted in a previous version of this proposal

1. It could be. My first impression is that perhaps its related to some reentrancy in mesa EGL...the deadlock definitely seems to be on EGL internal mutexes (At the API level, not the driver/platform level). Of course I haven't completely ruled out a mir deadlock causing this.

2. Whoops! Of course not. Put this in for debugging for holidays...thanks!

3. Mm :(. The internal client itself is already covered by tests. I couldn't really find a test to write here that I thought was meaningful.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-03: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/606/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-01-04: Posted in a previous version of this proposal

The use of static to share this information between platforms strikes me as strange.

also
8 +#include <stdio.h>
9 +

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-01-06: Posted in a previous version of this proposal

(4) We're not crippling non-nested multi-monitor with this, are we?
69 +// TODO: Currently this will not work in multimonitor scenarios!. Use of an internal client inside a nested Mir on Mesa will end in
70 +// a deadlock inside the Mesa-EGL layer between the multiple compositor threads. ~racarr

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-01-29: Posted in a previous version of this proposal

Setting to Work in progress again. Racarr's clearly too busy to discuss this right now.

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-02-27: Posted in a previous version of this proposal

Old comments addressed. It doesn't affect non nested multi-monitor. The bug seems to be in the client code:

https://bugs.launchpad.net/mir/+bug/1285500

Referenced the bug in the comment

Revision history for this message

Daniel van Vugt (vanvugt) on 2014-02-27: Posted in a previous version of this proposal

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-27: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/952/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-02-27: Posted in a previous version of this proposal

82 + static bool internal_display_clients_present;
83 + static std::shared_ptr<InternalNativeDisplay> internal_native_display;

Public mutable data is a race condition waiting to happen.

These should probably be file scope and either atomic or protected by a mutex. (A static member function can server the needs of mir_server_mesa_egl_native_display_is_valid()).

Also, I see no value in having both variables.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-03: Posted in a previous version of this proposal

Good call. Pushed a cleanup.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-03: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/970/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-04: Posted in a previous version of this proposal

Nit: nullptr is the new 0

~~~~

132 return ((mgm::Platform::internal_display_clients_present) &&
133 - (display == mgm::Platform::internal_native_display.get()));
134 + (display == mgm::Platform::internal_native_display.get())) ||
135 + (mgm::NativePlatform::internal_native_display_in_use() && (display == mgm::NativePlatform::internal_native_display().get()));

This looks like poor encapsulation (and poor formatting). Maybe the two primary sub expressions could be separated out?

And names like "NativePlatform::internal_native_display()" vs "Platform::internal_native_display" don't clearly distinguish the intent.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-03-07: Posted in a previous version of this proposal

How about like this?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-07: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1011/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-10: Posted in a previous version of this proposal

(1) I suspect your description and code comments are now out of date, following a couple of recent nesting/multimonitor fixes in Mir 0.1.6 and 0.1.7. Please check...

(2) This looks like it should return a MirBool:
int mir_server_mesa_egl_native_display_is_valid
however we'd have to move the definition of MirBool from client_types.h into common.h, which is probably fine.

(3) Needs Fixing: The methods of mgm::NativePlatform:: look racy and not thread safe. They have internal locking but the caller gets a shared_ptr to the same objects, and the caller is not locked. If you can't safely move the locking up and out of NativePlatform then perhaps try returning copies of objects so you're never returning the locked instance.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-10: Posted in a previous version of this proposal

> (1) I suspect your description and code comments are now out of date,
> following a couple of recent nesting/multimonitor fixes in Mir 0.1.6 and
> 0.1.7. Please check...

I suspect that too.

> (3) Needs Fixing: The methods of mgm::NativePlatform:: look racy and not
> thread safe. They have internal locking but the caller gets a shared_ptr to
> the same objects, and the caller is not locked. If you can't safely move the
> locking up and out of NativePlatform then perhaps try returning copies of
> objects so you're never returning the locked instance.

I disagree: the mutex is needed to protect native_display not the object referenced by it (nor the reference count).

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-12:

FAILED: Continuous integration, rev:1298
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/nested-internal-egl-on-mesa-without-caveat/+merge/210514/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1042/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-12:

Now that you mention it, I can't be sure that is a thread safety issue. Although to be properly sure we really should be returning shared_ptrs to const objects instead of mutable ones.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-12:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1045/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-12:

131 + bool nested_internal_display_in_use = mgm::NativePlatform::internal_native_display_in_use();
132 + bool host_internal_display_in_use = mgm::Platform::internal_display_clients_present;
133 +
134 + if (host_internal_display_in_use)
135 + return (display == mgm::Platform::internal_native_display.get());
136 + else if (nested_internal_display_in_use)
137 + return (display == mgm::NativePlatform::internal_native_display().get());

By making calls to (for example) mgm::NativePlatform::internal_native_display_in_use() and mgm::Platform::internal_native_display.get()) without any locking there is a risk of state changing. But I doubt we have a real problem.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-13:

Land the thing! After 3 months I think we're out of reasons not to.

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-03-13:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Mir development team

Robert Carr

 === modified file 'src/platform/graphics/mesa/native_platform.cpp'
 --- src/platform/graphics/mesa/native_platform.cpp	2014-03-06 06:05:17 +0000
 +++ src/platform/graphics/mesa/native_platform.cpp	2014-03-12 02:47:17 +0000
@@ -25,8 +25,13 @@
  #include "mir/graphics/platform_ipc_package.h"
  #include "mir/graphics/nested_context.h"
++#include "internal_client.h"
++#include "internal_native_display.h"
++
  #include <boost/exception/errinfo_errno.hpp>
  #include <boost/throw_exception.hpp>
++
++#include <mutex>
  #include <stdexcept>
  namespace mg = mir::graphics;
@@ -42,6 +47,11 @@
      nested_context->drm_set_gbm_device(gbm.device);
+ }
++mgm::NativePlatform::~NativePlatform()
++{
++    finish_internal_native_display();
++}
++
  std::shared_ptr<mg::GraphicBufferAllocator> mgm::NativePlatform::create_buffer_allocator(
          std::shared_ptr<mg::BufferInitializer> const& buffer_initializer)
+ {
@@ -82,7 +92,8 @@
  std::shared_ptr<mg::InternalClient> mgm::NativePlatform::create_internal_client()
+ {
--    BOOST_THROW_EXCEPTION(std::runtime_error("MesaNativePlatform::create_internal_client is not implemented yet!"));
++    auto nd = ensure_internal_native_display(get_ipc_package());
++    return std::make_shared<mgm::InternalClient>(nd);
+ }
  void mgm::NativePlatform::fill_ipc_package(BufferIPCPacker* packer, Buffer const* buffer) const
@@ -106,3 +117,37 @@
+ {
      return std::make_shared<mgm::NativePlatform>();
+ }
++
++namespace
++{
++std::shared_ptr<mgm::InternalNativeDisplay> native_display = nullptr;
++std::mutex native_display_guard;
++}
++
++bool mgm::NativePlatform::internal_native_display_in_use()
++{
++    std::unique_lock<std::mutex> lg(native_display_guard);
++    return native_display != nullptr;
++}
++
++std::shared_ptr<mgm::InternalNativeDisplay> mgm::NativePlatform::internal_native_display()
++{
++    std::unique_lock<std::mutex> lg(native_display_guard);
++    return native_display;
++}
++
++std::shared_ptr<mgm::InternalNativeDisplay> mgm::NativePlatform::ensure_internal_native_display(
++    std::shared_ptr<mg::PlatformIPCPackage> const& package)
++{
++    std::unique_lock<std::mutex> lg(native_display_guard);
++    if (!native_display)
++        native_display = std::make_shared<mgm::InternalNativeDisplay>(package);
++    return native_display;
++}
++
++void mgm::NativePlatform::finish_internal_native_display()
++{
++    std::unique_lock<std::mutex> lg(native_display_guard);
++    native_display.reset();
++}
++
 === modified file 'src/platform/graphics/mesa/native_platform.h'
 --- src/platform/graphics/mesa/native_platform.h	2014-03-06 06:05:17 +0000
 +++ src/platform/graphics/mesa/native_platform.h	2014-03-12 02:47:17 +0000
@@ -30,20 +30,30 @@
+ {
  namespace mesa
+ {
++class InternalNativeDisplay;
++
  class NativePlatform : public graphics::NativePlatform
+ {
  public:
++    virtual ~NativePlatform();
++
      void initialize(std::shared_ptr<NestedContext> const& nested_context);
      std::shared_ptr<GraphicBufferAllocator> create_buffer_allocator(
          std::shared_ptr<BufferInitializer> const& buffer_initializer) override;
      std::shared_ptr<PlatformIPCPackage> get_ipc_package() override;
      std::shared_ptr<InternalClient> create_internal_client() override;
      void fill_ipc_package(BufferIPCPacker* packer, Buffer const* buffer) const override;
++
++    static std::shared_ptr<InternalNativeDisplay> internal_native_display();
++    static bool internal_native_display_in_use();
  private:
      int drm_fd;
      std::shared_ptr<NestedContext> nested_context;
      helpers::GBMHelper gbm;
++
++    static std::shared_ptr<InternalNativeDisplay> ensure_internal_native_display(std::shared_ptr<PlatformIPCPackage> const& package);
++    static void finish_internal_native_display();
  };
+ }
+ }
 === modified file 'src/platform/graphics/mesa/platform.cpp'
 --- src/platform/graphics/mesa/platform.cpp	2014-03-06 06:05:17 +0000
 +++ src/platform/graphics/mesa/platform.cpp	2014-03-12 02:47:17 +0000
@@ -17,6 +17,7 @@
   */
  #include "platform.h"
++#include "native_platform.h"
  #include "buffer_allocator.h"
  #include "display.h"
  #include "internal_client.h"
@@ -179,6 +180,12 @@
  extern "C" int mir_server_mesa_egl_native_display_is_valid(MirMesaEGLNativeDisplay* display)
+ {
--    return ((mgm::Platform::internal_display_clients_present) &&
--            (display == mgm::Platform::internal_native_display.get()));
++    bool nested_internal_display_in_use = mgm::NativePlatform::internal_native_display_in_use();
++    bool host_internal_display_in_use = mgm::Platform::internal_display_clients_present;
++
++    if (host_internal_display_in_use)
++        return (display == mgm::Platform::internal_native_display.get());
++    else if (nested_internal_display_in_use)
++        return (display == mgm::NativePlatform::internal_native_display().get());
++    return 0;
+ }

Mir

Merge lp:~robertcarr/mir/nested-internal-egl-on-mesa-without-caveat into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers