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

Update drm sys to get new caps #190

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

PolyMeilex
Copy link
Member

Atomic async page flip needs new capabilities from the kernel, this updates drm-rs to include those, as well as some unrelated Client caps.

related: Smithay/smithay#1325

@PolyMeilex
Copy link
Member Author

Well this fails as CI generates its own binding for the older version of DRM.
Ideas on how to tackle this welcome.

@i509VCB
Copy link
Member

i509VCB commented Feb 17, 2024

Fixes #186 as well

@PolyMeilex
Copy link
Member Author

closes #189 as well

@Drakulix
Copy link
Member

Well this fails as CI generates its own binding for the older version of DRM. Ideas on how to tackle this welcome.

The best solution would be to query the headers from another source. E.g. directly from kernel sources.

@PolyMeilex
Copy link
Member Author

PolyMeilex commented Feb 17, 2024

Done, included the kernel headers and added a feature to generate from them instead of libdrm ones.

Now it looks like latest nightly rust is not happy about our type casts, that appear to be correct as far as I can tell:

Error logs error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused --> src/control/mod.rs:1133:34 | 1128 | let event = unsafe { &*(self.event_buf.as_ptr().add(self.i) as *const ffi::drm_event) }; | --------------------------------------------------------------- backing allocation comes from here ... 1133 | unsafe { &*(event as *const _ as *const ffi::drm_event_vblank) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: casting from `drm_event` (8 bytes) to `drm_event_vblank` (32 bytes) = note: `#[deny(invalid_reference_casting)]` on by default

error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
--> src/control/mod.rs:1147:34
|
1128 | let event = unsafe { &*(self.event_buf.as_ptr().add(self.i) as const ffi::drm_event) };
| --------------------------------------------------------------- backing allocation comes from here
...
1147 | unsafe { &
(event as *const _ as *const ffi::drm_event_vblank) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: casting from drm_event (8 bytes) to drm_event_vblank (32 bytes)

Anyway, that's unrelated to this PR. Disabled the lint for now

@PolyMeilex PolyMeilex force-pushed the update-drm-sys-to-get-new-caps branch from 9241a76 to 614fd85 Compare February 17, 2024 20:57
@PolyMeilex
Copy link
Member Author

Ping, anything more I can do to move this forward?

@Drakulix
Copy link
Member

Ping, anything more I can do to move this forward?

Sorry, forgot to take a look at this. Looks mostly good to me, I wonder though, if we can add the update.sh as a step to the update-bindings/compare-bindings ci job, so that we always compare with up-to-date header files and perhaps also include updated header files in the automatically created pull requests?

@PolyMeilex
Copy link
Member Author

Ping, anything more I can do to move this forward?

Sorry, forgot to take a look at this. Looks mostly good to me, I wonder though, if we can add the update.sh as a step to the update-bindings/compare-bindings ci job, so that we always compare with up-to-date header files and perhaps also include updated header files in the automatically created pull requests?

Added a download headers step right before bindgen is called, and this seems to do the trick (at least on my test repo)

@Drakulix
Copy link
Member

I am confused, looking through your PR, I saw that we already added downloading kernel headers at some point here: https://github.com/Smithay/drm-rs/pull/176/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR195-R199

So maybe that isn't necessary and we can just merge this without?

@PolyMeilex PolyMeilex force-pushed the update-drm-sys-to-get-new-caps branch from 4ad92f6 to d7467bb Compare March 12, 2024 23:41
@PolyMeilex
Copy link
Member Author

Dropped the kernel gen commits and created the backup branch for them

But yeah, the CI seems to be downgrading the bindings now 🤷

@PolyMeilex
Copy link
Member Author

PolyMeilex commented Mar 12, 2024

The Fetch drm headers seems to be only called for test step, so that's probably why checks fail.

So do we disable regeneration for check steps, or fetch in them as well?

@Drakulix
Copy link
Member

So do we disable regeneration for check steps, or fetch in them as well?

I guess fetch as well? We would want to use the same ones throughout after all.

@PolyMeilex
Copy link
Member Author

@Drakulix Done, we now fetch on check steps as well, and CI is happy

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

Thanks!

@Drakulix Drakulix merged commit c371ef6 into Smithay:develop Mar 27, 2024
16 checks passed
@PolyMeilex PolyMeilex deleted the update-drm-sys-to-get-new-caps branch March 27, 2024 19:54
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