On Fri, 2025-11-21 at 18:03 -0500, Lyude Paul wrote:
> I haven't gone through this fully yet. I meant to today, but I ended up
> needing way more time to explain some of my review comments w/r/t some
> ww_mutex bindings for rust then I was expecting. But I do already have some
> comments worth reading below:
>
> On Tue, 2025-11-18 at 14:25 +0100, Philipp Stanner wrote:
> >
> > +
> > +/// Container for driver data which the driver gets back in its callback
> > once the fence gets
> > +/// signalled.
> > +#[pin_data]
> > +pub struct DmaFenceCb<T: DmaFenceCbFunc> {
> > + /// C struct needed for the backend.
> > + #[pin]
> > + inner: Opaque<bindings::dma_fence_cb>,
> > + /// Driver data.
> > + #[pin]
> > + pub data: T,
>
> It's entirely possible I've just never seen someone do this before but - is
> are we actually able to make pinned members of structs `pub`? I would have
> thought that wouldn't be allowed (especially if `data` was exposed as just
> `T`, since a user could then move it pretty easily and break the pinning
> guarantee).
>
> …snip…
>
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `ptr`must be a valid pointer to a [`DmaFence`].
> > + unsafe fn dec_ref(ptr: NonNull<Self>) {
> > + // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is
> > called
> > + // the fence is by definition still valid.
> > + let fence = unsafe { (*ptr.as_ptr()).inner.get() };
> > +
> > + // SAFETY: Valid because `fence` was created validly above.
> > + unsafe { bindings::dma_fence_put(fence) }
> > + }
> > +}
> > +
> > +impl<T> DmaFence<T> {
> > + // TODO: There could be a subtle potential problem here? The LLVM
> > compiler backend can create
> > + // several versions of this constant. Their content would be
> > identical, but their addresses
> > + // different.
> > + const OPS: bindings::dma_fence_ops = Self::ops_create();
>
> oh no, not you too!!! D:
>
> I can answer this question - yes, `OPS` definitely won't have a unique memory
> address. Whether that's an issue or not depends on if you actually need to
> check what pointer a `DmaFence` has its `dma_fence_ops` set to and compare it
> against another. If not though, it's probably fine.
In C, there are some use cases where people check the fence_ops addr to
see to whom the fence belongs, AFAIK.
I, so far, can live with there being several ops as long as they all
point to the same functions:
get_driver_name() and get_timeline_name() won't be called by anyone any
time soon (maybe we could even remove them from C, but so far they are
mandatory), and release() receives its data pointer from the C backend,
and since all is pinned we should be good.
However, it's probably wise to at least leave a comment (without the
"TODO") there to make future extenders aware that they cannot identify
a fence by its ops.
> >
> >
[…]
> > +
> > + /// Signal the fence. This will invoke all registered callbacks.
> > + pub fn signal(&self) -> Result {
> > + // SAFETY: `self` is refcounted.
> > + let ret = unsafe { bindings::dma_fence_signal(self.as_raw()) };
> > + if ret != 0 {
> > + return Err(Error::from_errno(ret));
> > + }
>
> You can just use to_result()
OK.
--
I want to present a new version of DmaFence soonish which takes the
separate spinlocks into account that Christian is working on.
P.
