Re: [PATCH v4 13/20] gpu: nova-core: register sysmem flush page

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Jun 2, 2025 at 8:09 PM JST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 05:57:44PM -0400, Lyude Paul wrote:
>> On Wed, 2025-05-21 at 15:45 +0900, Alexandre Courbot wrote:
>> > Reserve a page of system memory so sysmembar can perform a read on it if
>> > a system write occurred since the last flush. Do this early as it can be
>> > required to e.g. reset the GPU falcons.
>> > 
>> > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> > ---
>> >  drivers/gpu/nova-core/gpu.rs  | 45 +++++++++++++++++++++++++++++++++++++++++--
>> >  drivers/gpu/nova-core/regs.rs | 10 ++++++++++
>> >  2 files changed, 53 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> > index 50417f608dc7b445958ae43444a13c7593204fcf..a4e2cf1b529cc25fc168f68f9eaa6f4a7a9748eb 100644
>> > --- a/drivers/gpu/nova-core/gpu.rs
>> > +++ b/drivers/gpu/nova-core/gpu.rs
>> > @@ -2,6 +2,7 @@
>> >  
>> >  use kernel::{device, devres::Devres, error::code::*, pci, prelude::*};
>> >  
>> > +use crate::dma::DmaObject;
>> >  use crate::driver::Bar0;
>> >  use crate::firmware::{Firmware, FIRMWARE_VERSION};
>> >  use crate::gfw;
>> > @@ -158,12 +159,32 @@ fn new(bar: &Bar0) -> Result<Spec> {
>> >  }
>> >  
>> >  /// Structure holding the resources required to operate the GPU.
>> > -#[pin_data]
>> > +#[pin_data(PinnedDrop)]
>> >  pub(crate) struct Gpu {
>> >      spec: Spec,
>> >      /// MMIO mapping of PCI BAR 0
>> >      bar: Devres<Bar0>,
>> >      fw: Firmware,
>> > +    /// System memory page required for flushing all pending GPU-side memory writes done through
>> > +    /// PCIE into system memory.
>> > +    sysmem_flush: DmaObject,
>> > +}
>> > +
>> > +#[pinned_drop]
>> > +impl PinnedDrop for Gpu {
>> > +    fn drop(self: Pin<&mut Self>) {
>> > +        // Unregister the sysmem flush page before we release it.
>> > +        let _ = self.bar.try_access_with(|b| {
>> > +            regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default()
>> > +                .set_adr_39_08(0)
>> > +                .write(b);
>> > +            if self.spec.chipset >= Chipset::GA102 {
>> > +                regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default()
>> > +                    .set_adr_63_40(0)
>> > +                    .write(b);
>> > +            }
>> > +        });
>> > +    }
>
> Sorry that I haven't noticed this before -- I think this should be self
> contained in a new type (e.g. SysmemFlush).

Agreed, a new type sounds like the best way to do and would also address
Lyude's comment. Note however that the destructor will need access to
the Bar, so somehow it will need to be called from the Gpu's own
`PinnedDrop` implementation (or the solution you come with to do it from
`Device::remove`).