| 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: <20250219113422.GA26386@robin.jannau.net>
Date: Wed, 19 Feb 2025 12:34:22 +0100
From: Janne Grunau <j@...nau.net>
To: fnkl.kernel@...il.com
Cc: Sven Peter <sven@...npeter.dev>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Hector Martin <marcan@...can.st>,
Ulf Hansson <ulf.hansson@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, asahi@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
linux-media@...r.kernel.org, imx@...ts.linux.dev,
Eileen Yoon <eyn@....com>, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH 4/5] media: apple: Add Apple ISP driver
Hej,
On Wed, Feb 19, 2025 at 10:27:00AM +0100, Sasha Finkelstein via B4 Relay wrote:
> From: Eileen Yoon <eyn@....com>
>
> This is the ISP and camera module present on certain Apple laptops
>
> Signed-off-by: Eileen Yoon <eyn@....com>
> Co-developed-by: Hector Martin <marcan@...can.st>
> Signed-off-by: Hector Martin <marcan@...can.st>
> Co-developed-by: Asahi Lina <lina@...hilina.net>
> Signed-off-by: Asahi Lina <lina@...hilina.net>
> Co-developed-by: Janne Grunau <j@...nau.net>
> Signed-off-by: Janne Grunau <j@...nau.net>
> Co-developed-by: Sasha Finkelstein <fnkl.kernel@...il.com>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@...il.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/apple/Kconfig | 5 +
> drivers/media/platform/apple/Makefile | 3 +
> drivers/media/platform/apple/isp/Kconfig | 16 +
> drivers/media/platform/apple/isp/Makefile | 3 +
> drivers/media/platform/apple/isp/isp-cam.c | 414 ++++++++++++
> drivers/media/platform/apple/isp/isp-cam.h | 21 +
> drivers/media/platform/apple/isp/isp-cmd.c | 635 +++++++++++++++++++
> drivers/media/platform/apple/isp/isp-cmd.h | 692 ++++++++++++++++++++
> drivers/media/platform/apple/isp/isp-drv.c | 586 +++++++++++++++++
> drivers/media/platform/apple/isp/isp-drv.h | 284 +++++++++
> drivers/media/platform/apple/isp/isp-fw.c | 770 +++++++++++++++++++++++
> drivers/media/platform/apple/isp/isp-fw.h | 24 +
> drivers/media/platform/apple/isp/isp-iommu.c | 251 ++++++++
> drivers/media/platform/apple/isp/isp-iommu.h | 20 +
> drivers/media/platform/apple/isp/isp-ipc.c | 258 ++++++++
> drivers/media/platform/apple/isp/isp-ipc.h | 25 +
> drivers/media/platform/apple/isp/isp-regs.h | 56 ++
> drivers/media/platform/apple/isp/isp-v4l2.c | 900 +++++++++++++++++++++++++++
> drivers/media/platform/apple/isp/isp-v4l2.h | 16 +
> 22 files changed, 4982 insertions(+)
quick partial review
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dea7239ee0f5464b31efed5a2e0e5e602bcb6439..60517f7dcee14fc942dd3f77ed5d58eae394f7fa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2248,6 +2248,7 @@ F: drivers/i2c/busses/i2c-pasemi-platform.c
> F: drivers/iommu/apple-dart.c
> F: drivers/iommu/io-pgtable-dart.c
> F: drivers/irqchip/irq-apple-aic.c
> +F: drivers/media/platform/apple/*
> F: drivers/nvme/host/apple.c
> F: drivers/nvmem/apple-efuses.c
> F: drivers/pinctrl/pinctrl-apple-gpio.c
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 85d2627776b6a424fbd392187669535c4159ec97..ba75cfdb57f710cca086136e4524d3e1bc1910ac 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -65,6 +65,7 @@ config VIDEO_MUX
> source "drivers/media/platform/allegro-dvt/Kconfig"
> source "drivers/media/platform/amlogic/Kconfig"
> source "drivers/media/platform/amphion/Kconfig"
> +source "drivers/media/platform/apple/Kconfig"
> source "drivers/media/platform/aspeed/Kconfig"
> source "drivers/media/platform/atmel/Kconfig"
> source "drivers/media/platform/broadcom/Kconfig"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index ace4e34483ddce6c3361479989086145dd495f29..e59e4259064bf04b718ea8d128031af859a13d2e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -8,6 +8,7 @@
> obj-y += allegro-dvt/
> obj-y += amlogic/
> obj-y += amphion/
> +obj-y += apple/
> obj-y += aspeed/
> obj-y += atmel/
> obj-y += broadcom/
> diff --git a/drivers/media/platform/apple/Kconfig b/drivers/media/platform/apple/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..f16508bff5242a0bc433bf8a1d8e3f29737d20d1
> --- /dev/null
> +++ b/drivers/media/platform/apple/Kconfig
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +comment "Apple media platform drivers"
> +
> +source "drivers/media/platform/apple/isp/Kconfig"
> diff --git a/drivers/media/platform/apple/Makefile b/drivers/media/platform/apple/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..d8fe985b0e6c377de6c77d30a3a796c40f3da116
> --- /dev/null
> +++ b/drivers/media/platform/apple/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-y += isp/
> diff --git a/drivers/media/platform/apple/isp/Kconfig b/drivers/media/platform/apple/isp/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..8e94962990031304d51cdd7cd6190b05b05b40bb
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config VIDEO_APPLE_ISP
> + tristate "Apple Silicon Image Signal Processor driver"
> + select VIDEOBUF2_CORE
> + select VIDEOBUF2_V4L2
> + select VIDEOBUF2_DMA_SG
> + depends on ARCH_APPLE || COMPILE_TEST
> + depends on V4L_PLATFORM_DRIVERS
> + depends on VIDEO_DEV
missing dependency on DRM for drm_mm. I don't remember why iova's top
down allocation was a problem but if we can avoid drm_mm that would be
an option as well.
> + help
> + Say Y here to enable support for the ISP and cameras persent
> + in Apple ARM laptops.
> +
> + To compile this driver as a module, choose M here. The module will be
> + called apple_isp
> diff --git a/drivers/media/platform/apple/isp/Makefile b/drivers/media/platform/apple/isp/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..4649f32987f025a639945a37d774d4ecdc83b02a
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +apple-isp-y := isp-cam.o isp-cmd.o isp-drv.o isp-fw.o isp-iommu.o isp-ipc.o isp-v4l2.o
> +obj-$(CONFIG_VIDEO_APPLE_ISP) += apple-isp.o
...
> diff --git a/drivers/media/platform/apple/isp/isp-drv.c b/drivers/media/platform/apple/isp/isp-drv.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b0c73b4f43d73f4ee29093fe62ed1d39ccfa33dd
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/isp-drv.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Apple Image Signal Processor driver
> + *
> + * Copyright (C) 2023 The Asahi Linux Contributors
> + */
> +
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/workqueue.h>
> +
> +#include "isp-cam.h"
> +#include "isp-fw.h"
> +#include "isp-iommu.h"
> +#include "isp-v4l2.h"
> +
...
> +static int apple_isp_init_iommu(struct apple_isp *isp)
> +{
> + struct device *dev = isp->dev;
> + phys_addr_t heap_base;
> + size_t heap_size;
> + u64 vm_size;
> + int err;
> + int size;
> + struct device_node *mem_node;
> + const __be32 *maps, *end;
> +
> + isp->domain = iommu_get_domain_for_dev(isp->dev);
> + if (!isp->domain)
> + return -ENODEV;
> + isp->shift = __ffs(isp->domain->pgsize_bitmap);
> +
> + mem_node = of_parse_phandle(dev->of_node, "memory-region", 0);
> + if (!mem_node) {
> + dev_err(dev, "No memory-region found for heap\n");
> + return -ENODEV;
> + }
> +
> + maps = of_get_property(mem_node, "iommu-addresses", &size);
> + if (!maps || !size) {
> + dev_err(dev, "No valid iommu-addresses found for heap\n");
> + return -ENODEV;
> + }
> +
> + end = maps + size / sizeof(__be32);
> +
> + while (maps < end) {
> + maps++;
> + maps = of_translate_dma_region(dev->of_node, maps, &heap_base,
> + &heap_size);
> + }
The hand-rolled reserved memory parsing looks like it can be replaced
with of_iommu_get_resv_region();
> +
> + isp->fw.heap_top = heap_base + heap_size;
> +
> + err = of_property_read_u64(dev->of_node, "apple,dart-vm-size",
> + &vm_size);
This is not necessary and can be inferred from
isp->domain->geometry.aperture_{start,end}.
> + if (err) {
> + dev_err(dev, "failed to read 'apple,dart-vm-size': %d\n", err);
> + return err;
> + }
> +
> + drm_mm_init(&isp->iovad, isp->fw.heap_top,
> + vm_size - (heap_base & 0xffffffff));
drm_mm probably should be replaced with something else. As I wrote above
I don't understand / remember what the problem was with relying on the
DMA api and iova. I can't imagine that the firmware cares about top-down
vs. bottom-up allocation. I could image it was related to
"apple,dart-vm-size" and iova's top down allocation only allocated
outside of the device's aperture. That should be solved by our current
downstream handling of the "vm-base" and "vm-size" properties from
Apple's devicetree.
ciao Janne
Powered by blists - more mailing lists