Platform api merge #2962
Merged
Platform api merge #2962
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2962 +/- ##
==========================================
- Coverage 77.67% 75.88% -1.80%
==========================================
Files 1056 1058 +2
Lines 73473 74132 +659
==========================================
- Hits 57071 56252 -819
- Misses 16402 17880 +1478
... and 30 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Saviq
reviewed
Jun 20, 2023
Saviq
reviewed
Jun 20, 2023
AlanGriffiths
requested changes
Jun 20, 2023
src/platform/symbols.map
Outdated
Comment on lines
223
to
229
|
|
||
| MIR_PLATFORM_2.13 { | ||
| global: | ||
| extern "C++" { | ||
| mir::graphics::DRMFormat::from_mir_format*; | ||
| }; | ||
| } MIR_PLATFORM_2.11; |
There was a problem hiding this comment.
No. We don't need a new stanza (even if the name were correct)
We have an ABI break, not additional APIs
Comment on lines
15
to
16
| * | ||
| * Authored by: Christopher James Halse Rogers |
There was a problem hiding this comment.
Suggested change
| * | |
| * Authored by: Christopher James Halse Rogers |
| @@ -37,12 +37,17 @@ add_library( | |||
| egl_helper.cpp | |||
| quirks.cpp | |||
| quirks.h | |||
| kms_framebuffer.h | |||
There was a problem hiding this comment.
This adds the header, but not the implementation. Is it needed?
|
I tried this with DisplayLink: It crashed... |
AlanGriffiths
previously requested changes
Jun 27, 2023
include/renderers/gl/mir/renderer/gl/basic_buffer_render_target.h
Outdated
Show resolved
Hide resolved
include/renderers/gl/mir/renderer/gl/basic_buffer_render_target.h
Outdated
Show resolved
Hide resolved
| ${CMAKE_CURRENT_SOURCE_DIR}/test_display.cpp | ||
| # ${CMAKE_CURRENT_SOURCE_DIR}/test_display.cpp |
There was a problem hiding this comment.
The corresponding file hasn't been deleted. What is the intent?
|
It would be good to rebase on origin/main: then we'll get a convenient Miriway branch to experiment with |
AlanGriffiths
force-pushed
the
platform-API-merge
branch
from
bc7cc24
to
5b58989
Compare
|
@RAOF as you are away, I've rebased this on main to fix CI (and also to get a Miriway branch) |
|
I don't think this breaks anything, but I've not yet been able to coax my hardware into doing anything that doesn't work on main |
I'll try something out in the lab… maybe render on Intel where Nvidia has the output, or the other way around. |
I think this should now work on DisplayLink? If not, I'd be interested in logs! |
Closer, but it dies as follows: |
I think you might be testing the wrong code? Unless my local branch has gone screwy, this code does not contain the string “Detected single-GPU DisplayBuffer. Rendering will be sent directly to output”, which appears in your log? |
Even simpler: uploading the wrong log: |
|
I tried this out remotely on a device with the following connectors: Smoke tests are happy on both rendering platforms: Running manually I was able to run Frame under both renderers, but had to manually match the renderer used by glmark2-es2-wayland, not sure if that's by design? Otherwise I ended up with either: Or reverting to llvmpipe, if Frame rendered on Nvidia but glmark2 didn't have access to Nvidia userspace. At one point, trying to VNC, I ended up with ScreenShooter errors, but can't reproduce now: |
|
What's suspect, is that the Intel GPU ends up with higher scores to Nvidia on glmark2: This is a 4K display, though. Weston scores slightly higher, too: |
|
Argh, EGL context handling. Let's give that a try on DisplayLink! |
|
This now starts up on evdi, but there are issues with the display configuration: 8>< ---------------------------------------------------
layouts:
default: # the current layout
cards:
# a list of cards (currently matched by card-id)
- card-id: 0
eDP-1:
# This output supports the following modes: 2560x1440@60.0
#
# Uncomment the following to enforce the selected configuration.
# Or amend as desired.
#
state: enabled # {enabled, disabled}, defaults to enabled
mode: 2560x1440@60.0 # Defaults to preferred mode
position: [0, 0] # Defaults to [0, 0]
orientation: normal # {normal, left, right, inverted}, defaults to normal
scale: 1
group: 0 # Outputs with the same non-zero value are treated as a single display
DisplayPort-1:
# (disconnected)
HDMI-A-1:
# (disconnected)
DisplayPort-2:
# (disconnected)
HDMI-A-2:
# (disconnected)
DVI-I-1:
# This output supports the following modes: 1920x1600@60.0, 1920x1080@60.0,
# 1920x1080@60.0, 1920x1080@59.9, 1920x1080@50.0, 1920x1080@30.0, 1920x1080@30.0,
# 1920x1080@25.0, 1920x1080@24.0, 1920x1080@24.0, 1680x1050@59.9, 1600x900@60.0,
# 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x800@59.9, 1152x864@75.0,
# 1280x720@60.0, 1280x720@59.9, 1280x720@50.0, 1024x768@75.0, 1024x768@60.0,
# 832x624@74.5, 800x600@75.0, 800x600@60.3, 720x576@50.0, 720x480@60.0,
# 720x480@59.9, 640x480@75.0, 640x480@60.0, 640x480@59.9, 720x400@70.1
#
# Uncomment the following to enforce the selected configuration.
# Or amend as desired.
#
state: enabled # {enabled, disabled}, defaults to enabled
mode: 1920x1600@60.0 # Defaults to preferred mode
position: [0, 0] # Defaults to [0, 0]
orientation: normal # {normal, left, right, inverted}, defaults to normal
scale: 1
group: 0 # Outputs with the same non-zero value are treated as a single display
DVI-I-1:
# This output supports the following modes: 1920x1600@60.0, 1920x1080@60.0,
# 1920x1080@60.0, 1920x1080@59.9, 1920x1080@50.0, 1920x1080@30.0, 1920x1080@30.0,
# 1920x1080@25.0, 1920x1080@24.0, 1920x1080@24.0, 1680x1050@59.9, 1600x900@60.0,
# 1280x1024@75.0, 1280x1024@60.0, 1440x900@59.9, 1280x800@59.9, 1152x864@75.0,
# 1280x720@60.0, 1280x720@59.9, 1280x720@50.0, 1024x768@75.0, 1024x768@60.0,
# 832x624@74.5, 800x600@75.0, 800x600@60.3, 720x576@50.0, 720x480@60.0,
# 720x480@59.9, 640x480@75.0, 640x480@60.0, 640x480@59.9, 720x400@70.1
#
# Uncomment the following to enforce the selected configuration.
# Or amend as desired.
#
state: enabled # {enabled, disabled}, defaults to enabled
mode: 1920x1600@60.0 # Defaults to preferred mode
position: [0, 0] # Defaults to [0, 0]
orientation: normal # {normal, left, right, inverted}, defaults to normal
scale: 1
group: 0 # Outputs with the same non-zero value are treated as a single display
DVI-I-1:
# (disconnected)
DVI-I-1:
# (disconnected)
8>< ---------------------------------------------------Both the external monitors are being named [update] Looking elsewhere in the log I see: So these evdi devices are being reported inconsistently. [update 1] And getting miriway to generate a .display config doesn't show them: |
|
I see this when running on my hybrid system - seems Mir doesn't like its Nvidia support: I assume that means it didn't find an output to render to. And didn't consider it for rendering? |
What do |
I don't. (It has been a while since I used Nvidia, probably lost along the way) |
AlanGriffiths
requested changes
Oct 13, 2023
There was a problem hiding this comment.
The design surrounding RenderingPlatform is much more complex than we currently need. We don't need a RendererInterfaceBase, type tags or associated dynamic type deduction.
I assume this intended as a customization point to support non-gl renderers but without a real usecase we can't be sure this is necessary or sufficient.
| */ | ||
| } | ||
|
|
||
| class RendererInterfaceBase |
There was a problem hiding this comment.
Words like "Interface" and "Base" scattered around a type hierarchy usually don't add much. And "InterfaceBase" sounds a bit tautological.
There is only one derived type: GLRenderingProvider, and it is not obvious why that needs an "InterfaceBase". (I realise it is part of the RenderingPlatform::acquire_interface mechanism, but that seems overengineered.)
- `
Comment on lines
197
to
198
| template<typename Interface> | ||
| static auto acquire_interface(std::shared_ptr<RenderingPlatform> platform) -> std::shared_ptr<Interface> |
There was a problem hiding this comment.
This seems like needless complexity: There is only one specialization of this template used:
auto gl_provider = mg::RenderingPlatform::acquire_interface<mg::GLRenderingProvider>(...)
Comment on lines
218
to
235
| /** | ||
| * Acquire a specific hardware interface | ||
| * | ||
| * This should perform any runtime checks necessary to verify the requested interface is | ||
| * expected to work and return a pointer to an implementation of that interface. | ||
| * | ||
| * This function is guaranteed to be called with `this` managed by a `shared_ptr`; if | ||
| * the returned value needs to share ownership with `this`, calls to std::shared_from_this | ||
| * can be expected to work. | ||
| * | ||
| * \param type_tag [in] An instance of the Tag type for the requested interface. | ||
| * Implementations are expected to dynamic_cast<> this to | ||
| * discover the specific interface being requested. | ||
| * \return A pointer to an implementation of the RenderInterfaceBase-derived | ||
| * interface that corresponds to the most-derived type of tag_type. | ||
| */ | ||
| virtual auto maybe_create_interface( | ||
| RendererInterfaceBase::Tag const& type_tag) -> std::shared_ptr<RendererInterfaceBase> = 0; |
There was a problem hiding this comment.
Overly generic: This is only ever called with GLRenderingProvider::Tag{} and the return type is always downcast to shared_ptr<GLRenderingProvider>
|
Running on plain gbm-kms there's a failure when connecting or disconnecting an output the leads to a crash. I'll just drop this |
Move some destructor instantiations from the header, where there's a `unique_ptr<IncompleteType>` member, to the implementation where `IncompleteType` is no longer incomplete. I'm not sure why this built in CI, nor why it only just *now* fails for me?
|
Of course I can't reproduce this (on Lunar), either with Intel or AMD doing the rendering on my laptop. That said, at least the backtrace is clearly |
Better naming for rendering classes, functions and variables
Tracing through Mesa I can't see anything that would return |
This is correct: it's an extension point for non-GL renderers. It can be removed (and re-added later if necessary). |
RAOF
force-pushed
the
platform-API-merge
branch
from
e0e8635
to
36ccfe8
Compare
|
I can confirm that 36ccfe8 fixes the plug/unplug problem |
…s; and they no longer compile. While it would be good to have specific tests fail if the features these should test for don't work, we won't miss bugs from not running these
|
First time I've seen this in CI, but miral-test.WaylandExtensions failed on armhf (in qemu, so…) https://github.com/MirServer/mir/actions/runs/6532933229/job/17737084346?pr=2962#step:8:9441 |
configuration is only written once
Decoupling writing the display configuration from processing it
# Conflicts: # src/miral/display_configuration_option.cpp # src/platforms/gbm-kms/server/kms/kms_framebuffer.h # src/platforms/gbm-kms/server/surfaceless_egl_context.h
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rework the graphics platform API to support multiple features:
Probing & construction are reworked so that a each
DisplayPlatformandRenderingPlatforminstance is associated with a single hardware device - so systems with multiple GPUs get aDisplayPlatforminstance per GPU and aRenderingPlatforminstance per GPU.Because rendering and display (and client buffer allocation) are linked, there's a selection dance that occurs:
DisplayPlatforms, on the assumption that whatever display hardware is there is there.RenderingPlatforms, giving a list of the display platforms selected.This selects the set of platforms loaded.
Since each output can be driven by a different
DisplayPlatformthere is a subsequent probing phase in theDefaultDisplayBufferCompositorFactory. Here, the best rendering platform for the specific output is selected from the set of loaded platforms.An additional constraint is
the_buffer_allocator- the renderer needs to be able to import the buffers allocated by the clients.Currently, the
RenderingPlatformthat suppliesthe_buffer_allocatoris chosen arbitrarily. In future, all loadedRenderingPlatforms could be used instead, to provide the full range of client buffer allocation options (although care will be needed to ensure that amir::Buffercan be imported as a texture on all activeRenderingPlatforms).The various interfaces and how they relate to the rest of Mir:
GLRenderingProvider: This is the primary (rendering) interface between the platforms and the rest of Mir.gl::OutputSurfacefor a display - an interface which provides a GL context and acommit() -> Framebuffermethod to get a handle to submit for display.as_texture(Buffer)to take amir::Bufferand return something implementinggl::Texture, for use in the Renderer.DisplayBuffer: This loses its GL-context-providing function, and instead takes aFramebufferto put on screen