Re: [PATCH 4/9] drm/tyr: add McuVa and claim-checked MappedBo views

[Daniel Almeida <daniel.almeida@collabora.com>]

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