Re: [PATCH v2 8/8] gpu: nova-core: convert to new dma::Coherent API

["Gary Guo" <gary@garyguo.net>]

On Fri Mar 20, 2026 at 7:45 PM GMT, Danilo Krummrich wrote:
> From: Gary Guo <gary@garyguo.net>
>
> Remove all usages of dma::CoherentAllocation and use the new
> dma::Coherent type instead.
>
> Note that there are still remainders of the old dma::CoherentAllocation
> API, such as as_slice() and as_slice_mut().

Hi Danilo,

It looks like with Alex's "gpu: nova-core: create falcon firmware DMA objects
lazily" landed, all others users of the old API are now gone.

So this line could be dropped and `impl CoherentAllocation` and the type alias
can be removed after this patch.

Best,
Gary

>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Co-developed-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/gpu/nova-core/dma.rs      | 19 ++++++-------
>  drivers/gpu/nova-core/falcon.rs   |  5 ++--
>  drivers/gpu/nova-core/gsp.rs      | 21 ++++++++------
>  drivers/gpu/nova-core/gsp/cmdq.rs | 21 ++++++--------
>  drivers/gpu/nova-core/gsp/fw.rs   | 46 ++++++++++---------------------
>  5 files changed, 47 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
> index 7215398969da..3c19d5ffcfe8 100644
> --- a/drivers/gpu/nova-core/dma.rs
> +++ b/drivers/gpu/nova-core/dma.rs
> @@ -9,13 +9,13 @@
>  
>  use kernel::{
>      device,
> -    dma::CoherentAllocation,
> +    dma::Coherent,
>      page::PAGE_SIZE,
>      prelude::*, //
>  };
>  
>  pub(crate) struct DmaObject {
> -    dma: CoherentAllocation<u8>,
> +    dma: Coherent<[u8]>,
>  }
>  
>  impl DmaObject {
> @@ -24,23 +24,22 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>              .map_err(|_| EINVAL)?
>              .pad_to_align()
>              .size();
> -        let dma = CoherentAllocation::alloc_coherent(dev, len, GFP_KERNEL | __GFP_ZERO)?;
> +        let dma = Coherent::zeroed_slice(dev, len, GFP_KERNEL)?;
>  
>          Ok(Self { dma })
>      }
>  
>      pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
> -        Self::new(dev, data.len()).and_then(|mut dma_obj| {
> -            // SAFETY: We have just allocated the DMA memory, we are the only users and
> -            // we haven't made the device aware of the handle yet.
> -            unsafe { dma_obj.write(data, 0)? }
> -            Ok(dma_obj)
> -        })
> +        let dma_obj = Self::new(dev, data.len())?;
> +        // SAFETY: We have just allocated the DMA memory, we are the only users and
> +        // we haven't made the device aware of the handle yet.
> +        unsafe { dma_obj.as_mut()[..data.len()].copy_from_slice(data) };
> +        Ok(dma_obj)
>      }
>  }
>  
>  impl Deref for DmaObject {
> -    type Target = CoherentAllocation<u8>;
> +    type Target = Coherent<[u8]>;
>  
>      fn deref(&self) -> &Self::Target {
>          &self.dma
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 7097a206ec3c..5bf8da8760bf 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -26,8 +26,7 @@
>      gpu::Chipset,
>      num::{
>          self,
> -        FromSafeCast,
> -        IntoSafeCast, //
> +        FromSafeCast, //
>      },
>      regs,
>      regs::macros::RegisterBase, //
> @@ -653,7 +652,7 @@ fn dma_wr(
>              }
>              FalconMem::Dmem => (
>                  0,
> -                dma_obj.dma_handle_with_offset(load_offsets.src_start.into_safe_cast())?,
> +                dma_obj.dma_handle() + DmaAddress::from(load_offsets.src_start),
>              ),
>          };
>          if dma_start % DmaAddress::from(DMA_LEN) > 0 {
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index f0a50bdc4c00..a045c4189989 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -6,13 +6,15 @@
>      device,
>      dma::{
>          Coherent,
> -        CoherentAllocation,
>          CoherentBox,
>          DmaAddress, //
>      },
>      pci,
>      prelude::*,
> -    transmute::AsBytes, //
> +    transmute::{
> +        AsBytes,
> +        FromBytes, //
> +    }, //
>  };
>  
>  pub(crate) mod cmdq;
> @@ -44,6 +46,9 @@
>  #[repr(C)]
>  struct PteArray<const NUM_ENTRIES: usize>([u64; NUM_ENTRIES]);
>  
> +/// SAFETY: arrays of `u64` implement `FromBytes` and we are but a wrapper around one.
> +unsafe impl<const NUM_ENTRIES: usize> FromBytes for PteArray<NUM_ENTRIES> {}
> +
>  /// SAFETY: arrays of `u64` implement `AsBytes` and we are but a wrapper around one.
>  unsafe impl<const NUM_ENTRIES: usize> AsBytes for PteArray<NUM_ENTRIES> {}
>  
> @@ -71,26 +76,24 @@ fn entry(start: DmaAddress, index: usize) -> Result<u64> {
>  /// then pp points to index into the buffer where the next logging entry will
>  /// be written. Therefore, the logging data is valid if:
>  ///   1 <= pp < sizeof(buffer)/sizeof(u64)
> -struct LogBuffer(CoherentAllocation<u8>);
> +struct LogBuffer(Coherent<[u8]>);
>  
>  impl LogBuffer {
>      /// Creates a new `LogBuffer` mapped on `dev`.
>      fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          const NUM_PAGES: usize = RM_LOG_BUFFER_NUM_PAGES;
>  
> -        let mut obj = Self(CoherentAllocation::<u8>::alloc_coherent(
> +        let obj = Self(Coherent::<u8>::zeroed_slice(
>              dev,
>              NUM_PAGES * GSP_PAGE_SIZE,
> -            GFP_KERNEL | __GFP_ZERO,
> +            GFP_KERNEL,
>          )?);
>  
>          let start_addr = obj.0.dma_handle();
>  
>          // SAFETY: `obj` has just been created and we are its sole user.
> -        let pte_region = unsafe {
> -            obj.0
> -                .as_slice_mut(size_of::<u64>(), NUM_PAGES * size_of::<u64>())?
> -        };
> +        let pte_region =
> +            unsafe { &mut obj.0.as_mut()[size_of::<u64>()..][..NUM_PAGES * size_of::<u64>()] };
>  
>          // Write values one by one to avoid an on-stack instance of `PteArray`.
>          for (i, chunk) in pte_region.chunks_exact_mut(size_of::<u64>()).enumerate() {
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index d36a62ba1c60..f38790601a0f 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -7,7 +7,7 @@
>  use kernel::{
>      device,
>      dma::{
> -        CoherentAllocation,
> +        Coherent,
>          DmaAddress, //
>      },
>      dma_write,
> @@ -207,7 +207,7 @@ unsafe impl AsBytes for GspMem {}
>  // that is not a problem because they are not used outside the kernel.
>  unsafe impl FromBytes for GspMem {}
>  
> -/// Wrapper around [`GspMem`] to share it with the GPU using a [`CoherentAllocation`].
> +/// Wrapper around [`GspMem`] to share it with the GPU using a [`Coherent`].
>  ///
>  /// This provides the low-level functionality to communicate with the GSP, including allocation of
>  /// queue space to write messages to and management of read/write pointers.
> @@ -218,7 +218,7 @@ unsafe impl FromBytes for GspMem {}
>  ///   pointer and the GSP read pointer. This region is returned by [`Self::driver_write_area`].
>  /// * The driver owns (i.e. can read from) the part of the GSP message queue between the CPU read
>  ///   pointer and the GSP write pointer. This region is returned by [`Self::driver_read_area`].
> -struct DmaGspMem(CoherentAllocation<GspMem>);
> +struct DmaGspMem(Coherent<GspMem>);
>  
>  impl DmaGspMem {
>      /// Allocate a new instance and map it for `dev`.
> @@ -226,21 +226,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          const MSGQ_SIZE: u32 = num::usize_into_u32::<{ size_of::<Msgq>() }>();
>          const RX_HDR_OFF: u32 = num::usize_into_u32::<{ mem::offset_of!(Msgq, rx) }>();
>  
> -        let gsp_mem =
> -            CoherentAllocation::<GspMem>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
> +        let gsp_mem = Coherent::<GspMem>::zeroed(dev, GFP_KERNEL)?;
>  
>          let start = gsp_mem.dma_handle();
>          // Write values one by one to avoid an on-stack instance of `PteArray`.
>          for i in 0..GspMem::PTE_ARRAY_SIZE {
> -            dma_write!(gsp_mem, [0]?.ptes.0[i], PteArray::<0>::entry(start, i)?);
> +            dma_write!(gsp_mem, .ptes.0[i], PteArray::<0>::entry(start, i)?);
>          }
>  
>          dma_write!(
>              gsp_mem,
> -            [0]?.cpuq.tx,
> +            .cpuq.tx,
>              MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES)
>          );
> -        dma_write!(gsp_mem, [0]?.cpuq.rx, MsgqRxHeader::new());
> +        dma_write!(gsp_mem, .cpuq.rx, MsgqRxHeader::new());
>  
>          Ok(Self(gsp_mem))
>      }
> @@ -255,10 +254,9 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          let rx = self.gsp_read_ptr() as usize;
>  
>          // SAFETY:
> -        // - The `CoherentAllocation` contains exactly one object.
>          // - We will only access the driver-owned part of the shared memory.
>          // - Per the safety statement of the function, no concurrent access will be performed.
> -        let gsp_mem = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];
> +        let gsp_mem = unsafe { &mut *self.0.as_mut() };
>          // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
>          let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
>  
> @@ -309,10 +307,9 @@ fn driver_write_area_size(&self) -> usize {
>          let rx = self.cpu_read_ptr() as usize;
>  
>          // SAFETY:
> -        // - The `CoherentAllocation` contains exactly one object.
>          // - We will only access the driver-owned part of the shared memory.
>          // - Per the safety statement of the function, no concurrent access will be performed.
> -        let gsp_mem = &unsafe { self.0.as_slice(0, 1) }.unwrap()[0];
> +        let gsp_mem = unsafe { &*self.0.as_ptr() };
>          let data = &gsp_mem.gspq.msgq.data;
>  
>          // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 0d8daf6a80b7..847b5eb215d4 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -40,8 +40,7 @@
>      },
>  };
>  
> -// TODO: Replace with `IoView` projections once available; the `unwrap()` calls go away once we
> -// switch to the new `dma::Coherent` API.
> +// TODO: Replace with `IoView` projections once available.
>  pub(super) mod gsp_mem {
>      use core::sync::atomic::{
>          fence,
> @@ -49,10 +48,9 @@ pub(super) mod gsp_mem {
>      };
>  
>      use kernel::{
> -        dma::CoherentAllocation,
> +        dma::Coherent,
>          dma_read,
> -        dma_write,
> -        prelude::*, //
> +        dma_write, //
>      };
>  
>      use crate::gsp::cmdq::{
> @@ -60,49 +58,35 @@ pub(super) mod gsp_mem {
>          MSGQ_NUM_PAGES, //
>      };
>  
> -    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &Coherent<GspMem>) -> u32 {
> +        dma_read!(qs, .gspq.tx.0.writePtr) % MSGQ_NUM_PAGES
>      }
>  
> -    pub(in crate::gsp) fn gsp_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    pub(in crate::gsp) fn gsp_read_ptr(qs: &Coherent<GspMem>) -> u32 {
> +        dma_read!(qs, .gspq.rx.0.readPtr) % MSGQ_NUM_PAGES
>      }
>  
> -    pub(in crate::gsp) fn cpu_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    pub(in crate::gsp) fn cpu_read_ptr(qs: &Coherent<GspMem>) -> u32 {
> +        dma_read!(qs, .cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES
>      }
>  
> -    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> +    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &Coherent<GspMem>, count: u32) {
>          let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
>  
>          // Ensure read pointer is properly ordered.
>          fence(Ordering::SeqCst);
>  
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result {
> -            dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
> -            Ok(())
> -        }()
> -        .unwrap()
> +        dma_write!(qs, .cpuq.rx.0.readPtr, rptr);
>      }
>  
> -    pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    pub(in crate::gsp) fn cpu_write_ptr(qs: &Coherent<GspMem>) -> u32 {
> +        dma_read!(qs, .cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES
>      }
>  
> -    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> +    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &Coherent<GspMem>, count: u32) {
>          let wptr = cpu_write_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
>  
> -        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> -        || -> Result {
> -            dma_write!(qs, [0]?.cpuq.tx.0.writePtr, wptr);
> -            Ok(())
> -        }()
> -        .unwrap();
> +        dma_write!(qs, .cpuq.tx.0.writePtr, wptr);
>  
>          // Ensure all command data is visible before triggering the GSP read.
>          fence(Ordering::SeqCst);