Hi Laura,
It’s been a while since I wrote this, and I now see this could use some
improvements.
I think the first problem is that this conflates "MappedBo" with "the MappedBo
backing the shared region". As soon as we try to add more users, this will be a
problem. I propose that we split this into two types that build upon each other:
a) a generic layer MappedBo, which remains roughly as-is:
gem.rs:
pub(crate) struct MappedBo<'drm> {
vmap: shmem::VMapOwned<BoData>,
bo: KernelBo<'drm>,
}
impl<'drm> MappedBo<'drm> {
// pub(crate) API:
// no Arc, McuMappedBo has to hold it by value, otherwise a second holder
// could write around the claims.
fn new(bo: KernelBo<'drm>) -> Result<Self>
fn va_range(...) -> ...
fn vmap(...) -> ...
}
impl Deref for MappedBo<'_> { type Target = Bo ... }
b) A MCU-specific BO type with the claims + offset logic:
fw/mcu_bo.rs:
pub(super) struct McuVa(...) + related McuVa impls.
#[pin_data]
pub(super) struct McuMappedBo<'drm> {
bo: MappedBo<'drm>, // private, no accessor
#[pin]
claims: Mutex<KVec<Range<u64>>>
}
impl<'drm> McuMappedBo<'drm> {
// pub super API
// builds its own MappedBo, checks the 32-bit range before allocating and
// copies the section data from the fw binary before any view can exist.
new(ddev, mcu_vm: Arc<Vm<'drm>>, va: McuVa, size: u64, flags: VmMapFlags,
data: &[u8]) -> Result<impl PinInit<Self,Error>> {...}
// views borrow: &'a self instead of self: &Arc<Self>
fn try_view<'a>(&'a self, va: McuVa, len: u64, reach: u64)
-> Result<McuView<'a>>
fn try_view_mut<'a>(&'a self, va: McuVa, len: u64, reach: u64)
-> Result<McuViewMut<'a>>
}
c) Views get renamed to McuView, McuViewMut:
#[derive(Clone)]
pub(super) struct McuView<'a> {
vmap: &'a VMapOwned<BoData>,
base: u64,
range: Range<u64>,
reach: u64, // <———— Store this
}
pub(super) struct McuViewMut<'a> {
view: McuView<'a>,
claims: &'a Mutex<KVec<Range<u64>>>,
}
impl<'a> Deref for McuViewMut<'a> {
type Target = McuView<'a>;
fn deref(&self) -> &Self::Target {
&self.view
}
}
impl Drop for McuViewMut<'_> {
// return claim here.
}
>
> +// SAFETY: `MappedBo` may move between threads: the CPU mapping's address is
> +// valid from any thread; `KernelBo`'s teardown (GPU unmap through `Arc<Vm>`
> +// and the GEM object release) goes through thread-safe C APIs; and the only
> +// interior mutability (`claims`) is mutex-protected.
> +unsafe impl Send for MappedBo<'_> {}
> +// SAFETY: `&MappedBo` exposes the mutex-protected claims table, the `Deref`
> +// surface to the GEM object (thread-safe C APIs), and the mapping itself,
> +// whose contents are only ever accessed through volatile operations. The
> +// memory is shared with the MCU by design, so concurrent access is part of
> +// the model rather than a race the type system must rule out.
> +unsafe impl Sync for MappedBo<'_> {}
I don’t think we need a manual implementation here.
The rest of the patch looks good to me.
— Daniel
