Re: [PATCH v6 05/10] accel/rocket: Add a new driver for Rockchip's NPU

[Robin Murphy <robin.murphy@arm.com>]

[ Since Daniel made me look... ]

On 2025-06-04 8:57 am, Tomeu Vizoso wrote:
[...]
> diff --git a/drivers/accel/rocket/Kconfig b/drivers/accel/rocket/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..9a59c6c61bf4d6460d8008b16331f001c97de67d
> --- /dev/null
> +++ b/drivers/accel/rocket/Kconfig
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config DRM_ACCEL_ROCKET
> +       tristate "Rocket (support for Rockchip NPUs)"
> +       depends on DRM
> +       depends on ARM64 || COMPILE_TEST

Best make that "(ARCH_ROCKCHIP && ARM64) || COMPILE_TEST" now before 
someone else inevitably does. Or perhaps just a pre-emptive 
"ARCH_ROCKCHIP || COMPILE_TEST" if this is the same NPU that's in RV1126 
etc.

> +       depends on MMU
> +       select DRM_SCHED
> +       select IOMMU_SUPPORT

Selecting user-visible symbols is often considered bad form, but this 
one isn't even functional - all you're doing here is forcing the 
top-level availability of all the IOMMU driver/API options.

If you really want to nanny the user and dissuade them from building a 
config which is unlikely to be useful in practice, then at best maybe 
"depends on ROCKCHIP_IOMMU || COMPILE_TEST", but TBH I wouldn't even 
bother with that. Even if you want to rely on using the IOMMU client API 
unconditionally, it'll fail decisively enough at runtime if there's no 
IOMMU present (or the API is stubbed out entirely).

> +       select IOMMU_IO_PGTABLE_LPAE

And I have no idea what this might think it's here for :/

Thanks,
Robin.

> +       select DRM_GEM_SHMEM_HELPER
> +       help
> +	  Choose this option if you have a Rockchip SoC that contains a
> +	  compatible Neural Processing Unit (NPU), such as the RK3588. Called by
> +	  Rockchip either RKNN or RKNPU, it accelerates inference of neural
> +	  networks.
> +
> +	  The interface exposed to userspace is described in
> +	  include/uapi/drm/rocket_accel.h and is used by the Rocket userspace
> +	  driver in Mesa3D.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rocket.