Log platform selection #3212

RAOF · 2024-02-01T06:45:51Z

Some logging, so I can get a snap on the Pi.

codecov · 2024-02-01T08:00:48Z

Codecov Report

Attention: 81 lines in your changes are missing coverage. Please review.

Comparison is base (5ca823e) 77.80% compared to head (60642de) 77.75%.

Files	Patch %	Lines
src/platforms/gbm-kms/server/buffer_allocator.cpp	0.00%	14 Missing ⚠️
src/platforms/gbm-kms/server/kms/platform.cpp	0.00%	12 Missing ⚠️
src/platforms/eglstream-kms/server/platform.cpp	0.00%	8 Missing ⚠️
src/platforms/eglstream-kms/server/display.cpp	0.00%	6 Missing ⚠️
src/platforms/gbm-kms/server/kms/display.cpp	0.00%	6 Missing ⚠️
...latforms/common/server/cpu_copy_output_surface.cpp	0.00%	4 Missing ⚠️
...on/server/kms_cpu_addressable_display_provider.cpp	0.00%	4 Missing ⚠️
...platforms/gbm-kms/server/gbm_display_allocator.cpp	0.00%	4 Missing ⚠️
...rc/platforms/gbm-kms/server/kms/display_buffer.cpp	0.00%	4 Missing ⚠️
src/platforms/x11/graphics/display_sink.cpp	0.00%	4 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3212      +/-   ##
==========================================
- Coverage   77.80%   77.75%   -0.05%     
==========================================
  Files        1062     1073      +11     
  Lines       74623    75146     +523     
==========================================
+ Hits        58061    58431     +370     
- Misses      16562    16715     +153

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlanGriffiths

There's also a legacy CStrFree hidden in existing code

AlanGriffiths · 2024-02-01T11:19:40Z

src/platforms/common/server/owning_c_str.h

+namespace mir
+{
+using CStr = std::unique_ptr<char[], CStringFree>;
+}


That looks like a recipe for an ODR violation - thanks to the anon namespace CStringFree is a different type in each translation unit and that makes the mir::CStr definition different in each TU.

AlanGriffiths · 2024-02-01T11:41:59Z

src/platforms/common/server/owning_c_str.h

+#include <cstdlib>
+#include <memory>
+
+namespace
+{
+class CStringFree
+{
+public:
+    void operator()(char* str)
+    {
+        if(str)
+        {
+            free(str);
+        }
+    }
+};
+}
+
+namespace mir
+{
+using CStr = std::unique_ptr<char[], CStringFree>;
+}


what? No header guards?

The null check is redundant

The operator() can be const

This doesn't handle const c_strings

Suggested change

#include <cstdlib>

#include <memory>

namespace

{

class CStringFree

{

public:

void operator()(char* str)

{

if(str)

{

free(str);

}

}

};

}

namespace mir

{

using CStr = std::unique_ptr<char[], CStringFree>;

}

#ifndef MIR_OWNING_C_STR_

#define MIR_OWNING_C_STR_

#include <cstdlib>

#include <memory>

namespace mir

{

using CStr = std::unique_ptr<char[], decltype([](char const* p) { ::free(const_cast<char *>(p)); })>;

}

#endif

AlanGriffiths · 2024-02-01T12:06:42Z

src/server/graphics/default_configuration.cpp

-        rendering_platforms.reserve(rendering_platform_map.size());
-
-        // We want to make the egl-generic platform the last resort
-        // So don't push it into rendering_platforms until the rest are done
-        std::shared_ptr<graphics::RenderingPlatform> egl_generic;
-
-        for (auto const& rp : rendering_platform_map)
-        {
-            if (rp.first != "mir:egl-generic")
-            {
-                rendering_platforms.push_back(rp.second);
-            }
-            else
-            {
-                egl_generic = rp.second;
-            }
-        }
-
-        // If we skipped egl-generic, add it to the end
-        if (egl_generic)
-        {
-            rendering_platforms.push_back(egl_generic);
-        }
-


I know this code is ugly, but why are we dropping it?

The commit comment "FIXME: Ooops! Don't throw away most of our hardware platforms!" doesn't reflect the change. (It just makes the order non-deterministic.)

AlanGriffiths · 2024-02-01T12:12:29Z

src/platforms/gbm-kms/server/kms/display_buffer.cpp

@@ -311,6 +312,13 @@ auto mgg::DisplaySink::gbm_device() const -> std::shared_ptr<struct gbm_device>
    return gbm;
 }

+auto mgg::DisplaySink::describe_output() const -> std::string
+{
+    std::unique_ptr<char[], void(*)(char*)> drm_node{drmGetDeviceNameFromFd2(drm_fd()), [](char* to_free) { free(to_free); }};


Another incantation for CStr

This reverts commit 7a918eb.

Saviq · 2024-02-01T12:23:12Z

Can confirm this brings Pi's performance back to what's expected.

@freefos this is work-in-progress, but could you confirm with snap refresh ubuntu-frame --channel 22/edge/mir-pr3212 please?

AlanGriffiths · 2024-02-01T13:37:57Z

Can confirm this brings Pi's performance back to what's expected.

I'm struggling to know what changed to make that happen.

Apart from logging, the change to src/server/graphics/default_configuration.cpp looks like the only thing. But that avoids sorting rendering_platforms to "mostly alphabetical, with mir:egl-generic at the end" and leaves it in "order encountered". That probably just makes the first viable platform non-deterministic, which isn't necessarily a good thing.

Saviq · 2024-02-01T14:04:53Z

I'm struggling to know what changed to make that happen.

Here's @RAOF's explanation from Matrix:

In the “load egl-generic last” patch we made it so that we only keep the last device's platform for any given platform type.
So all the “make sure to match the renderer to the display when possible” code is selecting from a set that doesn't include the renderer that matches the display!

Yes, it needs a better solution (and bringing back the determinism), but this PR, so far, just confirmed the issue.

With both 2.15 and this PR, MotionMark in WPE detects 60fps with Frame at ~15% CPU, compared to 2.16.2 detecting 15fps at 50-60% CPU usage of Frame.

The logging from this branch provides:

Platform selection: Output gbm-kms (/dev/dri/card0) - KMS connector id: 89: Selected renderer: GBM surface on
/dev/dri/card0

AlanGriffiths · 2024-02-01T15:42:25Z

In the “load egl-generic last” patch we made it so that we only keep the last device's platform for any given platform type.

Oh! We load multiple instances of each platform? I hadn't understood that. Easy to fix. (Incoming...)

This reverts commit 595f864.

RAOF · 2024-02-01T23:24:57Z

src/platforms/common/server/owning_c_str.h

+#include <memory>
+namespace mir
+{
+using CStr = std::unique_ptr<char[], decltype([](char const* p) { ::free(const_cast<char *>(p)); })>;


Huh. You learn something new every day. decltype(<lambda literal>) is a neat way of getting a type with a specific operator()!

freefos · 2024-02-02T07:18:30Z

@Saviq I can confirm this fixes the fps issue.
129-mir2.16.2+dev81

freefos · 2024-02-02T07:25:13Z

@Saviq With this release wpe-webkit-mir-kiosk restarts once in a while.
log_129-mir2.16.2+dev81_wpe-kiosk.txt

freefos · 2024-02-02T07:58:44Z

@Saviq Its again the issue where the memory usage increases until it crashes.
As described here: canonical/ubuntu-frame#159
This hapens only when there is a vnc connection open.
But now it wasn't our web application but the motionmark website that is active.

Saviq · 2024-02-02T08:20:38Z

Thanks @freefos. #3213 is what we're going to backport to 2.16 soon.

REBASE: Add describe_Output()

04b246b

RAOF marked this pull request as draft February 1, 2024 06:45

REBASE: Some debugging

98ad96b

RAOF force-pushed the log-platform-selection branch from 0af4664 to 98ad96b Compare February 1, 2024 06:53

RAOF added 3 commits February 1, 2024 18:37

More debugging for render/output selection

9472fa8

FIXME: Ooops! Don't throw away most of our hardware platforms!

595f864

REBASE: Oh, yeah. I *do* need to add all the files to git!

ddf316d

ci: skip armhf for now

7a918eb

AlanGriffiths reviewed Feb 1, 2024

View reviewed changes

AlanGriffiths added 2 commits February 1, 2024 12:16

Revert "ci: skip armhf for now"

b626dc6

This reverts commit 7a918eb.

Fix armhf right

351e518

AlanGriffiths added 4 commits February 1, 2024 16:27

Revert "FIXME: Ooops! Don't throw away most of our hardware platforms!"

b0b184d

This reverts commit 595f864.

Allow for multiple platforms of each type

ae96e09

Also fix CStr

936e25c

Don't fight clang over how ranges work

a643dfa

RAOF commented Feb 1, 2024

View reviewed changes

Implement remaining debug output descriptors

30c9dec

RAOF mentioned this pull request Feb 2, 2024

platform/graphics: Allow for multiple RenderingPlatforms of each type #3213

Merged

Drop some not super useful logging

60642de

freefos mentioned this pull request Feb 2, 2024

/snap/wpe-webkit-mir-kiosk/101/usr/bin/cog memory increases until it crashes. Igalia/cog#693

Open

RAOF mentioned this pull request Feb 6, 2024

miroil: Pass through list of GLRenderingProviders when making the Compositor` #3219

Draft

Saviq added the triaged Triage into JIRA to plan it in label Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log platform selection #3212

Log platform selection #3212

RAOF commented Feb 1, 2024 •

edited

codecov bot commented Feb 1, 2024 •

edited

AlanGriffiths left a comment

AlanGriffiths Feb 1, 2024

AlanGriffiths Feb 1, 2024

AlanGriffiths Feb 1, 2024

AlanGriffiths Feb 1, 2024

Saviq commented Feb 1, 2024

AlanGriffiths commented Feb 1, 2024

Saviq commented Feb 1, 2024

AlanGriffiths commented Feb 1, 2024

RAOF Feb 1, 2024

freefos commented Feb 2, 2024

freefos commented Feb 2, 2024

freefos commented Feb 2, 2024

Saviq commented Feb 2, 2024

Log platform selection #3212

Are you sure you want to change the base?

Log platform selection #3212

Conversation

RAOF commented Feb 1, 2024 • edited

codecov bot commented Feb 1, 2024 • edited

Codecov Report

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Feb 1, 2024

Choose a reason for hiding this comment

AlanGriffiths Feb 1, 2024

Choose a reason for hiding this comment

AlanGriffiths Feb 1, 2024

Choose a reason for hiding this comment

AlanGriffiths Feb 1, 2024

Choose a reason for hiding this comment

Saviq commented Feb 1, 2024

AlanGriffiths commented Feb 1, 2024

Saviq commented Feb 1, 2024

AlanGriffiths commented Feb 1, 2024

RAOF Feb 1, 2024

Choose a reason for hiding this comment

freefos commented Feb 2, 2024

freefos commented Feb 2, 2024

freefos commented Feb 2, 2024

Saviq commented Feb 2, 2024

RAOF commented Feb 1, 2024 •

edited

codecov bot commented Feb 1, 2024 •

edited