[PATCH 0/4] drm/vc4: Reset the V3D block through the reset controller

[PATCH 0/4] drm/vc4: Reset the V3D block through the reset controller

[Maíra Canal]

vc4_reset() recovers a hung GPU by dropping the runtime PM usage count to
zero so that the V3D power domain goes down, and then taking it again.
Forcing the count to zero only works if the driver knows exactly how many
references it holds, which is why vc4 unconventionaly wraps every
pm_runtime_get_sync() call in a private refcount and mutex, making the
code cumbersome.

The BCM2835 PM block has exposed a reset line for the V3D block since
commit 670c672608a1 ("soc: bcm: bcm2835-pm: Add support for power domains
under a new binding."), added for exactly this reason, but vc4 never
picked it up. This series switches vc4 over to it and drops the private
refcount, leaving vc4_v3d_pm_get/put() as plain runtime PM wrappers.

The reset line is optional, so device trees that do not describe one keep
working: they get the driver-side recovery in vc4_irq_reset() while the
hardware is left untouched. Of the two in-tree platforms,

  1. BCM2835 gains the property in PATCH 4.
  2. Cygnus has no V3D power domain to begin with, so the power-cycle
     only ever gated its clock there.

PATCH 1 is an independent cleanup. "brcm,vc4-v3d" was never documented
and was never used by any in-tree device tree, so it is a candidate for
removal on its own. It can be dropped from the series if it turns out
to be contentious.

This series was tested on the Raspberry Pi 3B and 3B+.

Best regards,
- Maíra

---
Maíra Canal (4):
      drm/vc4: Drop the undocumented brcm,vc4-v3d compatible
      dt-bindings: display: bcm2835-v3d: Add an optional reset line
      drm/vc4: Use the reset controller to recover from a GPU hang
      ARM: dts: bcm2835: Add the V3D reset line

 .../bindings/display/brcm,bcm2835-v3d.yaml         |  3 ++
 arch/arm/boot/dts/broadcom/bcm2835-common.dtsi     |  1 +
 drivers/gpu/drm/vc4/vc4_drv.c                      |  1 -
 drivers/gpu/drm/vc4/vc4_drv.h                      | 13 ++++---
 drivers/gpu/drm/vc4/vc4_gem.c                      | 40 +++++++++++++---------
 drivers/gpu/drm/vc4/vc4_irq.c                      |  7 ++--
 drivers/gpu/drm/vc4/vc4_v3d.c                      | 37 +++++++-------------
 7 files changed, 51 insertions(+), 51 deletions(-)
---
base-commit: 09b47186a4164f3aaa3591313f80794443117342
change-id: 20260812-vc4-reset-control-cddf3eb8a9e9

[PATCH 1/4] drm/vc4: Drop the undocumented brcm,vc4-v3d compatible

[Maíra Canal]

"brcm,vc4-v3d" is the compatible string V3D support originally shipped
with in commit d3f5168a0810 ("drm/vc4: Bind and initialize the V3D
engine."). Commit 90d7116061f8 ("drm/vc4: Recognize a more specific
compatible string for V3D.") added "brcm,bcm2835-v3d" in v4.6 and kept
the original one around, because the Raspberry Pi firmware was shipping
device trees that used it.

That string was never part of the binding: the documentation added in
commit 4653f22e9ab0 ("dt-bindings: Add binding docs for V3D.") describes
only "brcm,bcm2835-v3d", and no upstream device tree has ever used
"brcm,vc4-v3d": the first in-tree V3D node, from commit 49ac67e0c39c
("ARM: bcm2835: Add VC4 to the device tree."), already carried the
specific name. The firmware device trees the compatible was kept for
predate v4.6, released in 2016.

Drop it from both match tables.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 1 -
 drivers/gpu/drm/vc4/vc4_v3d.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 616caf9d9915..eb859c65a453 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -279,7 +279,6 @@ static const struct of_device_id vc4_dma_range_matches[] = {
 	{ .compatible = "brcm,bcm2835-hvs" },
 	{ .compatible = "brcm,bcm2835-v3d" },
 	{ .compatible = "brcm,cygnus-v3d" },
-	{ .compatible = "brcm,vc4-v3d" },
 	{}
 };
 
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index d31b906cb8e7..379ab77243ad 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -531,7 +531,6 @@ static void vc4_v3d_dev_remove(struct platform_device *pdev)
 const struct of_device_id vc4_v3d_dt_match[] = {
 	{ .compatible = "brcm,bcm2835-v3d" },
 	{ .compatible = "brcm,cygnus-v3d" },
-	{ .compatible = "brcm,vc4-v3d" },
 	{}
 };
 

-- 
2.55.0

Re: [PATCH 1/4] drm/vc4: Drop the undocumented brcm,vc4-v3d compatible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Removing the 'brcm,vc4-v3d' compatible string breaks Device Tree ABI compatibility with older Raspberry Pi firmware.
--

--- Patch [1]: [PATCH 1/4] drm/vc4: Drop the undocumented brcm,vc4-v3d compatible ---
commit 0d9402a3018a535c49f9c7ef653bfa693de83e10
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Drop the undocumented brcm,vc4-v3d compatible

This commit removes the "brcm,vc4-v3d" compatible string from the VC4 driver's
match tables. It notes that the string was never part of the upstream device
tree bindings and was only kept for older Raspberry Pi firmware.

> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -279,7 +279,6 @@ static const struct of_device_id vc4_dma_range_matches[] = {
>  	{ .compatible = "brcm,bcm2835-hvs" },
>  	{ .compatible = "brcm,bcm2835-v3d" },
>  	{ .compatible = "brcm,cygnus-v3d" },
> -	{ .compatible = "brcm,vc4-v3d" },

[Severity: Critical]
Does removing this compatible string break Device Tree ABI compatibility?

The commit message mentions that firmware device trees predating v4.6
(released in 2016) use this string. Since the Linux kernel generally
treats Device Tree as a stable ABI, will dropping this cause users
who upgrade their kernel but not their boot firmware to lose DMA
range matching and 3D hardware acceleration?

>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -531,7 +531,6 @@ static void vc4_v3d_dev_remove(struct platform_device *pdev)
>  const struct of_device_id vc4_v3d_dt_match[] = {
>  	{ .compatible = "brcm,bcm2835-v3d" },
>  	{ .compatible = "brcm,cygnus-v3d" },
> -	{ .compatible = "brcm,vc4-v3d" },

[Severity: Critical]
Similarly here in vc4_v3d_dt_match, does removing this prevent the VC4
V3D driver from successfully probing on those older firmware device trees?

>  	{}
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816-vc4-reset-control-v1-0-896444373716@igalia.com?part=1

[PATCH 2/4] dt-bindings: display: bcm2835-v3d: Add an optional reset line

[Maíra Canal]

The BCM2835 PM block has exposed a reset line for the V3D block since
commit 670c672608a1 ("soc: bcm: bcm2835-pm: Add support for power domains
under a new binding."), which allows the GPU to be reset without
power-cycling its domain. Document it so that the vc4 driver can use it to
recover from a GPU hang.

Keep the property optional, as existing device trees do not describe it.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 Documentation/devicetree/bindings/display/brcm,bcm2835-v3d.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-v3d.yaml b/Documentation/devicetree/bindings/display/brcm,bcm2835-v3d.yaml
index c55a8217de25..1063e686647c 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm2835-v3d.yaml
+++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-v3d.yaml
@@ -27,6 +27,9 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg

-- 
2.55.0

[PATCH 3/4] drm/vc4: Use the reset controller to recover from a GPU hang

[Maíra Canal]

vc4_reset() recovers a hung GPU by dropping the runtime PM usage count to
zero so that the power domain goes down, and then taking it again. Such an
unconventional approach only works if the driver knows exactly how many
references it holds, which is why vc4 wrapped every pm_runtime_get_sync()
call in a private refcount and mutex.

Commit 670c672608a1 ("soc: bcm: bcm2835-pm: Add support for power domains
under a new binding.") exposed a V3D reset line for exactly this reason,
so that the block can be reset without power-cycling its domain, but the
vc4 driver never picked it up. Use it now, which removes the need for the
private refcount and leaves vc4_v3d_pm_get/put() as plain runtime PM
wrappers.

The reset line is optional, to accommodate older device trees. Device
trees that do not describe one still get the driver-side recovery in
vc4_irq_reset(), but the hardware is left untouched. Two in-tree platforms
use the VC4 V3D block: BCM2835 gains the resets property in the next
commit, and Cygnus is no worse off than it was, as its V3D node has no
power domain and the power-cycle only ever gated its clock.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h | 13 ++++++++-----
 drivers/gpu/drm/vc4/vc4_gem.c | 40 ++++++++++++++++++++++++----------------
 drivers/gpu/drm/vc4/vc4_irq.c |  7 +++----
 drivers/gpu/drm/vc4/vc4_v3d.c | 36 ++++++++++++------------------------
 4 files changed, 47 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 0f5958c1e6b6..86e0e7c12901 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -212,14 +212,9 @@ struct vc4_dev {
 
 	struct work_struct overflow_mem_work;
 
-	int power_refcount;
-
 	/* Set to true when the load tracker is active. */
 	bool load_tracker_enabled;
 
-	/* Mutex controlling the power refcount. */
-	struct mutex power_lock;
-
 	struct {
 		struct timer_list timer;
 		struct work_struct reset_work;
@@ -294,6 +289,13 @@ struct vc4_v3d {
 	struct platform_device *pdev;
 	void __iomem *regs;
 	struct clk *clk;
+
+	/* Reset line for the V3D block, used to recover from a GPU hang.
+	 * NULL if the device tree does not describe one, in which case the
+	 * GPU cannot be reset.
+	 */
+	struct reset_control *reset;
+
 	struct debugfs_regset32 regset;
 };
 
@@ -1056,6 +1058,7 @@ int vc4_v3d_bin_bo_get(struct vc4_dev *vc4, bool *used);
 void vc4_v3d_bin_bo_put(struct vc4_dev *vc4);
 int vc4_v3d_pm_get(struct vc4_dev *vc4);
 void vc4_v3d_pm_put(struct vc4_dev *vc4);
+void vc4_v3d_init_hw(struct drm_device *dev);
 int vc4_v3d_debugfs_init(struct drm_minor *minor);
 
 /* vc4_validate.c */
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e231c906709c..3212b9167620 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -23,7 +23,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/sched/signal.h>
@@ -292,19 +292,22 @@ vc4_save_hang_state(struct drm_device *dev)
 static void
 vc4_reset(struct drm_device *dev)
 {
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_v3d *v3d = to_vc4_dev(dev)->v3d;
+	int ret;
 
-	DRM_INFO("Resetting GPU.\n");
+	vc4_irq_disable(dev);
 
-	mutex_lock(&vc4->power_lock);
-	if (vc4->power_refcount) {
-		/* Power the device off and back on the by dropping the
-		 * reference on runtime PM.
-		 */
-		pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
-		pm_runtime_get_sync(&vc4->v3d->pdev->dev);
+	if (v3d->reset) {
+		drm_info(dev, "Resetting GPU.\n");
+
+		ret = reset_control_reset(v3d->reset);
+		if (ret)
+			drm_err(dev, "Failed to reset the GPU: %d\n", ret);
+
+		vc4_v3d_init_hw(dev);
+	} else {
+		drm_info_once(dev, "No reset line; GPU state is not reset.\n");
 	}
-	mutex_unlock(&vc4->power_lock);
 
 	vc4_irq_reset(dev);
 
@@ -320,10 +323,19 @@ vc4_reset_work(struct work_struct *work)
 {
 	struct vc4_dev *vc4 =
 		container_of(work, struct vc4_dev, hangcheck.reset_work);
+	int ret;
+
+	/* Make sure the device is not suspended during the reset. */
+	ret = vc4_v3d_pm_get(vc4);
+	if (ret) {
+		drm_err(&vc4->base, "Failed to resume V3D for GPU reset: %d\n", ret);
+		return;
+	}
 
 	vc4_save_hang_state(&vc4->base);
-
 	vc4_reset(&vc4->base);
+
+	vc4_v3d_pm_put(vc4);
 }
 
 static void
@@ -1177,10 +1189,6 @@ int vc4_gem_init(struct drm_device *dev)
 
 	INIT_WORK(&vc4->job_done_work, vc4_job_done_work);
 
-	ret = drmm_mutex_init(dev, &vc4->power_lock);
-	if (ret)
-		return ret;
-
 	INIT_LIST_HEAD(&vc4->purgeable.list);
 
 	ret = drmm_mutex_init(dev, &vc4->purgeable.lock);
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 8e5141bb5075..90c5194a1c93 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -336,10 +336,9 @@ void vc4_irq_reset(struct drm_device *dev)
 	V3D_WRITE(V3D_INTCTL, V3D_DRIVER_IRQS);
 
 	/*
-	 * Turn all our interrupts on.  Binner out of memory is the
-	 * only one we expect to trigger at this point, since we've
-	 * just come from poweron and haven't supplied any overflow
-	 * memory yet.
+	 * Turn all our interrupts on. Binner out of memory is the only
+	 * one we expect to trigger at this point, since the reset cleared
+	 * the overflow memory address and none has been supplied yet.
 	 */
 	V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS);
 
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 379ab77243ad..d2da8d2f8eeb 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -9,6 +9,7 @@
 #include <linux/component.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #include <drm/drm_print.h>
 
@@ -122,29 +123,13 @@ static int vc4_v3d_debugfs_ident(struct seq_file *m, void *unused)
 	return 0;
 }
 
-/*
- * Wraps pm_runtime_get_sync() in a refcount, so that we can reliably
- * get the pm_runtime refcount to 0 in vc4_reset().
- */
 int
 vc4_v3d_pm_get(struct vc4_dev *vc4)
 {
 	if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
 		return -ENODEV;
 
-	mutex_lock(&vc4->power_lock);
-	if (vc4->power_refcount++ == 0) {
-		int ret = pm_runtime_get_sync(&vc4->v3d->pdev->dev);
-
-		if (ret < 0) {
-			vc4->power_refcount--;
-			mutex_unlock(&vc4->power_lock);
-			return ret;
-		}
-	}
-	mutex_unlock(&vc4->power_lock);
-
-	return 0;
+	return pm_runtime_resume_and_get(&vc4->v3d->pdev->dev);
 }
 
 void
@@ -153,15 +138,10 @@ vc4_v3d_pm_put(struct vc4_dev *vc4)
 	if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
 		return;
 
-	mutex_lock(&vc4->power_lock);
-	if (--vc4->power_refcount == 0) {
-		pm_runtime_mark_last_busy(&vc4->v3d->pdev->dev);
-		pm_runtime_put_autosuspend(&vc4->v3d->pdev->dev);
-	}
-	mutex_unlock(&vc4->power_lock);
+	pm_runtime_put_autosuspend(&vc4->v3d->pdev->dev);
 }
 
-static void vc4_v3d_init_hw(struct drm_device *dev)
+void vc4_v3d_init_hw(struct drm_device *dev)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 
@@ -447,6 +427,14 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(v3d->clk))
 		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
 
+	v3d->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(v3d->reset))
+		return dev_err_probe(dev, PTR_ERR(v3d->reset),
+				     "Failed to get reset control\n");
+
+	if (!v3d->reset)
+		drm_warn(drm, "No V3D reset line in the device tree");
+
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
 		return ret;

-- 
2.55.0

Re: [PATCH 3/4] drm/vc4: Use the reset controller to recover from a GPU hang

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Removing the fallback GPU power-cycle reset mechanism breaks GPU hang recovery and introduces a critical DMA Use-After-Free vulnerability on older device trees lacking a reset line.

Pre-existing issues:
- [High] The hangcheck timer and associated work items are never canceled during device teardown, leading to a Use-After-Free if they execute after the DRM device is freed.
- [High] Requesting exclusive resources using `devm_` APIs inside a component `bind` callback causes re-bind operations to fail with -EBUSY.
--

--- Patch [3]: [PATCH 3/4] drm/vc4: Use the reset controller to recover from a GPU hang ---
commit 1ede01913981622e3c76606cafec272dc32b7700
Author: Maíra Canal <mcanal@igalia.com>

drm/vc4: Use the reset controller to recover from a GPU hang

vc4_reset() recovers a hung GPU by dropping the runtime PM usage count to
zero so that the power domain goes down, and then taking it again. Such an
unconventional approach only works if the driver knows exactly how many
references it holds, which is why vc4 wrapped every pm_runtime_get_sync()
call in a private refcount and mutex.

Commit 670c672608a1 ("soc: bcm: bcm2835-pm: Add support for power domains
under a new binding.") exposed a V3D reset line for exactly this reason,
so that the block can be reset without power-cycling its domain, but the
vc4 driver never picked it up. Use it now, which removes the need for the
private refcount and leaves vc4_v3d_pm_get/put() as plain runtime PM
wrappers.

The reset line is optional, to accommodate older device trees. Device
trees that do not describe one still get the driver-side recovery in
vc4_irq_reset(), but the hardware is left untouched. Two in-tree platforms
use the VC4 V3D block: BCM2835 gains the resets property in the next
commit, and Cygnus is no worse off than it was, as its V3D node has no
power domain and the power-cycle only ever gated its clock.

Signed-off-by: Maíra Canal <mcanal@igalia.com>

> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index e231c906709c1..3212b91676201 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c

[ ... ]

> @@ -292,19 +292,22 @@ vc4_save_hang_state(struct drm_device *dev)
>  static void
>  vc4_reset(struct drm_device *dev)
>  {
> -	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct vc4_v3d *v3d = to_vc4_dev(dev)->v3d;
> +	int ret;
>  
> -	DRM_INFO("Resetting GPU.\n");
> +	vc4_irq_disable(dev);
>  
> -	mutex_lock(&vc4->power_lock);
> -	if (vc4->power_refcount) {
> -		/* Power the device off and back on the by dropping the
> -		 * reference on runtime PM.
> -		 */
> -		pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
> -		pm_runtime_get_sync(&vc4->v3d->pdev->dev);
> +	if (v3d->reset) {
> +		drm_info(dev, "Resetting GPU.\n");
> +
> +		ret = reset_control_reset(v3d->reset);
> +		if (ret)
> +			drm_err(dev, "Failed to reset the GPU: %d\n", ret);
> +
> +		vc4_v3d_init_hw(dev);
> +	} else {
> +		drm_info_once(dev, "No reset line; GPU state is not reset.\n");
>  	}
> -	mutex_unlock(&vc4->power_lock);
>  
>  	vc4_irq_reset(dev);

[Severity: Critical]
Does removing the fallback GPU power-cycle reset mechanism break GPU hang
recovery and introduce a DMA use-after-free regression on older device
trees lacking a reset line?

If the device tree lacks a reset line, vc4_reset() skips resetting the
hardware since v3d->reset is NULL, but still proceeds to call
vc4_irq_reset().

This cancels the active software jobs, unreferences their buffer objects,
and frees the backing memory in vc4_irq_reset() and vc4_cancel_bin_job().
Because the GPU hardware was not actually stopped or reset, it
asynchronously continues executing the hanging command stream and performs
DMA reads/writes into the newly freed memory pages.

Could this memory corruption be triggered by any unprivileged user that
submits a hanging shader?

[ ... ]

> @@ -1177,10 +1189,6 @@ int vc4_gem_init(struct drm_device *dev)
>  
>  	INIT_WORK(&vc4->job_done_work, vc4_job_done_work);
>  
> -	ret = drmm_mutex_init(dev, &vc4->power_lock);
> -	if (ret)
> -		return ret;
> -
>  	INIT_LIST_HEAD(&vc4->purgeable.list);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free regression
because the hangcheck timer and associated work items are never canceled
during device teardown?

In vc4_gem_init(), the driver initializes the hangcheck.timer,
hangcheck.reset_work, and job_done_work.

However, neither vc4_gem_destroy() nor vc4_irq_disable() appears to
cancel these asynchronous workers using del_timer_sync() or
cancel_work_sync().

If the driver is unloaded or unbound while a hangcheck timer is pending
or work is scheduled, will the callbacks fire after the vc4_dev memory
has been freed, leading to a kernel panic?

> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 379ab77243ad3..d2da8d2f8eebb 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c

[ ... ]

> @@ -447,6 +427,14 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>  	if (IS_ERR(v3d->clk))
>  		return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
>  
> +	v3d->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(v3d->reset))
> +		return dev_err_probe(dev, PTR_ERR(v3d->reset),
> +				     "Failed to get reset control\n");
> +
> +	if (!v3d->reset)
> +		drm_warn(drm, "No V3D reset line in the device tree");
> +
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does requesting exclusive resources using
devm_ APIs inside a component bind callback cause re-bind operations to
fail with -EBUSY?

The vc4_v3d_bind() function uses devm_ APIs attached to the component's
device (pdev->dev). When the component unbinds, these resources are not
freed because the platform device itself is not removed.

If the DRM master defers probing and retries, or is unbound and rebound,
vc4_v3d_bind() is called again. Does the second attempt to acquire
exclusive resources like the reset control fail with -EBUSY, permanently
breaking V3D initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816-vc4-reset-control-v1-0-896444373716@igalia.com?part=3

[PATCH 4/4] ARM: dts: bcm2835: Add the V3D reset line

[Maíra Canal]

The PM block exposes a reset line for the V3D block, which the vc4 driver
uses to reset the GPU when recovering from a hang instead of forcing its
power domain down. Wire it up in the shared V3D node, covering BCM2835,
BCM2836 and BCM2837.

BCM2711 already describes the same reset line for its V3D node.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 arch/arm/boot/dts/broadcom/bcm2835-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi b/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
index 1e76b290510d..39eea8ce1058 100644
--- a/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
@@ -139,6 +139,7 @@ v3d: v3d@7ec00000 {
 			compatible = "brcm,bcm2835-v3d";
 			reg = <0x7ec00000 0x1000>;
 			interrupts = <1 10>;
+			resets = <&pm BCM2835_RESET_V3D>;
 		};
 
 	};

-- 
2.55.0

Re: [PATCH 4/4] ARM: dts: bcm2835: Add the V3D reset line

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] GPU hang recovery is permanently broken for older BCM2835 device trees that lack the `resets` property.
- [Medium] Missing `pm_runtime_mark_last_busy()` in `vc4_v3d_pm_put()` causes premature device autosuspend.
--

--- Patch [4]: [PATCH 4/4] ARM: dts: bcm2835: Add the V3D reset line ---
commit 911a73ec97f6adb52d8c5867c354702414eb11dd
Author: Maíra Canal <mcanal@igalia.com>

ARM: dts: bcm2835: Add the V3D reset line

The PM block exposes a reset line for the V3D block, which the vc4 driver
uses to reset the GPU when recovering from a hang instead of forcing its
power domain down. Wire it up in the shared V3D node, covering BCM2835,
BCM2836 and BCM2837.

> diff --git a/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi b/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
> index 1e76b290510d3..39eea8ce10583 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2835-common.dtsi
> @@ -139,6 +139,7 @@ v3d: v3d@7ec00000 {
>  			compatible = "brcm,bcm2835-v3d";
>  			reg = <0x7ec00000 0x1000>;
>  			interrupts = <1 10>;
> +			resets = <&pm BCM2835_RESET_V3D>;

[Severity: High]
Does this change mean GPU hang recovery is permanently broken for older
BCM2835 device trees that lack the resets property?

Looking at vc4_reset() in drivers/gpu/drm/vc4/vc4_gem.c, it explicitly skips
resetting the hardware if the reset line is absent:

	if (v3d->reset) {
		drm_info(dev, "Resetting GPU.\n");

		ret = reset_control_reset(v3d->reset);
		if (ret)
			drm_err(dev, "Failed to reset the GPU: %d\n", ret);

		vc4_v3d_init_hw(dev);
	} else {
		drm_info_once(dev, "No reset line; GPU state is not reset.\n");
	}

Will this cause the system to be unable to recover from a GPU hang on older
device trees, making the GPU effectively unusable after the first hang, and
breaking Device Tree backward compatibility guarantees?

[Severity: Medium]
Also, does the driver suffer from a performance regression due to premature
device autosuspend?

Looking at vc4_v3d_pm_put() in drivers/gpu/drm/vc4/vc4_v3d.c:

void
vc4_v3d_pm_put(struct vc4_dev *vc4)
{
	if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
		return;

	pm_runtime_put_autosuspend(&vc4->v3d->pdev->dev);
}

Because pm_runtime_mark_last_busy() is not called before dropping the
reference, the autosuspend delay is calculated from the resume time, not the
idle time. If the job took longer than the autosuspend delay, could the
device suspend immediately upon dropping the reference, causing power-cycling
overhead between back-to-back jobs?

>  		};
>  
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816-vc4-reset-control-v1-0-896444373716@igalia.com?part=4