linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Asahi Lina <lina@asahilina.net>
To: "Maíra Canal" <mcanal@igalia.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"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>
Cc: linaro-mm-sig@lists.linaro.org, rust-for-linux@vger.kernel.org,
	Karol Herbst <kherbst@redhat.com>,
	asahi@lists.linux.dev, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, Mary <mary@mary.zone>,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>,
	linux-sgx@vger.kernel.org, Ella Stanforth <ella@iglunix.org>,
	Faith Ekstrand <faith.ekstrand@collabora.com>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction
Date: Thu, 9 Mar 2023 23:16:25 +0900	[thread overview]
Message-ID: <ac5a748a-bd1c-1076-a17f-42367494976c@asahilina.net> (raw)
In-Reply-To: <8e091158-7826-1215-e717-081b25f48108@igalia.com>

On 09/03/2023 20.47, Maíra Canal wrote:
> On 3/9/23 02:25, Asahi Lina wrote:
>> On 08/03/2023 22.38, Maíra Canal wrote:
>>> On 3/7/23 11:25, Asahi Lina wrote:
>>>> The DRM shmem helper includes common code useful for drivers which
>>>> allocate GEM objects as anonymous shmem. Add a Rust abstraction for
>>>> this. Drivers can choose the raw GEM implementation or the shmem layer,
>>>> depending on their needs.
>>>>
>>>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>>>> ---
>>>>    drivers/gpu/drm/Kconfig         |   5 +
>>>>    rust/bindings/bindings_helper.h |   2 +
>>>>    rust/helpers.c                  |  67 +++++++
>>>>    rust/kernel/drm/gem/mod.rs      |   3 +
>>>>    rust/kernel/drm/gem/shmem.rs    | 381 ++++++++++++++++++++++++++++++++++++++++
>>>>    5 files changed, 458 insertions(+)
>>>>
>>>
>>> [...]
>>>
>>>> +unsafe extern "C" fn gem_create_object<T: DriverObject>(
>>>> +    raw_dev: *mut bindings::drm_device,
>>>> +    size: usize,
>>>> +) -> *mut bindings::drm_gem_object {
>>>> +    // SAFETY: GEM ensures the device lives as long as its objects live,
>>>> +    // so we can conjure up a reference from thin air and never drop it.
>>>> +    let dev = ManuallyDrop::new(unsafe { device::Device::from_raw(raw_dev) });
>>>> +
>>>> +    let inner = match T::new(&*dev, size) {
>>>> +        Ok(v) => v,
>>>> +        Err(e) => return e.to_ptr(),
>>>> +    };
>>>> +
>>>> +    let p = unsafe {
>>>> +        bindings::krealloc(
>>>> +            core::ptr::null(),
>>>> +            Object::<T>::SIZE,
>>>> +            bindings::GFP_KERNEL | bindings::__GFP_ZERO,
>>>> +        ) as *mut Object<T>
>>>> +    };
>>>> +
>>>> +    if p.is_null() {
>>>> +        return ENOMEM.to_ptr();
>>>> +    }
>>>> +
>>>> +    // SAFETY: p is valid as long as the alloc succeeded
>>>> +    unsafe {
>>>> +        addr_of_mut!((*p).dev).write(dev);
>>>> +        addr_of_mut!((*p).inner).write(inner);
>>>> +    }
>>>> +
>>>> +    // SAFETY: drm_gem_shmem_object is safe to zero-init, and
>>>> +    // the rest of Object has been initialized
>>>> +    let new: &mut Object<T> = unsafe { &mut *(p as *mut _) };
>>>> +
>>>> +    new.obj.base.funcs = &Object::<T>::VTABLE;
>>>> +    &mut new.obj.base
>>>> +}
>>>
>>> It would be nice to allow to set wc inside the gem_create_object callback,
>>> as some drivers do it so, like v3d, vc4, panfrost, lima...
>>
>> This is actually a bit tricky to do safely, because we can't just have a
>> callback that takes the drm_gem_shmem_object instance inside
>> gem_create_object because it is not fully initialized yet from the point
>> of view of the gem shmem API. Maybe we could have some sort of temporary
>> proxy object that only lets you do safe things like set map_wc? Or maybe
>> the new() callback could return something like a ShmemTemplate<T> type
>> that contains both the inner data and some miscellaneous fields like the
>> initial map_wc state?
> 
> I see that most drivers use this hook to set map_wc and set funcs. What
> are your thoughts on something like this?
> 
> Best Regards,
> - Maíra Canal
> 
>  From 61f23f4a39028c9d34d3df58d7640bfcd64e9af9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ma=C3=ADra=20Canal?= <mcanal@igalia.com>
> Date: Thu, 9 Mar 2023 08:24:09 -0300
> Subject: [PATCH] rust: drm: gem: shmem: Set map_wc on gem_create_object
>   callback
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Some drivers use the gem_create_object callback to define the mapping of
> the object write-combined (map_wc). Currently, the DRM Rust abstractions
> doesn't allow such operation. So, add a method to the DriverObject trait
> to allow drivers to set map_wc on the gem_create_object callback. By
> default, the method returns false, which is the shmem default value.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   rust/kernel/drm/gem/shmem.rs | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 8f17eba0be99..a7f33b66f60a 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -24,6 +24,11 @@ use gem::BaseObject;
>   pub trait DriverObject: gem::BaseDriverObject<Object<Self>> {
>       /// Parent `Driver` for this object.
>       type Driver: drv::Driver;
> +
> +    /// Define the map object write-combined
> +    fn set_wc() -> bool {
> +        false
> +    }
>   }

I think if you're going to make it a static function like that, we might
as well just make it an associated constant like `DEFAULT_WC`? After all
there is no information gem_create_object gets other than the size so we
can't really do anything more useful, and `set_wc()` can't do much other
than return a constant ^^

The only corner case I can think of is cases where the WC mode depends
on the device (for example, if some devices want to enable it or not
depending on whether the particular hardware variant is cache-coherent),
but then it should probably just be part of the return value for T::new
since that function already gets all available information (device and
size). But I think a constant works for now, we can always extend it
when a use case comes for doing more.

~~ Lina

  reply	other threads:[~2023-03-09 14:16 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 [this message]
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
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=ac5a748a-bd1c-1076-a17f-42367494976c@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=daniel@ffwll.ch \
    --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=mcanal@igalia.com \
    --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).