linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Asahi Lina <lina@asahilina.net>
To: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>,
	"Luben Tuikov" <luben.tuikov@amd.com>,
	"Jarkko Sakkinen" <jarkko@kernel.org>,
	"Dave Hansen" <dave.hansen@linux.intel.com>,
	"Alyssa Rosenzweig" <alyssa@rosenzweig.io>,
	"Karol Herbst" <kherbst@redhat.com>,
	"Ella Stanforth" <ella@iglunix.org>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>,
	Mary <mary@mary.zone>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	rust-for-linux@vger.kernel.org, linux-media@vger.kernel.org,
	linaro-mm-sig@lists.linaro.org, linux-sgx@vger.kernel.org,
	asahi@lists.linux.dev
Subject: Re: [PATCH RFC 09/18] rust: drm: syncobj: Add DRM Sync Object abstraction
Date: Fri, 7 Apr 2023 01:04:59 +0900	[thread overview]
Message-ID: <d2615710-28c3-2bbb-e0b5-b87eb8d1aa8f@asahilina.net> (raw)
In-Reply-To: <ZC1qn/VCVpudivh9@phenom.ffwll.local>

On 05/04/2023 21.33, Daniel Vetter wrote:
> 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@asahilina.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 :-/

Oof, yeah, that's not possible today. Maybe in the future though, it's 
similar to the execution context stuff...

> 
>> +
>> +        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?

Yeah, that should work! Lifetimes are perfect for this kind of stuff. I 
need to test it out and see what the right way to do it is (lifetime 
parameter or actual reference straight into the drm_syncobj) and see how 
it fits into the driver but I don't see why it wouldn't work, since I 
don't hold onto sync objects for longer than the ioctl. Might just need 
some minor refactoring since the current driver ioctl code wasn't 
written with lifetimes in mind ^^

> 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).

Yeah, if you think restricting the API for this on the Rust side makes 
sense it works for me! I'm all for not abstracting features that aren't 
considered particularly useful/safe/a good idea.

> -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
>>
> 

~~ Lina


  reply	other threads:[~2023-04-06 16:05 UTC|newest]

Thread overview: 122+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-07 14:25 [PATCH RFC 00/18] Rust DRM subsystem abstractions (& preview AGX driver) Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction Asahi Lina
2023-03-07 14:48   ` Karol Herbst
2023-03-07 14:51     ` Karol Herbst
2023-03-07 15:32   ` Maíra Canal
2023-03-09  5:32     ` Asahi Lina
2023-03-09  6:15       ` Dave Airlie
2023-03-09 12:09         ` Maíra Canal
2023-03-07 17:34   ` Björn Roy Baron
2023-03-09  6:04     ` Asahi Lina
2023-03-09 20:24       ` Faith Ekstrand
2023-03-09 20:39         ` Karol Herbst
2023-03-10  6:21           ` Asahi Lina
2023-04-13  9:23   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 02/18] rust: drm: Add Device and Driver abstractions Asahi Lina
2023-03-07 18:19   ` Björn Roy Baron
2023-03-09  6:10     ` Asahi Lina
2023-03-10 18:56   ` Boqun Feng
2023-03-11  5:41   ` Boqun Feng
2023-04-05 17:10   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 03/18] rust: drm: file: Add File abstraction Asahi Lina
2023-03-09 21:16   ` Faith Ekstrand
2023-03-09 22:16     ` Asahi Lina
2023-03-13 17:49       ` Faith Ekstrand
2023-03-14  2:07         ` Boqun Feng
2023-04-05 11:25           ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 04/18] rust: drm: gem: Add GEM object abstraction Asahi Lina
2023-04-05 11:08   ` Daniel Vetter
2023-04-05 11:19     ` Miguel Ojeda
2023-04-05 11:22       ` Daniel Vetter
2023-04-05 12:32         ` Miguel Ojeda
2023-04-05 12:36           ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 05/18] drm/gem-shmem: Export VM ops functions Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction Asahi Lina
2023-03-08 13:38   ` Maíra Canal
2023-03-09  5:25     ` Asahi Lina
2023-03-09 11:47       ` Maíra Canal
2023-03-09 14:16         ` Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 07/18] rust: drm: mm: Add DRM MM Range Allocator abstraction Asahi Lina
2023-04-06 14:15   ` Daniel Vetter
2023-04-06 15:28     ` Miguel Ojeda
2023-04-06 15:45       ` Daniel Vetter
2023-04-06 17:19         ` Miguel Ojeda
2023-04-06 15:53     ` Asahi Lina
2023-04-06 16:13       ` [Linaro-mm-sig] " Daniel Vetter
2023-04-06 16:39         ` Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 08/18] rust: dma_fence: Add DMA Fence abstraction Asahi Lina
2023-04-05 11:10   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 09/18] rust: drm: syncobj: Add DRM Sync Object abstraction Asahi Lina
2023-04-05 12:33   ` Daniel Vetter
2023-04-06 16:04     ` Asahi Lina [this message]
2023-03-07 14:25 ` [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback Asahi Lina
2023-03-08  8:46   ` Christian König
2023-03-08  9:41     ` Asahi Lina
2023-03-08 10:00       ` Christian König
2023-03-08 14:53         ` Asahi Lina
2023-03-08 15:30           ` Christian König
2023-03-08 16:44             ` Asahi Lina
2023-03-08 17:57               ` Christian König
2023-03-08 19:05                 ` Asahi Lina
2023-03-08 19:12                   ` Christian König
2023-03-08 19:45                     ` Asahi Lina
2023-03-08 20:14                       ` Christian König
2023-03-09  6:30                         ` Asahi Lina
2023-03-09  8:05                           ` Christian König
2023-03-09  9:14                             ` Asahi Lina
2023-03-09 18:50                               ` Faith Ekstrand
2023-03-10  9:16                                 ` Asahi Lina
2023-03-08 12:39     ` Karol Herbst
2023-03-08 13:47       ` Christian König
2023-03-08 14:43         ` Karol Herbst
2023-03-08 15:02           ` Christian König
2023-03-08 15:19             ` Karol Herbst
2023-03-16 13:40               ` Daniel Vetter
2023-04-05 13:40   ` Daniel Vetter
2023-04-05 14:14     ` Christian König
2023-04-05 14:21       ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down Asahi Lina
2023-03-08  9:57   ` Maarten Lankhorst
2023-03-08 10:03     ` Christian König
2023-03-08 15:18       ` Asahi Lina
2023-03-08 15:42         ` Christian König
2023-03-08 17:32           ` Asahi Lina
2023-03-08 18:12             ` Christian König
2023-03-08 19:37               ` Asahi Lina
2023-03-09  8:42                 ` Christian König
2023-03-09  9:43                   ` Asahi Lina
2023-03-09 11:47                     ` Christian König
2023-03-09 13:48                       ` Asahi Lina
2023-03-09 19:59                     ` Faith Ekstrand
2023-03-10  9:58                       ` Asahi Lina
2023-03-13 20:11                         ` Faith Ekstrand
2023-03-08 17:39           ` alyssa
2023-03-08 17:44             ` Asahi Lina
2023-03-08 18:13             ` Christian König
2023-04-05 13:52   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 12/18] rust: drm: sched: Add GPU scheduler abstraction Asahi Lina
2023-04-05 15:43   ` Daniel Vetter
2023-04-05 19:29     ` Daniel Vetter
2023-04-18  8:45       ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 13/18] drm/gem: Add a flag to control whether objects can be exported Asahi Lina
2023-04-05 14:55   ` Daniel Vetter
2023-03-07 14:25 ` [PATCH RFC 14/18] rust: drm: gem: Add set_exportable() method Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 15/18] drm/asahi: Add the Asahi driver UAPI [DO NOT MERGE] Asahi Lina
2023-03-07 15:28   ` Karol Herbst
2023-03-07 14:25 ` [PATCH RFC 16/18] rust: bindings: Bind the Asahi DRM UAPI Asahi Lina
2023-03-07 14:25 ` [PATCH RFC 17/18] rust: macros: Add versions macro Asahi Lina
2023-03-07 16:17 ` [PATCH RFC 00/18] Rust DRM subsystem abstractions (& preview AGX driver) Asahi Lina
     [not found] ` <20230307-rust-drm-v1-18-917ff5bc80a8@asahilina.net>
2023-04-05 14:44   ` [PATCH RFC 18/18] drm/asahi: Add the Asahi driver for Apple AGX GPUs Daniel Vetter
2023-04-06  5:02     ` Asahi Lina
2023-04-06  5:09       ` Asahi Lina
2023-04-06 11:25       ` [Linaro-mm-sig] " Daniel Vetter
2023-04-06 13:32         ` Asahi Lina
2023-04-06 13:54           ` Daniel Vetter
     [not found]   ` <ZC2HtBOaoUAzVCVH@phenom.ffwll.local>
2023-04-06  4:44     ` Asahi Lina
2023-04-06  5:09       ` Asahi Lina
2023-04-06 11:26         ` Daniel Vetter
2023-04-06 10:42       ` [Linaro-mm-sig] " Daniel Vetter
2023-04-06 11:55       ` Daniel Vetter
2023-04-06 13:15         ` Asahi Lina
2023-04-06 13:48           ` Daniel Vetter
2023-04-06 15:19             ` Asahi Lina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d2615710-28c3-2bbb-e0b5-b87eb8d1aa8f@asahilina.net \
    --to=lina@asahilina.net \
    --cc=airlied@gmail.com \
    --cc=alex.gaynor@gmail.com \
    --cc=alyssa@rosenzweig.io \
    --cc=asahi@lists.linux.dev \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=christian.koenig@amd.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ella@iglunix.org \
    --cc=faith.ekstrand@collabora.com \
    --cc=gary@garyguo.net \
    --cc=jarkko@kernel.org \
    --cc=kherbst@redhat.com \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luben.tuikov@amd.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mary@mary.zone \
    --cc=mripard@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tzimmermann@suse.de \
    --cc=wedsonaf@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).