Severe graphical corruption (mostly horizontal streaks/lines) running software clients (including Xmir) on android

[Daniel van Vugt]

I'm seeing severe graphical corruption running mir_demo_client_flicker on mako.

Start a few instances of the client (or just one flicker and some other clients) and you'll see the mir_demo_client_flicker window contains significant corruption.

I don't think it's an overlays issue. The problem occurs even without overlays.

[Daniel van Vugt (vanvugt)]

This bug is now hurting Xmir on phones.

[Daniel van Vugt (vanvugt)]

I recall Alberto fixed a corruption bug just like this using magic HWC flags on the affected buffer. Not sure where that change went or if we still have it.

[Daniel van Vugt (vanvugt)]

Alberto's previous fix is here:

int mga::AndroidAllocAdaptor::convert_to_android_usage(BufferUsage usage)
{
    switch (usage)
    {
    case mga::BufferUsage::use_hardware:
        return (GRALLOC_USAGE_HW_TEXTURE | GRALLOC_USAGE_HW_RENDER);
    case mga::BufferUsage::use_framebuffer_gles:
        return (GRALLOC_USAGE_HW_RENDER | GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_FB);
    case mga::BufferUsage::use_software:
        return (GRALLOC_USAGE_SW_WRITE_OFTEN | GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_HW_COMPOSER | GRALLOC_USAGE_HW_TEXTURE);
    default:
        return -1;
    }
}

[Daniel van Vugt (vanvugt)]

Is it safe to mix HW and SW flags?

[Alberto Aguirre (albaguirre)]

@Daniel,

Yeah, those flags are used by the implementations to know when they need to do cache coherency operations (flush, invalidate, etc).

In android, software buffers CAN be used as backing to textures for opengl and can be passed directly to Hardware Composer HAL (HW Composer), hence they are marked as such.

[Daniel van Vugt (vanvugt)]

I'm fairly confident the problem is entirely in Mir. Not in Xmir. Especially given it's easy to reproduce with mir-demos.

[Daniel van Vugt (vanvugt)]

OK, I think this might be the problem. The client API is releasing its shared pointer the the software buffer region prematurely (before mir_buffer_stream_get_graphics_region even returns). This means we are calling Android's gralloc->unlock() way before the client has even started filling the buffer!

void mir_buffer_stream_get_graphics_region(
    MirBufferStream *buffer_stream,
    MirGraphicsRegion *region_out)
try
{
    mcl::ClientBufferStream *bs = reinterpret_cast<mcl::ClientBufferStream*>(buffer_stream);

    auto secured_region = bs->secure_for_cpu_write();
    region_out->width = secured_region->width.as_uint32_t();
    region_out->height = secured_region->height.as_uint32_t();
    region_out->stride = secured_region->stride.as_uint32_t();
    region_out->pixel_format = secured_region->format;
    region_out->vaddr = secured_region->vaddr.get();
}
catch (std::exception const& ex)
{
    MIR_LOG_UNCAUGHT_EXCEPTION(ex);
}

[Daniel van Vugt (vanvugt)]

Hmm, no. BufferStream::secured_region seems to hold it for us.

[Daniel van Vugt (vanvugt)]

Should we be concerned about this?...

src/platforms/android/server/buffer.cpp:
    //TODO: we should make use of the android egl fence extension here to update the fence.
    // if the extension is not available, we should pass out a token that the user
    // will have to keep until the completion of the gl draw

[Daniel van Vugt (vanvugt)]

In case anyone is thinking it, switching Xmir to glamor to work around this bug is not a good idea. We default to glamor in Xmir on desktop and it's still too buggy (work in progress). The full list of issues for Xmir+glamor is here: https://bugs.launchpad.net/ubuntu/+source/xorg-server/+bugs?field.tag=xmir

[Kevin DuBois (kdub)]

re:
Should we be concerned about this?...
no, these comments are a bit stale

[Kevin DuBois (kdub)]

The problem looks like a cpu cache problem.
We call gralloc's unlock function, and digging through the gralloc code, it looks like the internal native buffer flag that should trigger a cache flush is behaving properly. Now seeing what I can find out in the kernel....

[Kevin DuBois (kdub)]

I've been able to trace the calls down into the kernel, and it does look like the kernel is flushing the cache at the appropriate time before sending the buffer back to the client.

[kevin gunn (kgunn72)]

Just discovered something while working on bug 1513815

Basically, i was running pd on N4 set this env variable
SAL_USE_VCLPLUGIN=gen
which according to bjoern "forces LO to ignore the gtk plugin and uses the old and trusty X11-only backend" ...figure this is true for all Xapps?
anyhow, i launched FF in full screen mode...no corruption, then connected mouse...i see corruption...moused to full screen, no corruption. but there's a black bar when no corruption, but the contents are all shown...
see video as it's easier to understand
https://www.youtube.com/watch?v=PHbyv25Ekx0

[Kevin DuBois (kdub)]

Its seeming like the gpu-facing cache is the one that needs invalidation from the GL/gralloc driver, but is not being flushed. This is a CPU/GPU coordination issue, and the GPU-based scenarios all work through the same cache and do not experience this problem. If we use overlays, this is not an issue, as the MDP/CPU cache sync seems to work. (Unfortunately, overlays are not supported on external displays)

[Kevin DuBois (kdub)]

A (bad) workaround is to map the buffer server-side, and then use glTexImage2D (instead of glTargetTexture2D)

[Kevin DuBois (kdub)]

External indication that adreno drivers have an issue flushing their texture caches: https://codereview.chromium.org1241433003/

[Kevin DuBois (kdub)]

glFinish and other flushing-commands (eg, eglClientWaitSyncKHR with the flush bit) seem to be other alternatives that work, but those aren't very good alternatives either.

[Kevin DuBois (kdub)]

The corruption appears on krillin too, so probably not a specific chipset problem.

[Kevin DuBois (kdub)]

After today's investigation, I think the cachelines that are appearing as corruption are not from an lines that should have been invalidated, but are the flushed lines from the next frame. IE, we're releasing the buffer too early, and the flushed lines from the backbuffer render are appearing as corruption in the previous frame. I've been able to use the sync extensions from EGL_KHR_fence_sync to stabilize cpu rendering and make sure that OES_EGL_image_external is synchronized. Will continue testing before saying its figured out, but seems to be improved.

It might take a bit more work to come up with the proper patch that makes both android and mesa happy though.

[Kevin DuBois (kdub)]

my spike branch that averts the problem is here, based on 0.17.1:
lp:~kdub/mir/0.17.1-fix-1406725
This branch doesnt have unit tests, and needs a bit more work on the new internal interfaces, so I'll probably have to rework the branch to get it to land in lp:mir.

[Kevin DuBois (kdub)]

Testing the EGLSyncFence extensions with xmir still has some corruption. Using these extensions stabilizes the mir_demo_client_flicker rendering. So, its looking like there's two problems going on here.

[Kevin DuBois (kdub)]

So, we have two issues in this bug.

1) The majority of the xmir corruption was caused by calling mir_buffer_stream_get_graphics_region() when xmir wanted to know the buffer size. On android, this would call down into gralloc, and invalidate/flush the cache when called. Its reasonable to just call into gralloc on once per swapbuffers.

2) The mir_demo_client_flicker corruption was caused by releasing the buffer resource a bit too early on the server side. This could occur in xmir as well, but seems to be rare for xmir's use case. This issue is corrected by using eglSyncFenceKHR extensions to ensure that the texture is uploaded to the gpu before releasing the buffer back to the server. (This is the more extensive

A quick fix for both issues is in here: lp:~kdub/mir/0.17.1-fix-1406725. I'll repropose tests+fixes to lp:mir, but this should get xmir going in the meantime.

2) is the more extensive fix, so I'll split that bug out into a different report

[PS Jenkins bot (ps-jenkins)]

Fix committed into lp:mir at revision None, scheduled for release in mir, milestone 0.18.0

[Launchpad Janitor (janitor)]

This bug was fixed in the package mir - 0.18.0+16.04.20151216.1-0ubuntu1

---------------
mir (0.18.0+16.04.20151216.1-0ubuntu1) xenial; urgency=medium

  [ Kevin DuBois ]
  * New upstream release 0.18.0 (https://launchpad.net/mir/+milestone/0.18.0)
    - ABI summary: Only servers need rebuilding;
      . Mirclient ABI unchanged at 9
      . Mirserver ABI bumped to 36
      . Mircommon ABI unchanged at 5
      . Mirplatform ABI unchanged at 11
      . Mirprotobuf ABI unchanged at 3
      . Mirplatformgraphics ABI bumped to 7
      . Mirclientplatform ABI unchanged at 3
      . Mirinputplatform ABI added. Current version is 4
    - Enhancements:
      . Use libinput by default, and remove the android input stack
      . Add x11 input probing
      . Add alternative buffer swapping mechanism internally, available with
        --nbuffers 0
      . Automatic searching and selection of input platforms
      . Better support for themed cursors
      . Add demo client that uses multiple buffer streams in one surface
      . Improve fingerpaint demo to use touch pressure
      . Allow for configuring cursor acceleration, scroll speed and left or
        right handed mice
      . Allow for setting a base display configuration via client api
      . Various nested server multimonitor fixes and stability improvements
      . Remove DepthId from the SurfaceStack
    - Bug fixes:
      . Unit test failures in Display.* on Android (LP: #1519276)
      . Build failure due to missing dependency of client rpc code on mir
        protobuf (LP: #1518372)
      . Test failure in
        NestedServer.display_configuration_reset_when_application_exits
        (LP: #1517990)
      . CI test failures in various NesterServer tests (LP: #1517781)
      . FTBFS with -DMIR_PLATFORM=android (LP: #1517532)
      . Nesting Mir servers with assorted display configs causes lockup
        (LP: #1516670)
      . [testsfail] RaiseSurfaces.motion_events_dont_prevent_raise
        (LP: #1515931)
      . CI test failures in GLMark2Test (LP: #1515660)
      . Shells that inject user input events need to agree with the system
        compositor on the clock to use (LP: #1515515)
      . mircookie-dev is missing nettle-dev dependency (LP: #1514391)
      . Segmentation fault on server shutdown with mesa-kms (LP: #1513901)
      . mircookie requires nettle but libmircookie-dev doesn't depend on it
        (LP: #1513792)
      . libmircookie1 package does not list libnettle as dependency
        (LP: #1513225)
      . display configuration not reset when application exits (LP: #1511798)
      . unplugging external monitor causes nested server to throttle client
        (LP: #1511723)
      . 1/2 screen on external monitor (LP: #1511538)
      . unity-system-compositor crash, no interaction on windowed mode
        (LP: #1511095)
      . [regression] arm64/powerpc cross compile doesn't build any more
        (LP: #1510778)
      . mir_connection_get_egl_pixel_format() crashes if libEGL is loaded
        RTLD_LAZY (LP: #1510218)
      . [multimonitor] nested server surface positioning incorrect
        (LP: #1506846)
      . unity-system-compositor fails to build against lp:mir r3027
   ...