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

DrmOutputManager #1576

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

cmeissl
Copy link
Collaborator

@cmeissl cmeissl commented Nov 3, 2024

highly wip of the ideas described here: pop-os/cosmic-comp#969 (comment)

it implements most of the stuff, except:

  • the capabilities stuff
  • the actual format selection
  • some things are stubs and need to be implemented properly

not extensively tested, but anvil seems to still be able to launch.
returning an error from DrmCompositor::new also successfully triggers the format selection
logic

TODO

A lot...

  • Verify that this does not mess up the render logic with multiple commits triggering vblanks
    If this turns out to be problematic we can disable the event for commit_frame, but we need to make sure
    to kick off rendering in this case somehow..

src/backend/drm/compositor/mod.rs Outdated Show resolved Hide resolved
src/backend/drm/output.rs Outdated Show resolved Hide resolved
src/backend/drm/output.rs Outdated Show resolved Hide resolved
@cmeissl cmeissl force-pushed the feature/drm_auto_select branch from 542a21a to b10a373 Compare November 10, 2024 16:21
@Drakulix Drakulix force-pushed the feature/drm_auto_select branch 3 times, most recently from 27a4681 to 15198e2 Compare November 28, 2024 17:14
@Drakulix
Copy link
Member

Drakulix commented Dec 3, 2024

TODO

A lot...

* [ ]  Verify that this does not mess up the render logic with multiple commits triggering vblanks
  If this turns out to be problematic we can disable the event for `commit_frame`, but we need to make sure
  to kick off rendering in this case somehow..

Addressed in 3fe19dd. While it works okay-ish for anvil, it definitely would mess up state tracking in niri and cosmic-comp and while potentially work-arounds exists, at least in the latter with multi-threading this requires ugly synchronization, while the point of this api is to hide that.

@Drakulix Drakulix force-pushed the feature/drm_auto_select branch 2 times, most recently from 3417378 to 90e699b Compare December 9, 2024 16:22
@Drakulix Drakulix force-pushed the feature/drm_auto_select branch 4 times, most recently from ad6c3b5 to cb9b280 Compare December 10, 2024 18:41
@Drakulix Drakulix marked this pull request as ready for review December 10, 2024 18:49
@Drakulix Drakulix force-pushed the feature/drm_auto_select branch from cb9b280 to afa1117 Compare December 12, 2024 16:43
@Drakulix
Copy link
Member

All the code paths seem to now work pretty nicely in pop-os/cosmic-comp#1020 (though I don't have any devices that actually have to use these paths, I just forced them manually). This just needs a new pixman-rs release to pass tests.

Copy link
Collaborator Author

@cmeissl cmeissl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in general I believe the changes, except the one limiting the primary format, are pretty harmless. The DrmOutputManager is a really nice optional addition which will definitely need some more work in the future. But I feel like this provides a solid base to do so. I am really happy how this turned out, thanks!

I really like the approach with DrmOutputRenderElements, nice work :)

.expect("We have a buffer for the primary plane")
.buffer
{
if slot.format() != element_config.properties.format {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be configurable from the outside, I am just not sure how and in what detail. Having this restriction will kill performance on lower end hardware. In my case I would only enable this on general purpose x68 hardware and only when more than a single output is enabled.

Copy link
Member

@Drakulix Drakulix Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe just have this be another bit for FrameMode? We might also have a need for a new flag here. So maybe FrameMode should become FrameFlags and have something like ALLOW_PRIMARY_SCANOUT instead of PRIMARY_SCANOUT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a commit doing this: 54379b2

src/backend/drm/output.rs Outdated Show resolved Hide resolved
@cmeissl
Copy link
Collaborator Author

cmeissl commented Dec 15, 2024

pixman-rs 0.2.1 is out

@Drakulix Drakulix force-pushed the feature/drm_auto_select branch from 54379b2 to 06c52e6 Compare December 17, 2024 12:30
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.

3 participants