Message-ID: <20250205155646.00003c2f@nvidia.com>
Date: Wed, 5 Feb 2025 15:56:46 +0200
From: Zhi Wang <zhiw@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
CC: <airlied@...il.com>, <simona@...ll.ch>, <corbet@....net>,
	<maarten.lankhorst@...ux.intel.com>, <mripard@...nel.org>,
	<tzimmermann@...e.de>, <ajanulgu@...hat.com>, <lyude@...hat.com>,
	<pstanner@...hat.com>, <cjia@...dia.com>, <jhubbard@...dia.com>,
	<bskeggs@...dia.com>, <acurrid@...dia.com>, <ojeda@...nel.org>,
	<alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
	<bjorn3_gh@...tonmail.com>, <benno.lossin@...ton.me>,
	<a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
	<dri-devel@...ts.freedesktop.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <nouveau@...ts.freedesktop.org>,
	<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] gpu: nova-core: add initial documentation

On Tue,  4 Feb 2025 20:03:12 +0100
Danilo Krummrich <dakr@...nel.org> wrote:

> Add the initial documentation of the Nova project.
> 
> The initial project documentation consists out of a brief introduction
> of the project, as well as project guidelines both general and nova-core
> specific and a task list for nova-core specifically.
> 
> The task list is divided into tasks for general Rust infrastructure
> required by the project, tasks regarding GSP enablement and firmware
> abstraction, general GPU driver tasks as well as tasks related to
> external API design and test infrastructure.
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>   - Add task "Generic register abstraction".
>   - Change complexity of "Debugfs abstractions".
> ---
>  Documentation/gpu/drivers.rst              |   1 +
>  Documentation/gpu/nova/core/guidelines.rst |  24 ++
>  Documentation/gpu/nova/core/todo.rst       | 445 +++++++++++++++++++++
>  Documentation/gpu/nova/guidelines.rst      |  73 ++++
>  Documentation/gpu/nova/index.rst           |  30 ++
>  MAINTAINERS                                |   1 +
>  6 files changed, 574 insertions(+)
>  create mode 100644 Documentation/gpu/nova/core/guidelines.rst
>  create mode 100644 Documentation/gpu/nova/core/todo.rst
>  create mode 100644 Documentation/gpu/nova/guidelines.rst
>  create mode 100644 Documentation/gpu/nova/index.rst
> 
> diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
> index 1f17ad0790d7..7c2c5dcb5fd4 100644
> --- a/Documentation/gpu/drivers.rst
> +++ b/Documentation/gpu/drivers.rst
> @@ -24,6 +24,7 @@ GPU Driver Documentation
>     panfrost
>     panthor
>     zynqmp
> +   nova/index
>  
>  .. only::  subproject and html
>  
> diff --git a/Documentation/gpu/nova/core/guidelines.rst b/Documentation/gpu/nova/core/guidelines.rst
> new file mode 100644
> index 000000000000..a389d65d7982
> --- /dev/null
> +++ b/Documentation/gpu/nova/core/guidelines.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +==========
> +Guidelines
> +==========
> +
> +This documents contains the guidelines for nova-core. Additionally, all common
> +guidelines of the Nova project do apply.
> +
> +Driver API
> +==========
> +
> +One main purpose of nova-core is to implement the abstraction around the
> +firmware interface of GSP and provide a firmware (version) independent API for
> +2nd level drivers, such as nova-drm or the vGPU manager VFIO driver.
> +
> +Therefore, it is not permitted to leak firmware (version) specifics, through the
> +driver API, to 2nd level drivers.
> +
> +Acceptance Criteria
> +===================
> +
> +- To the extend possible, patches submitted to nova-core must be tested for
> +  regressions with all 2nd level drivers.
> diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
> new file mode 100644
> index 000000000000..5e66ec35c5e3
> --- /dev/null
> +++ b/Documentation/gpu/nova/core/todo.rst
> @@ -0,0 +1,445 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +=========
> +Task List
> +=========
> +

...

> +
> +Generic register abstraction
> +----------------------------
> +
> +Work out how register constants and structures can be automatically generated
> +through generalized macros.
> +
> +Example:
> +
> +.. code-block:: rust
> +
> +	register!(BOOT0, 0x0, u32, pci::Bar<SIZE>, Fields [
> +	   MINOR_REVISION(3:0, RO),
> +	   MAJOR_REVISION(7:4, RO),
> +	   REVISION(7:0, RO), // Virtual register combining major and minor rev.
> +	])
> +

I think it is better not to tie this to pci::Bar and its operations. It
would be better to have a intermediate container as the macro param. The
container holds the register region vaddr pointer, size, read/write traits.
The macro expands it from there, thus, we can also use this on firmware
memory structures, e.g. GSP WPR2 info.

Probably we are looking for a even more generic solution/type for deferring
a structure in the vaddr and generating the the accessing methods
accordingly. It might also be useful later in GSP message queue
manipulation, ELF header extraction, page table manipulation? (to avoid
ambitious unsafe statements in the rust driver)

> +This could expand to something like:
> +
> +.. code-block:: rust
> +
> +	const BOOT0_OFFSET: usize = 0x00000000;
> +	const BOOT0_MINOR_REVISION_SHIFT: u8 = 0;
> +	const BOOT0_MINOR_REVISION_MASK: u32 = 0x0000000f;
> +	const BOOT0_MAJOR_REVISION_SHIFT: u8 = 4;
> +	const BOOT0_MAJOR_REVISION_MASK: u32 = 0x000000f0;
> +	const BOOT0_REVISION_SHIFT: u8 = BOOT0_MINOR_REVISION_SHIFT;
> +	const BOOT0_REVISION_MASK: u32 = BOOT0_MINOR_REVISION_MASK | BOOT0_MAJOR_REVISION_MASK;
> +
> +	struct Boot0(u32);
> +
> +	impl Boot0 {
> +	   #[inline]
> +	   fn read(bar: &RevocableGuard<'_, pci::Bar<SIZE>>) -> Self {
> +	      Self(bar.readl(BOOT0_OFFSET))
> +	   }
> +
> +	   #[inline]
> +	   fn minor_revision(&self) -> u32 {
> +	      (self.0 & BOOT0_MINOR_REVISION_MASK) >> BOOT0_MINOR_REVISION_SHIFT
> +	   }
> +
> +	   #[inline]
> +	   fn major_revision(&self) -> u32 {
> +	      (self.0 & BOOT0_MAJOR_REVISION_MASK) >> BOOT0_MAJOR_REVISION_SHIFT
> +	   }
> +
> +	   #[inline]
> +	   fn revision(&self) -> u32 {
> +	      (self.0 & BOOT0_REVISION_MASK) >> BOOT0_REVISION_SHIFT
> +	   }
> +	}
> +
> +Usage:
> +
> +.. code-block:: rust
> +
> +	let bar = bar.try_access().ok_or(ENXIO)?;
> +
> +	let boot0 = Boot0::read(&bar);
> +	pr_info!("Revision: {}\n", boot0.revision());
> +
> +| Complexity: Advanced
> +
> +Delay / Sleep abstractions
> +--------------------------

...

> +
> +VRAM memory allocator
> +---------------------
> +
> +Investigate options for a VRAM memory allocator.
> +
> +Some possible options:
> +  - Rust abstractions for
> +    - RB tree (interval tree) / drm_mm
> +    - maple_tree
> +  - native Rust collections
> +
> +| Complexity: Advanced
> +

I am leaning towards having the abstractions at a high level APIs, e.g.
wrapping drm_mm and possibly the rust side can choose the backend type of
drm_mm if it really needs a different type of data structure other
than default supported by drm_mm. If we need more type of data structures,
we can extend drm_mm in C side. That can save us some efforts.

> +Instance Memory
> +---------------
> +
> +Implement support for instmem (bar2) used to store page tables.
> +
> +| Complexity: Intermediate
> +| Contact: Dave Airlie
> +
> +GPU System Processor (GSP)
> +==========================

...

> +
> +External APIs
> +=============
> +
> +nova-core base API
> +------------------
> +
> +Work out the common pieces of the API to connect 2nd level drivers, i.e. vGPU
> +manager and nova-drm.
> +
> +| Complexity: Advanced
> +
> +vGPU manager API
> +----------------
> +
> +Work out the API parts required by the vGPU manager, which are not covered by
> +the base API.
> +
> +| Complexity: Advanced
> +
> +nova-core C API
> +---------------
> +
> +Implement a C wrapper for the APIs required by the vGPU manager driver.
> +
> +| Complexity: Intermediate

Thanks for calling this out.

I believe the "vGPU manager API" is not a standalone task, as many of the
required APIs will intersect with other components in nova-core.

As one of nova-core’s users, vGPU represents the simplest use case to get
started with, offering significant value to both nova-core and its users
in the near term.

I was thinking that if we could align with the folks on making vGPU +
nova-core our initial short-term goal, it would be beneficial for
nova-drm's development, since the APIs required for nova-drm are a
superset of those needed for vGPU.

It would be valuable for us to be involved in key areas related to vGPU,
including:

- Task review
- Design discussions
- Unit testing

Additionally, we are working on a vGPU requirements document that will
outline the complete API needs for vGPU beyond those already covered in
the RFC patches. Hope that will be published soon.

> +
> +Testing
> +=======
> +
> +CI pipeline
> +-----------
> +
> +Investigate option for continuous integration testing.
> +
> +This can go from as simple as running KUnit tests over running (graphics) CTS to
> +booting up (multiple) guest VMs to test VFIO use-cases.
> +
> +It might also be worth to consider the introduction of a new test suite directly
> +sitting on top of the uAPI for more targeted testing and debugging. There may be
> +options for collaboration / shared code with the Mesa project.
> +
> +| Complexity: Advanced
> diff --git a/Documentation/gpu/nova/guidelines.rst b/Documentation/gpu/nova/guidelines.rst
> new file mode 100644
> index 000000000000..28a959f51c2c
> --- /dev/null
> +++ b/Documentation/gpu/nova/guidelines.rst
> @@ -0,0 +1,73 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +

I think this will develop into a maintainer handbook in the future which
reflects maintainer's requirements, thoughts, tips...maybe we can make it
explicit? I think it is rules of book that we agree to follow.

A similar one can be found here.
https://lore.kernel.org/kvm/20230411171651.1067966-1-seanjc@google.com/

> +==========
> +Guidelines
> +==========
> +
> +This document describes the general project guidelines that apply to nova-core
> +and nova-drm.
> +
> +Language
> +========
> +
> +The Nova project uses the Rust programming language. In this context, the
> +following rules apply.
> +
> +- Unless technically necessary otherwise (e.g. uAPI), any driver code is written
> +  in Rust.
> +
> +- Direct FFI calls to C kernel APIs must be avoided; instead C kernel APIs
> +  should be accessed through shared Rust abstractions.
> +
> +- Unless technically necessary, unsafe Rust code must be avoided. In case of
> +  technical necessity, unsafe code should be isolated in a separate component
> +  providing a safe API for other driver code to use.

Also need to comment why the unsafe is the last possible approach to go.
the last thing we want to see is "unsafe" flying here and there in a rust
driver. :)

> +
> +Style
> +-----
> +
> +All rules of the Rust for Linux project as documented in
> +:doc:`../../rust/coding-guidelines` apply. Additionally, the following rules
> +apply.
> +
> +- Code must be formatted with the ``rustfmt`` make target.
> +
> +- Code submitted for inclusion into the Nova driver project must pass the Rust
> +  linter, which can be enabled with ``CLIPPY=1``.
> +

It would be also helpful to make the process explicit. E.g. sharing your
command lines used to checking the patches. So folks can align with the
expected outcome, e.g. command line parameters.

Z.

> +Documentation
> +=============
> +
> +The availability of proper documentation is essential in terms of scalability,
> +accessability for new contributors and maintainability of a project in general,
> +but especially for a driver running as complex hardware as Nova is targeting.
> +
> +Hence, adding documentation of any kind is very much encouraged by the project.
> +
> +Besides that, there are some minimum requirements.
> +
> +- Every non-private structure needs at least a brief doc comment explaining the
> +  semantical sense of the structure, as well as potential locking and lifetime
> +  requirements. It is encouraged to have the same minimum documentation for
> +  non-trivial private structures.
> +
> +- uAPIs must be fully documented with kernel-doc comments; additionally, the
> +  semantical behavior must be explained including potential special or corner
> +  cases.
> +
> +- The APIs connecting the 1st level driver (nova-core) with 2nd level drivers
> +  must be fully documented. This includes doc comments, potential locking and
> +  lifetime requirements, as well as example code if applicable.
> +
> +- Abbreviations must be explained when introduced; terminology must be uniquely
> +  defined.
> +
> +- Register addresses, layouts, shift values and masks must be defined properly;
> +  unless obvious, the semantical sense must be documented. This only applies if
> +  the author is able to obtain the corresponding information.
> +
> +Acceptance Criteria
> +===================
> +
> +- Patches must only be applied if reviewed by at least one other person on the
> +  mailing list; this also applies for maintainers.
> diff --git a/Documentation/gpu/nova/index.rst b/Documentation/gpu/nova/index.rst
> new file mode 100644
> index 000000000000..2701b3f4af35
> --- /dev/null
> +++ b/Documentation/gpu/nova/index.rst
> @@ -0,0 +1,30 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +=======================
> +nova NVIDIA GPU drivers
> +=======================
> +
> +The nova driver project consists out of two separate drivers nova-core and
> +nova-drm and intends to supersede the nouveau driver for NVIDIA GPUs based on
> +the GPU System Processor (GSP).
> +
> +The following documents apply to both nova-core and nova-drm.
> +
> +.. toctree::
> +   :titlesonly:
> +
> +   guidelines
> +
> +nova-core
> +=========
> +
> +The nova-core driver is the core driver for NVIDIA GPUs based on GSP. nova-core,
> +as the 1st level driver, provides an abstraction around the GPUs hard- and
> +firmware interfaces providing a common base for 2nd level drivers, such as the
> +vGPU manager VFIO driver and the nova-drm driver.
> +
> +.. toctree::
> +   :titlesonly:
> +
> +   core/guidelines
> +   core/todo
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f7ddca7de0ef..07455c945834 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7454,6 +7454,7 @@ Q:	https://patchwork.freedesktop.org/project/nouveau/
>  B:	https://gitlab.freedesktop.org/drm/nova/-/issues
>  C:	irc://irc.oftc.net/nouveau
>  T:	git https://gitlab.freedesktop.org/drm/nova.git nova-next
> +F:	Documentation/gpu/nova/
>  F:	drivers/gpu/nova-core/
>  
>  DRM DRIVER FOR OLIMEX LCD-OLINUXINO PANELS

Powered by blists - more mailing lists