syncobj: Fix import_sync_file and add timeline exports/imports

[Drakulix]

This addresses the broken api reported in #224 and also provides entry point for the new optional timeline argument.

Ping @ids1024, @YaLTeR, @MarijnS95

[MarijnS95]

Can we maybe have a clearer standalone title? When looking at git log, I don't have a mapping to github issues.

Will look at the code later.

[Drakulix]

Can we maybe have a clearer standalone title? When looking at git log, I don't have a mapping to github issues.

Changed the PR title and split the commits into smaller better described chunks.

[MarijnS95]

Could you make the same change to fn create() and maybe other functions?

OTOH it looks intended for the drm-ffi crate to not reason about which files may have been modified inside the ioctl struct, which is both a consistent pattern but also makes these functions a really weird abstraction in relation to the high-level drm crate.

[Drakulix]

Yeah I am not sure, perhaps I should change them back to return the whole struct for consistency. Possibly we shouldn't even split things like fd_to_syncobj and sync_file_into_syncobj given they use the same ioctl. However then we have two Options and incorrect combinations, making correct usage much more difficult to address.

I am not opposed to change drm-ffi to use some more higher-level types including OwnedFd, but I don't really want to do this as part of this PR.

I guess the original idea for this abstraction was to make it possible to use the calls directly if further down the line new uses get added and/or existing padded-bytes re-used etc. This is an example of that not really working given point was added later.

Maybe @Slabity has some opinions on this?

[MarijnS95]

If that was the case, perhaps now is the time to call it on that experiment and unify things back into a single drm/drm-sys crate in a separate PR.

The only alternative I can see is also accepting the struct as an input, but that's already the API that ioctl::syncobj::xxx() takes :)

[Drakulix]

I changed this PR to return the whole ioctl struct again and do the conversion in drm-rs. But I am happy to continue this discussion in a future PR or issue on the matter.

[Slabity]

So the original idea with the division of crates was to follow what was, at the time, best practices for interfacing with system libraries and calls.

• The sys crate would be exclusively generated bindings to the raw C calls and structures. The only logic involved here would be the build scripts and how they clean things up.
• The ffi crate would do the bare minimum to allow making the calls safely without doing any sort of logic involved. Such as setting up memory pointers/lengths for the various structures that use them. No assumptions other than the bare minimum required to make the ioctl calls are done here.
• The rs crate would do everything higher order and try to closely reflect what libdrm does, leveraging Rust's ability to divide things through traits and also provide a clean, well documented interface. Any sort of logic, dynamic allocation, or other non-trivial things would be done here.

To be honest, I don't know if that's considered best practice now. I do think a separate ffi crate makes things more flexible if someone doesn't like how a certain drm-rs call is. It's also pretty clear 90% of the code in ffi is basically zero cost as well. But I also don't know much about the syncobj API here to argue about it cleanly.

The concern about a stable interface is something that will need to be addressed eventually though. To be honest, I don't think it's possible to follow semantic versioning well in the sys or ffi crates.

• The sys crate is obvious because we have no control over any changes in the userspace API and we rely on the promise that "We don't break userspace".
• The ffi crate because technically ioctl calls can be extended and may require new arguments to be added, which is a breaking change.

As far as I know, it's normal to never really stabilize the sys or ffi crates. But I don't really know if the "best practice" here changed at all.

[MarijnS95]

I changed this PR to return the whole ioctl struct again and do the conversion in drm-rs.

Note that you went out of your way to document the input arguments to these functions, but didn't document what return fields of the struct are expected to be relevant - I think that's where my main disconnect with this API design comes from.

---

• The ffi crate would do the bare minimum to allow making the calls safely without doing any sort of logic involved. Such as setting up memory pointers/lengths for the various structures that use them. No assumptions other than the bare minimum required to make the ioctl calls are done here.

Fair enough though it does all become cloudy when considering inout parameters, optional fields and such. The caller has a clear view of what goes into the function, not what comes out.

In other words, having this crate doesn't seem too bad, but returning the full ioctl() struct is what seems to be confusing perhaps?

---

As far as I know, it's normal to never really stabilize the sys or ffi crates. But I don't really know if the "best practice" here changed at all.

Not sure if this is standard, but most high-level crates might still utilize sys types in their public API for a variety of reasons. Assuming they're not always plain type aliases, any breaking change to the -sys crate automatically means a break to the high-level crate.

Though for the crates I work on (i.e. ndk) those breaking changes in the underlying C API are already "disallowed" as they would also break ABI. Certain cases like enums are exempt via #[non_exhaustive] tricks.

Assuming that is the case for the sys crate here as well (adding #[non_exhaustive] to all structs, ioctl()s should be aware of size for new/optional fields?) that could very well be "stabilizable". It leaves ffi as "the odd one out" when having to extend function parameter interfaces.