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

[Andreas Pokorny]

Fix corrupt AMD/radeon cursor again (LP: #1417581)

Include the omitted parts back into the bugfix

Back then when the cursor issue of radeons and kaveris was handled I omitted this check to simplify the fix. This part was necessary to make GBM return correct stride sizes. GBM seems to stick to the size provided on construction, and does not verify what buffer was returned by the driver. Yes this is broken in GBM, but we are currently not alone with this workaround.

Back when this bug first fixed we had two checks for the buffer sizes of cursors. In the process of the review I omitted one. But this check is essential, as it changes the size used in the request.

This MP adds the check again.

[Alexandros Frantzis (afrantzis)]

So, the assumption is that the cursor buffer returned by the driver has dimensions equal to DRM_CAP_CURSOR_WIDTH/HEIGHT; it seem reasonable although it's not clear if these dimensions represent the maximum, the preferred or the only acceptable cursor sizes.

> "Yes this is broken in GBM, but we are currently not alone with this workaround."

Looking at the GBM code, I would say that this is a problem with DRI apis. GBM is asking DRI for a WxH image and DRI is silently returning a differently sized image instead of failing? Not good API design.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

Yes, I suggest we apply the fix for us, but keep the bug open for mesa.. Untill we entirely drop using this API and switch to the plane API..

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2319
http://jenkins.qa.ubuntu.com/job/mir-ci/2975/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1257
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/972
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/972/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1257
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1257/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4294
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18059

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Happy to get a fix. Sad Mesa/GBM is still broken. I think I saw this same workaround in Weston. Sorry I made you remove it before; I didn't know just how broken Mesa is.

P.S. Can you make that comment point to the upstream Mesa bug URL? Create it if necessary.

P.P.S. Gerry needs to test this.

review: Approve

[lp:~andreas-pokorny/mir/fix-1417581]

2320. By Andreas Pokorny on 2015-02-16

freedesktop bug id added

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2320
http://jenkins.qa.ubuntu.com/job/mir-ci/2983/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1309
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1308
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1265
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/980
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/980/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1265
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1265/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4302
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18080

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

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

Works!

review: Approve (functional)

=== modified file 'src/platforms/mesa/server/cursor.cpp'
--- src/platforms/mesa/server/cursor.cpp	2015-01-22 09:00:14 +0000
+++ src/platforms/mesa/server/cursor.cpp	2015-02-16 09:03:15 +0000
@@ -24,6 +24,9 @@
 #include "mir/geometry/rectangle.h"
 #include "mir/graphics/cursor_image.h"
 
+#include <xf86drm.h>
+#include <drm/drm.h>
+
 #include <boost/exception/errinfo_errno.hpp>
 
 #include <stdexcept>
@@ -35,7 +38,7 @@
 
 namespace
 {
-const uint64_t requested_cursor_size = 64;
+const uint64_t fallback_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)
@@ -53,13 +56,38 @@
         return vector;
     }
 }
+// support for older drm headers
+#ifndef DRM_CAP_CURSOR_WIDTH
+#define DRM_CAP_CURSOR_WIDTH            0x8
+#define DRM_CAP_CURSOR_HEIGHT           0x9
+#endif
+
+// In certain combinations of DRI backends and drivers GBM
+// returns a stride size that matches the requested buffers size,
+// instead of the underlying buffer:
+// https://bugs.freedesktop.org/show_bug.cgi?id=89164
+int get_drm_cursor_height(int fd)
+{
+   uint64_t height;
+   if (drmGetCap(fd, DRM_CAP_CURSOR_HEIGHT, &height) < 0)
+       height = fallback_cursor_size;
+   return int(height);
+}
+
+int get_drm_cursor_width(int fd)
+{
+   uint64_t width;
+   if (drmGetCap(fd, DRM_CAP_CURSOR_WIDTH, &width) < 0)
+       width = fallback_cursor_size;
+   return int(width);
+}
 }
 
 mgm::Cursor::GBMBOWrapper::GBMBOWrapper(gbm_device* gbm) :
     buffer(gbm_bo_create(
                 gbm,
-                requested_cursor_size,
-                requested_cursor_size,
+                get_drm_cursor_width(gbm_device_get_fd(gbm)),
+                get_drm_cursor_height(gbm_device_get_fd(gbm)),
                 GBM_FORMAT_ARGB8888,
                 GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE))
 {