Merge lp:~hikiko/mir/mir.bug-1206633 into lp:~mir-team/mir/trunk

[Eleni Maria Stea]

This branch seems to fix the bug #1206633.
Changes:
- added significant messages to all errors in open_drm_device (not only those with errno)
- added drmSetMaster drmDropMaster around the drmSetInterfaceVersion (only drm master can run this command)
- added a call to drmSetMaster right before returning the selected and opened drm device to avoid the crash (some operations that need mir to be the master take place before we call drmSetMaster otherwise)

Question:
I think that the drmSetInterfaceVersion is not 100% necessary (we can just assume that the device is "usable" based on the other checks we have), also this call doesn't exist in older libdrm versions. Do you think I should remove it?

This branch seems to fix the bug #1206633.
Changes:
- added significant messages to all errors in open_drm_device (not only those with errno)
- added drmSetMaster drmDropMaster around the drmSetInterfaceVersion (only drm master can run this command)
- added a call to drmSetMaster right before returning the selected and opened drm device to avoid the crash (some operations that need mir to be the master take place before we call drmSetMaster otherwise)

Question:
I think that the drmSetInterfaceVersion is not 100% necessary (we can just assume that the device is "usable" based on the other checks we have), also this call doesn't exist in older libdrm versions. Do you think I should remove it?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1055
http://jenkins.qa.ubuntu.com/job/mir-ci/1448/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1956
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1841
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/686/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1448/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1056
http://jenkins.qa.ubuntu.com/job/mir-ci/1452/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1960
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1845
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/690/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1452/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1057
http://jenkins.qa.ubuntu.com/job/mir-ci/1453/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1961
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1846
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/691/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1453/rebuild

review: Needs Fixing (continuous-integration)

=== modified file 'src/server/graphics/gbm/gbm_display_helpers.cpp'
--- src/server/graphics/gbm/gbm_display_helpers.cpp	2013-09-02 11:32:59 +0000
+++ src/server/graphics/gbm/gbm_display_helpers.cpp	2013-09-10 13:47:55 +0000
@@ -32,6 +32,9 @@
 namespace mgg = mir::graphics::gbm;
 namespace mggh = mir::graphics::gbm::helpers;
 
+static void drm_set_master(int fd);
+static void drm_drop_master(int fd);
+
 /**************
  * UdevHelper *
  **************/
@@ -210,6 +213,7 @@
 
 int mggh::DRMHelper::open_drm_device(UdevHelper const& udev)
 {    
+    std::string excstr;
     int tmp_fd = -1;
     int error;
 
@@ -237,6 +241,7 @@
 
         if ((error = is_appropriate_device(udev, dev)))
         {
+            excstr = "Udev is not appropriate";
             udev_device_unref(dev);
             continue;
         }
@@ -247,6 +252,7 @@
         tmp_fd = open(dev_path, O_RDWR, O_CLOEXEC);
         if (tmp_fd < 0)
         {
+            excstr = "Direct opening of drm device failed";
             error = errno;
             udev_device_unref(dev);
             continue;
@@ -254,6 +260,7 @@
 
         udev_device_unref(dev);
 
+        drm_set_master(tmp_fd);
         // Check that the drm device is usable by setting the interface version we use (1.4)
         drmSetVersion sv;
         sv.drm_di_major = 1;
@@ -263,10 +270,13 @@
 
         if ((error = drmSetInterfaceVersion(tmp_fd, &sv)))
         {
+            excstr = "Invalid interface version";
+            drm_drop_master(tmp_fd);
             close(tmp_fd);
             tmp_fd = -1;
             continue;
         }
+        drm_drop_master(tmp_fd);
 
         // Stop if this device has connections to display on
         if (count_connections(tmp_fd) > 0)
@@ -281,16 +291,19 @@
     {
         BOOST_THROW_EXCEPTION(
             boost::enable_error_info(
-                std::runtime_error("Error opening DRM device")) << boost::errinfo_errno(-error));
+                std::runtime_error("Error opening DRM device. Reason: " + excstr)) << boost::errinfo_errno(-error));
     }
 
+    drm_set_master(tmp_fd);
     return tmp_fd;
 }
 
 mggh::DRMHelper::~DRMHelper()
 {
     if (fd >= 0)
+    {
         mgg::drm_close_threadsafe(fd);
+    }
 }
 
 /*************
@@ -470,3 +483,38 @@
 {
     f(egl_display, egl_config);
 }
+
+void drm_set_master(int fd)
+{
+    if (fd < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            std::runtime_error("Tried to set DRM master without a DRM device"));
+    }
+
+    int ret = drmSetMaster(fd);
+    if (ret < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to set DRM master"))
+                    << boost::errinfo_errno(-ret));
+    }
+}
+
+void drm_drop_master(int fd)
+{
+    if (fd < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            std::runtime_error("Tried to drop DRM master without a DRM device"));
+    }
+    int ret = drmDropMaster(fd);
+    if (ret < 0)
+    {
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to drop DRM master"))
+                    << boost::errinfo_errno(-ret));
+    }
+}

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_display.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_display.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_display.cpp	2013-09-10 13:47:55 +0000
@@ -647,7 +647,7 @@
     using namespace testing;
 
     EXPECT_CALL(mock_drm, drmDropMaster(mock_drm.fake_drm.fd()))
-        .Times(1);
+        .Times(2);
 
     auto display = create_display(create_platform());
 
@@ -659,7 +659,7 @@
     using namespace testing;
 
     EXPECT_CALL(mock_drm, drmSetMaster(mock_drm.fake_drm.fd()))
-        .Times(1);
+        .Times(3);
 
     auto display = create_display(create_platform());
 
@@ -670,10 +670,17 @@
 {
     using namespace testing;
 
+    // drmDropMaster, drmSetMaster will be successful one time because they
+    // are called with a valid file descriptor in gbm_display_helpers.cpp
+    // open_drm_device()
+
     EXPECT_CALL(mock_drm, drmDropMaster(_))
+        .WillOnce(Return(0))
         .WillOnce(SetErrnoAndReturn(EACCES, -EACCES));
 
     EXPECT_CALL(mock_drm, drmSetMaster(_))
+        .WillOnce(Return(0))
+        .WillOnce(Return(0))
         .WillOnce(SetErrnoAndReturn(EPERM, -EPERM));
 
     EXPECT_CALL(*mock_report, report_drm_master_failure(EACCES));