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

Add syncobj support and poll-based example #149

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

CirrusNeptune
Copy link
Contributor

@CirrusNeptune CirrusNeptune commented Jul 10, 2023

  • syncobj::Handle and syncobj::SyncFile types added
  • These types may be used in device-specific Card implementations to add multiple-event asynchronicity of command submissions.

Tested on Raspberry Pi VC4 device in https://github.com/CirrusNeptune/rpi-drm/blob/main/vc4-drm/src/card.rs#L1043

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.

While on first glance the syncobj code looks quite good (thanks for working on this!), I have a couple of questions regarding the example and edition upgrade.

  • Why does this require tokio? I guess just using nix::poll would be totally sufficient to demonstrate this use-case?
  • If we want a tokio example (@Slabity opinions?), can you please separate these into separate commits? So one for adding the syncobj-apis, one upgrading the edition and one adding tokio + the example.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented Jul 10, 2023

Why does this require tokio? I guess just using nix::poll would be totally sufficient to demonstrate this use-case?

Would it make sense to have the example use poll and describe possible tokio usage in a doc comment?

If we want a tokio example (@Slabity opinions?), can you please separate these into separate commits? So one for adding the syncobj-apis, one upgrading the edition and one adding tokio + the example.

Sure! Though if we just have the example use poll, this could be skipped.

@Drakulix
Copy link
Member

Drakulix commented Jul 10, 2023

Would it make sense to have the example use poll and describe possible tokio usage in a doc comment?

I think that makes sense. It is certainly nice to let less knowledgeable users now, that they can use this with tokio.

Sure! Though if we just have the example use poll, this could be skipped.

Great. I agree we don't need this then.

@CirrusNeptune CirrusNeptune changed the title Add syncobj support and tokio-based example Add syncobj support and poll-based example Jul 10, 2023
@CirrusNeptune
Copy link
Contributor Author

Poll based example is pushed with edition removed. Please let me know if anything seems amiss.

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.

I think this is good to go! Thanks again :)

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented Jul 14, 2023

It looks like a new lint was added to 1.70 which doesn't like the re-exported bindgen primitive types.

I can't see a great way to solve this without getting rid of the drm_sys::* or nix::libc::* glob.
Shall I silence this with #![allow(ambiguous_glob_reexports)]?

@Drakulix
Copy link
Member

I can't see a great way to solve this without getting rid of the drm_sys::* or nix::libc::* glob. Shall I silence this with #![allow(ambiguous_glob_reexports)]?

Right, that looks indeed like that is the easiest workaround given, that this is generated code.
But please try to add the annotation to the use-statements directly instead of allowing this globally. We only want to ignore this for bindgen imports.

@CirrusNeptune
Copy link
Contributor Author

Removing the libc glob may actually be the better solution, then the lint would remain active: use nix::libc::{c_int, c_ulong};.

Would this be acceptable?

@Drakulix
Copy link
Member

Removing the libc glob may actually be the better solution, then the lint would remain active: use nix::libc::{c_int, c_ulong};.

Would this be acceptable?

Yes. :)

@CirrusNeptune
Copy link
Contributor Author

Done, that should pass all builtin and clippy lints for all features.

@CirrusNeptune
Copy link
Contributor Author

I see the problem. std::os::fd was made public in 1.66.0. Should we raise the minimum version or avoid these traits?

@Drakulix
Copy link
Member

I see the problem. std::os::fd was made public in 1.66.0. Should we raise the minimum version or avoid these traits?

Please use std::os::unix::io instead, that works on both old and newer versions.

* syncobj::Handle and syncobj::SyncFile types added
* These types may be used in device-specific Card implementations to add
  multiple-event asynchronicity of command submissions.
@CirrusNeptune
Copy link
Contributor Author

Done. Thank you for the assistance!

@Drakulix Drakulix merged commit 7a63fc7 into Smithay:develop Jul 14, 2023
14 checks passed
@CirrusNeptune CirrusNeptune deleted the syncobjs branch July 14, 2023 20:36
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