Mir

Merge lp:~albaguirre/mir/fix-1549359 into lp:mir

  1. fix-1549359
  2. Merge into development-branch
Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 3335
Proposed branch: lp:~albaguirre/mir/fix-1549359
Merge into: lp:mir
Diff against target: 233 lines (+97/-9)
3 files modified
src/platforms/mesa/server/kms/display.cpp (+9/-3)
src/platforms/mesa/server/kms/display.h (+2/-1)
tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp (+86/-5)
To merge this branch: bzr merge lp:~albaguirre/mir/fix-1549359
Reviewer Review Type Date Requested Status
Mir CI Bot continuous-integration Approve
Cemil Azizoglu (community) Approve
Alan Griffiths Approve
Review via email: mp+287057@code.launchpad.net

Commit message

mesa-kms: display configuration - only query drm when a change is notified

Querying drm display configuration can be expensive - only query when necessary

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

FAILED: Continuous integration, rev:3333
https://mir-jenkins.ubuntu.com/job/mir-ci/389/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/182/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/196
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/192
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/192
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/187/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/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/187/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/187
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/187/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/187/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

^-- Failure looks like lp:1523621

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Looks like a sensible way to reduce the impact of the underlying problem. But we really ought to avoid any chance of doing significant work (like probing display hardware) on the IPC thread.

review: Approve
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

LGTM

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3333
https://mir-jenkins.ubuntu.com/job/mir-ci/390/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/183
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/199
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/188/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/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/188/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/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/188/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/188/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/platforms/mesa/server/kms/display.cpp'
--- src/platforms/mesa/server/kms/display.cpp 2016-02-01 22:53:06 +0000
+++ src/platforms/mesa/server/kms/display.cpp 2016-02-24 16:29:41 +0000
@@ -96,6 +96,7 @@
96 output_container{drm->fd,96 output_container{drm->fd,
97 std::make_shared<KMSPageFlipper>(drm->fd, listener)},97 std::make_shared<KMSPageFlipper>(drm->fd, listener)},
98 current_display_configuration{drm->fd},98 current_display_configuration{drm->fd},
99 dirty_configuration{false},
99 bypass_option(bypass_option),100 bypass_option(bypass_option),
100 gl_config{gl_config}101 gl_config{gl_config}
101{102{
@@ -133,8 +134,12 @@
133{134{
134 std::lock_guard<std::mutex> lg{configuration_mutex};135 std::lock_guard<std::mutex> lg{configuration_mutex};
135136
136 /* Give back a copy of the latest configuration information */137 if (dirty_configuration)
137 current_display_configuration.update();138 {
139 /* Give back a copy of the latest configuration information */
140 current_display_configuration.update();
141 dirty_configuration = false;
142 }
138 return std::unique_ptr<mg::DisplayConfiguration>(143 return std::unique_ptr<mg::DisplayConfiguration>(
139 new mgm::RealKMSDisplayConfiguration(current_display_configuration)144 new mgm::RealKMSDisplayConfiguration(current_display_configuration)
140 );145 );
@@ -268,9 +273,10 @@
268 make_module_ptr<std::function<void(int)>>(273 make_module_ptr<std::function<void(int)>>(
269 [conf_change_handler, this](int)274 [conf_change_handler, this](int)
270 {275 {
271 monitor.process_events([conf_change_handler]276 monitor.process_events([conf_change_handler, this]
272 (mir::udev::Monitor::EventType, mir::udev::Device const&)277 (mir::udev::Monitor::EventType, mir::udev::Device const&)
273 {278 {
279 dirty_configuration = true;
274 conf_change_handler();280 conf_change_handler();
275 });281 });
276 }));282 }));
277283
=== modified file 'src/platforms/mesa/server/kms/display.h'
--- src/platforms/mesa/server/kms/display.h 2016-02-01 22:53:06 +0000
+++ src/platforms/mesa/server/kms/display.h 2016-02-24 16:29:41 +0000
@@ -25,8 +25,8 @@
25#include "display_helpers.h"25#include "display_helpers.h"
26#include "platform_common.h"26#include "platform_common.h"
2727
28#include <atomic>
28#include <mutex>29#include <mutex>
29
30#include <vector>30#include <vector>
3131
32namespace mir32namespace mir
@@ -106,6 +106,7 @@
106 std::vector<std::unique_ptr<DisplayBuffer>> display_buffers;106 std::vector<std::unique_ptr<DisplayBuffer>> display_buffers;
107 RealKMSOutputContainer output_container;107 RealKMSOutputContainer output_container;
108 mutable RealKMSDisplayConfiguration current_display_configuration;108 mutable RealKMSDisplayConfiguration current_display_configuration;
109 mutable std::atomic<bool> dirty_configuration;
109110
110 BypassOption bypass_option;111 BypassOption bypass_option;
111 std::weak_ptr<Cursor> cursor;112 std::weak_ptr<Cursor> cursor;
112113
=== modified file 'tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp'
--- tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp 2016-01-29 08:18:22 +0000
+++ tests/unit-tests/graphics/mesa/kms/test_display_configuration.cpp 2016-02-24 16:29:41 +0000
@@ -20,22 +20,26 @@
20#include "mir/graphics/display_configuration.h"20#include "mir/graphics/display_configuration.h"
21#include "mir/graphics/display.h"21#include "mir/graphics/display.h"
22#include "mir/graphics//default_display_configuration_policy.h"22#include "mir/graphics//default_display_configuration_policy.h"
23#include "mir/time/steady_clock.h"
24#include "mir/glib_main_loop.h"
23#include "src/platforms/mesa/server/kms/platform.h"25#include "src/platforms/mesa/server/kms/platform.h"
24#include "src/platforms/mesa/server/kms/kms_display_configuration.h"26#include "src/platforms/mesa/server/kms/kms_display_configuration.h"
25#include "mir/test/doubles/null_emergency_cleanup.h"
26#include "src/server/report/null_report_factory.h"27#include "src/server/report/null_report_factory.h"
27#include "mir/test/doubles/null_virtual_terminal.h"28
29#include "mir/test/signal.h"
30#include "mir/test/auto_unblock_thread.h"
2831
29#include "mir/test/doubles/mock_egl.h"32#include "mir/test/doubles/mock_egl.h"
30#include "mir/test/doubles/mock_gl.h"33#include "mir/test/doubles/mock_gl.h"
34#include "mir/test/doubles/mock_drm.h"
35#include "mir/test/doubles/mock_gbm.h"
36#include "mir/test/doubles/null_emergency_cleanup.h"
37#include "mir/test/doubles/null_virtual_terminal.h"
31#include "mir/test/doubles/stub_gl_config.h"38#include "mir/test/doubles/stub_gl_config.h"
32#include "mir/test/doubles/stub_gl_program_factory.h"39#include "mir/test/doubles/stub_gl_program_factory.h"
3340
34#include "mir_test_framework/udev_environment.h"41#include "mir_test_framework/udev_environment.h"
3542
36#include "mir/test/doubles/mock_drm.h"
37#include "mir/test/doubles/mock_gbm.h"
38
39#include <gmock/gmock.h>43#include <gmock/gmock.h>
40#include <gtest/gtest.h>44#include <gtest/gtest.h>
4145
@@ -44,6 +48,7 @@
44namespace mg = mir::graphics;48namespace mg = mir::graphics;
45namespace mgm = mir::graphics::mesa;49namespace mgm = mir::graphics::mesa;
46namespace geom = mir::geometry;50namespace geom = mir::geometry;
51namespace mt = mir::test;
47namespace mtd = mir::test::doubles;52namespace mtd = mir::test::doubles;
48namespace mtf = mir_test_framework;53namespace mtf = mir_test_framework;
4954
@@ -65,6 +70,24 @@
65 return mg::DisplayConfigurationMode{size, vrefresh_hz};70 return mg::DisplayConfigurationMode{size, vrefresh_hz};
66}71}
6772
73struct MainLoop
74{
75 MainLoop()
76 {
77 int const owner{0};
78 mt::Signal mainloop_started;
79 ml.enqueue(&owner, [&] { mainloop_started.raise(); });
80 mt::AutoUnblockThread t([this]{ml.stop();}, [this]{ml.run();});
81 bool const started = mainloop_started.wait_for(std::chrono::seconds(10));
82 if (!started)
83 throw std::runtime_error("Failed to start main loop");
84
85 ml_thread = std::move(t);
86 }
87 mir::GLibMainLoop ml{std::make_shared<mir::time::SteadyClock>()};
88 mt::AutoUnblockThread ml_thread;
89};
90
68class MesaDisplayConfigurationTest : public ::testing::Test91class MesaDisplayConfigurationTest : public ::testing::Test
69{92{
70public:93public:
@@ -362,6 +385,7 @@
362TEST_F(MesaDisplayConfigurationTest, returns_updated_configuration)385TEST_F(MesaDisplayConfigurationTest, returns_updated_configuration)
363{386{
364 using namespace ::testing;387 using namespace ::testing;
388 using namespace std::chrono_literals;
365389
366 uint32_t const invalid_id{0};390 uint32_t const invalid_id{0};
367 std::vector<uint32_t> const crtc_ids{10, 11};391 std::vector<uint32_t> const crtc_ids{10, 11};
@@ -468,6 +492,7 @@
468492
469 /* Set up DRM resources and check */493 /* Set up DRM resources and check */
470 mtd::FakeDRMResources& resources(mock_drm.fake_drm);494 mtd::FakeDRMResources& resources(mock_drm.fake_drm);
495 auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
471496
472 resources.reset();497 resources.reset();
473498
@@ -530,6 +555,13 @@
530555
531 resources.prepare();556 resources.prepare();
532557
558 /* Fake a device change so display can fetch updated configuration*/
559 MainLoop ml;
560 mt::Signal handler_signal;
561 display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
562 fake_devices.emit_device_changed(syspath);
563 ASSERT_TRUE(handler_signal.wait_for(10s));
564
533 conf = display->configuration();565 conf = display->configuration();
534566
535 card_count = 0;567 card_count = 0;
@@ -556,6 +588,7 @@
556TEST_F(MesaDisplayConfigurationTest, new_monitor_defaults_to_preferred_mode)588TEST_F(MesaDisplayConfigurationTest, new_monitor_defaults_to_preferred_mode)
557{589{
558 using namespace ::testing;590 using namespace ::testing;
591 using namespace std::chrono_literals;
559592
560 uint32_t const crtc_ids[1] = {10};593 uint32_t const crtc_ids[1] = {10};
561 uint32_t const encoder_ids[2] = {20, 21};594 uint32_t const encoder_ids[2] = {20, 21};
@@ -612,6 +645,7 @@
612 };645 };
613646
614 mtd::FakeDRMResources& resources(mock_drm.fake_drm);647 mtd::FakeDRMResources& resources(mock_drm.fake_drm);
648 auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
615649
616 resources.reset();650 resources.reset();
617 resources.add_crtc(crtc_ids[0], modes0[1]);651 resources.add_crtc(crtc_ids[0], modes0[1]);
@@ -644,6 +678,12 @@
644 connector_physical_sizes_mm_after);678 connector_physical_sizes_mm_after);
645 resources.prepare();679 resources.prepare();
646680
681 MainLoop ml;
682 mt::Signal handler_signal;
683 display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
684 fake_devices.emit_device_changed(syspath);
685 ASSERT_TRUE(handler_signal.wait_for(10s));
686
647 conf = display->configuration();687 conf = display->configuration();
648 output_count = 0;688 output_count = 0;
649 conf->for_each_output([&](mg::DisplayConfigurationOutput const& output)689 conf->for_each_output([&](mg::DisplayConfigurationOutput const& output)
@@ -654,3 +694,44 @@
654 });694 });
655 EXPECT_EQ(noutputs, output_count);695 EXPECT_EQ(noutputs, output_count);
656}696}
697
698TEST_F(MesaDisplayConfigurationTest, does_not_query_drm_unnecesarily)
699{
700 using namespace ::testing;
701 using namespace std::chrono_literals;
702
703 uint32_t const crtc_id{10};
704 uint32_t const encoder_id{20};
705 uint32_t const connector_id{30};
706 geom::Size const connector_physical_sizes_mm{480, 270};
707 std::vector<uint32_t> possible_encoder_ids_empty;
708
709 uint32_t const possible_crtcs_mask_empty{0};
710 mtd::FakeDRMResources& resources(mock_drm.fake_drm);
711 resources.reset();
712 resources.add_crtc(crtc_id, modes0[1]);
713 resources.add_encoder(encoder_id, crtc_id, possible_crtcs_mask_empty);
714 resources.add_connector(connector_id, DRM_MODE_CONNECTOR_Composite,
715 DRM_MODE_CONNECTED, encoder_id,
716 modes0, possible_encoder_ids_empty,
717 connector_physical_sizes_mm);
718 resources.prepare();
719
720 auto const syspath = fake_devices.add_device("drm", "card2", NULL, {}, {"DEVTYPE", "drm_minor"});
721
722 auto display = create_display(create_platform());
723
724 // No display change reported yet so display should not query drm
725 EXPECT_CALL(mock_drm, drmModeGetConnector(_,_)).Times(0);
726 display->configuration();
727
728 // Now signal a device change
729 MainLoop ml;
730 mt::Signal handler_signal;
731 display->register_configuration_change_handler(ml.ml, [&handler_signal]{handler_signal.raise();});
732 fake_devices.emit_device_changed(syspath);
733 ASSERT_TRUE(handler_signal.wait_for(10s));
734
735 EXPECT_CALL(mock_drm, drmModeGetConnector(_,_)).Times(1);
736 display->configuration();
737}

Subscribers

People subscribed via source and target branches