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

drm: Add vrr property helpers #1578

Merged
merged 1 commit into from
Nov 25, 2024
Merged

drm: Add vrr property helpers #1578

merged 1 commit into from
Nov 25, 2024

Conversation

Drakulix
Copy link
Member

@Drakulix Drakulix commented Nov 8, 2024

Draft while this is missing docs and more testing.

Adds helper methods to make it possible to set VRR per frame, which makes more dynamic VRR policies like "only enable, when we have a fullscreen application" more easily possible. Also simplifies VRR checks.

@Drakulix Drakulix requested a review from cmeissl November 8, 2024 15:07
@Drakulix Drakulix force-pushed the drm/set_vrr branch 4 times, most recently from ef8f243 to e3a80a4 Compare November 11, 2024 20:55
@Drakulix Drakulix marked this pull request as ready for review November 18, 2024 19:23
@Drakulix Drakulix force-pushed the drm/set_vrr branch 2 times, most recently from d8b3de8 to dc461f7 Compare November 18, 2024 19:29
@Drakulix
Copy link
Member Author

Now in use here and seems to work well: pop-os/cosmic-comp#1004

Copy link
Collaborator

@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.

wp_presentation version 1 requires 0 to be sent for vrr

If the output does not have a constant refresh rate, explicit video mode switches excluded, then the refresh argument must be zero.

@Drakulix
Copy link
Member Author

@cmeissl I pushed new (separate) commits to make review easier (though I actually plan to merge most of them).

I hope the DrmCompositor cursor skip is now correct.

And I added an example to anvil for sending the right PresentationTime refresh rate, though I would prefer to drop that commit given anvil doesn't implement VRR at all. It is just there to show off how easy this is to do with the vrr_enabled-method. I thought about adding it to the FrameResult (as we only really know the refresh rate mode we send off to the hardware on submit, but making the into a proper struct with meta data felt overkill, when the check is this simple.

Copy link
Collaborator

@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.

We should keep cursor only updates in case we have re-drawn the plane or otherwise we would loose the rather costly rendering step we already did. This would also result in constant re-drawing of the plane until we do not skip it.

src/backend/drm/compositor/mod.rs Outdated Show resolved Hide resolved
src/backend/drm/compositor/mod.rs Outdated Show resolved Hide resolved
@cmeissl
Copy link
Collaborator

cmeissl commented Nov 25, 2024

And I added an example to anvil for sending the right PresentationTime refresh rate, though I would prefer to drop that commit given anvil doesn't implement VRR at all. It is just there to show off how easy this is to do with the vrr_enabled-method. I thought about adding it to the FrameResult (as we only really know the refresh rate mode we send off to the hardware on submit, but making the into a proper struct with meta data felt overkill, when the check is this simple.

Looks goods and I agree that we can remove that from anvil as it does not support vrr at all. I hope that the breaking change for the refresh parameter is enough to get people implement it correctly.

@Drakulix
Copy link
Member Author

@cmeissl Merged your changes, removed self.vrr, dropped the anvil commit and squashed. Ready for merge?

@cmeissl
Copy link
Collaborator

cmeissl commented Nov 25, 2024

@cmeissl Merged your changes, removed self.vrr, dropped the anvil commit and squashed. Ready for merge?

Yes :)

@Drakulix
Copy link
Member Author

@cmeissl Merged your changes, removed self.vrr, dropped the anvil commit and squashed. Ready for merge?

Yes :)

Thanks! :)

@Drakulix Drakulix merged commit bc1d732 into master Nov 25, 2024
13 checks passed
@Drakulix Drakulix deleted the drm/set_vrr branch November 25, 2024 18:29
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.

2 participants