Message-ID: <ZC1qn/VCVpudivh9@phenom.ffwll.local>
Date:   Wed, 5 Apr 2023 14:33:35 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Asahi Lina <lina@...hilina.net>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Luben Tuikov <luben.tuikov@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Karol Herbst <kherbst@...hat.com>,
        Ella Stanforth <ella@...unix.org>,
        Faith Ekstrand <faith.ekstrand@...labora.com>,
        Mary <mary@...y.zone>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
        linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
        linux-sgx@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH RFC 09/18] rust: drm: syncobj: Add DRM Sync Object
 abstraction

On Tue, Mar 07, 2023 at 11:25:34PM +0900, Asahi Lina wrote:
> DRM Sync Objects are a container for a DMA fence, and can be waited on
> signaled, exported, and imported from userspace. Add a Rust abstraction
> so Rust DRM drivers can support this functionality.
> 
> Signed-off-by: Asahi Lina <lina@...hilina.net>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers.c                  | 19 ++++++++++
>  rust/kernel/drm/mod.rs          |  1 +
>  rust/kernel/drm/syncobj.rs      | 77 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 98 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 705af292a5b4..b6696011f3a4 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -12,6 +12,7 @@
>  #include <drm/drm_gem.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_ioctl.h>
> +#include <drm/drm_syncobj.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/dma-fence.h>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 8e906a7a7d8a..11965b1e2f4e 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>  
>  #include <drm/drm_gem.h>
>  #include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_syncobj.h>
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
>  #include <linux/device.h>
> @@ -461,6 +462,24 @@ __u64 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
>  }
>  EXPORT_SYMBOL_GPL(rust_helper_drm_vma_node_offset_addr);
>  
> +void rust_helper_drm_syncobj_get(struct drm_syncobj *obj)
> +{
> +	drm_syncobj_get(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_get);
> +
> +void rust_helper_drm_syncobj_put(struct drm_syncobj *obj)
> +{
> +	drm_syncobj_put(obj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_put);
> +
> +struct dma_fence *rust_helper_drm_syncobj_fence_get(struct drm_syncobj *syncobj)
> +{
> +	return drm_syncobj_fence_get(syncobj);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_drm_syncobj_fence_get);
> +
>  #ifdef CONFIG_DRM_GEM_SHMEM_HELPER
>  
>  void rust_helper_drm_gem_shmem_object_free(struct drm_gem_object *obj)
> diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs
> index 73fab2dee3af..dae98826edfd 100644
> --- a/rust/kernel/drm/mod.rs
> +++ b/rust/kernel/drm/mod.rs
> @@ -8,3 +8,4 @@ pub mod file;
>  pub mod gem;
>  pub mod ioctl;
>  pub mod mm;
> +pub mod syncobj;
> diff --git a/rust/kernel/drm/syncobj.rs b/rust/kernel/drm/syncobj.rs
> new file mode 100644
> index 000000000000..10eed05eb27a
> --- /dev/null
> +++ b/rust/kernel/drm/syncobj.rs
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +//! DRM Sync Objects
> +//!
> +//! C header: [`include/linux/drm/drm_syncobj.h`](../../../../include/linux/drm/drm_syncobj.h)
> +
> +use crate::{bindings, dma_fence::*, drm, error::Result, prelude::*};
> +
> +/// A DRM Sync Object
> +///
> +/// # Invariants
> +/// ptr is a valid pointer to a drm_syncobj and we own a reference to it.
> +pub struct SyncObj {
> +    ptr: *mut bindings::drm_syncobj,
> +}
> +
> +impl SyncObj {
> +    /// Looks up a sync object by its handle for a given `File`.
> +    pub fn lookup_handle(file: &impl drm::file::GenericFile, handle: u32) -> Result<SyncObj> {
> +        // SAFETY: The arguments are all valid per the type invariants.
> +        let ptr = unsafe { bindings::drm_syncobj_find(file.raw() as *mut _, handle) };

Just an aside, but the semantics of this are nasty: You're not allowed to
hold any locks while calling this. We have runtime checks for that (if you
enable lockdep), but I don't see any way to encode that on the rust side
and check it at compile time :-/

> +
> +        if ptr.is_null() {
> +            Err(ENOENT)
> +        } else {
> +            Ok(SyncObj { ptr })
> +        }
> +    }
> +
> +    /// Returns the DMA fence associated with this sync object, if any.
> +    pub fn fence_get(&self) -> Option<Fence> {
> +        let fence = unsafe { bindings::drm_syncobj_fence_get(self.ptr) };
> +        if fence.is_null() {
> +            None
> +        } else {
> +            // SAFETY: The pointer is non-NULL and drm_syncobj_fence_get acquired an
> +            // additional reference.
> +            Some(unsafe { Fence::from_raw(fence) })
> +        }
> +    }
> +
> +    /// Replaces the DMA fence with a new one, or removes it if fence is None.
> +    pub fn replace_fence(&self, fence: Option<&Fence>) {
> +        unsafe {
> +            bindings::drm_syncobj_replace_fence(
> +                self.ptr,
> +                fence.map_or(core::ptr::null_mut(), |a| a.raw()),
> +            )
> +        };
> +    }
> +
> +    /// Adds a new timeline point to the syncobj.
> +    pub fn add_point(&self, chain: FenceChain, fence: &Fence, point: u64) {
> +        // SAFETY: All arguments should be valid per the respective type invariants.
> +        // This takes over the FenceChain ownership.
> +        unsafe { bindings::drm_syncobj_add_point(self.ptr, chain.into_raw(), fence.raw(), point) };
> +    }
> +}
> +
> +impl Drop for SyncObj {
> +    fn drop(&mut self) {
> +        // SAFETY: We own a reference to this syncobj.
> +        unsafe { bindings::drm_syncobj_put(self.ptr) };
> +    }
> +}
> +
> +impl Clone for SyncObj {
> +    fn clone(&self) -> Self {
> +        // SAFETY: `ptr` is valid per the type invariant and we own a reference to it.
> +        unsafe { bindings::drm_syncobj_get(self.ptr) };

So yeah syncobj are refcounted because they're shareable uapi objects (you
can pass them around as fd), but that really should be entirely the
subsystems business, not for drivers.

This is kinda like drm_file, which is also refcounted (by virtue of
hanging of struct file), but the refcounting is entirely handled by the
vfs and all drivers get is a borrowed reference, which nicely bounds the
lifetime to the callback (which is usually an ioctl handler). I think we
want the same semantics for syncobj, because if a driver is hanging onto a
syncobj for longer than the ioctl. If my rust understanding is right we'd
get that by dropping Clone here and relying on lookup_handle only being
able to return stuff that's bound by the drm_file?

People are talking about drivers holding onto syncobj for longer, but I'm
still not sold on the idea that this is any good and doesn't just bend the
dma_fence and syncobj rules a bit too much over the breaking point. For
kernel drivers it really should be just a different way to lookup and
return dma_fence from the ioctl, pretty much matching what you could also
do with sync_file (but since syncobj provides generic compat ioctl to
convert to/from sync_file drivders only need to handle syncobj).
-Daniel


> +        SyncObj { ptr: self.ptr }
> +    }
> +}
> +
> +// SAFETY: drm_syncobj operations are internally locked.
> +unsafe impl Sync for SyncObj {}
> +unsafe impl Send for SyncObj {}
> 
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists