Merge lp:~robert-ancell/mir/gbm-check-connections into lp:~mir-team/mir/trunk

[Robert Ancell]

Only use a DRM device if it has connections

Pick the first DRM device that has connections.

I'm least sure about the testing changes, but I discussed then with Alexandros on a hangout and I think they match what he expected.

[Alexandros Frantzis (afrantzis)]

8 +int mggh::DRMHelper::count_connections(int fd)

In drm_mode_resources.h we have a class DRMModeResources for dealing with resources in a more elegant and safe way. We should use it here. See kms_display_configuration.cpp for an example of how to use it.

72 - EXPECT_CALL(mock_drm, drmModeFreeResources(_))
73 - .Times(Exactly(0));
74 -
75 - EXPECT_CALL(mock_drm, drmClose(_))
76 - .Times(Exactly(1));

We should keep these. They check that we handle failure gracefully (i.e., that we don't try to free null resources, and that we shut down drm properly).

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:804
http://jenkins.qa.ubuntu.com/job/mir-ci/907/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1163
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1048
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/145
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/145/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/552

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

> 72 - EXPECT_CALL(mock_drm, drmModeFreeResources(_))
> 73 - .Times(Exactly(0));
> 74 -
> 75 - EXPECT_CALL(mock_drm, drmClose(_))
> 76 - .Times(Exactly(1));
>
> We should keep these. They check that we handle failure gracefully (i.e., that
> we don't try to free null resources, and that we shut down drm properly).

drmClose isn't called, because the DRM device is open()ed and close()d directly.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:807
http://jenkins.qa.ubuntu.com/job/mir-ci/915/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1174
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1059
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/153
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/153/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/559

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> drmClose isn't called, because the DRM device is open()ed and close()d directly.

We are still using drmClose:

$ grep -R drmClose src/
src/server/graphics/gbm/drm_close_threadsafe.cpp: return drmClose(fd);

$ grep -R drm_close_threadsafe\( src/
src/server/graphics/gbm/gbm_platform.cpp: mgg::drm_close_threadsafe(ipc_fds[0]);
src/server/graphics/gbm/drm_close_threadsafe.cpp:int mgg::drm_close_threadsafe(int fd)
src/server/graphics/gbm/drm_close_threadsafe.h:int drm_close_threadsafe(int fd);
src/server/graphics/gbm/gbm_display_helpers.cpp: mgg::drm_close_threadsafe(fd);

[Robert Ancell (robert-ancell)]

> > drmClose isn't called, because the DRM device is open()ed and close()d
> directly.
>
> We are still using drmClose:

Yes, drmClose is the codebase but *in this particular case* drmClose is not called because the new code bails out earlier than before. I've updated the test to make this clearer.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:809
http://jenkins.qa.ubuntu.com/job/mir-ci/930/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1079
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/168
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/168/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/571

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

135 - resources.add_connector(id, DRM_MODE_DISCONNECTED, invalid_id,
136 - modes_empty, possible_encoder_ids_empty,
137 + resources.add_connector(id, DRM_MODE_CONNECTED, encoder0_id,

This looks suspiciously like changing one test scenario to an other, where adding the new scenario would be better.

But as Alexandros is more familiar with this logic I'll let him decide.

review: Abstain

[Alexandros Frantzis (afrantzis)]

> > drmClose isn't called, because the DRM device is open()ed and close()d
> directly.
>
> We are still using drmClose:

OK, in that case the test is no longer a GBMDisplay test, but a GBMPlatform test, and an adaptation of it should be placed in test_gbm_platform.cpp. For this test I propose the following changes:

TEST_F(GBMDisplayTest, create_display_kms_failure)
{
    using namespace testing;

    auto platform = create_platform();

    Mock::VerifyAndClearExpectations(&mock_drm);

    EXPECT_CALL(mock_drm, drmModeGetResources(_))
        .Times(Exactly(1))
        .WillOnce(Return(reinterpret_cast<drmModeRes*>(0)));

    EXPECT_CALL(mock_drm, drmModeFreeResources(_))
        .Times(Exactly(0));

    EXPECT_CALL(mock_drm, drmClose(_))
        .Times(Exactly(1));

    EXPECT_THROW({
        auto display = create_display(platform);
    }, std::runtime_error) << "Expected that c'tor of GBMDisplay throws";
}

[Alexandros Frantzis (afrantzis)]

> 135 - resources.add_connector(id, DRM_MODE_DISCONNECTED, invalid_id,
> 136 - modes_empty, possible_encoder_ids_empty,
> 137 + resources.add_connector(id, DRM_MODE_CONNECTED, encoder0_id,
>
> This looks suspiciously like changing one test scenario to an other, where
> adding the new scenario would be better.
>
> But as Alexandros is more familiar with this logic I'll let him decide.

These particular tests aren't affected by these changes; they don't care about the connection status of the connectors, so we can safely change them.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:811
http://jenkins.qa.ubuntu.com/job/mir-ci/956/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1120
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/194
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/194/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/596

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:813
http://jenkins.qa.ubuntu.com/job/mir-ci/958/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1238
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/196
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/196/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/598

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

=== modified file 'src/server/graphics/gbm/gbm_display_helpers.cpp'
--- src/server/graphics/gbm/gbm_display_helpers.cpp	2013-07-04 07:20:35 +0000
+++ src/server/graphics/gbm/gbm_display_helpers.cpp	2013-07-10 01:52:27 +0000
@@ -194,6 +194,20 @@
     return ENOMEDIUM;
 }
 
+int mggh::DRMHelper::count_connections(int fd)
+{
+    DRMModeResources resources{fd};
+
+    int n_connected = 0;
+    resources.for_each_connector([&](DRMModeConnectorUPtr connector)
+    {
+        if (connector->connection == DRM_MODE_CONNECTED)
+            n_connected++;
+    });
+
+    return n_connected;
+}
+
 int mggh::DRMHelper::open_drm_device(UdevHelper const& udev)
 {    
     int tmp_fd = -1;
@@ -254,8 +268,12 @@
             continue;
         }
 
-        // We currently only handle one DRM device
-        break;
+        // Stop if this device has connections to display on
+        if (count_connections(tmp_fd) > 0)
+            break;
+
+        close(tmp_fd);
+        tmp_fd = -1;
     }
     udev_enumerate_unref(enumerator);
 

=== modified file 'src/server/graphics/gbm/gbm_display_helpers.h'
--- src/server/graphics/gbm/gbm_display_helpers.h	2013-06-25 06:39:50 +0000
+++ src/server/graphics/gbm/gbm_display_helpers.h	2013-07-10 01:52:27 +0000
@@ -84,6 +84,8 @@
     // handling >1 DRM device.
     int is_appropriate_device(UdevHelper const& udev, udev_device* dev);
 
+    int count_connections(int fd);
+
     int open_drm_device(UdevHelper const& udev);
 };
 

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_display.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_display.cpp	2013-07-03 17:48:30 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_display.cpp	2013-07-10 01:52:27 +0000
@@ -347,6 +347,10 @@
 {
     using namespace testing;
 
+    auto platform = create_platform();
+
+    Mock::VerifyAndClearExpectations(&mock_drm);
+
     EXPECT_CALL(mock_drm, drmModeGetResources(_))
         .Times(Exactly(1))
         .WillOnce(Return(reinterpret_cast<drmModeRes*>(0)));
@@ -357,8 +361,6 @@
     EXPECT_CALL(mock_drm, drmClose(_))
         .Times(Exactly(1));
 
-    auto platform = create_platform();
-
     EXPECT_THROW({
         auto display = create_display(platform);
     }, std::runtime_error) << "Expected that c'tor of GBMDisplay throws";

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_display_configuration.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_display_configuration.cpp	2013-07-01 06:53:02 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_display_configuration.cpp	2013-07-10 01:52:27 +0000
@@ -209,19 +209,23 @@
 
 TEST_F(GBMDisplayConfigurationTest, get_kms_connector_id_returns_correct_id)
 {
-    uint32_t const invalid_id{0};
+    uint32_t const crtc0_id{10};
+    uint32_t const encoder0_id{20};
+    uint32_t const possible_crtcs_mask_empty{0};
     std::vector<uint32_t> const connector_ids{30, 31};
-    std::vector<uint32_t> possible_encoder_ids_empty;
+    std::vector<uint32_t> encoder_ids{20};
 
     /* Set up DRM resources */
     mtd::FakeDRMResources& resources(mock_drm.fake_drm);
 
     resources.reset();
 
+    resources.add_crtc(crtc0_id, modes0[1]);
+    resources.add_encoder(encoder0_id, crtc0_id, possible_crtcs_mask_empty);
     for (auto id : connector_ids)
     {
-        resources.add_connector(id, DRM_MODE_DISCONNECTED, invalid_id,
-                                modes_empty, possible_encoder_ids_empty,
+        resources.add_connector(id, DRM_MODE_CONNECTED, encoder0_id,
+                                modes0, encoder_ids,
                                 geom::Size());
     }
 
@@ -248,19 +252,23 @@
 
 TEST_F(GBMDisplayConfigurationTest, get_kms_connector_id_throws_on_invalid_id)
 {
-    uint32_t const invalid_id{0};
+    uint32_t const crtc0_id{10};
+    uint32_t const encoder0_id{20};
+    uint32_t const possible_crtcs_mask_empty{0};
     std::vector<uint32_t> const connector_ids{30, 31};
-    std::vector<uint32_t> possible_encoder_ids_empty;
+    std::vector<uint32_t> encoder_ids{20};
 
     /* Set up DRM resources */
     mtd::FakeDRMResources& resources(mock_drm.fake_drm);
 
     resources.reset();
 
+    resources.add_crtc(crtc0_id, modes0[1]);
+    resources.add_encoder(encoder0_id, crtc0_id, possible_crtcs_mask_empty);
     for (auto id : connector_ids)
     {
-        resources.add_connector(id, DRM_MODE_DISCONNECTED, invalid_id,
-                                modes_empty, possible_encoder_ids_empty,
+        resources.add_connector(id, DRM_MODE_CONNECTED, encoder0_id,
+                                modes0, encoder_ids,
                                 geom::Size());
     }
 

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_platform.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_platform.cpp	2013-07-08 16:02:07 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_platform.cpp	2013-07-10 01:52:27 +0000
@@ -120,6 +120,22 @@
     FAIL() << "Expected an exception to be thrown.";
 }
 
+TEST_F(GBMGraphicsPlatform, fails_if_no_resources)
+{
+    using namespace ::testing;
+
+    EXPECT_CALL(mock_drm, drmModeGetResources(_))
+        .Times(Exactly(1))
+        .WillOnce(Return(reinterpret_cast<drmModeRes*>(0)));
+
+    EXPECT_CALL(mock_drm, drmModeFreeResources(_))
+        .Times(Exactly(0));
+
+    EXPECT_THROW({
+        auto platform = create_platform();
+    }, std::runtime_error) << "Expected that c'tor of GBMPlatform throws";
+}
+
 /* ipc packaging tests */
 TEST_F(GBMGraphicsPlatform, test_ipc_data_packed_correctly)
 {