Re: [PATCH 2/7] gpu: nova-core: firmware: riscv: use dma::Coherent

[Alexandre Courbot]

On Mon Mar 23, 2026 at 10:05 PM JST, Gary Guo wrote:
> On Mon Mar 23, 2026 at 6:15 AM GMT, Alexandre Courbot wrote:
>> On Sat Mar 21, 2026 at 11:58 PM JST, Gary Guo wrote:
>>> On Sat Mar 21, 2026 at 1:36 PM GMT, Alexandre Courbot wrote:
>>>> Replace the nova-core local `DmaObject` with a `Coherent` that can
>>>> fulfill the same role.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>>>> ---
>>>>  drivers/gpu/nova-core/firmware/riscv.rs | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
>>>> index 14aad2f0ee8a..2afa7f36404e 100644
>>>> --- a/drivers/gpu/nova-core/firmware/riscv.rs
>>>> +++ b/drivers/gpu/nova-core/firmware/riscv.rs
>>>> @@ -5,13 +5,13 @@
>>>>  
>>>>  use kernel::{
>>>>      device,
>>>> +    dma::Coherent,
>>>>      firmware::Firmware,
>>>>      prelude::*,
>>>>      transmute::FromBytes, //
>>>>  };
>>>>  
>>>>  use crate::{
>>>> -    dma::DmaObject,
>>>>      firmware::BinFirmware,
>>>>      num::FromSafeCast, //
>>>>  };
>>>> @@ -66,7 +66,7 @@ pub(crate) struct RiscvFirmware {
>>>>      /// Application version.
>>>>      pub(crate) app_version: u32,
>>>>      /// Device-mapped firmware image.
>>>> -    pub(crate) ucode: DmaObject,
>>>> +    pub(crate) ucode: Coherent<[u8]>,
>>>>  }
>>>>  
>>>>  impl RiscvFirmware {
>>>> @@ -81,7 +81,7 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, fw: &Firmware) -> Result<
>>>>              let len = usize::from_safe_cast(bin_fw.hdr.data_size);
>>>>              let end = start.checked_add(len).ok_or(EINVAL)?;
>>>>  
>>>> -            DmaObject::from_data(dev, fw.data().get(start..end).ok_or(EINVAL)?)?
>>>> +            Coherent::from_slice(dev, fw.data().get(start..end).ok_or(EINVAL)?, GFP_KERNEL)?
>>>
>>> `DmaObject` rounds the data up to be page-sized, while this new API doesn't.
>>>
>>> It has impact on alignment, as the allocator aligns things to the largest
>>> power-of-two exponent of the allocated size.
>>
>> Doesn't `dma_alloc_coherent` always allocate from the page pool and thus
>> returns page-aligned memory though?
>
> Oh you're right, this is not from DMA pool, so allocations are page-aligned
> indeed.
>
> I brought this up because I got bite by this size adjustment behaviour
> because in the `from_data` I initially put
>
>     unsafe { dma_obj.as_mut().copy_from_slice(data) };
>
> but that doesn't work due to the size being aligned up so dma_obj.len() !=
> data.len().
>
> But if this behaviour is not needed it does simplify things quite a bit.

I was contemplating hardening the `Coherent` allocations by padding the
structs when there are specific alignment needs (in nova-core it's only
a couple of types affected), but then thought this was quite restrictive
and should be handled by an alignment parameter on `Coherent` directly -
and `Coherent` doesn't have such a parameter because the underlying C
API doesn't either, and the page alignment seems to be a property of the
API itself.

Maybe that point is worth mentioning in the documentation of `Coherent`?

>
>> I'm not sure why `DmaObject` was doing that but it was redundant I think.
>
> A question for yourself I guess? :-)
>
> https://lore.kernel.org/all/20250619-nova-frts-v6-13-ecf41ef99252@xxxxxxxxxx/

Ha, I was precisely wondering who was the lesser engineer who wrote that. :)