* [PATCH v2 6/8] PCI: aardvark: Add 100 ms delay after link training
From: Hans Zhang @ 2026-05-06 15:23 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, jianjun.wang,
claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, linux-mediatek,
linux-renesas-soc, linux-pci, linux-kernel, Hans Zhang
In-Reply-To: <20260506152346.166056-1-18255117159@163.com>
The Aardvark PCIe controller driver waits for the link to come up but
does not implement the mandatory 100 ms delay after link training
completes for speeds greater than 5.0 GT/s (PCIe r6.0 sec 6.6.1).
The driver already maintains a 'link_gen' field that holds the negotiated
link speed. Use it together with pcie_wait_after_link_train() to insert
the required delay immediately after confirming that the link is up.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/pci-aardvark.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e34bea1ff0ac..526351c21c49 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -350,8 +350,10 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
/* check if the link is up or not */
for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
- if (advk_pcie_link_up(pcie))
+ if (advk_pcie_link_up(pcie)) {
+ pcie_wait_after_link_train(pcie->link_gen);
return 0;
+ }
usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
}
--
2.34.1
^ permalink raw reply related
* [PATCH v2 1/8] PCI: Add pcie_wait_after_link_train() helper
From: Hans Zhang @ 2026-05-06 15:23 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, jianjun.wang,
claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, linux-mediatek,
linux-renesas-soc, linux-pci, linux-kernel, Hans Zhang
In-Reply-To: <20260506152346.166056-1-18255117159@163.com>
PCIe r6.0, sec 6.6.1 (Conventional Reset) requires that for a Downstream
Port supporting Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending any
Configuration Request.
Introduce a static inline helper pcie_wait_after_link_train() that checks
the given max_link_speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, etc.) and calls
msleep(100) only when the speed is greater than 5.0 GT/s. The helper uses
the existing PCIE_RESET_CONFIG_WAIT_MS macro defined in pci.h.
This allows multiple host controller drivers to share the same mandatory
delay without duplicating the logic.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/pci.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4a14f88e543a..a8705a2a2d85 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -60,6 +60,19 @@ struct pcie_tlp_log;
*/
#define PCIE_RESET_CONFIG_WAIT_MS 100
+/**
+ * pcie_wait_after_link_train - Wait 100 ms if link speed > 5 GT/s
+ * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, ...)
+ *
+ * Must be called after Link training completes and before the first
+ * Configuration Request is sent.
+ */
+static inline void pcie_wait_after_link_train(int max_link_speed)
+{
+ if (max_link_speed > 2)
+ msleep(PCIE_RESET_CONFIG_WAIT_MS);
+}
+
/* Parameters for the waiting for link up routine */
#define PCIE_LINK_WAIT_MAX_RETRIES 10
#define PCIE_LINK_WAIT_SLEEP_MS 90
--
2.34.1
^ permalink raw reply related
* [PATCH v2 4/8] PCI: j721e: Set max_link_speed to enable 100 ms delay after link up
From: Hans Zhang @ 2026-05-06 15:23 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, jianjun.wang,
claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, linux-mediatek,
linux-renesas-soc, linux-pci, linux-kernel, Hans Zhang
In-Reply-To: <20260506152346.166056-1-18255117159@163.com>
Set cdns_pcie.max_link_speed to the maximum supported link speed
(obtained from the device tree property "max-link-speed") in
j721e_pcie_set_link_speed(). This activates the post-link delay logic
added in cdns_pcie_host_start_link() when the controller supports
speeds greater than 5 GT/s.
As required by PCIe r6.0 sec 6.6.1, and following the same approach as
commit 80dc18a0cba8d ("PCI: dwc: Ensure that dw_pcie_wait_for_link()
waits 100 ms after link up"), this ensures a 100 ms delay after link
training completes before any Configuration Request is sent.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/cadence/pci-j721e.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index bfdfe98d5aba..ee85b8e04f5b 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -206,6 +206,7 @@ static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
(pcie_get_link_speed(link_speed) == PCI_SPEED_UNKNOWN))
link_speed = 2;
+ pcie->cdns_pcie.max_link_speed = link_speed;
val = link_speed - 1;
ret = regmap_update_bits(syscon, offset, GENERATION_SEL_MASK, val);
if (ret)
--
2.34.1
^ permalink raw reply related
* [PATCH v2 0/8] PCI: Add common helper for 100 ms delay after link training
From: Hans Zhang @ 2026-05-06 15:23 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, jianjun.wang,
claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, linux-mediatek,
linux-renesas-soc, linux-pci, linux-kernel, Hans Zhang
PCIe r6.0, sec 6.6.1 (Conventional Reset) states:
- For a Downstream Port that supports Link speeds greater than 5.0 GT/s,
software must wait a minimum of 100 ms **after Link training completes**
before sending a Configuration Request to the device immediately below
that Port.
Several PCIe host controller drivers currently omit this 100 ms delay
when the negotiated link speed is Gen3 (8 GT/s) or higher. Only the DWC
driver already implements it. The missing delay can lead to violations
of the PCIe specification.
To fix this consistently and avoid code duplication, this series:
1. Adds a static inline helper `pcie_wait_after_link_train()` in
drivers/pci/pci.h. The helper checks the given max_link_speed
(or negotiated speed) and calls msleep(100) if the speed is > 5 GT/s.
2. Converts the DWC driver to use this helper.
3. Adds the missing 100 ms delay to the Cadence PCIe controller
(both LGA - Legacy Architecture IP - and HPA - High Performance
Architecture IP) after introducing a `max_link_speed` field in
struct cdns_pcie.
4. Adds the delay to the Aardvark, MediaTek Gen3, and Renesas RZ/G3S
host drivers, reusing their existing link speed fields.
All changes are placed exactly where the driver has just finished
waiting for the link to come up, i.e., immediately after link training
completes and before any Configuration Request would be issued.
---
Our company's product is based on the HPA IP from Cadence. When connecting
to different devices, we encountered issues with the enumeration failure
when connecting to the NVIDIA RTX5070 GPU and the NVMe SSD with PCIe 5.0
interface. Our code is based on: 80dc18a0cba8d ("PCI: dwc: Ensure that
dw_pcie_wait_for_link() waits 100 ms after link up").
---
Changes since v2:
- Add pcie_wait_after_link_train() helper
- Reduce repetitive code comments and have each Root Port driver use the
helper function instead.
- Increase the delay to 100ms after enabling the link-up that distinguishes
between Cadence LGA and HPA IPs.
- Add the Aardvark, MediaTek Gen3, and Renesas RZ/G3S Root Port driver. When
the speed is greater than GEN2, a delay of 100ms should be applied.
v1:
https://patchwork.kernel.org/project/linux-pci/patch/20260501153553.66382-1-18255117159@163.com/
---
Hans Zhang (8):
PCI: Add pcie_wait_after_link_train() helper
PCI: cadence: LGA: Add max_link_speed field and 100 ms delay after
link training
PCI: cadence: HPA: Add 100 ms delay after link training
PCI: j721e: Set max_link_speed to enable 100 ms delay after link up
PCI: dwc: Use common pcie_wait_after_link_train() helper
PCI: aardvark: Add 100 ms delay after link training
PCI: mediatek-gen3: Add 100 ms delay after link training
PCI: rzg3s-host: Add 100 ms delay after link training
drivers/pci/controller/cadence/pci-j721e.c | 1 +
.../controller/cadence/pcie-cadence-host-common.c | 4 ++++
.../pci/controller/cadence/pcie-cadence-host-hpa.c | 3 +++
drivers/pci/controller/cadence/pcie-cadence.h | 2 ++
drivers/pci/controller/dwc/pcie-designware.c | 8 +-------
drivers/pci/controller/pci-aardvark.c | 4 +++-
drivers/pci/controller/pcie-mediatek-gen3.c | 2 ++
drivers/pci/controller/pcie-rzg3s-host.c | 2 ++
drivers/pci/pci.h | 13 +++++++++++++
9 files changed, 31 insertions(+), 8 deletions(-)
base-commit: a293ec25d59dd96309058c70df5a4dd0f889a1e4
--
2.34.1
^ permalink raw reply
* [PATCH v2 2/8] PCI: cadence: LGA: Add max_link_speed field and 100 ms delay after link training
From: Hans Zhang @ 2026-05-06 15:23 UTC (permalink / raw)
To: bhelgaas, lpieralisi, kwilczynski, mani, vigneshr, jingoohan1,
thomas.petazzoni, pali, ryder.lee, jianjun.wang,
claudiu.beznea.uj, mpillai
Cc: robh, s-vadapalli, linux-omap, linux-arm-kernel, linux-mediatek,
linux-renesas-soc, linux-pci, linux-kernel, Hans Zhang
In-Reply-To: <20260506152346.166056-1-18255117159@163.com>
The Cadence LGA (Legacy Architecture IP) PCIe host controller currently
lacks the mandatory 100 ms delay after link training completes for speeds
> 5.0 GT/s, as required by PCIe r6.0 sec 6.6.1.
Add a 'max_link_speed' field to struct cdns_pcie to record the maximum
supported link speed (or the currently configured speed). In the common
host layer function cdns_pcie_host_start_link(), after the link has been
successfully established, call pcie_wait_after_link_train() to insert the
required delay if max_link_speed > 2.
Glue drivers must set max_link_speed appropriately (e.g., from the device
tree property "max-link-speed") to enable the delay.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/cadence/pcie-cadence-host-common.c | 4 ++++
drivers/pci/controller/cadence/pcie-cadence.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-common.c b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
index 2b0211870f02..51376f69d007 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host-common.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host-common.c
@@ -14,6 +14,7 @@
#include "pcie-cadence.h"
#include "pcie-cadence-host-common.h"
+#include "../../pci.h"
#define LINK_RETRAIN_TIMEOUT HZ
@@ -115,6 +116,9 @@ int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc,
if (!ret && rc->quirk_retrain_flag)
ret = cdns_pcie_retrain(pcie, pcie_link_up);
+ if (!ret)
+ pcie_wait_after_link_train(pcie->max_link_speed);
+
return ret;
}
EXPORT_SYMBOL_GPL(cdns_pcie_host_start_link);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 574e9cf4d003..e222b095d2b6 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -86,6 +86,7 @@ struct cdns_plat_pcie_of_data {
* @ops: Platform-specific ops to control various inputs from Cadence PCIe
* wrapper
* @cdns_pcie_reg_offsets: Register bank offsets for different SoC
+ * @max_link_speed: maximum supported link speed
*/
struct cdns_pcie {
void __iomem *reg_base;
@@ -98,6 +99,7 @@ struct cdns_pcie {
struct device_link **link;
const struct cdns_pcie_ops *ops;
const struct cdns_plat_pcie_of_data *cdns_pcie_reg_offsets;
+ int max_link_speed;
};
/**
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 6/8] drm/panthor: Explicit expansion of locked VM region
From: Nicolas Frattaroli @ 2026-05-06 15:14 UTC (permalink / raw)
To: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Boris Brezillon, Steven Price, Liviu Dudau,
Daniel Almeida, Alice Ryhl, Matthias Brugger,
AngeloGioacchino Del Regno, Ketil Johnsen, Ketil Johnsen
Cc: dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek
In-Reply-To: <20260505140516.1372388-7-ketil.johnsen@arm.com>
On Tuesday, 5 May 2026 16:05:12 Central European Summer Time Ketil Johnsen wrote:
> Currently the panthor_vm_lock_region() function will implicitly expand
> an already locked VM region. This can be problematic because the caller
> do not reliably know if it needs to call panthor_vm_unlock_region()
> or not.
>
> Worth noting, there is currently no known issues with this as the code
> is written today.
>
> This change introduces panthor_vm_expand_region() which will only work
> if there is already a locked VM region. This again means that the
> original lock and unlock functions can work as a pair. This pairing is
> needed for subsequent protected memory changes.
>
> Signed-off-by: Ketil Johnsen <ketil.johnsen@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_mmu.c | 69 +++++++++++++++++++--------
> 1 file changed, 50 insertions(+), 19 deletions(-)
>
While trying this series, I attempted my usual
`modprobe -r panthor && modprobe panthor protected_heap_name=default_cma_region`.
Unfortunately, it oopses when attempting to unmap the sg for a bo labeled
"FW section" on panthor module unload, and I bisected it to this patch.
The oops:
[ 598.515550] Unable to handle kernel paging request at virtual address 0000000000400267
[ 598.516864] Mem abort info:
[ 598.517676] ESR = 0x0000000096000004
[ 598.518560] EC = 0x25: DABT (current EL), IL = 32 bits
[ 598.520414] SET = 0, FnV = 0
[ 598.521275] EA = 0, S1PTW = 0
[ 598.522099] FSC = 0x04: level 0 translation fault
[ 598.523069] Data abort info:
[ 598.524311] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 598.525566] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 598.526850] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 598.527905] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000104056000
[ 598.529019] [0000000000400267] pgd=0000000000000000, p4d=0000000000000000
[ 598.530170] Internal error: Oops: 0000000096000004 [#1] SMP
[ 598.531158] Modules linked in: btusb btrtl btmtk btintel btbcm bluetooth ecdh_generic ecc kpp snd_soc_hdmi_codec cfg80211 r8169 rfkill_gpio pwm_fan rfkill snd_soc_es8316 rtc_hym8563 rk805_pwrkey at24 fusb302 tcpm aux_hpd_bridge display_connector snd_soc_simple_card phy_rockchip_samsung_hdptx phy_rockchip_usbdp rockchip_thermal typec phy_rockchip_naneng_combphy rockchip_saradc industrialio_triggered_buffer kfifo_buf rockchipdrm inno_hdmi dw_dp hantro_vpu rockchip_vdec dw_mipi_dsi2 v4l2_jpeg v4l2_vp9 rockchip_rga dw_mipi_dsi v4l2_h264 synopsys_hdmirx v4l2_dv_timings spi_rockchip_sfc videobuf2_dma_contig videobuf2_dma_sg v4l2_mem2mem videobuf2_memops dw_hdmi_qp onboard_usb_dev analogix_dp videobuf2_v4l2 videobuf2_common snd_soc_rockchip_i2s_tdm dw_hdmi videodev mc drm_display_helper nvme cec panthor(-) drm_gpuvm drm_exec gpu_sched drm_dp_aux_bus drm_dma_helper drm_client_lib nvme_core drm_kms_helper drm pci_endpoint_test backlight snd_soc_audio_graph_card snd_soc_simple_card_utils fuse dm_mod
[ 598.541237] CPU: 6 UID: 0 PID: 806 Comm: modprobe Not tainted 7.1.0-rc2-00726-g8ab0a3092b56-dirty #2 PREEMPT
[ 598.542733] Hardware name: Radxa ROCK 5T (DT)
[ 598.543746] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 598.544991] pc : dma_unmap_sg_attrs (kernel/dma/mapping.c:0)
[ 598.546021] lr : panthor_gem_free_object (include/linux/dma-mapping.h:565 drivers/gpu/drm/panthor/panthor_gem.c:308 drivers/gpu/drm/panthor/panthor_gem.c:469) panthor
[ 598.547180] sp : ffff80008835bb90
[ 598.548123] x29: ffff80008835bb90 x28: ffff00012610bf00 x27: 0000000000000000
[ 598.549412] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[ 598.550696] x23: ffff000127194600 x22: ffff0001271943d0 x21: ffff000118671000
[ 598.551984] x20: ffff000127194200 x19: ffff000127194600 x18: 00000000002ab980
[ 598.553273] x17: 00000000002ab980 x16: ffffa39a6b372a04 x15: 0000000000000000
[ 598.554568] x14: 0000000000000010 x13: 0000000000000000 x12: 000000000000003c
[ 598.555864] x11: 0000000000000002 x10: ffff000100a5d000 x9 : ffff000118671000
[ 598.557164] x8 : ffff000102f8b490 x7 : ffff000149569000 x6 : ffff000149569000
[ 598.558461] x5 : ffff000100faa7e8 x4 : 0000000000000000 x3 : 0000000000000000
[ 598.559763] x2 : 0000000000000010 x1 : ffff000127194000 x0 : 000000000040000f
[ 598.561069] Call trace:
[ 598.561961] dma_unmap_sg_attrs (kernel/dma/mapping.c:0) (P)
[ 598.563038] panthor_gem_free_object (include/linux/dma-mapping.h:565 drivers/gpu/drm/panthor/panthor_gem.c:308 drivers/gpu/drm/panthor/panthor_gem.c:469) panthor
[ 598.564218] drm_gem_object_free (drivers/gpu/drm/drm_gem.c:1148) drm
[ 598.565386] panthor_kernel_bo_destroy (include/linux/kref.h:65 include/drm/drm_gem.h:565 include/drm/drm_gem.h:578 drivers/gpu/drm/panthor/panthor_gem.c:1317) panthor
[ 598.566575] panthor_fw_unplug (drivers/gpu/drm/panthor/panthor_fw.c:1306) panthor
[ 598.567705] panthor_device_unplug (drivers/gpu/drm/panthor/panthor_device.c:103) panthor
[ 598.568878] panthor_remove (drivers/gpu/drm/panthor/panthor_drv.c:1846) panthor
[ 598.569991] platform_remove (drivers/base/platform.c:1435)
[ 598.571029] device_release_driver_internal (drivers/base/dd.c:619 drivers/base/dd.c:1352 drivers/base/dd.c:1375)
[ 598.572209] driver_detach (drivers/base/dd.c:1438)
[ 598.573237] bus_remove_driver (drivers/base/bus.c:825)
[ 598.574304] driver_unregister (drivers/base/driver.c:277)
[ 598.575363] platform_driver_unregister (drivers/base/platform.c:920)
[ 598.576494] cleanup_module (drivers/gpu/drm/panthor/panthor_devfreq.c:134) panthor
[ 598.577617] __arm64_sys_delete_module (kernel/module/main.c:863 kernel/module/main.c:804 kernel/module/main.c:804)
[ 598.578751] invoke_syscall (arch/arm64/kernel/syscall.c:35 arch/arm64/kernel/syscall.c:49)
[ 598.579794] el0_svc_common (arch/arm64/kernel/syscall.c:121)
[ 598.580842] do_el0_svc (arch/arm64/kernel/syscall.c:140)
[ 598.581862] el0_svc (arch/arm64/kernel/entry-common.c:723)
[ 598.582853] el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:742)
[ 598.583949] el0t_64_sync (arch/arm64/kernel/entry.S:594)
Kind regards,
Nicolas Frattaroli
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: add myself and Lorenzo Bianconi as Airoha SoC co-maintainers
From: AngeloGioacchino Del Regno @ 2026-05-06 15:12 UTC (permalink / raw)
To: Christian Marangi, Matthias Brugger, linux-kernel,
linux-arm-kernel, linux-mediatek
Cc: Lorenzo Bianconi
In-Reply-To: <20260505122416.2609-1-ansuelsmth@gmail.com>
On 5/5/26 14:24, Christian Marangi wrote:
> The development and integration of patches for the Airoha SoC stalled a
> bit lately with the result of some patches getting lost, not applied or
> even flagged as accepted, archived but never merged. The Airoha SoC, given
> the similarities with the Mediatek SoC, all goes through the mediatek tree
> and it's understandable, given the amount of traffic, that some patch for
> a different SoC are missed.
>
> To help and improve the situation, add myself and Lorenzo Bianconi as
> co-maintainers of the Airoha SoC.
>
> While at it also change the status from "Odd fixes" to "Maintained" as
> there is an active effort of maintaining this SoC.
>
> Cc: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Acked-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Boris Brezillon @ 2026-05-06 15:05 UTC (permalink / raw)
To: Maxime Ripard
Cc: Ketil Johnsen, David Airlie, Simona Vetter, Maarten Lankhorst,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260506-golden-python-of-aptitude-ff972a@houat>
On Wed, 6 May 2026 15:12:37 +0200
Maxime Ripard <mripard@kernel.org> wrote:
> On Wed, May 06, 2026 at 12:50:15PM +0200, Boris Brezillon wrote:
> > On Wed, 6 May 2026 12:08:24 +0200
> > Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > Hi,
> > >
> > > On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> > > > From: Florent Tomasin <florent.tomasin@arm.com>
> > > >
> > > > This patch allows Panthor to allocate buffer objects from a
> > > > protected heap. The Panthor driver should be seen as a consumer
> > > > of the heap and not an exporter.
> > > >
> > > > Protected memory buffers needed by the Panthor driver:
> > > > - On CSF FW load, the Panthor driver must allocate a protected
> > > > buffer object to hold data to use by the FW when in protected
> > > > mode. This protected buffer object is owned by the device
> > > > and does not belong to a process.
> > > > - On CSG creation, the Panthor driver must allocate a protected
> > > > suspend buffer object for the FW to store data when suspending
> > > > the CSG while in protected mode. The kernel owns this allocation
> > > > and does not allow user space mapping. The format of the data
> > > > in this buffer is only known by the FW and does not need to be
> > > > shared with other entities.
> > > >
> > > > The driver will retrieve the protected heap using the name of the
> > > > heap provided to the driver as module parameter.
> > >
> > > I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
> > > better in the device tree and lookup through the device node? heaps are
> > > going to have a node anyway, right?
> >
> > I'm not too sure. Take the PROTMEM (name="protected,xxxx") dma_heaps
> > instantiated by optee for instance, I don't think the originating
> > tee_device comes from a device node, nor is the underlying heap
> > described as a device node. The reserved memory pool this protected heap
> > comes from is most likely defined somewhere as reserved memory in the
> > DT, but there's nothing to correlate this range of reserved mem to some
> > sub-range that the TEE implementation is carving out to provide
> > protected memory.
>
> Maybe we should be working on a dt bindings for heaps then? Something
> simple like we have for clocks with a phandle and an ID would probably
> be enough. In optee's case, it looks like it would map nicely with
> TEE_DMA_HEAP_* flags too.
Sure.
>
> The only two that wouldn't be covered would be the system and default
> CMA heap if not setup in the DT, which shouldn't be too bad for this
> particular use-case.
I'm not opposed to the idea of describing the association through the
DT (with a <phandle, ID> pair). My main fear is that it drags us into
endless discussions around what's considered HW description and what's
not (PTSD of all those DT-bindings discussions I suppose :-)), which
ends up delaying the merging of Panthor's protected memory support.
Honestly, at this point I'm considering going back to my initial
suggestion to add a dedicated ioctl() (requiring high privilege) to let
the user pass the memory for the FW protected sections as a dmabuf FD.
Given we don't need those sections to be populated for the FW to boot,
it wouldn't block the probe of the driver, it would just prevent PROTM
usage until those sections are populated.
This would let us make progress with the rest of the changes in this
patchset, while the community decides how they want to expose dma_heaps
to in-kernel users.
^ permalink raw reply
* Re: [PATCH 0/4] media: mediatek: vcodec: VP9 slice setup cleanups and fixes
From: Nicolas Dufresne @ 2026-05-06 15:02 UTC (permalink / raw)
To: Haoxiang Li, tiffany.lin, andrew-ct.chen, yunfei.dong, mchehab,
matthias.bgg, angelogioacchino.delregno, hverkuil+cisco,
laurent.pinchart, p.zabel, benjamin.gaignard
Cc: linux-media, linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <20260506084203.202882-1-lihaoxiang@isrc.iscas.ac.cn>
[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]
Hi,
Le mercredi 06 mai 2026 à 16:41 +0800, Haoxiang Li a écrit :
> Patches 1 and 2 change the signatures of two functions to void, removing
> the now-unnecessary return value and its associated dead error checks.
> Patches 3 and 4 add missing memory release operations in error paths,
> fixing resource leaks.
>
> These patches are carried out under the guidance of Nicolas Dufresne.
> Thanks, Nicolas!
Thanks a lot for the patches. Something to improve, as I suppose you are fairly
knew to this, is that when sending a new version of a existing patch, we try and
make it clear and make version the subjects (e.g. [PATCH v2 ...) and in the
cover letter, we summarize the changes from v1 -> v2. If you use tools like b4,
the process become painless. Meanwhile, this is fresh in my memory, so its fine.
cheers,
Nicolas
>
> Haoxiang Li (4):
> media: mediatek: vcodec: remove redundant return value of
> vdec_vp9_slice_setup_lat_buffer()
> media: mediatek: vcodec: remove redundant return value of
> vdec_vp9_slice_setup_prob_buffer()
> media: mediatek: vcodec: free working buf on error path in
> vdec_vp9_slice_setup_lat()
> media: mediatek: vcodec: free working buf in
> vdec_vp9_slice_setup_single()
>
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 29 +++++++------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: mediatek: mt8188-geralt: enable Wi-Fi card
From: Icenowy Zheng @ 2026-05-06 14:14 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Hui Liu, linux-gpio,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <CAGXv+5F6BSmqq5HEybuCSwt75LVzh5gvs2wQpqy3vgfLi60Dcg@mail.gmail.com>
在 2026-05-04一的 15:34 +0800，Chen-Yu Tsai写道：
> Hi,
>
> On Mon, May 4, 2026 at 3:28 PM Icenowy Zheng
> <zhengxingda@iscas.ac.cn> wrote:
> >
> > The mainline pcie-mediatek-gen3 driver does not have code managing
> > downstream device power / reset.
> >
> > As the Wi-Fi card on ciri is a fixed device, set the related
> > regulator
> > to always-on and use GPIO hog to set the status of its reset pin.
>
> The plan now is to model it as an M.2 E-key slot (even though the
> chip
> is actually soldered on the main board).
Interestingly I saw a "PCI_PWRCTRL_GENERIC" driver in 7.1, although it
does not support toggling #PERST now -- maybe this should be done and
used instead? (Well it looks like the driver had existed for some time,
but it was for "slots" previously)
Thanks,
Icenowy
>
> I have some of the patches ready, but I'm still working out the USB
> side of it.
>
>
> ChenYu
>
> > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > ---
> > arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
> > index 8e423504ec052..c25780098103b 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
> > @@ -544,6 +544,11 @@ &mt6359codec {
> > mediatek,mic-type-2 = <2>; /* DMIC */
> > };
> >
> > +&mt6359_vcn18_ldo_reg {
> > + /* Used by WLAN */
> > + regulator-always-on;
> > +};
> > +
> > &mt6359_vcore_buck_reg {
> > regulator-always-on;
> > };
> > @@ -1145,6 +1150,12 @@ pins-en-pp3300-wlan {
> > output-low;
> > };
> > };
> > +
> > + wlan-reset-hog {
> > + gpio-hog;
> > + gpios = <145 GPIO_ACTIVE_HIGH>;
> > + output-high;
> > + };
> > };
> >
> > &pmic {
> > --
> > 2.52.0
> >
> >
^ permalink raw reply
* Re: [PATCH] dt-bindings: remoteproc: mtk,scp: Allow multiple memory regions
From: Krzysztof Kozlowski @ 2026-05-06 13:36 UTC (permalink / raw)
To: Arnab Layek, Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
Cc: linux-remoteproc, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Project_Global_Chrome_Upstream_Group
In-Reply-To: <20260506133157.3283204-1-arnab.layek@mediatek.com>
On 06/05/2026 15:31, Arnab Layek wrote:
> Update the memory-region property to support 1-2 reserved memory
> regions instead of exactly one. This is needed for newer MediaTek
> SoCs like MT8188 which require additional memory regions for SCP
> operation.
So where is the compatible? Already there?
>
> Tested on MT8188 Geralt platform.
You cannot test binding.
>
> Signed-off-by: Arnab Layek <arnab.layek@mediatek.com>
> ---
> .../devicetree/bindings/remoteproc/mtk,scp.yaml | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> index bdbb12118da4..9f6dca94ff40 100644
> --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> @@ -55,7 +55,10 @@ properties:
> initializing SCP.
>
> memory-region:
> - maxItems: 1
> + description:
> + Phandle to the reserved memory regions.
> + minItems: 1
> + maxItems: 2
Just like I sent some patches long time ago for iommus Mediatek media
bindings - fix it the same way. Pity the patches were not picked up,
even though sending twice, but that does not give you easier way to get
incorrect code in. You just multiple poor patterns...
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH] dt-bindings: remoteproc: mtk,scp: Allow multiple memory regions
From: Arnab Layek @ 2026-05-06 13:31 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
Cc: linux-remoteproc, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Project_Global_Chrome_Upstream_Group, Arnab Layek
Update the memory-region property to support 1-2 reserved memory
regions instead of exactly one. This is needed for newer MediaTek
SoCs like MT8188 which require additional memory regions for SCP
operation.
Tested on MT8188 Geralt platform.
Signed-off-by: Arnab Layek <arnab.layek@mediatek.com>
---
.../devicetree/bindings/remoteproc/mtk,scp.yaml | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
index bdbb12118da4..9f6dca94ff40 100644
--- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
@@ -55,7 +55,10 @@ properties:
initializing SCP.
memory-region:
- maxItems: 1
+ description:
+ Phandle to the reserved memory regions.
+ minItems: 1
+ maxItems: 2
cros-ec-rpmsg:
$ref: /schemas/embedded-controller/google,cros-ec.yaml
@@ -123,7 +126,10 @@ patternProperties:
initializing sub cores of multi-core SCP.
memory-region:
- maxItems: 1
+ description:
+ Phandle to the reserved memory regions.
+ minItems: 1
+ maxItems: 2
cros-ec-rpmsg:
$ref: /schemas/embedded-controller/google,cros-ec.yaml
--
2.45.2
^ permalink raw reply related
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Maxime Ripard @ 2026-05-06 13:31 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: Ketil Johnsen, David Airlie, Simona Vetter, Maarten Lankhorst,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Boris Brezillon, Steven Price, Liviu Dudau,
Daniel Almeida, Alice Ryhl, Matthias Brugger,
AngeloGioacchino Del Regno, dri-devel, linux-doc, linux-kernel,
linux-media, linaro-mm-sig, linux-arm-kernel, linux-mediatek,
Florent Tomasin
In-Reply-To: <SurytM7FTOazQNVXXqCU7g@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 3341 bytes --]
On Wed, May 06, 2026 at 02:43:42PM +0200, Nicolas Frattaroli wrote:
> On Wednesday, 6 May 2026 12:08:24 Central European Summer Time Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> > > From: Florent Tomasin <florent.tomasin@arm.com>
> > >
> > > This patch allows Panthor to allocate buffer objects from a
> > > protected heap. The Panthor driver should be seen as a consumer
> > > of the heap and not an exporter.
> > >
> > > Protected memory buffers needed by the Panthor driver:
> > > - On CSF FW load, the Panthor driver must allocate a protected
> > > buffer object to hold data to use by the FW when in protected
> > > mode. This protected buffer object is owned by the device
> > > and does not belong to a process.
> > > - On CSG creation, the Panthor driver must allocate a protected
> > > suspend buffer object for the FW to store data when suspending
> > > the CSG while in protected mode. The kernel owns this allocation
> > > and does not allow user space mapping. The format of the data
> > > in this buffer is only known by the FW and does not need to be
> > > shared with other entities.
> > >
> > > The driver will retrieve the protected heap using the name of the
> > > heap provided to the driver as module parameter.
> >
> > I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
> > better in the device tree and lookup through the device node? heaps are
> > going to have a node anyway, right?
> >
> > This would allow you to have a default that works and not mess to much
> > with the kernel parameters that aren't always easy to change for
> > end-users.
>
> Hopefully the kernel parameters aren't easy to change for end-users on
> systems that deploy this. :) The use-case is copy protection for embedded
> devices running on locked-down systems. Though admittedly the mechanism
> works even on "tampered"-with systems, as long as the underlying hardware
> implements the access restrictions properly.
I guess it wasn't just about the user tampering it, but also about
distros shipping, say, a signed UKI that would support multiple
platforms with 42 versions of optee but all using panthor. I'm not sure
we can expect a single heap name to work for all of them.
> I'm a bit hesitant about making this DT myself. It would solve the problem
> that panthor could probe before the heap provider and needs to handle
> deferral by itself, but it does mean that we'd be putting software
> configuration into the device tree.
Is it? If the system has a protected allocator, and if panthor
absolutely needs to allocate from that allocator, it's not software
configuration: it's a description of what the platform looks like from
Linux PoV.
Or put it differently, it's not more software than optee is, and yet it
has its own node.
> Having the secure heap be a node with no address would allow the tee
> (or whatever else) to still dynamically allocate it wherever, and let
> us handle the dependency relationship between dma heap and GPU, but
> then we require that tee heap driver implementations play nice with
> this scheme, and bring OF into the dma_heap APIs.
I mean, it's a dma_heap API that we create so we don't bring anything
more to it :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply
* Re: [PATCH 06/13] thermal: mediatek: add pmic thermal support
From: Andy Shevchenko @ 2026-05-06 13:26 UTC (permalink / raw)
To: Roman Vivchar
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Srinivas Kandagatla,
Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Lee Jones, linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-pm, Ben Grisdale
In-Reply-To: <cgsML96DsJh_Ow9XsSnyrZ3NhlCnNj1rKegzYNR3eQzkWoF5xQB5-aU6Zi7pKcT8GfUnO3B-D71je60APUtorB7p_A3GtUCp2oer3KYLn6k=@protonmail.com>
On Wed, May 06, 2026 at 11:22:15AM +0000, Roman Vivchar wrote:
> On Tuesday, May 5th, 2026 at 11:16 AM, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Mon, May 04, 2026 at 09:24:58PM +0300, Roman Vivchar via B4 Relay wrote:
...
> > > +struct mtk_pmic_sensor {
> > > + struct mtk_pmic_thermal *mt;
> > > + int id;
> > > + struct iio_channel *adc_channel;
> > > + struct thermal_zone_device *tzdev;
> > > +};
> >
> > Can you confirm with `pahole` that this is the best layout (taking into account
> > the use in the below data structure)?
> >
> > > +struct mtk_pmic_thermal {
> > > + struct device *dev;
> > > + struct regmap *regmap;
> > > + struct mtk_pmic_sensor sensors[MAX_SENSORS];
> > > +
> > > + s32 t_slope1;
> > > + s32 t_slope2;
> > > + s32 t_intercept;
> > > +
> > > + const struct mtk_thermal_data *data;
> > > +};
>
> On the ARMv7 it shouldn't be an issue, because pointer size equals to
> the s32 or int. However, I've reordered the fields to group pointers
> and integers together.
>
> struct mtk_pmic_sensor {
> struct mtk_pmic_thermal * mt; /* 0 4 */
> struct iio_channel * adc_channel; /* 4 4 */
> struct thermal_zone_device * tzdev; /* 8 4 */
> int id; /* 12 4 */
>
> /* size: 16, cachelines: 1, members: 4 */
> /* last cacheline: 16 bytes */
> };
>
> struct mtk_pmic_thermal {
> struct device * dev; /* 0 4 */
> struct regmap * regmap; /* 4 4 */
> const struct mtk_thermal_data * data; /* 8 4 */
> s32 t_slope1; /* 12 4 */
> s32 t_slope2; /* 16 4 */
> s32 t_intercept; /* 20 4 */
> struct mtk_pmic_sensor sensors[1]; /* 24 16 */
>
> /* size: 40, cachelines: 1, members: 7 */
> /* last cacheline: 40 bytes */
> };
>
> The compiler will still add some padding on the AArch64 though.
>
> struct mtk_pmic_sensor {
> struct mtk_pmic_thermal * mt; /* 0 8 */
> struct iio_channel * adc_channel; /* 8 8 */
> struct thermal_zone_device * tzdev; /* 16 8 */
> int id; /* 24 4 */
>
> /* size: 32, cachelines: 1, members: 4 */
> /* padding: 4 */
> /* last cacheline: 32 bytes */
> };
>
> struct mtk_pmic_thermal {
> struct device * dev; /* 0 8 */
> struct regmap * regmap; /* 8 8 */
> const struct mtk_thermal_data * data; /* 16 8 */
> s32 t_slope1; /* 24 4 */
> s32 t_slope2; /* 28 4 */
> s32 t_intercept; /* 32 4 */
>
> /* XXX 4 bytes hole, try to pack */
^^^^
> struct mtk_pmic_sensor sensors[1]; /* 40 32 */
>
> /* size: 72, cachelines: 2, members: 7 */
> /* sum members: 68, holes: 1, sum holes: 4 */
> /* last cacheline: 8 bytes */
> };
>
> Is this good enough?
In the last it seems moving the s32 members to be the last will removes
the 4-byte hole.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Maxime Ripard @ 2026-05-06 13:12 UTC (permalink / raw)
To: Boris Brezillon
Cc: Ketil Johnsen, David Airlie, Simona Vetter, Maarten Lankhorst,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260506125015.0108ef44@fedora>
[-- Attachment #1: Type: text/plain, Size: 3374 bytes --]
On Wed, May 06, 2026 at 12:50:15PM +0200, Boris Brezillon wrote:
> On Wed, 6 May 2026 12:08:24 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
>
> > Hi,
> >
> > On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> > > From: Florent Tomasin <florent.tomasin@arm.com>
> > >
> > > This patch allows Panthor to allocate buffer objects from a
> > > protected heap. The Panthor driver should be seen as a consumer
> > > of the heap and not an exporter.
> > >
> > > Protected memory buffers needed by the Panthor driver:
> > > - On CSF FW load, the Panthor driver must allocate a protected
> > > buffer object to hold data to use by the FW when in protected
> > > mode. This protected buffer object is owned by the device
> > > and does not belong to a process.
> > > - On CSG creation, the Panthor driver must allocate a protected
> > > suspend buffer object for the FW to store data when suspending
> > > the CSG while in protected mode. The kernel owns this allocation
> > > and does not allow user space mapping. The format of the data
> > > in this buffer is only known by the FW and does not need to be
> > > shared with other entities.
> > >
> > > The driver will retrieve the protected heap using the name of the
> > > heap provided to the driver as module parameter.
> >
> > I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
> > better in the device tree and lookup through the device node? heaps are
> > going to have a node anyway, right?
>
> I'm not too sure. Take the PROTMEM (name="protected,xxxx") dma_heaps
> instantiated by optee for instance, I don't think the originating
> tee_device comes from a device node, nor is the underlying heap
> described as a device node. The reserved memory pool this protected heap
> comes from is most likely defined somewhere as reserved memory in the
> DT, but there's nothing to correlate this range of reserved mem to some
> sub-range that the TEE implementation is carving out to provide
> protected memory.
Maybe we should be working on a dt bindings for heaps then? Something
simple like we have for clocks with a phandle and an ID would probably
be enough. In optee's case, it looks like it would map nicely with
TEE_DMA_HEAP_* flags too.
The only two that wouldn't be covered would be the system and default
CMA heap if not setup in the DT, which shouldn't be too bad for this
particular use-case.
> > This would allow you to have a default that works and not mess to much
> > with the kernel parameters that aren't always easy to change for
> > end-users.
>
> I guess we can have a default list of heaps that we know provide
> protected memory for GPU rendering if that helps. Right now this list
> would contain only "protected,trusted-ui" :D. The other option would be
> to make this list a panthor Kconfig option and not expose it as a module
> param.
My main concern is that firmware builds are board specific, and thus its
capabilities isn't something we can reasonably expect to be consistent
across boards, SoCs and platforms. Kernel images (and the kernel
parameters) however can be made generic and unreasonably hard for users
to modify once you start playing with things like secure boot or
measured boot.
The only thing bridging the gap between the two is the DT.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply
* Re: [PATCH 01/13] dt-bindings: iio: adc: add mt6323 PMIC AUXADC
From: Krzysztof Kozlowski @ 2026-05-06 13:08 UTC (permalink / raw)
To: Roman Vivchar
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Srinivas Kandagatla,
Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Lee Jones, linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-pm, Ben Grisdale
In-Reply-To: <evJBbJD1B9GjUyj5xyiJVUw-Ufib330tbUxxamM8-e4kJeNlwcENufbYZHdDWb0ZItz1fsEAbDaSFk0ObL4y0zenRr4vA4KOn6IHxuBP4VQ=@protonmail.com>
On 06/05/2026 12:59, Roman Vivchar wrote:
> On Wednesday, May 6th, 2026 at 10:56 AM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
>> On 04/05/2026 20:24, Roman Vivchar via B4 Relay wrote:
>>> From: Roman Vivchar <rva333@protonmail.com>
>>>
>>> The MediaTek mt6323 PMIC includes an AUXADC used for battery voltage,
>>> temperature, and other internal measurements.
>>>
>
> ...
>
>>> +
>>> +properties:
>>> + compatible:
>>> + const: mediatek,mt6323-auxadc
>>> +
>>> + "#io-channel-cells":
>>> + const: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - "#io-channel-cells"
>>
>>
>> This is heavily incomplete node... or unnecessarily split. Why it cannot
>> be part of parent binding? Or even parent node?
>>
>
> The MediaTek mt6359 AUXADC also has a very similar binding. The mt6323
> cannot be merged there because it has a different hardware.
I did not propose that, but since you are mentioning it: it is 100% the
same, so I do not understand why it cannot be merged. That would save
you a few questions :/
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Nicolas Frattaroli @ 2026-05-06 12:43 UTC (permalink / raw)
To: Ketil Johnsen, Maxime Ripard
Cc: David Airlie, Simona Vetter, Maarten Lankhorst, Thomas Zimmermann,
Jonathan Corbet, Shuah Khan, Sumit Semwal, Benjamin Gaignard,
Brian Starkey, John Stultz, T.J. Mercier, Christian König,
Boris Brezillon, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260506-energetic-azure-pig-2b6ec4@houat>
On Wednesday, 6 May 2026 12:08:24 Central European Summer Time Maxime Ripard wrote:
> Hi,
>
> On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> > From: Florent Tomasin <florent.tomasin@arm.com>
> >
> > This patch allows Panthor to allocate buffer objects from a
> > protected heap. The Panthor driver should be seen as a consumer
> > of the heap and not an exporter.
> >
> > Protected memory buffers needed by the Panthor driver:
> > - On CSF FW load, the Panthor driver must allocate a protected
> > buffer object to hold data to use by the FW when in protected
> > mode. This protected buffer object is owned by the device
> > and does not belong to a process.
> > - On CSG creation, the Panthor driver must allocate a protected
> > suspend buffer object for the FW to store data when suspending
> > the CSG while in protected mode. The kernel owns this allocation
> > and does not allow user space mapping. The format of the data
> > in this buffer is only known by the FW and does not need to be
> > shared with other entities.
> >
> > The driver will retrieve the protected heap using the name of the
> > heap provided to the driver as module parameter.
>
> I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
> better in the device tree and lookup through the device node? heaps are
> going to have a node anyway, right?
>
> This would allow you to have a default that works and not mess to much
> with the kernel parameters that aren't always easy to change for
> end-users.
Hopefully the kernel parameters aren't easy to change for end-users on
systems that deploy this. :) The use-case is copy protection for embedded
devices running on locked-down systems. Though admittedly the mechanism
works even on "tampered"-with systems, as long as the underlying hardware
implements the access restrictions properly.
I'm a bit hesitant about making this DT myself. It would solve the problem
that panthor could probe before the heap provider and needs to handle
deferral by itself, but it does mean that we'd be putting software
configuration into the device tree. Having the secure heap be a node with
no address would allow the tee (or whatever else) to still dynamically
allocate it wherever, and let us handle the dependency relationship
between dma heap and GPU, but then we require that tee heap driver
implementations play nice with this scheme, and bring OF into the
dma_heap APIs.
I'm not against making the dma heap a phandle property for the GPU
node and then extending the dma-heap API to get a heap by name or
by index from a user device's standardised phandle property/names
property, but that's potentially a very large can of worms to open.
>
> Maxime
>
Kind regards,
Nicolas Frattaroli
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Nicolas Frattaroli @ 2026-05-06 12:28 UTC (permalink / raw)
To: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Boris Brezillon, Steven Price, Liviu Dudau,
Daniel Almeida, Alice Ryhl, Matthias Brugger,
AngeloGioacchino Del Regno, Ketil Johnsen
Cc: dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin, Ketil Johnsen
In-Reply-To: <20260505140516.1372388-5-ketil.johnsen@arm.com>
Thanks for sending this series! A few quick notes in-line.
On Tuesday, 5 May 2026 16:05:10 Central European Summer Time Ketil Johnsen wrote:
> From: Florent Tomasin <florent.tomasin@arm.com>
>
> This patch allows Panthor to allocate buffer objects from a
> protected heap. The Panthor driver should be seen as a consumer
> of the heap and not an exporter.
>
> Protected memory buffers needed by the Panthor driver:
> - On CSF FW load, the Panthor driver must allocate a protected
> buffer object to hold data to use by the FW when in protected
> mode. This protected buffer object is owned by the device
> and does not belong to a process.
> - On CSG creation, the Panthor driver must allocate a protected
> suspend buffer object for the FW to store data when suspending
> the CSG while in protected mode. The kernel owns this allocation
> and does not allow user space mapping. The format of the data
> in this buffer is only known by the FW and does not need to be
> shared with other entities.
>
> The driver will retrieve the protected heap using the name of the
> heap provided to the driver as module parameter.
>
> If the heap is not yet available, the panthor driver will defer
> the probe until created. It is an integration error to provide
> a heap name that does not exist or is never created.
>
> Panthor is calling the DMA heap allocation function
> and obtains a DMA buffer from it. This buffer is then
> registered to GEM and imported.
>
> Signed-off-by: Florent Tomasin <florent.tomasin@arm.com>
> Co-developed-by: Ketil Johnsen <ketil.johnsen@arm.com>
> Signed-off-by: Ketil Johnsen <ketil.johnsen@arm.com>
> ---
> Documentation/gpu/panthor.rst | 47 +++++++++++++++
> drivers/gpu/drm/panthor/Kconfig | 1 +
> drivers/gpu/drm/panthor/panthor_device.c | 28 ++++++++-
> drivers/gpu/drm/panthor/panthor_device.h | 6 ++
> drivers/gpu/drm/panthor/panthor_fw.c | 29 ++++++++-
> drivers/gpu/drm/panthor/panthor_fw.h | 2 +
> drivers/gpu/drm/panthor/panthor_gem.c | 77 ++++++++++++++++++++++--
> drivers/gpu/drm/panthor/panthor_gem.h | 16 ++++-
> drivers/gpu/drm/panthor/panthor_heap.c | 2 +
> drivers/gpu/drm/panthor/panthor_sched.c | 11 +++-
> 10 files changed, 208 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/gpu/panthor.rst b/Documentation/gpu/panthor.rst
> index 7a841741278fb..be20eadea6dd5 100644
> --- a/Documentation/gpu/panthor.rst
> +++ b/Documentation/gpu/panthor.rst
> @@ -54,3 +54,50 @@ sync object arrays and heap chunks. Because they are all allocated and pinned
> at creation time, only `panthor-resident-memory` is necessary to tell us their
> size. `panthor-active-memory` shows the size of kernel BO's associated with
> VM's and groups currently being scheduled for execution by the GPU.
> +
> +Panthor Protected Memory Integration
> +=====================================
> +
> +Panthor requires the platform to provide a protected DMA HEAP.
> +This DMA heap must be identifiable via a string name.
> +The name is defined by the system integrator, it could be hard coded
> +in the heap driver, defined by a module parameter of the heap driver
> +or else.
> +
> +.. code-block:: none
> +
> + User
> + ┌─────────────────────────────┐
> + | Application |
> + └─────────────▲───────────────┘
> + | | |
> + | DMA-BUF | | Protected
> + | | | Job Submission
> + --------|---------|----------|---------
> + Kernel | | |
> + | | |
> + | | DMA-BUF |
> + ┌───────▼─────────────┐ ┌─▼───────┐
> + | DMA PROTECTED HEAP |◄───| Panthor |
> + | (Vendor specific) | | |
> + └─────────────────────┘ └─────────┘
> + | |
> + --------|--------------------|---------
> + HW | |
> + | |
> + ┌───────▼───────────────┐ ┌─▼───┐
> + | Trusted FW | | |
> + | Protected Memory ◄──► GPU |
> + └───────────────────────┘ └─────┘
> +
> +To configure Panthor to use the protected memory heap, pass the protected memory
> +heap string name as module parameter of the Panthor module.
> +
> +Example:
> +
> + .. code-block:: shell
> +
> + insmod panthor.ko protected_heap_name=“vendor_protected_heap"
> +
> +If `protected_heap_name` module parameter is not provided, Panthor will not support
> +protected job execution.
> diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
> index 911e7f4810c39..fb0bad9a0fd2b 100644
> --- a/drivers/gpu/drm/panthor/Kconfig
> +++ b/drivers/gpu/drm/panthor/Kconfig
> @@ -7,6 +7,7 @@ config DRM_PANTHOR
> depends on !GENERIC_ATOMIC64 # for IOMMU_IO_PGTABLE_LPAE
> depends on MMU
> select DEVFREQ_GOV_SIMPLE_ONDEMAND
> + select DMABUF_HEAPS
> select DRM_EXEC
> select DRM_GPUVM
> select DRM_SCHED
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index bc62a498a8a84..3a5cdfa99e5fe 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -5,7 +5,9 @@
> /* Copyright 2025 ARM Limited. All rights reserved. */
>
> #include <linux/clk.h>
> +#include <linux/dma-heap.h>
> #include <linux/mm.h>
> +#include <linux/of.h>
Can be dropped, none of the added code in this file requires it.
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> @@ -27,6 +29,10 @@
> #include "panthor_regs.h"
> #include "panthor_sched.h"
>
> +MODULE_PARM_DESC(protected_heap_name, "DMA heap name, from which to allocate protected buffers");
> +static char *protected_heap_name;
> +module_param(protected_heap_name, charp, 0444);
> +
> static int panthor_gpu_coherency_init(struct panthor_device *ptdev)
> {
> BUILD_BUG_ON(GPU_COHERENCY_NONE != DRM_PANTHOR_GPU_COHERENCY_NONE);
> @@ -127,6 +133,9 @@ void panthor_device_unplug(struct panthor_device *ptdev)
> panthor_gpu_unplug(ptdev);
> panthor_pwr_unplug(ptdev);
>
> + if (ptdev->protm.heap)
> + dma_heap_put(ptdev->protm.heap);
> +
> pm_runtime_dont_use_autosuspend(ptdev->base.dev);
> pm_runtime_put_sync_suspend(ptdev->base.dev);
>
> @@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev)
> return ret;
> }
>
> + /* If a protected heap name is specified but not found, defer the probe until created */
> + if (protected_heap_name && strlen(protected_heap_name)) {
> + ptdev->protm.heap = dma_heap_find(protected_heap_name);
> + if (!ptdev->protm.heap) {
> + drm_warn(&ptdev->base,
> + "Protected heap \'%s\' not (yet) available - deferring probe",
> + protected_heap_name);
The escaping of the single quotes here is redundant, and I think this
is better as a debug message rather than a drm_warn: probe deferral
is normal.
Though I'm wondering whether we're open-coding dependency handling here,
I guess there's no way for any core to order things for us because the
dependency is on a name and the name is from a driver-specific module
parameter, so there's no generic solution to this. This second paragraph
is just me ruminating though and not an actionable request for changes.
> + ret = -EPROBE_DEFER;
> + goto err_rpm_put;
> + }
> + }
> +
> ret = panthor_hw_init(ptdev);
> if (ret)
> - goto err_rpm_put;
> + goto err_dma_heap_put;
>
> ret = panthor_pwr_init(ptdev);
> if (ret)
> @@ -343,6 +364,11 @@ int panthor_device_init(struct panthor_device *ptdev)
>
> err_rpm_put:
> pm_runtime_put_sync_suspend(ptdev->base.dev);
> +
> +err_dma_heap_put:
> + if (ptdev->protm.heap)
> + dma_heap_put(ptdev->protm.heap);
> +
This is ordered wrong. Getting the dma heap happens after getting rpm,
so the unwind should put the dma heap before putting rpm. Right now,
a failure of panthor_hw_init would leave rpm enabled.
As Boris already mentioned in his review though, using devres helpers
would get rid of this manual put on error or driver remove entirely,
which is preferable.
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 5cba272f9b4de..d51fec97fc5fa 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -7,6 +7,7 @@
> #define __PANTHOR_DEVICE_H__
>
> #include <linux/atomic.h>
> +#include <linux/dma-heap.h>
> #include <linux/io-pgtable.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> @@ -329,6 +330,11 @@ struct panthor_device {
> struct list_head node;
> } gems;
> #endif
> + /** @protm: Protected mode related data. */
> + struct {
> + /** @heap: Pointer to the protected heap */
> + struct dma_heap *heap;
> + } protm;
> };
>
> struct panthor_gpu_usage {
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 0d07a133dc3af..1aba29b9779b6 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -500,6 +500,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>
> mem = panthor_kernel_bo_create(ptdev, ptdev->fw->vm, SZ_8K,
> DRM_PANTHOR_BO_NO_MMAP,
> + 0,
> DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC |
> DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED,
> PANTHOR_VM_KERNEL_AUTO_VA,
> @@ -534,6 +535,26 @@ panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size)
> {
> return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size,
> DRM_PANTHOR_BO_NO_MMAP,
> + 0,
> + DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
> + PANTHOR_VM_KERNEL_AUTO_VA,
> + "suspend_buf");
Looks like we're effectively renaming this from "FW suspend buffer"
to "suspend_buf", and calling the protm suspend buf "FW suspend buffer".
This seems a little confusing, to the point where the diff algorithm
also had a hard time. Naming comes down to a matter of taste, but I
want to make sure the rename was intentional here.
> +}
> +
> +/**
> + * panthor_fw_alloc_protm_suspend_buf_mem() - Allocate a protm suspend buffer
> + * for a command stream group.
> + * @ptdev: Device.
> + * @size: Size of the protm suspend buffer.
> + *
> + * Return: A valid pointer in case of success, an ERR_PTR() otherwise.
> + */
> +struct panthor_kernel_bo *
> +panthor_fw_alloc_protm_suspend_buf_mem(struct panthor_device *ptdev, size_t size)
> +{
> + return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size,
> + DRM_PANTHOR_BO_NO_MMAP,
> + DRM_PANTHOR_KBO_PROTECTED_HEAP,
> DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
> PANTHOR_VM_KERNEL_AUTO_VA,
> "FW suspend buffer");
> @@ -547,6 +568,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> ssize_t vm_pgsz = panthor_vm_page_size(ptdev->fw->vm);
> struct panthor_fw_binary_section_entry_hdr hdr;
> struct panthor_fw_section *section;
> + u32 kbo_flags = 0;
> u32 section_size;
> u32 name_len;
> int ret;
> @@ -585,10 +607,13 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> return -EINVAL;
> }
>
> - if (hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) {
> + if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) && !ptdev->protm.heap) {
> drm_warn(&ptdev->base,
> "Firmware protected mode entry is not supported, ignoring");
> return 0;
> + } else if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) && ptdev->protm.heap) {
> + drm_info(&ptdev->base, "Firmware protected mode entry supported");
> + kbo_flags = DRM_PANTHOR_KBO_PROTECTED_HEAP;
Instead of the duplicated check in both branches of the condition,
nesting it may be less clunky:
if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) {
if (!ptdev->protm.heap) {
drm_warn(&ptdev->base,
"Firmware protected mode entry is not supported, ignoring");
return 0;
}
drm_info(&ptdev->base, "Firmware protected mode entry supported");
kbo_flags = DRM_PANTHOR_KBO_PROTECTED_HEAP;
}
That being said, we might want to rethink the warning/info entirely. If
it's normal behaviour for a platform to load this fw section, even if
the platform doesn't support protm, as I suspect is the case due to the
`return 0`, then the warning has always been a bit too noisy.
I think info level that also gives some identifier for what section was
ignored (e.g. fw offset start/end) would be fine. The other branch, i.e.
"Firmware protected mode entry supported", may be best something that
gets printed along with other details about what the GPU supports, so
that each protected section does not print this over and over. It feels
wrong to do it in panthor_hw_info_init since that's printing features of
the hardware rather than of panthor's configuration, so maybe just do it
in panthor_device_init after ptdev->protm.heap is non-NULL.
> }
>
> if (hdr.va.start == CSF_MCU_SHARED_REGION_START &&
> @@ -653,7 +678,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
>
> section->mem = panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev),
> section_size,
> - DRM_PANTHOR_BO_NO_MMAP,
> + DRM_PANTHOR_BO_NO_MMAP, kbo_flags,
> vm_map_flags, va, "FW section");
> if (IS_ERR(section->mem))
> return PTR_ERR(section->mem);
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
> index fbdc21469ba32..0cf3761abf789 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.h
> +++ b/drivers/gpu/drm/panthor/panthor_fw.h
> @@ -509,6 +509,8 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
> u32 *input_fw_va, u32 *output_fw_va);
> struct panthor_kernel_bo *
> panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size);
> +struct panthor_kernel_bo *
> +panthor_fw_alloc_protm_suspend_buf_mem(struct panthor_device *ptdev, size_t size);
>
> struct panthor_vm *panthor_fw_vm(struct panthor_device *ptdev);
>
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 13295d7a593df..08fe4a5e43817 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -20,12 +20,17 @@
> #include <drm/drm_print.h>
> #include <drm/panthor_drm.h>
>
> +#include <uapi/linux/dma-heap.h>
> +
> #include "panthor_device.h"
> #include "panthor_drv.h"
> #include "panthor_fw.h"
> #include "panthor_gem.h"
> #include "panthor_mmu.h"
>
> +MODULE_IMPORT_NS("DMA_BUF");
> +MODULE_IMPORT_NS("DMA_BUF_HEAP");
> +
> void panthor_gem_init(struct panthor_device *ptdev)
> {
> int err;
> @@ -466,7 +471,6 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
> }
>
> drm_gem_object_release(obj);
> -
Unrelated whitespace change (though I do like it), don't know how we
handle "too small for its own commit but also function isn't touched
by anything else in this commit" type whitespace cleanups.
> kfree(bo);
> drm_gem_object_put(vm_root_gem);
> }
> @@ -1026,6 +1030,7 @@ panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags,
> }
>
> panthor_gem_debugfs_set_usage_flags(bo, usage_flags);
> +
Unrelated whitespace change (though again, I do like it)
> return bo;
>
> err_put:
> @@ -1033,6 +1038,54 @@ panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags,
> return ERR_PTR(ret);
> }
>
> +static struct panthor_gem_object *
> +panthor_gem_create_protected(struct panthor_device *ptdev, size_t size,
> + uint32_t flags, struct panthor_vm *exclusive_vm,
> + u32 usage_flags)
> +{
> + struct dma_buf *dma_bo = NULL;
> + struct drm_gem_object *gem_obj;
> + struct panthor_gem_object *bo;
> + int ret;
> +
> + if (!ptdev->protm.heap)
> + return ERR_PTR(-EINVAL);
> +
> + if (flags != DRM_PANTHOR_BO_NO_MMAP)
> + return ERR_PTR(-EINVAL);
> +
> + if (!exclusive_vm)
> + return ERR_PTR(-EINVAL);
> +
> + dma_bo = dma_heap_buffer_alloc(ptdev->protm.heap, size, DMA_HEAP_VALID_FD_FLAGS,
> + DMA_HEAP_VALID_HEAP_FLAGS);
> + if (IS_ERR(dma_bo))
> + return ERR_PTR(PTR_ERR(dma_bo));
> +
> + gem_obj = drm_gem_prime_import(&ptdev->base, dma_bo);
I agree with Boris that putting the dma_buf here is the cleanest
solution.
Adding a cleanup.h DEFINE_FREE helper for dmabufs would be a longer-
term refactor with its own pros and cons, but would allow us to get
rid of the explicit put entirely by adorning the local with a __free
attribute.
> + if (IS_ERR(gem_obj)) {
> + ret = PTR_ERR(gem_obj);
> + goto err_free_dma_bo;
> + }
> +
> + bo = to_panthor_bo(gem_obj);
> + bo->flags = flags;
> +
> + panthor_gem_debugfs_set_usage_flags(bo, usage_flags);
> +
> + bo->exclusive_vm_root_gem = panthor_vm_root_gem(exclusive_vm);
> + drm_gem_object_get(bo->exclusive_vm_root_gem);
> + bo->base.resv = bo->exclusive_vm_root_gem->resv;
> +
> + return bo;
> +
> +err_free_dma_bo:
> + if (dma_bo)
> + dma_buf_put(dma_bo);
> +
> + return ERR_PTR(ret);
> +}
> +
> struct drm_gem_object *
> panthor_gem_prime_import_sg_table(struct drm_device *dev,
> struct dma_buf_attachment *attach,
Kind regards,
Nicolas Frattaroli
^ permalink raw reply
* Re: [PATCH 06/13] thermal: mediatek: add pmic thermal support
From: Roman Vivchar @ 2026-05-06 11:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Srinivas Kandagatla,
Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Lee Jones, linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-pm, Ben Grisdale
In-Reply-To: <afmnUG8dG0N0HpV6@ashevche-desk.local>
On Tuesday, May 5th, 2026 at 11:16 AM, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Mon, May 04, 2026 at 09:24:58PM +0300, Roman Vivchar via B4 Relay wrote:
...
> > +struct mtk_pmic_sensor {
> > + struct mtk_pmic_thermal *mt;
> > + int id;
> > + struct iio_channel *adc_channel;
> > + struct thermal_zone_device *tzdev;
> > +};
>
> Can you confirm with `pahole` that this is the best layout (taking into account
> the use in the below data structure)?
>
> > +struct mtk_pmic_thermal {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct mtk_pmic_sensor sensors[MAX_SENSORS];
> > +
> > + s32 t_slope1;
> > + s32 t_slope2;
> > + s32 t_intercept;
> > +
> > + const struct mtk_thermal_data *data;
> > +};
>
On the ARMv7 it shouldn't be an issue, because pointer size equals to
the s32 or int. However, I've reordered the fields to group pointers
and integers together.
struct mtk_pmic_sensor {
struct mtk_pmic_thermal * mt; /* 0 4 */
struct iio_channel * adc_channel; /* 4 4 */
struct thermal_zone_device * tzdev; /* 8 4 */
int id; /* 12 4 */
/* size: 16, cachelines: 1, members: 4 */
/* last cacheline: 16 bytes */
};
struct mtk_pmic_thermal {
struct device * dev; /* 0 4 */
struct regmap * regmap; /* 4 4 */
const struct mtk_thermal_data * data; /* 8 4 */
s32 t_slope1; /* 12 4 */
s32 t_slope2; /* 16 4 */
s32 t_intercept; /* 20 4 */
struct mtk_pmic_sensor sensors[1]; /* 24 16 */
/* size: 40, cachelines: 1, members: 7 */
/* last cacheline: 40 bytes */
};
The compiler will still add some padding on the AArch64 though.
struct mtk_pmic_sensor {
struct mtk_pmic_thermal * mt; /* 0 8 */
struct iio_channel * adc_channel; /* 8 8 */
struct thermal_zone_device * tzdev; /* 16 8 */
int id; /* 24 4 */
/* size: 32, cachelines: 1, members: 4 */
/* padding: 4 */
/* last cacheline: 32 bytes */
};
struct mtk_pmic_thermal {
struct device * dev; /* 0 8 */
struct regmap * regmap; /* 8 8 */
const struct mtk_thermal_data * data; /* 16 8 */
s32 t_slope1; /* 24 4 */
s32 t_slope2; /* 28 4 */
s32 t_intercept; /* 32 4 */
/* XXX 4 bytes hole, try to pack */
struct mtk_pmic_sensor sensors[1]; /* 40 32 */
/* size: 72, cachelines: 2, members: 7 */
/* sum members: 68, holes: 1, sum holes: 4 */
/* last cacheline: 8 bytes */
};
Is this good enough?
Other comments will be fixed in the v2.
Best regards,
Roman
^ permalink raw reply
* Re: [PATCH 01/13] dt-bindings: iio: adc: add mt6323 PMIC AUXADC
From: Roman Vivchar @ 2026-05-06 10:59 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Srinivas Kandagatla,
Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Lee Jones, linux-iio, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-pm, Ben Grisdale
In-Reply-To: <389917bb-c64c-4ac2-ab8f-b28ab0a89ecb@kernel.org>
On Wednesday, May 6th, 2026 at 10:56 AM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 04/05/2026 20:24, Roman Vivchar via B4 Relay wrote:
> > From: Roman Vivchar <rva333@protonmail.com>
> >
> > The MediaTek mt6323 PMIC includes an AUXADC used for battery voltage,
> > temperature, and other internal measurements.
> >
...
> > +
> > +properties:
> > + compatible:
> > + const: mediatek,mt6323-auxadc
> > +
> > + "#io-channel-cells":
> > + const: 1
> > +
> > +required:
> > + - compatible
> > + - "#io-channel-cells"
>
>
> This is heavily incomplete node... or unnecessarily split. Why it cannot
> be part of parent binding? Or even parent node?
>
The MediaTek mt6359 AUXADC also has a very similar binding. The mt6323
cannot be merged there because it has a different hardware.
Should this go as a child for the mt6397 mfd like mediatek,mt6323-led?
It currently references only mediatek,mt6359-auxadc.yaml, so perhaps
the mt6359 should be merged to the mfd, and then use oneOf for the
compatible property?
Best regards,
Roman
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Boris Brezillon @ 2026-05-06 10:50 UTC (permalink / raw)
To: Maxime Ripard
Cc: Ketil Johnsen, David Airlie, Simona Vetter, Maarten Lankhorst,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260506-energetic-azure-pig-2b6ec4@houat>
On Wed, 6 May 2026 12:08:24 +0200
Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> > From: Florent Tomasin <florent.tomasin@arm.com>
> >
> > This patch allows Panthor to allocate buffer objects from a
> > protected heap. The Panthor driver should be seen as a consumer
> > of the heap and not an exporter.
> >
> > Protected memory buffers needed by the Panthor driver:
> > - On CSF FW load, the Panthor driver must allocate a protected
> > buffer object to hold data to use by the FW when in protected
> > mode. This protected buffer object is owned by the device
> > and does not belong to a process.
> > - On CSG creation, the Panthor driver must allocate a protected
> > suspend buffer object for the FW to store data when suspending
> > the CSG while in protected mode. The kernel owns this allocation
> > and does not allow user space mapping. The format of the data
> > in this buffer is only known by the FW and does not need to be
> > shared with other entities.
> >
> > The driver will retrieve the protected heap using the name of the
> > heap provided to the driver as module parameter.
>
> I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
> better in the device tree and lookup through the device node? heaps are
> going to have a node anyway, right?
I'm not too sure. Take the PROTMEM (name="protected,xxxx") dma_heaps
instantiated by optee for instance, I don't think the originating
tee_device comes from a device node, nor is the underlying heap
described as a device node. The reserved memory pool this protected heap
comes from is most likely defined somewhere as reserved memory in the
DT, but there's nothing to correlate this range of reserved mem to some
sub-range that the TEE implementation is carving out to provide
protected memory.
>
> This would allow you to have a default that works and not mess to much
> with the kernel parameters that aren't always easy to change for
> end-users.
I guess we can have a default list of heaps that we know provide
protected memory for GPU rendering if that helps. Right now this list
would contain only "protected,trusted-ui" :D. The other option would be
to make this list a panthor Kconfig option and not expose it as a module
param.
^ permalink raw reply
* Re: [PATCH 5/8] drm/panthor: Minor scheduler refactoring
From: Boris Brezillon @ 2026-05-06 10:33 UTC (permalink / raw)
To: Ketil Johnsen
Cc: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260505140516.1372388-6-ketil.johnsen@arm.com>
On Tue, 5 May 2026 16:05:11 +0200
Ketil Johnsen <ketil.johnsen@arm.com> wrote:
> From: Florent Tomasin <florent.tomasin@arm.com>
>
> Refactor parts of the group scheduling logic into new helper functions.
> This will simplify addition of the protected mode feature.
>
> Remove redundant assignments of csg_slot.
>
> Signed-off-by: Florent Tomasin <florent.tomasin@arm.com>
> Co-developed-by: Ketil Johnsen <ketil.johnsen@arm.com>
> Signed-off-by: Ketil Johnsen <ketil.johnsen@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_sched.c | 135 +++++++++++++++---------
> 1 file changed, 86 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5ee386338005c..987072bd867c4 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1934,6 +1934,12 @@ static void csgs_upd_ctx_init(struct panthor_csg_slots_upd_ctx *ctx)
> memset(ctx, 0, sizeof(*ctx));
> }
>
> +static void csgs_upd_ctx_ring_doorbell(struct panthor_csg_slots_upd_ctx *ctx,
> + u32 csg_id)
> +{
> + ctx->update_mask |= BIT(csg_id);
> +}
> +
> static void csgs_upd_ctx_queue_reqs(struct panthor_device *ptdev,
> struct panthor_csg_slots_upd_ctx *ctx,
> u32 csg_id, u32 value, u32 mask)
> @@ -1944,7 +1950,8 @@ static void csgs_upd_ctx_queue_reqs(struct panthor_device *ptdev,
>
> ctx->requests[csg_id].value = (ctx->requests[csg_id].value & ~mask) | (value & mask);
> ctx->requests[csg_id].mask |= mask;
> - ctx->update_mask |= BIT(csg_id);
> +
> + csgs_upd_ctx_ring_doorbell(ctx, csg_id);
> }
>
> static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev,
> @@ -1961,8 +1968,12 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev,
> while (update_slots) {
> struct panthor_fw_csg_iface *csg_iface;
> u32 csg_id = ffs(update_slots) - 1;
> + u32 req_mask = ctx->requests[csg_id].mask;
>
> update_slots &= ~BIT(csg_id);
> + if (!req_mask)
> + continue;
Looks like something that should be in patch 7, where you update the
doorbell_req register, and then call csgs_upd_ctx_ring_doorbell(),
meaning req_mask can be zero. The other option would be to teach
panthor_csg_slots_upd_ctx about CS doorbells, and let
csgs_upd_ctx_apply_locked() toggle the doorbell_req.
> +
> csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> panthor_fw_update_reqs(csg_iface, req,
> ctx->requests[csg_id].value,
> @@ -1979,6 +1990,9 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev,
> int ret;
>
> update_slots &= ~BIT(csg_id);
> + if (!req_mask)
> + continue;
> +
> csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
>
> ret = panthor_fw_csg_wait_acks(ptdev, csg_id, req_mask, &acked, 100);
^ permalink raw reply
* Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor
From: Maxime Ripard @ 2026-05-06 10:08 UTC (permalink / raw)
To: Ketil Johnsen
Cc: David Airlie, Simona Vetter, Maarten Lankhorst, Thomas Zimmermann,
Jonathan Corbet, Shuah Khan, Sumit Semwal, Benjamin Gaignard,
Brian Starkey, John Stultz, T.J. Mercier, Christian König,
Boris Brezillon, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin
In-Reply-To: <20260505140516.1372388-5-ketil.johnsen@arm.com>
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
Hi,
On Tue, May 05, 2026 at 04:05:10PM +0200, Ketil Johnsen wrote:
> From: Florent Tomasin <florent.tomasin@arm.com>
>
> This patch allows Panthor to allocate buffer objects from a
> protected heap. The Panthor driver should be seen as a consumer
> of the heap and not an exporter.
>
> Protected memory buffers needed by the Panthor driver:
> - On CSF FW load, the Panthor driver must allocate a protected
> buffer object to hold data to use by the FW when in protected
> mode. This protected buffer object is owned by the device
> and does not belong to a process.
> - On CSG creation, the Panthor driver must allocate a protected
> suspend buffer object for the FW to store data when suspending
> the CSG while in protected mode. The kernel owns this allocation
> and does not allow user space mapping. The format of the data
> in this buffer is only known by the FW and does not need to be
> shared with other entities.
>
> The driver will retrieve the protected heap using the name of the
> heap provided to the driver as module parameter.
I know it's what dma_heap_find asks for, but I wonder if it wouldn't be
better in the device tree and lookup through the device node? heaps are
going to have a node anyway, right?
This would allow you to have a default that works and not mess to much
with the kernel parameters that aren't always easy to change for
end-users.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply
* Re: [PATCH 8/8] drm/panthor: Expose protected rendering features
From: Boris Brezillon @ 2026-05-06 9:14 UTC (permalink / raw)
To: Ketil Johnsen
Cc: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek
In-Reply-To: <20260505140516.1372388-9-ketil.johnsen@arm.com>
On Tue, 5 May 2026 16:05:14 +0200
Ketil Johnsen <ketil.johnsen@arm.com> wrote:
> Add query for protected rendering capability.
> Add flag to group creation to specify need for protected rendering.
> Bump panthor version number.
>
> Signed-off-by: Ketil Johnsen <ketil.johnsen@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_drv.c | 21 +++++++++++-
> drivers/gpu/drm/panthor/panthor_sched.c | 21 +++++++-----
> include/uapi/drm/panthor_drm.h | 45 +++++++++++++++++++++++--
> 3 files changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 73fc983dc9b44..817df17f31f15 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -177,6 +177,7 @@ panthor_get_uobj_array(const struct drm_panthor_obj_array *in, u32 min_stride,
> PANTHOR_UOBJ_DECL(struct drm_panthor_csif_info, pad), \
> PANTHOR_UOBJ_DECL(struct drm_panthor_timestamp_info, current_timestamp), \
> PANTHOR_UOBJ_DECL(struct drm_panthor_group_priorities_info, pad), \
> + PANTHOR_UOBJ_DECL(struct drm_panthor_protected_info, features), \
> PANTHOR_UOBJ_DECL(struct drm_panthor_sync_op, timeline_value), \
> PANTHOR_UOBJ_DECL(struct drm_panthor_queue_submit, syncs), \
> PANTHOR_UOBJ_DECL(struct drm_panthor_queue_create, ringbuf_size), \
> @@ -928,12 +929,20 @@ static void panthor_query_group_priorities_info(struct drm_file *file,
> }
> }
>
> +static void panthor_query_protected_info(struct panthor_device *ptdev,
> + struct drm_panthor_protected_info *arg)
> +{
> + arg->features =
> + ptdev->protm.heap ? DRM_PANTHOR_PROTECTED_FEATURE_BASIC : 0;
> +}
> +
> static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct drm_file *file)
> {
> struct panthor_device *ptdev = container_of(ddev, struct panthor_device, base);
> struct drm_panthor_dev_query *args = data;
> struct drm_panthor_timestamp_info timestamp_info;
> struct drm_panthor_group_priorities_info priorities_info;
> + struct drm_panthor_protected_info protected_info;
> int ret;
>
> if (!args->pointer) {
> @@ -954,6 +963,10 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
> args->size = sizeof(priorities_info);
> return 0;
>
> + case DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO:
> + args->size = sizeof(protected_info);
> + return 0;
> +
> default:
> return -EINVAL;
> }
> @@ -984,6 +997,11 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d
> panthor_query_group_priorities_info(file, &priorities_info);
> return PANTHOR_UOBJ_SET(args->pointer, args->size, priorities_info);
>
> + case DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO:
> + panthor_query_protected_info(ptdev, &protected_info);
> + return PANTHOR_UOBJ_SET(args->pointer, args->size,
> + protected_info);
> +
> default:
> return -EINVAL;
> }
> @@ -1779,6 +1797,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
> * - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
> * - adds drm_panthor_gpu_info::selected_coherency
> * - 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags
> + * - 1.9 - adds DEV_QUERY_PROTECTED_INFO query
It's adding more than just DEV_QUERY_PROTECTED_INFO (it also adds a new
flags field to group_create and a flag that tells that the group intends
to use protected mode).
> */
> static const struct drm_driver panthor_drm_driver = {
> .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1792,7 +1811,7 @@ static const struct drm_driver panthor_drm_driver = {
> .name = "panthor",
> .desc = "Panthor DRM driver",
> .major = 1,
> - .minor = 8,
> + .minor = 9,
>
> .gem_prime_import_sg_table = panthor_gem_prime_import_sg_table,
> .gem_prime_import = panthor_gem_prime_import,
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index acb04250c7def..0e8a1059de589 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -3868,6 +3868,7 @@ static void add_group_kbo_sizes(struct panthor_device *ptdev,
> }
>
> #define MAX_GROUPS_PER_POOL 128
> +#define GROUP_CREATE_FLAGS DRM_PANTHOR_GROUP_CREATE_PROTECTED
>
> int panthor_group_create(struct panthor_file *pfile,
> const struct drm_panthor_group_create *group_args,
> @@ -3882,10 +3883,10 @@ int panthor_group_create(struct panthor_file *pfile,
> u32 gid, i, suspend_size;
> int ret;
>
> - if (group_args->pad)
> + if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT)
> return -EINVAL;
>
> - if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT)
> + if (group_args->flags & ~GROUP_CREATE_FLAGS)
> return -EINVAL;
>
> if ((group_args->compute_core_mask & ~ptdev->gpu_info.shader_present) ||
> @@ -3937,12 +3938,16 @@ int panthor_group_create(struct panthor_file *pfile,
> goto err_put_group;
> }
>
> - suspend_size = csg_iface->control->protm_suspend_size;
> - group->protm_suspend_buf = panthor_fw_alloc_protm_suspend_buf_mem(ptdev, suspend_size);
> - if (IS_ERR(group->protm_suspend_buf)) {
> - ret = PTR_ERR(group->protm_suspend_buf);
> - group->protm_suspend_buf = NULL;
> - goto err_put_group;
> + if (group_args->flags & DRM_PANTHOR_GROUP_CREATE_PROTECTED) {
> + suspend_size = csg_iface->control->protm_suspend_size;
> + group->protm_suspend_buf =
> + panthor_fw_alloc_protm_suspend_buf_mem(ptdev,
> + suspend_size);
> + if (IS_ERR(group->protm_suspend_buf)) {
> + ret = PTR_ERR(group->protm_suspend_buf);
> + group->protm_suspend_buf = NULL;
> + goto err_put_group;
> + }
> }
>
> group->syncobjs = panthor_kernel_bo_create(ptdev, group->vm,
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index 0e455d91e77d4..914110003bcd1 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -253,6 +253,11 @@ enum drm_panthor_dev_query_type {
> * @DRM_PANTHOR_DEV_QUERY_GROUP_PRIORITIES_INFO: Query allowed group priorities information.
> */
> DRM_PANTHOR_DEV_QUERY_GROUP_PRIORITIES_INFO,
> +
> + /**
> + * @DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO: Query supported protected rendering information.
> + */
> + DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO,
> };
>
> /**
> @@ -504,6 +509,28 @@ struct drm_panthor_group_priorities_info {
> __u8 pad[3];
> };
>
> +/**
> + * enum drm_panthor_protected_feature_flags - Supported protected rendering features
Protected rendering is a bit vague, especially since it's usually
referred as protected memory/content in graphics APIs. Maybe we should
have a short paragraph explaining what we mean by that (access of
protected memory from the GPU).
> + *
> + * Place new types at the end, don't re-order, don't remove or replace.
> + */
> +enum drm_panthor_protected_feature_flags {
> + /** @DRM_PANTHOR_PROTECTED_FEATURE_BASIC: Protected rendering available */
> + DRM_PANTHOR_PROTECTED_FEATURE_BASIC = 1 << 0,
I'm not a huge fan of this _BASIC specifier, since it doesn't
tell much about the actual implementation, and what the UMD
has to do to access protected memory from the GPU. Given the
feature/CS-instruction is named _PROTM, I'd go for
/**
* @DRM_PANTHOR_PROTECTED_FEATURE_PROTM: Protected memory access
* based on PROTM CS instructions
*
* This is currently the only option to access protected
* memory from the GPU. Other modes or advanced features might
* be added at some point.
*/
DRM_PANTHOR_PROTECTED_FEATURE_PROTM = 1 << 0,
> +};
> +
> +/**
> + * struct drm_panthor_protected_info - protected support information
> + *
> + * Structure grouping all queryable information relating to the allowed group priorities.
> + */
> +struct drm_panthor_protected_info {
> + /**
> + * @features: Combination of enum drm_panthor_protected_feature_flags flags.
> + */
> + __u32 features;
> +};
> +
> /**
> * struct drm_panthor_dev_query - Arguments passed to DRM_PANTHOR_IOCTL_DEV_QUERY
> */
> @@ -843,6 +870,18 @@ enum drm_panthor_group_priority {
> PANTHOR_GROUP_PRIORITY_REALTIME,
> };
>
> +/**
> + * enum drm_panthor_group_create_flags - Group create flags
> + */
> +enum drm_panthor_group_create_flags {
s/drm_panthor_group_create_flags/drm_panthor_group_feature_flags/
> + /**
> + * @DRM_PANTHOR_GROUP_CREATE_PROTECTED: Support protected mode
> + *
> + * Enable protected rendering work to be executed on this group.
> + */
> + DRM_PANTHOR_GROUP_CREATE_PROTECTED = 1 << 0,
I'd go directly DRM_PANTHOR_GROUP_FEATURE_PROTM, since this is the
instruction the group will use to enter protected mode. If we ever have
multiple ways to do protected rendering, I guess it would materialize
as a different flag, allowing the KMD to know exactly the way
protected rendering is going to be done.
> +};
> +
> /**
> * struct drm_panthor_group_create - Arguments passed to DRM_IOCTL_PANTHOR_GROUP_CREATE
> */
> @@ -877,8 +916,10 @@ struct drm_panthor_group_create {
> /** @priority: Group priority (see enum drm_panthor_group_priority). */
> __u8 priority;
>
> - /** @pad: Padding field, MBZ. */
> - __u32 pad;
> + /**
> + * @flags: Flags. Must be a combination of drm_panthor_group_create_flags flags.
> + */
> + __u32 flags;
>
> /**
> * @compute_core_mask: Mask encoding cores that can be used for compute jobs.
^ permalink raw reply
* Re: [PATCH 7/8] drm/panthor: Add support for entering and exiting protected mode
From: Boris Brezillon @ 2026-05-06 8:51 UTC (permalink / raw)
To: Ketil Johnsen
Cc: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet, Shuah Khan, Sumit Semwal,
Benjamin Gaignard, Brian Starkey, John Stultz, T.J. Mercier,
Christian König, Steven Price, Liviu Dudau, Daniel Almeida,
Alice Ryhl, Matthias Brugger, AngeloGioacchino Del Regno,
dri-devel, linux-doc, linux-kernel, linux-media, linaro-mm-sig,
linux-arm-kernel, linux-mediatek, Florent Tomasin, Paul Toadere,
Samuel Percival
In-Reply-To: <20260505140516.1372388-8-ketil.johnsen@arm.com>
On Tue, 5 May 2026 16:05:13 +0200
Ketil Johnsen <ketil.johnsen@arm.com> wrote:
Here comes the second part of the review :-).
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 2ab444ee8c710..e93042eaf3fc8 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -100,8 +100,11 @@ static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status)
> fault_status, panthor_exception_name(ptdev, fault_status & 0xFF),
> address);
> }
> - if (status & GPU_IRQ_PROTM_FAULT)
> + if (status & GPU_IRQ_PROTM_FAULT) {
> drm_warn(&ptdev->base, "GPU Fault in protected mode\n");
> + panthor_gpu_disable_protm_fault_interrupt(ptdev);
It's only used in a single place, so I'd just inline the content of
panthor_gpu_disable_protm_fault_interrupt() here. Also, I think
panthor_gpu_disable_protm_fault_interrupt() is not taking the right
lock (see below).
> + panthor_device_schedule_reset(ptdev);
> + }
>
> spin_lock(&ptdev->gpu->reqs_lock);
> if (status & ptdev->gpu->pending_reqs) {
> @@ -367,6 +370,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev)
> unsigned long flags;
>
> spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> +
> + /** Re-enable the protm_irq_fault when reset is complete */
> + ptdev->gpu->irq.mask |= GPU_IRQ_PROTM_FAULT;
panthor_irq::mask should only be modified with the
panthor_irq::mask_lock held. Besides, we have a helper for
that:
panthor_gpu_irq_enable_events(&ptdev->gpu->irq, GPU_IRQ_PROTM_FAULT);
> +
> if (!drm_WARN_ON(&ptdev->base,
> ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED)) {
> ptdev->gpu->pending_reqs |= GPU_IRQ_RESET_COMPLETED;
> @@ -427,3 +434,8 @@ void panthor_gpu_resume(struct panthor_device *ptdev)
> panthor_hw_l2_power_on(ptdev);
> }
>
> +void panthor_gpu_disable_protm_fault_interrupt(struct panthor_device *ptdev)
> +{
> + scoped_guard(spinlock_irqsave, &ptdev->gpu->reqs_lock)
> + ptdev->gpu->irq.mask &= ~GPU_IRQ_PROTM_FAULT;
Same problem with wrong lock being taken to modify the mask, and
panthor_gpu_irq_disable_events() probably being a better option?
> +}
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.h b/drivers/gpu/drm/panthor/panthor_gpu.h
> index 12c263a399281..ca66c73f543e6 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.h
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.h
> @@ -54,4 +54,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev);
> void panthor_gpu_power_changed_off(struct panthor_device *ptdev);
> int panthor_gpu_power_changed_on(struct panthor_device *ptdev);
>
> +/**
> + * panthor_gpu_disable_protm_fault_interrupt() - Disable GPU_PROTECTED_FAULT interrupt
> + * @ptdev: Device.
> + */
> +void panthor_gpu_disable_protm_fault_interrupt(struct panthor_device *ptdev);
> +
> #endif
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 07f54176ec1bf..702f537905b56 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -31,6 +31,7 @@
> #include <linux/sizes.h>
>
> #include "panthor_device.h"
> +#include "panthor_fw.h"
> #include "panthor_gem.h"
> #include "panthor_gpu.h"
> #include "panthor_heap.h"
> @@ -1704,8 +1705,12 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
> if (drm_WARN_ON(&ptdev->base, vm->locked_region.size))
> return -EINVAL;
>
> + down_read(&ptdev->protm.lock);
> +
> mutex_lock(&ptdev->mmu->as.slots_lock);
> if (vm->as.id >= 0 && size) {
> + panthor_fw_protm_exit_sync(ptdev);
> +
> /* Lock the region that needs to be updated */
> gpu_write64(ptdev, AS_LOCKADDR(vm->as.id),
> pack_region_range(ptdev, &start, &size));
> @@ -1720,6 +1725,9 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
> }
> mutex_unlock(&ptdev->mmu->as.slots_lock);
>
> + if (ret)
> + up_read(&ptdev->protm.lock);
Let's try to keep the locked section local to a function: the protm.lock
should, IMHO, be taken/released in panthor_vm_exec_op(). If we go for
that, this also makes the panthor_vm_lock_region() vs
panthor_vm_expand_locked_region() distinction useless, though it's
probably fine to keep it for clarity.
> +
> return ret;
> }
>
> @@ -1805,6 +1813,8 @@ static void panthor_vm_unlock_region(struct panthor_vm *vm)
> vm->locked_region.start = 0;
> vm->locked_region.size = 0;
> mutex_unlock(&ptdev->mmu->as.slots_lock);
> +
> + up_read(&ptdev->protm.lock);
> }
>
> static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 987072bd867c4..acb04250c7def 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -308,6 +308,15 @@ struct panthor_scheduler {
> */
> struct list_head stopped_groups;
> } reset;
> +
> + /** @protm: Protected mode related fields. */
> + struct {
> + /** @protected_mode: True if GPU is in protected mode. */
> + bool protected_mode;
nit: s/protected_mode/enabled/s. But do we even need that boolean?
Isn't active_group != NULL providing the same info?
> +
> + /** @active_group: The active protected group. */
> + struct panthor_group *active_group;
> + } protm;
> };
>
> /**
> @@ -570,6 +579,16 @@ struct panthor_group {
> /** @fatal_queues: Bitmask reflecting the queues that hit a fatal exception. */
> u32 fatal_queues;
>
> + /**
> + * @protm_pending_queues: Bitmask reflecting the queues that are waiting
> + * on a CS_PROTM_PENDING.
> + *
> + * The GPU will set the bit associated to the queue pending protected mode
> + * when a PROT_REGION command is executing or when trying to resume previously
> + * suspended protected mode jobs.
> + */
> + u32 protm_pending_queues;
> +
> /** @tiler_oom: Mask of queues that have a tiler OOM event to process. */
> atomic_t tiler_oom;
>
> @@ -1176,6 +1195,7 @@ queue_resume_timeout(struct panthor_queue *queue)
> * @ptdev: Device.
> * @csg_id: Group slot ID.
> * @cs_id: Queue slot ID.
> + * @protm_ack: Acknowledge pending protected mode queues
> *
> * Program a queue slot with the queue information so things can start being
> * executed on this queue.
> @@ -1472,6 +1492,34 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority)
> return 0;
> }
>
> +static void
> +cs_slot_process_protm_pending_event_locked(struct panthor_device *ptdev,
> + u32 csg_id, u32 cs_id)
> +{
> + struct panthor_scheduler *sched = ptdev->scheduler;
> + struct panthor_csg_slot *csg_slot = &sched->csg_slots[csg_id];
> + struct panthor_group *group = csg_slot->group;
> +
> + lockdep_assert_held(&sched->lock);
> +
> + if (!group)
> + return;
> +
> + /* No protected memory heap, a user space program tried to
> + * submit a protected mode jobs resulting in the GPU raising
> + * a CS_PROTM_PENDING request.
> + *
> + * This scenario is invalid and the protected mode jobs must
> + * not be allowed to progress.
> + */
> + if (!ptdev->protm.heap)
> + return;
Should we flag the group unusable when that happens and schedule it out
as soon as possible?
if (!ptdev->protm.heap)
group->fatal_queues |= BIT(cs_id);
else
group->protm_pending_queues |= BIT(cs_id);
sched_queue_delayed_work(sched, tick, 0);
> +
> + group->protm_pending_queues |= BIT(cs_id);
> +
> + sched_queue_delayed_work(sched, tick, 0);
> +}
> +
> static void
> cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
> u32 csg_id, u32 cs_id)
> @@ -1718,6 +1766,9 @@ static bool cs_slot_process_irq_locked(struct panthor_device *ptdev,
> if (events & CS_TILER_OOM)
> cs_slot_process_tiler_oom_event_locked(ptdev, csg_id, cs_id);
>
> + if (events & CS_PROTM_PENDING)
> + cs_slot_process_protm_pending_event_locked(ptdev, csg_id, cs_id);
> +
> /* We don't acknowledge the TILER_OOM event since its handling is
> * deferred to a separate work.
> */
> @@ -1848,6 +1899,17 @@ static void sched_process_idle_event_locked(struct panthor_device *ptdev)
> sched_queue_delayed_work(ptdev->scheduler, tick, 0);
> }
>
> +static void sched_process_protm_exit_event_locked(struct panthor_device *ptdev)
> +{
> + lockdep_assert_held(&ptdev->scheduler->lock);
> +
> + /* Acknowledge the protm exit and schedule a tick. */
> + panthor_fw_protm_exit(ptdev);
Let's just inline the content of panthor_fw_protm_exit() here.*
It doesn't make sense to have all these indirections, especially
since PROTM and scheduling are intertwined anyway, so I consider
it part of the scheduler responsibility (just like the scheduler
deals with other GLB events).
The same goes for the other panthor_fw_protm_ helpers defined in
panthor_fw.c, I think panthor_sched.c would be a better fit for
those, or even inlining their content if they are only used in
a single place.
> + sched_queue_delayed_work(ptdev->scheduler, tick, 0);
> + ptdev->scheduler->protm.protected_mode = false;
> + ptdev->scheduler->protm.active_group = NULL;
> +}
> +
> /**
> * sched_process_global_irq_locked() - Process the scheduling part of a global IRQ
> * @ptdev: Device.
> @@ -1863,6 +1925,9 @@ static void sched_process_global_irq_locked(struct panthor_device *ptdev)
> ack = READ_ONCE(glb_iface->output->ack);
> evts = (req ^ ack) & GLB_EVT_MASK;
>
> + if (evts & GLB_PROTM_EXIT)
> + sched_process_protm_exit_event_locked(ptdev);
> +
> if (evts & GLB_IDLE)
> sched_process_idle_event_locked(ptdev);
> }
> @@ -1872,23 +1937,71 @@ static void process_fw_events_work(struct work_struct *work)
> struct panthor_scheduler *sched = container_of(work, struct panthor_scheduler,
> fw_events_work);
> u32 events = atomic_xchg(&sched->fw_events, 0);
> + u32 csg_events = events & ~JOB_INT_GLOBAL_IF;
> struct panthor_device *ptdev = sched->ptdev;
>
> mutex_lock(&sched->lock);
>
> + while (csg_events) {
> + u32 csg_id = ffs(csg_events) - 1;
> +
> + sched_process_csg_irq_locked(ptdev, csg_id);
> + csg_events &= ~BIT(csg_id);
> + }
I'm sure you have a good reason to re-order the processing
of CSG and GLB events, and it'd be good to have it documented
in a comment.
> +
> if (events & JOB_INT_GLOBAL_IF) {
> sched_process_global_irq_locked(ptdev);
> events &= ~JOB_INT_GLOBAL_IF;
> }
>
> - while (events) {
> - u32 csg_id = ffs(events) - 1;
> + mutex_unlock(&sched->lock);
> +}
>
> - sched_process_csg_irq_locked(ptdev, csg_id);
> - events &= ~BIT(csg_id);
> +static void handle_protm_fault(struct panthor_device *ptdev)
This is a bit of a misnomer, I think. It doesn't seem to be triggered
by a FAULT event, it's more a timeout on a PROTM section that would
lead to a reset being scheduled, and this "blocked-in-protm" situation
being detected as part of the reset.
> +{
> + struct panthor_scheduler *sched = ptdev->scheduler;
> + u32 csg_id;
> + struct panthor_group *protm_group;
> +
> + guard(mutex)(&sched->lock);
> +
> + if (!sched->protm.protected_mode)
> + return;
> +
> + protm_group = sched->protm.active_group;
> +
> + if (drm_WARN_ON(&ptdev->base, !protm_group))
> + return;
See, that's a perfect example of sched->protm.protected_mode being redundant.
Now you're left with a potential protected_mode=true,active_group=NULL
inconsistency you don't expect.
> +
> + /* Group will be terminated by the device reset */
> + protm_group->fatal_queues |= GENMASK(protm_group->queue_count - 1, 0);
> +
> + if (!panthor_fw_protm_exit_wait_event_timeout(ptdev))
> + goto cleanup_protm;
> +
> + /**
> + * GPU failed to exit protected mode. Mark all non-protected mode CSGs
/* GPU failed to exit protected mode. Mark all non-protected mode CSGs
> + * as suspended so that they are unaffected by the GPU reset.
> + */
> +
> + for (csg_id = 0; csg_id < sched->csg_slot_count; csg_id++) {
> + struct panthor_group *group = sched->csg_slots[csg_id].group;
> +
> + if (!group || group == protm_group)
> + continue;
> +
> + group->state = PANTHOR_CS_GROUP_SUSPENDED;
> +
> + group_unbind_locked(group);
> +
> + list_move(&group->run_node, group_is_idle(group) ?
> + &sched->groups.idle[group->priority] :
> + &sched->groups.runnable[group->priority]);
nit: Let's use a local struct list_head * variable to select the right list.
> }
>
> - mutex_unlock(&sched->lock);
> +cleanup_protm:
> + sched->protm.protected_mode = false;
> + sched->protm.active_group = NULL;
> }
>
> /**
> @@ -2029,6 +2142,7 @@ struct panthor_sched_tick_ctx {
> bool immediate_tick;
> bool stop_tick;
> u32 csg_upd_failed_mask;
> + struct panthor_group *protm_group;
> };
>
> static bool
> @@ -2299,6 +2413,7 @@ tick_ctx_evict_group(struct panthor_scheduler *sched,
>
> static void
> tick_ctx_reschedule_group(struct panthor_scheduler *sched,
> + struct panthor_sched_tick_ctx *ctx,
> struct panthor_csg_slots_upd_ctx *upd_ctx,
> struct panthor_group *group,
> int new_csg_prio)
> @@ -2321,6 +2436,30 @@ tick_ctx_reschedule_group(struct panthor_scheduler *sched,
> csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
> CSG_ENDPOINT_CONFIG);
> }
> +
> + if (ctx->protm_group == group) {
> + for (u32 q = 0; q < group->queue_count; q++) {
> + struct panthor_fw_cs_iface *cs_iface;
> +
> + if (!(group->protm_pending_queues & BIT(q)))
> + continue;
> +
> + cs_iface = panthor_fw_get_cs_iface(ptdev, group->csg_id, q);
> + panthor_fw_update_reqs(cs_iface, req, cs_iface->output->ack,
> + CS_PROTM_PENDING);
> + }
> +
> + panthor_fw_toggle_reqs(csg_iface, doorbell_req, doorbell_ack,
> + group->protm_pending_queues);
> + csgs_upd_ctx_ring_doorbell(upd_ctx, group->csg_id);
> + group->protm_pending_queues = 0;
> +
> + /*
> + * We only allow one protected group to run at same time,
> + * as it makes it easier to handle faults in protected mode.
It's more something to document in the panthor_scheduler::protm::active_group
section.
> + */
> + sched->protm.active_group = group;
Would it make sense to move this logic to a tick_ctx_handle_protm_group()
helper that's called before/after tick_ctx_reschedule_group()? This way
there's no extra if (ctx->protm_group == group) conditional branch in here.
static void
tick_ctx_handle_protm_group(struct panthor_scheduler *sched,
struct panthor_sched_tick_ctx *ctx,
struct panthor_csg_slots_upd_ctx *upd_ctx)
{
struct panthor_device *ptdev = sched->ptdev;
struct panthor_group *group = ctx->protm_group;
struct panthor_fw_csg_iface *csg_iface;
if (!group || drm_WARN_ON(&ptdev->base, group->csg_id < 0))
return;
csg_iface = panthor_fw_get_csg_iface(ptdev, group->csg_id);
for (u32 q = 0; q < group->queue_count; q++) {
struct panthor_fw_cs_iface *cs_iface;
if (!(group->protm_pending_queues & BIT(q)))
continue;
cs_iface = panthor_fw_get_cs_iface(ptdev, group->csg_id, q);
panthor_fw_update_reqs(cs_iface, req, cs_iface->output->ack,
CS_PROTM_PENDING);
}
panthor_fw_toggle_reqs(csg_iface, doorbell_req, doorbell_ack,
group->protm_pending_queues);
csgs_upd_ctx_ring_doorbell(upd_ctx, group->csg_id);
group->protm_pending_queues = 0;
sched->protm.active_group = group;
}
> + }
> }
>
> static void
> @@ -2336,6 +2475,17 @@ tick_ctx_schedule_group(struct panthor_scheduler *sched,
> group_bind_locked(group, csg_id);
> csg_slot_prog_locked(ptdev, csg_id, csg_prio);
>
> + /* If the group was waiting for protected mode before suspension,
> + * and the tick context enters this mode, it should be serviced
> + * immediately because the slot reset should have set the
> + * CS_PROTM_PENDING bit to zero, and cs_prog_slot_locked() sets it to
> + * zero too.
> + * It's not clear if we will get a new CS_PROTM_PENDING event in that
> + * case, but it should be safe either way.
> + */
> + if (group->protm_pending_queues && ctx->protm_group)
> + group->protm_pending_queues = 0;
I'd move this to the path where we do the SUSPEND, or group_unbind(), even.
> +
> csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, csg_id,
> group->state == PANTHOR_CS_GROUP_SUSPENDED ?
> CSG_STATE_RESUME : CSG_STATE_START,
> @@ -2365,7 +2515,7 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
>
> /* Update priorities on already running groups. */
> list_for_each_entry(group, &ctx->groups[prio], run_node) {
> - tick_ctx_reschedule_group(sched, &upd_ctx, group, new_csg_prio--);
> + tick_ctx_reschedule_group(sched, ctx, &upd_ctx, group, new_csg_prio--);
> }
> }
>
> @@ -2457,6 +2607,15 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
>
> sched->used_csg_slot_count = ctx->group_count;
> sched->might_have_idle_groups = ctx->idle_group_count > 0;
> +
> + if (ctx->protm_group) {
> + ret = panthor_fw_protm_enter(ptdev);
> + if (ret) {
> + panthor_device_schedule_reset(ptdev);
> + ctx->csg_upd_failed_mask = U32_MAX;
It's weird to flag it as all CSGs update failed. Should we instead
have
/* If we failed to enter PROTM, consider the group who
* requested it as failed.
*/
ctx->csg_upd_failed_mask |= BIT(ctx->protm_group->csg_id);
> + }
> + sched->protm.protected_mode = true;
I'd move that to a tick_ctx_service_protm_req() helper that has the
panthor_fw_protm_enter() inlined, because again, it doesn't make
sense to have this defined in panthor_fw.c if the only user lives
in panthor_sched.c
> + }
> }
>
> static u64
> @@ -2490,7 +2649,7 @@ static void tick_work(struct work_struct *work)
> u64 resched_target = sched->resched_target;
> u64 remaining_jiffies = 0, resched_delay;
> u64 now = get_jiffies_64();
> - int prio, ret, cookie;
> + int prio, protm_prio, ret, cookie;
> bool full_tick;
>
> if (!drm_dev_enter(&ptdev->base, &cookie))
> @@ -2564,14 +2723,49 @@ static void tick_work(struct work_struct *work)
> }
> }
>
> + /* Check if the highest priority group want to switch to protected mode */
> + for (protm_prio = PANTHOR_CSG_PRIORITY_COUNT - 1; protm_prio >= 0; protm_prio--) {
> + struct panthor_group *group;
> +
> + group = list_first_entry_or_null(&ctx.groups[protm_prio],
> + struct panthor_group,
> + run_node);
> + if (group) {
> + ctx.protm_group = group;
> + break;
> + }
Should this be
if (group) {
if (group->protm_pending_queues)
ctx.protm_group = group;
break;
}
?
> + }
> +
> /* If we have free CSG slots left, pick idle groups */
> - for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
> - prio >= 0 && !tick_ctx_is_full(sched, &ctx);
> - prio--) {
How about we keep it a single indentation level and skip higher prios if
PROTM is requested:
/* Pick only idle groups with equal or lower priority than the
* group triggering protected mode. Do not bother picking
* unscheduled idle groups.
*/
if (ctx.protm_group && prio < protm_prio)
continue;
This saves us an indentation level and limits the code duplication.
> - /* Check the old_group queue first to avoid reprogramming the slots */
> - tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], false, true);
> - tick_ctx_pick_groups_from_list(sched, &ctx, &sched->groups.idle[prio],
> - false, false);
> + if (ctx.protm_group) {
> + /* Pick only idle groups with equal or lower priority than the
> + * group triggering protected mode. Do not bother picking
> + * unscheduled idle groups.
> + */
> + for (prio = protm_prio;
> + prio >= 0 && !tick_ctx_is_full(sched, &ctx);
> + prio--)
> + tick_ctx_pick_groups_from_list(sched, &ctx,
> + &ctx.old_groups[prio],
> + false, true);
> + } else {
> + /* No switch to protected, just pick any idle group according
> + * to priority
> + */
> + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
> + prio >= 0 && !tick_ctx_is_full(sched, &ctx);
> + prio--) {
> + /* Check the old_group queue first to avoid
> + * reprogramming the slots
> + */
> + tick_ctx_pick_groups_from_list(sched, &ctx,
> + &ctx.old_groups[prio],
> + false, true);
> + tick_ctx_pick_groups_from_list(sched, &ctx,
> + &sched->groups.idle[prio],
> + false, false);
> + }
> +
> }
>
> tick_ctx_apply(sched, &ctx);
> @@ -2993,6 +3187,8 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
> cancel_work_sync(&sched->sync_upd_work);
> cancel_delayed_work_sync(&sched->tick_work);
>
> + handle_protm_fault(ptdev);
I actually wonder if this should be part of the panthor_sched_suspend()
logic. That is, we would automatically flag all non-protm groups as
suspended if the GPU was in PROTM mode at the time the hang happened.
> +
> panthor_sched_suspend(ptdev);
>
> /* Stop all groups that might still accept jobs, so we don't get passed
Regards,
Boris
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox