Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/egl context priority #1133

Merged
merged 4 commits into from
Sep 17, 2023

Conversation

cmeissl
Copy link
Collaborator

@cmeissl cmeissl commented Sep 15, 2023

This PR adds support for egl context priority using the EGL_IMG_context_priority egl extension.

It also changes the multigpu code to request a high priority context by default.

@cmeissl cmeissl marked this pull request as ready for review September 15, 2023 11:34
@Drakulix
Copy link
Member

Can we make the second commit add the priority as constructor arguments for the GbmGlesBackend and EglGlesBackend instead?

@cmeissl cmeissl requested a review from Drakulix September 15, 2023 11:36
@cmeissl
Copy link
Collaborator Author

cmeissl commented Sep 15, 2023

Can we make the second commit add the priority as constructor arguments for the GbmGlesBackend and EglGlesBackend instead?

Yes, though we only have a default impl and with_factory. I can add a new function taking the priority, but what should the default do? An alternative would be to not request any priority by default and use the factory for this. I considered that first, but that forces downstream to use the factory which can be easily forgotten.

@Drakulix
Copy link
Member

Drakulix commented Sep 15, 2023

Can we make the second commit add the priority as constructor arguments for the GbmGlesBackend and EglGlesBackend instead?

Yes, though we only have a default impl and with_factory. I can add a new function taking the priority, but what should the default do? An alternative would be to not request any priority by default and use the factory for this. I considered that first, but that forces downstream to use the factory which can be easily forgotten.

I guess we have two options:

  • Make Default do what it currently does. A context with "medium" or "default" priority. Additionally add a with_priority-constructor. The factory-constructor would remain unchanged.
  • Drop Default and force downstream to always specify a priority or at least explicitly pass None for "default priority" (if not using a factory).

I am happy with both options.

@i509VCB
Copy link
Member

i509VCB commented Sep 15, 2023

I believe vulkan has an extension similar to this, so we can elect to specify a priority from the multigpu code, although it's an optional extension: https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_KHR_global_priority.html

@cmeissl cmeissl force-pushed the feature/egl_context_priority branch 2 times, most recently from 1cd5b33 to 4f5f9a0 Compare September 15, 2023 13:27
@cmeissl
Copy link
Collaborator Author

cmeissl commented Sep 15, 2023

Can we make the second commit add the priority as constructor arguments for the GbmGlesBackend and EglGlesBackend instead?

Yes, though we only have a default impl and with_factory. I can add a new function taking the priority, but what should the default do? An alternative would be to not request any priority by default and use the factory for this. I considered that first, but that forces downstream to use the factory which can be easily forgotten.

I guess we have two options:

* Make `Default` do what it currently does. A context with "medium" or "default" priority. Additionally add a `with_priority`-constructor. The factory-constructor would remain unchanged.

* Drop `Default` and force downstream to always specify a priority or at least explicitly pass `None` for "default priority" (if not using a factory).

I am happy with both options.

I opted for the first option, which really seems more sensible as Medium is defined as the default by the spec (and it does not break the API)

@cmeissl cmeissl force-pushed the feature/egl_context_priority branch from 4f5f9a0 to e5e1d0c Compare September 17, 2023 10:19
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2023

Codecov Report

Patch coverage: 1.16% and project coverage change: -0.07% ⚠️

Comparison is base (5affbde) 22.94% compared to head (e5e1d0c) 22.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
- Coverage   22.94%   22.88%   -0.07%     
==========================================
  Files         143      143              
  Lines       22972    23050      +78     
==========================================
+ Hits         5272     5275       +3     
- Misses      17700    17775      +75     
Flag Coverage Δ
wlcs-buffer 20.00% <1.16%> (-0.07%) ⬇️
wlcs-core 19.65% <1.16%> (-0.07%) ⬇️
wlcs-output 8.10% <1.16%> (-0.03%) ⬇️
wlcs-pointer-input 21.67% <1.16%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/backend/egl/context.rs 0.00% <0.00%> (ø)
build.rs 80.72% <100.00%> (+0.23%) ⬆️

... and 2 files with indirect coverage changes

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

@Drakulix Drakulix merged commit 691bb28 into Smithay:master Sep 17, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants