Merge lp:~phablet-team/mir/track-plug-state into lp:mir

[Alfonso Sanchez-Beato]

Track the displays plugged state to avoid reporting configurations in case they are unplugged (LP #1531503).

Track the displays plugged state to avoid reporting configurations in case they are unplugged (LP #1531503).

[Andreas Pokorny (andreas-pokorny)]

Needs fixing: Instead of a map use an array std::atomic<bool> of size HWC_NUM_DISPLAY_TYPES.
Needs information: What about the other scenario .. the display was just plugged in but there is no display information available yet? Or do we only experience unplug + still display conf readable problems?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

I'm not very familiar with this code, so I may be missing something?

It seems excessive to have both inclusion in a map and a bool to represent the "plugged" state. And there are at most three elements!

Surely either a set (membership in the set implying the "plugged" state) or a three element array would be adequate.

E.g.

bool is_plugged_map[3] = { false, false, false };

...

if (!is_plugged_map[display_name]) return {};

(It would be nice to eliminate the "magic number" 3 by adding a DisplayName::max_names)

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

yes androdi hw_composer_defs.h or so devices a max number of displays, and we could put that into the DiplayNme enum ..

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Branch refreshed addressing comments.

Re: plugged in but no display information yet, I have not seen that. The only issue has been when unplugging the tablet.

[Alan Griffiths (alan-griffiths)]

+ if (is_plugged[mga::as_hwc_display(display_name)].load() == false)

Can be written

  if (!is_plugged[mga::as_hwc_display(display_name)])

~~~~

+ is_plugged[HWC_DISPLAY_PRIMARY].store(true);
+ is_plugged[HWC_DISPLAY_EXTERNAL].store(false);
+ is_plugged[HWC_DISPLAY_VIRTUAL].store(true);

Constructors should use initialization, not assignment

review: Needs Fixing

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

On Fri, Feb 12, 2016 at 3:01 PM, Alan Griffiths <email address hidden> wrote:

> Review: Needs Fixing
>
> + if (is_plugged[mga::as_hwc_display(display_name)].load() == false)
>
> Can be written
>
> if (!is_plugged[mga::as_hwc_display(display_name)])
>
> ~~~~
>
> + is_plugged[HWC_DISPLAY_PRIMARY].store(true);
> + is_plugged[HWC_DISPLAY_EXTERNAL].store(false);
> + is_plugged[HWC_DISPLAY_VIRTUAL].store(true);
>
> Constructors should use initialization, not assignment
>

The copy constructor is =deleted for atomic types apparently, so I could
not initialize directly.

> --
> https://code.launchpad.net/~phablet-team/mir/track-plug-state/+merge/285854
> Your team Ubuntu Phablet Team is subscribed to branch
> lp:~phablet-team/mir/track-plug-state.
>

[Alan Griffiths (alan-griffiths)]

OK.

I don't think the extra verbage of explicit store()/load() adds anything, but I won't block.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

oh there are really only three diplays possible HWC. builtin external and virtual. So this could just be an int. Instead of an array.

I guess we should integrate that to 0.20?

[Andreas Pokorny (andreas-pokorny)]

I am ok with that version. But pinged kdub on whether this should be a runtime switchable quirk or not.

review: Approve

[Kevin DuBois (kdub)]

in my reading of hwc spec, we're specifically guaranteed that checking the display config of a device is allowed (<groan>)
https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/hwcomposer.h#492

If it is a problem on this device though, this doesn't look bad, although I might follow up to solve the problem at a higher level and to add tests (this class is just supposed to be a wrapper)

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/2410/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5900
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4807
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5856/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/242
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/242/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/242
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/242/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5853
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5853/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8247/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27551

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Flaky infrastructure - landing by hand

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/softwareproperties/ppa.py", line 101, in _get_https_content_py3
    lp_page = urllib.request.urlopen(request, cafile=LAUNCHPAD_PPA_CERT)
  File "/usr/lib/python3.4/urllib/request.py", line 161, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.4/urllib/request.py", line 463, in open
    response = self._open(req, data)
  File "/usr/lib/python3.4/urllib/request.py", line 481, in _open
    '_open', req)
  File "/usr/lib/python3.4/urllib/request.py", line 441, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.4/urllib/request.py", line 1225, in https_open
    context=self._context, check_hostname=self._check_hostname)
  File "/usr/lib/python3.4/urllib/request.py", line 1184, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [Errno -3] Temporary failure in name resolution>

During handling of the above exception, another exception occurred:

=== modified file 'src/platforms/android/server/real_hwc_wrapper.cpp'
--- src/platforms/android/server/real_hwc_wrapper.cpp	2016-01-29 08:18:22 +0000
+++ src/platforms/android/server/real_hwc_wrapper.cpp	2016-02-12 14:23:01 +0000
@@ -89,6 +89,9 @@
     std::unique_lock<std::mutex> lk(callback_lock);
     hwc_callbacks.self = this;
     hwc_device->registerProcs(hwc_device.get(), reinterpret_cast<hwc_procs_t*>(&hwc_callbacks));
+    is_plugged[HWC_DISPLAY_PRIMARY].store(true);
+    is_plugged[HWC_DISPLAY_EXTERNAL].store(false);
+    is_plugged[HWC_DISPLAY_VIRTUAL].store(true);
 }
 
 mga::RealHwcWrapper::~RealHwcWrapper()
@@ -215,6 +218,8 @@
 
 void mga::RealHwcWrapper::hotplug(DisplayName name, bool connected) noexcept
 {
+    is_plugged[mga::as_hwc_display(name)].store(connected);
+
     std::unique_lock<std::mutex> lk(callback_map_lock);
     for(auto const& callbacks : callback_map)
     {
@@ -245,6 +250,11 @@
 
 std::vector<mga::ConfigId> mga::RealHwcWrapper::display_configs(DisplayName display_name) const
 {
+    //Check first if display is unplugged, as some hw composers incorrectly report display configurations
+    //when they have already triggered an unplug event.
+    if (!is_plugged[mga::as_hwc_display(display_name)].load())
+        return {};
+
     //No way to get the number of display configs. SF uses 128 possible spots, but that seems excessive.
     static size_t const max_configs = 16;
     size_t num_configs = max_configs;

=== modified file 'src/platforms/android/server/real_hwc_wrapper.h'
--- src/platforms/android/server/real_hwc_wrapper.h	2016-01-29 08:18:22 +0000
+++ src/platforms/android/server/real_hwc_wrapper.h	2016-02-12 14:23:01 +0000
@@ -25,6 +25,7 @@
 
 #include <mutex>
 #include <unordered_map>
+#include <atomic>
 
 namespace mir
 {
@@ -86,6 +87,7 @@
         std::function<void()> invalidate;
     };
     std::unordered_map<void const*, Callbacks> callback_map;
+    std::atomic<bool> is_plugged[HWC_NUM_DISPLAY_TYPES];
 };
 
 }