Merge lp:~andreas-pokorny/mir/fix-1391975 into lp:mir

[Andreas Pokorny]

Uses new DRM_CAP_CURSOR_WIDTH/HEIGHT to query maximum sizes for cursors and verifies the buffer size with gbm after allocation.

Kaveri based systems seem to provide 128x128 buffers whenever GBM_BO_USE_CURSOR is requested. (LP: #1391975)

This change adds some DRM capability test for cursor buffers, and only uses the buffer sizes reported by gbm_bo_get_width/height when writing cursor images. This should fix the graphical glitches seen on amd kaveri systems and possibly also on tegra.

[Alexandros Frantzis (afrantzis)]

89 + struct CursorCapabilities
90 + {
91 + CursorCapabilities(int connection);
92 + uint64_t width, height;
93 + } cursor_cap;

Is there a reason to expose this in the header?

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2106
http://jenkins.qa.ubuntu.com/job/mir-ci/2270/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/361
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/361
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/335
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/259
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/259/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/335
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/335/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3579
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16281

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

> 89 + struct CursorCapabilities
> 90 + {
> 91 + CursorCapabilities(int connection);
> 92 + uint64_t width, height;
> 93 + } cursor_cap;
>
> Is there a reason to expose this in the header?

As of this revision, only convenience, I guess. cursor.h is not really visible... and it only burdens the header with int and uint64_t

I could also do an anonymous get_drm_cursor_cap_width.. it only exists that way because in a previous version I also used it as a buffer_width/buffer_height.

[Andreas Pokorny (andreas-pokorny)]

+ 78 and + 79 assume that drm drivers will provide us with sanely sized and supported cursor buffer sizes... That assumption might be wrong. Should I set a something like maximum to request?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2107
http://jenkins.qa.ubuntu.com/job/mir-ci/2273/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/366
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/366
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/340
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/262
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/262/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3583
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16291

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

OK, so GBM_BO_USE_CURSOR_64X64 is not being honoured by radeon. Sounds like the GBM header needs to remove "64X64" if that's not a bug and GBM is not required to honour the "64x64" dimensions.

But aside from learning to not trust the "64X64", there are still two possibilities:
(a) gbo_bo_get_width/height() returns the right answer. If so then just rely on that. No need to query drmGetCap; or
(b) gbm_bo_get_width/height() returns the wrong answer. If so then we need to do a fix entirely in Mesa/DRM.

So you have either:
  (a) Mir needs fixing, but a much simpler fix than this; or
  (b) Mir doesn't need fixing and Mesa/DRM does instead.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Actually, Mir needs fixing either way, to query the actual buffer width/height.

[Andreas Pokorny (andreas-pokorny)]

GBM_BO_USE_CURSOR_64X64 is just commented deprecated in gbm now. In my tests gbm_bo_get_width/height used to return the right size. drmGetCap is only a defensive addition to make sure that creating the buffer wont fail.

[Daniel van Vugt (vanvugt)]

drmGetCap doesn't seem to be any more defensive than simply calling gbm_bo_get_width/height. In fact it might be more error-prone to risk requesting something that's not 64x64 for other drivers. So we should ensure we're always asking for 64x64 right now, but also be careful to query what we're actually given:
   gbm_bo_get_width
   gbm_bo_get_height

[Robert Carr (robertcarr)]

Looks like Alexandros concerned is addressed.

Not sure I understand Daniels concern. drmGetCap is used PRIOR to the gbm_bo existing and to choose the maximum width/height values which can be passed to gbm_bo_create. It's conceivable as per the API for example that you would see there is a 512x512 cursor buffer choose to allocate a 256x256 one and receive a 128x128 one in response. I cant imagine any situation where we dont want the maximum sized cursor buffer, but this does seem to be the only correct way to use the API.

>> but also be careful to query what we're actually given:
>> gbm_bo_get_width
>> gbm_bo_get_height

As this branch does.

[Robert Carr (robertcarr)]

+auto get_drm_cursor_width(int fd) -> int

I dont understand the point for usage of auto here.

90: whitespace
128: Is this the only option?
293: Debugging cout left in.

Looks ok to me though.

review: Approve

[Daniel van Vugt (vanvugt)]

I was thinking in a more pure sense that we should only aim to fix a bug in a bug fix and not enhance at the same time. So in that way, always requesting 64x64 would eliminate the need for all the drmGetCap stuff.

But yeah, if the goal is to enhance and add support for larger cursors in the future then you do need to do it this way.

review: Abstain

[Daniel van Vugt (vanvugt)]

> + 78 and + 79 assume that drm drivers will provide us with sanely sized and
> supported cursor buffer sizes... That assumption might be wrong. Should I set
> a something like maximum to request?

Yes that's a big concern actually. A driver could feasibly have no small limitations. If it gives you 4096x4096 for example, we'd end up allocating 64MB just for a cursor.

We do need to impose some limit on the dimensions in our code. And if that limit is 64x64 then you might not need the drmGetCap stuff either :)

review: Needs Fixing

[Kevin DuBois (kdub)]

104 + GBMBOWrapper(gbm_device* gbm, int width, int height);
112/113
could be richer with geom::Size

l132, 32
duplicated defines

159 + .WillByDefault(Invoke([](int, uint64_t, uint64_t *val) -> int
160 + {
161 + *val = 64;
162 + return 1;
163 + })
164 + );
WillByDefault(DoAll(SetArgPointee<2>(64), Return(1)))

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Since we're now in a world of dynamically sizeable cursor buffers, why not just make the cursor buffer the dimensions of the cursor image? :)

That's really an enhancement still though. If you wanted this branch out the way quickly, you could just continue asking for 64x64 and remember to gbm_bo_get_width/height. It's a trivially small change to just do that and declare the bug fixed.

[Andreas Pokorny (andreas-pokorny)]

> Since we're now in a world of dynamically sizeable cursor buffers, why not
> just make the cursor buffer the dimensions of the cursor image? :)
>
> That's really an enhancement still though. If you wanted this branch out the
> way quickly, you could just continue asking for 64x64 and remember to
> gbm_bo_get_width/height. It's a trivially small change to just do that and
> declare the bug fixed.

I believe we are not in that world yet. Drivers that support cursors through planes just have some fixed size plane allocated... And then still we get access to the whole buffer size. So it does not make sense to query for a weakly defined notion of cursor width and height capability, to then query the size, and then finally see what we received.

So yes reduced it a lot..

For larger cursors or for properly using pc hardware we should do the inverse anyhow. So just use planes and no cursors at all.. But that needs more work in several places.

[Alberto Aguirre (albaguirre)]

Looks much simpler :)

review: Approve

[Daniel van Vugt (vanvugt)]

review: Approve

=== modified file 'src/platform/graphics/mesa/cursor.cpp'
--- src/platform/graphics/mesa/cursor.cpp	2014-12-04 03:47:52 +0000
+++ src/platform/graphics/mesa/cursor.cpp	2014-12-04 10:08:41 +0000
@@ -35,8 +35,7 @@
 
 namespace
 {
-int const buffer_width = 64;
-int const buffer_height = 64;
+const uint64_t requested_cursor_size = 64;
 
 // Transforms a relative position within the display bounds described by \a rect which is rotated with \a orientation
 geom::Displacement transform(geom::Rectangle const& rect, geom::Displacement const& vector, MirOrientation orientation)
@@ -59,10 +58,10 @@
 mgm::Cursor::GBMBOWrapper::GBMBOWrapper(gbm_device* gbm) :
     buffer(gbm_bo_create(
                 gbm,
-                buffer_width,
-                buffer_height,
+                requested_cursor_size,
+                requested_cursor_size,
                 GBM_FORMAT_ARGB8888,
-                GBM_BO_USE_CURSOR_64X64 | GBM_BO_USE_WRITE))
+                GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE))
 {
     if (!buffer) BOOST_THROW_EXCEPTION(std::runtime_error("failed to create gbm buffer"));
 }
@@ -79,6 +78,8 @@
         current_position(),
         visible(true),
         buffer(gbm),
+        buffer_width(gbm_bo_get_width(buffer)),
+        buffer_height(gbm_bo_get_height(buffer)),
         current_configuration(current_configuration)
 {
     show(*initial_image);

=== modified file 'src/platform/graphics/mesa/cursor.h'
--- src/platform/graphics/mesa/cursor.h	2014-12-04 03:47:52 +0000
+++ src/platform/graphics/mesa/cursor.h	2014-12-04 10:08:41 +0000
@@ -108,6 +108,9 @@
         GBMBOWrapper& operator=(GBMBOWrapper const&) = delete;
     } buffer;
 
+    uint32_t buffer_width;
+    uint32_t buffer_height;
+
     std::shared_ptr<CurrentConfiguration> const current_configuration;
 };
 }

=== modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
--- tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-12-04 03:47:52 +0000
+++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-12-04 10:08:41 +0000
@@ -220,8 +220,22 @@
 };
 void const* StubCursorImage::image_data = reinterpret_cast<void*>(&StubCursorImage::image_data);
 
-struct MesaCursorTest : public ::testing::Test
+struct MesaCursorTest : ::testing::Test
 {
+    struct MockGBM : testing::NiceMock<mtd::MockGBM>
+    {
+        MockGBM()
+        {
+            using namespace ::testing;
+            const uint32_t default_cursor_size = 64;
+            ON_CALL(*this, gbm_bo_get_width(_))
+                .WillByDefault(Return(default_cursor_size));
+            ON_CALL(*this, gbm_bo_get_height(_))
+                .WillByDefault(Return(default_cursor_size));
+
+        }
+    } mock_gbm;
+
     MesaCursorTest()
         : cursor{mock_gbm.fake_gbm.device, output_container,
             mt::fake_shared(current_configuration),
@@ -231,11 +245,26 @@
 
     StubCurrentConfiguration current_configuration;
     StubCursorImage stub_image;
-    testing::NiceMock<mtd::MockGBM> mock_gbm;
     StubKMSOutputContainer output_container;
     mgm::Cursor cursor;
 };
 
+struct SinglePixelCursorImage : public StubCursorImage
+{
+    geom::Size size() const
+    {
+        return small_cursor_size;
+    }
+    void const* as_argb_8888() const
+    {
+        static uint32_t const pixel = 0xffffffff;
+        return &pixel;
+    }
+
+    size_t const cursor_side{1};
+    geom::Size const small_cursor_size{cursor_side, cursor_side};
+};
+
 }
 
 TEST_F(MesaCursorTest, creates_cursor_bo_image)
@@ -244,7 +273,19 @@
     EXPECT_CALL(mock_gbm, gbm_bo_create(mock_gbm.fake_gbm.device,
                                         cursor_side, cursor_side,
                                         GBM_FORMAT_ARGB8888,
-                                        GBM_BO_USE_CURSOR_64X64 | GBM_BO_USE_WRITE));
+                                        GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE));
+
+    mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
+        std::make_shared<StubCurrentConfiguration>(),
+        std::make_shared<StubCursorImage>()};
+}
+
+TEST_F(MesaCursorTest, queries_received_cursor_size)
+{
+    using namespace ::testing;
+
+    EXPECT_CALL(mock_gbm, gbm_bo_get_width(_));
+    EXPECT_CALL(mock_gbm, gbm_bo_get_height(_));
 
     mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
         std::make_shared<StubCurrentConfiguration>(),
@@ -266,12 +307,12 @@
 }
 
 // When we upload our 1x1 cursor we should upload a single white pixel and then transparency filling a 64x64 buffer.
-MATCHER(ContainsASingleWhitePixel, "")
+MATCHER_P(ContainsASingleWhitePixel, buffersize, "")
 {
     auto pixels = static_cast<uint32_t const*>(arg);
     if (pixels[0] != 0xffffffff)
         return false;
-    for (int i = 1; i < 64*64; i++)
+    for (decltype(buffersize) i = 1; i < buffersize; i++)
     {
         if (pixels[i] != 0x0)
             return false;
@@ -282,23 +323,6 @@
 TEST_F(MesaCursorTest, show_cursor_pads_missing_data)
 {
     using namespace testing;
-
-    struct SinglePixelCursorImage : public StubCursorImage
-    {
-        geom::Size size() const
-        {
-            return small_cursor_size;
-        }
-        void const* as_argb_8888() const
-        {
-            static uint32_t const pixel = 0xffffffff;
-            return &pixel;
-        }
-
-        size_t const cursor_side{1};
-        geom::Size const small_cursor_size{cursor_side, cursor_side};
-    };
-
     // We expect a full 64x64 pixel write as we will pad missing data with transparency.
     size_t const height = 64;
     size_t const width = 64;
@@ -306,11 +330,34 @@
     size_t const buffer_size_bytes{height * stride};
     ON_CALL(mock_gbm, gbm_bo_get_stride(_))
         .WillByDefault(Return(stride));
-    EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, ContainsASingleWhitePixel(), buffer_size_bytes));
+    EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, ContainsASingleWhitePixel(width*height), buffer_size_bytes));
 
     cursor.show(SinglePixelCursorImage());
 }
 
+TEST_F(MesaCursorTest, pads_missing_data_when_buffer_size_differs)
+{
+    using namespace ::testing;
+
+    size_t const height = 128;
+    size_t const width = 128;
+    size_t const stride = width * 4;
+    size_t const buffer_size_bytes{height * stride};
+
+    ON_CALL(mock_gbm, gbm_bo_get_width(mock_gbm.fake_gbm.bo))
+        .WillByDefault(Return(width));
+    ON_CALL(mock_gbm, gbm_bo_get_height(mock_gbm.fake_gbm.bo))
+        .WillByDefault(Return(height));
+    ON_CALL(mock_gbm, gbm_bo_get_stride(mock_gbm.fake_gbm.bo))
+        .WillByDefault(Return(stride));
+
+    EXPECT_CALL(mock_gbm, gbm_bo_write(mock_gbm.fake_gbm.bo, ContainsASingleWhitePixel(width*height), buffer_size_bytes));
+
+    mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
+        std::make_shared<StubCurrentConfiguration>(),
+        std::make_shared<SinglePixelCursorImage>()};
+}
+
 TEST_F(MesaCursorTest, throws_when_images_are_too_large)
 {
     using namespace testing;