Re: [PATCH v2 2/2] gpu: nova-core: add initial documentation

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

On Wed Feb 5, 2025 at 10:56 PM JST, Zhi Wang wrote:
> On Tue,  4 Feb 2025 20:03:12 +0100
> Danilo Krummrich <dakr@kernel.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@kernel.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.

Another reason for not tying this to a particular bus is that Tegra
doesn't use PCI to reach the registers of its integrated GPU. Maybe we
can remove that parameter from the register!() macro and have read()
take a generic argument for its `bar` parameter instead, so that method
gets automatically specialized for every type of bus we need to use?