| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f96b313a-fa0d-45d6-b358-8d2da40a9b95@oss.qualcomm.com>
Date: Fri, 21 Mar 2025 09:48:04 -0600
From: Jeff Hugo <jeff.hugo@....qualcomm.com>
To: Tomeu Vizoso <tomeu@...euvizoso.net>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
Oded Gabbay <ogabbay@...nel.org>, Jonathan Corbet <corbet@....net>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Sumit Semwal <sumit.semwal@...aro.org>,
Christian König <christian.koenig@....com>,
Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org,
linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v2 4/7] accel/rocket: Add a new driver for Rockchip's NPU
On 2/25/2025 12:55 AM, Tomeu Vizoso wrote:
>
> diff --git a/Documentation/accel/rocket/index.rst b/Documentation/accel/rocket/index.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..ad33194dec0325d0dab362768fd349e8dc286970
> --- /dev/null
> +++ b/Documentation/accel/rocket/index.rst
> @@ -0,0 +1,19 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=====================================
> + accel/rocket Rockchip NPU driver
> +=====================================
> +
> +The accel/rocket driver supports the Neural Processing Units (NPUs) inside some Rockchip SoCs such as the RK3588. Rockchip calls it RKNN and sometimes RKNPU.
> +
> +This NPU is closely based on the NVDLA IP released by NVIDIA as open hardware in 2018, along with open source kernel and userspace drivers.
> +
> +The frontend unit in Rockchip's NPU though is completely different from that in the open source IP, so this kernel driver is specific to Rockchip's version.
> +
> +The hardware is described in chapter 36 in the RK3588 TRM.
> +
> +This driver just powers the hardware on and off, allocates and maps buffers to the device and submits jobs to the frontend unit. Everything else is done in userspace, as a Gallium driver that is part of the Mesa3D project.
Nit: name the specific Gallium driver here?
> diff --git a/drivers/accel/rocket/Kconfig b/drivers/accel/rocket/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..83a401129ab2dc2847ccc30c6364e8802f43648d
> --- /dev/null
> +++ b/drivers/accel/rocket/Kconfig
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: GPL-2.0
I'm curious, is there a specific reason for "GPL-2.0"? This tag is
depreciated and everywhere else in the accel subsystem "GPL-2.0-only" is
used.
Same comment for the Makefile and everything else in this patch.
> +
> +config DRM_ACCEL_ROCKET
> + tristate "Rocket (support for Rockchip NPUs)"
> + depends on DRM
> + depends on ARM64 || COMPILE_TEST
> + depends on MMU
> + select DRM_SCHED
> + select IOMMU_SUPPORT
> + select IOMMU_IO_PGTABLE_LPAE
> + 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 userspace driver in
> + Mesa3D.
Nit: name the specific Mesa driver here?
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called rocket.
> diff --git a/drivers/accel/rocket/rocket_core.c b/drivers/accel/rocket/rocket_core.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..09d966c826b5b1090a18cb24b3aa4aba286a12d4
> --- /dev/null
> +++ b/drivers/accel/rocket/rocket_core.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2024 Tomeu Vizoso <tomeu@...euvizoso.net> */
> +
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
This list of includes seems insufficient. You use PTR_ERR but don't have
the include for it (linux/err.h). Same for dev_err/dev_info. I'm
guessing this comment applies elsewhere.
> +#include "rocket_core.h"
> +#include "rocket_registers.h"
> +
> +static int rocket_clk_init(struct rocket_core *core)
> +{
> + struct device *dev = core->dev;
> + int err;
> +
> + core->a_clk = devm_clk_get(dev, "aclk");
> + if (IS_ERR(core->a_clk)) {
> + err = PTR_ERR(core->a_clk);
> + dev_err(dev, "devm_clk_get_enabled failed %d for core %d\n", err, core->index);
Do you think it would be useful to mention "aclk" in the message? Seems
like if this or the hclk get fails, you won't know just from the kernel log.
> + return err;
> + }
> +
> + core->h_clk = devm_clk_get(dev, "hclk");
> + if (IS_ERR(core->h_clk)) {
> + err = PTR_ERR(core->h_clk);
> + dev_err(dev, "devm_clk_get_enabled failed %d for core %d\n", err, core->index);
> + clk_disable_unprepare(core->a_clk);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +int rocket_core_init(struct rocket_core *core)
> +{
> + struct device *dev = core->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + uint32_t version;
> + int err = 0;
> +
> + err = rocket_clk_init(core);
> + if (err) {
> + dev_err(dev, "clk init failed %d\n", err);
This feels redundant since rocket_clk_init() already printed an error
message with more details.
> + return err;
> + }
> +
> + core->iomem = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(core->iomem))
> + return PTR_ERR(core->iomem);
> +
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_autosuspend_delay(dev, 50); /* ~3 frames */
Frames? That feels like a rendering term, but this driver is not doing
rendering, right? Is there a better way to describe this?
> + pm_runtime_enable(dev);
> +
> + err = pm_runtime_get_sync(dev);
> +
> + version = rocket_read(core, REG_PC_VERSION);
> + version += rocket_read(core, REG_PC_VERSION_NUM) & 0xffff;
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + dev_info(dev, "Rockchip NPU core %d version: %d\n", core->index, version);
> +
> + return 0;
> +}
> +
> diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ce3b533f15c1011d8a7a23dd8132e907cc334c58
> --- /dev/null
> +++ b/drivers/accel/rocket/rocket_device.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2024 Tomeu Vizoso <tomeu@...euvizoso.net> */
> +
> +#include <linux/clk.h>
> +
> +#include "rocket_device.h"
> +
> +int rocket_device_init(struct rocket_device *rdev)
> +{
> + struct device *dev = rdev->cores[0].dev;
> + int err;
> +
> + rdev->clk_npu = devm_clk_get(dev, "npu");
> + rdev->pclk = devm_clk_get(dev, "pclk");
Are these optional? It would appear that error handling is missing.
> +
> + /* Initialize core 0 (top) */
> + err = rocket_core_init(&rdev->cores[0]);
> + if (err) {
> + rocket_device_fini(rdev);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static void rocket_drm_unbind(struct device *dev)
> +{
> + struct rocket_device *rdev = dev_get_drvdata(dev);
> + struct drm_device *ddev = &rdev->ddev;
> +
> + drm_dev_unregister(ddev);
You allocated this with devm, shouldn't that make this unnecessary?
> +
> + component_unbind_all(dev, rdev);
> +
> + rocket_device_fini(rdev);
> +}
Powered by blists - more mailing lists