[PATCH v2 0/8] Some Panfrost fixes and improvements

[PATCH v2 0/8] Some Panfrost fixes and improvements

[Adrián Larumbe]

This is v2 of https://lore.kernel.org/dri-devel/20241014233758.994861-1-adrian.larumbe@collabora.com/

This patch series is a collection of minor fixes and improvements I came up
with while working on driver related stuff.

Changelog:

v2:
 - Removed commit that provided an explicit fence cleanup function
 - Added commit for removing unused Panfrost device structure member
 - Refactored how optional job interrupt reenabling during reset is handled
 - Make the way errors and successful return values are delivered from inside
  panfrost_mmu_as_get more according to standard.
 - Simplify unmapping of already mapped area when mapping the pages of a BO
 - Fixing management of runtime-PM reference counts when failing HW job submission.

Adrián Larumbe (8):
  drm/panfrost: Replace DRM driver allocation method with newer one
  drm/panfrost: Handle inexistent GPU during probe
  drm/panfrost: Handle job HW submit errors
  drm/panfrost: Handle error when allocating AS number
  drm/panfrost: Handle page mapping failure
  drm/panfrost: Make re-enabling job interrupts at device reset optional
  drm/panfrost: Add forward declaration and types header
  drm/panfrost: Remove unused device property

 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  4 +-
 drivers/gpu/drm/panfrost/panfrost_device.c    | 60 ++++++------
 drivers/gpu/drm/panfrost/panfrost_device.h    |  9 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c       | 68 +++++--------
 drivers/gpu/drm/panfrost/panfrost_dump.c      |  8 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  4 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c       | 64 +++++++-----
 drivers/gpu/drm/panfrost/panfrost_job.c       | 97 +++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_job.h       |  3 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c       | 90 ++++++++++++-----
 drivers/gpu/drm/panfrost/panfrost_mmu.h       |  3 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c   | 31 +++---
 13 files changed, 256 insertions(+), 191 deletions(-)


base-commit: e511f8fc3b2786999987901dad94c1548e091040
-- 
2.47.0

[PATCH v2 1/8] drm/panfrost: Replace DRM driver allocation method with newer one

[Adrián Larumbe]

Drop the deprecated DRM driver allocation method in favour of
devm_drm_dev_alloc(). Overall just make it the same as in Panthor.
Also discard now superfluous generic and platform device pointers inside
the main panfrost device structure.

Some ancient checkpatch issues unearthed as a result of these changes
were also fixed, like lines too long or double assignment in one line.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  4 +-
 drivers/gpu/drm/panfrost/panfrost_device.c    | 49 ++++++-------
 drivers/gpu/drm/panfrost/panfrost_device.h    |  6 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c       | 68 +++++++------------
 drivers/gpu/drm/panfrost/panfrost_dump.c      |  8 +--
 drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  4 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c       | 49 ++++++-------
 drivers/gpu/drm/panfrost/panfrost_job.c       | 37 +++++-----
 drivers/gpu/drm/panfrost/panfrost_mmu.c       | 40 ++++++-----
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c   | 22 +++---
 11 files changed, 141 insertions(+), 152 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 3385fd3ef41a..2a26cd971d79 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -74,7 +74,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 
 	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
 
-	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+	dev_dbg(pfdev->base.dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
 		status->busy_time, status->total_time,
 		status->busy_time / (status->total_time / 100),
 		status->current_frequency / 1000 / 1000);
@@ -119,7 +119,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	int ret;
 	struct dev_pm_opp *opp;
 	unsigned long cur_freq;
-	struct device *dev = &pfdev->pdev->dev;
+	struct device *dev = pfdev->base.dev;
 	struct devfreq *devfreq;
 	struct thermal_cooling_device *cooling;
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index a45e4addcc19..4fe29286bbe3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -20,9 +20,9 @@
 
 static int panfrost_reset_init(struct panfrost_device *pfdev)
 {
-	pfdev->rstc = devm_reset_control_array_get_optional_exclusive(pfdev->dev);
+	pfdev->rstc = devm_reset_control_array_get_optional_exclusive(pfdev->base.dev);
 	if (IS_ERR(pfdev->rstc)) {
-		dev_err(pfdev->dev, "get reset failed %ld\n", PTR_ERR(pfdev->rstc));
+		dev_err(pfdev->base.dev, "get reset failed %ld\n", PTR_ERR(pfdev->rstc));
 		return PTR_ERR(pfdev->rstc);
 	}
 
@@ -39,22 +39,22 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 	int err;
 	unsigned long rate;
 
-	pfdev->clock = devm_clk_get(pfdev->dev, NULL);
+	pfdev->clock = devm_clk_get(pfdev->base.dev, NULL);
 	if (IS_ERR(pfdev->clock)) {
-		dev_err(pfdev->dev, "get clock failed %ld\n", PTR_ERR(pfdev->clock));
+		dev_err(pfdev->base.dev, "get clock failed %ld\n", PTR_ERR(pfdev->clock));
 		return PTR_ERR(pfdev->clock);
 	}
 
 	rate = clk_get_rate(pfdev->clock);
-	dev_info(pfdev->dev, "clock rate = %lu\n", rate);
+	dev_info(pfdev->base.dev, "clock rate = %lu\n", rate);
 
 	err = clk_prepare_enable(pfdev->clock);
 	if (err)
 		return err;
 
-	pfdev->bus_clock = devm_clk_get_optional(pfdev->dev, "bus");
+	pfdev->bus_clock = devm_clk_get_optional(pfdev->base.dev, "bus");
 	if (IS_ERR(pfdev->bus_clock)) {
-		dev_err(pfdev->dev, "get bus_clock failed %ld\n",
+		dev_err(pfdev->base.dev, "get bus_clock failed %ld\n",
 			PTR_ERR(pfdev->bus_clock));
 		err = PTR_ERR(pfdev->bus_clock);
 		goto disable_clock;
@@ -62,7 +62,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 
 	if (pfdev->bus_clock) {
 		rate = clk_get_rate(pfdev->bus_clock);
-		dev_info(pfdev->dev, "bus_clock rate = %lu\n", rate);
+		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
 
 		err = clk_prepare_enable(pfdev->bus_clock);
 		if (err)
@@ -87,7 +87,7 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 {
 	int ret, i;
 
-	pfdev->regulators = devm_kcalloc(pfdev->dev, pfdev->comp->num_supplies,
+	pfdev->regulators = devm_kcalloc(pfdev->base.dev, pfdev->comp->num_supplies,
 					 sizeof(*pfdev->regulators),
 					 GFP_KERNEL);
 	if (!pfdev->regulators)
@@ -96,12 +96,12 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 	for (i = 0; i < pfdev->comp->num_supplies; i++)
 		pfdev->regulators[i].supply = pfdev->comp->supply_names[i];
 
-	ret = devm_regulator_bulk_get(pfdev->dev,
+	ret = devm_regulator_bulk_get(pfdev->base.dev,
 				      pfdev->comp->num_supplies,
 				      pfdev->regulators);
 	if (ret < 0) {
 		if (ret != -EPROBE_DEFER)
-			dev_err(pfdev->dev, "failed to get regulators: %d\n",
+			dev_err(pfdev->base.dev, "failed to get regulators: %d\n",
 				ret);
 		return ret;
 	}
@@ -109,7 +109,7 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 	ret = regulator_bulk_enable(pfdev->comp->num_supplies,
 				    pfdev->regulators);
 	if (ret < 0) {
-		dev_err(pfdev->dev, "failed to enable regulators: %d\n", ret);
+		dev_err(pfdev->base.dev, "failed to enable regulators: %d\n", ret);
 		return ret;
 	}
 
@@ -144,7 +144,7 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
 	int err;
 	int i, num_domains;
 
-	num_domains = of_count_phandle_with_args(pfdev->dev->of_node,
+	num_domains = of_count_phandle_with_args(pfdev->base.dev->of_node,
 						 "power-domains",
 						 "#power-domain-cells");
 
@@ -156,7 +156,7 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
 		return 0;
 
 	if (num_domains != pfdev->comp->num_pm_domains) {
-		dev_err(pfdev->dev,
+		dev_err(pfdev->base.dev,
 			"Incorrect number of power domains: %d provided, %d needed\n",
 			num_domains, pfdev->comp->num_pm_domains);
 		return -EINVAL;
@@ -168,20 +168,21 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
 
 	for (i = 0; i < num_domains; i++) {
 		pfdev->pm_domain_devs[i] =
-			dev_pm_domain_attach_by_name(pfdev->dev,
-					pfdev->comp->pm_domain_names[i]);
+			dev_pm_domain_attach_by_name(pfdev->base.dev,
+						     pfdev->comp->pm_domain_names[i]);
 		if (IS_ERR_OR_NULL(pfdev->pm_domain_devs[i])) {
 			err = PTR_ERR(pfdev->pm_domain_devs[i]) ? : -ENODATA;
 			pfdev->pm_domain_devs[i] = NULL;
-			dev_err(pfdev->dev,
+			dev_err(pfdev->base.dev,
 				"failed to get pm-domain %s(%d): %d\n",
 				pfdev->comp->pm_domain_names[i], i, err);
 			goto err;
 		}
 
-		pfdev->pm_domain_links[i] = device_link_add(pfdev->dev,
-				pfdev->pm_domain_devs[i], DL_FLAG_PM_RUNTIME |
-				DL_FLAG_STATELESS | DL_FLAG_RPM_ACTIVE);
+		pfdev->pm_domain_links[i] =
+			device_link_add(pfdev->base.dev,
+					pfdev->pm_domain_devs[i], DL_FLAG_PM_RUNTIME |
+					DL_FLAG_STATELESS | DL_FLAG_RPM_ACTIVE);
 		if (!pfdev->pm_domain_links[i]) {
 			dev_err(pfdev->pm_domain_devs[i],
 				"adding device link failed!\n");
@@ -211,14 +212,14 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 
 	err = panfrost_clk_init(pfdev);
 	if (err) {
-		dev_err(pfdev->dev, "clk init failed %d\n", err);
+		dev_err(pfdev->base.dev, "clk init failed %d\n", err);
 		return err;
 	}
 
 	err = panfrost_devfreq_init(pfdev);
 	if (err) {
 		if (err != -EPROBE_DEFER)
-			dev_err(pfdev->dev, "devfreq init failed %d\n", err);
+			dev_err(pfdev->base.dev, "devfreq init failed %d\n", err);
 		goto out_clk;
 	}
 
@@ -231,7 +232,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 
 	err = panfrost_reset_init(pfdev);
 	if (err) {
-		dev_err(pfdev->dev, "reset init failed %d\n", err);
+		dev_err(pfdev->base.dev, "reset init failed %d\n", err);
 		goto out_regulator;
 	}
 
@@ -239,7 +240,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	if (err)
 		goto out_reset;
 
-	pfdev->iomem = devm_platform_ioremap_resource(pfdev->pdev, 0);
+	pfdev->iomem = devm_platform_ioremap_resource(to_platform_device(pfdev->base.dev), 0);
 	if (IS_ERR(pfdev->iomem)) {
 		err = PTR_ERR(pfdev->iomem);
 		goto out_pm_domain;
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index cffcb0ac7c11..d9aea2c2cbe5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -98,9 +98,7 @@ struct panfrost_compatible {
 };
 
 struct panfrost_device {
-	struct device *dev;
-	struct drm_device *ddev;
-	struct platform_device *pdev;
+	struct drm_device base;
 	int gpu_irq;
 	int mmu_irq;
 
@@ -181,7 +179,7 @@ struct panfrost_file_priv {
 
 static inline struct panfrost_device *to_panfrost_device(struct drm_device *ddev)
 {
-	return ddev->dev_private;
+	return container_of(ddev, struct panfrost_device, base);
 }
 
 static inline int panfrost_model_cmp(struct panfrost_device *pfdev, s32 id)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 04d615df5259..6284397a7030 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -35,7 +35,7 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
 {
 	int ret;
 
-	ret = pm_runtime_resume_and_get(pfdev->dev);
+	ret = pm_runtime_resume_and_get(pfdev->base.dev);
 	if (ret)
 		return ret;
 
@@ -43,14 +43,14 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
 	*arg = panfrost_timestamp_read(pfdev);
 	panfrost_cycle_counter_put(pfdev);
 
-	pm_runtime_put(pfdev->dev);
+	pm_runtime_put(pfdev->base.dev);
 	return 0;
 }
 
 static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct drm_file *file)
 {
 	struct drm_panfrost_get_param *param = data;
-	struct panfrost_device *pfdev = ddev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(ddev);
 	int ret;
 
 	if (param->pad != 0)
@@ -274,7 +274,7 @@ panfrost_copy_in_sync(struct drm_device *dev,
 static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct panfrost_file_priv *file_priv = file->driver_priv;
 	struct drm_panfrost_submit *args = data;
 	struct drm_syncobj *sync_out = NULL;
@@ -435,7 +435,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 {
 	struct panfrost_file_priv *priv = file_priv->driver_priv;
 	struct drm_panfrost_madvise *args = data;
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct drm_gem_object *gem_obj;
 	struct panfrost_gem_object *bo;
 	int ret = 0;
@@ -507,7 +507,7 @@ static int
 panfrost_open(struct drm_device *dev, struct drm_file *file)
 {
 	int ret;
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct panfrost_file_priv *panfrost_priv;
 
 	panfrost_priv = kzalloc(sizeof(*panfrost_priv), GFP_KERNEL);
@@ -604,8 +604,7 @@ static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
 
 static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 {
-	struct drm_device *dev = file->minor->dev;
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(file->minor->dev);
 
 	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
 
@@ -647,15 +646,12 @@ static const struct drm_driver panfrost_drm_driver = {
 static int panfrost_probe(struct platform_device *pdev)
 {
 	struct panfrost_device *pfdev;
-	struct drm_device *ddev;
 	int err;
 
-	pfdev = devm_kzalloc(&pdev->dev, sizeof(*pfdev), GFP_KERNEL);
-	if (!pfdev)
-		return -ENOMEM;
-
-	pfdev->pdev = pdev;
-	pfdev->dev = &pdev->dev;
+	pfdev = devm_drm_dev_alloc(&pdev->dev, &panfrost_drm_driver,
+				   struct panfrost_device, base);
+	if (IS_ERR(pfdev))
+		return PTR_ERR(pfdev);
 
 	platform_set_drvdata(pdev, pfdev);
 
@@ -665,14 +661,6 @@ static int panfrost_probe(struct platform_device *pdev)
 
 	pfdev->coherent = device_get_dma_attr(&pdev->dev) == DEV_DMA_COHERENT;
 
-	/* Allocate and initialize the DRM device. */
-	ddev = drm_dev_alloc(&panfrost_drm_driver, &pdev->dev);
-	if (IS_ERR(ddev))
-		return PTR_ERR(ddev);
-
-	ddev->dev_private = pfdev;
-	pfdev->ddev = ddev;
-
 	mutex_init(&pfdev->shrinker_lock);
 	INIT_LIST_HEAD(&pfdev->shrinker_list);
 
@@ -683,51 +671,47 @@ static int panfrost_probe(struct platform_device *pdev)
 		goto err_out0;
 	}
 
-	pm_runtime_set_active(pfdev->dev);
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_enable(pfdev->dev);
-	pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
-	pm_runtime_use_autosuspend(pfdev->dev);
+	pm_runtime_set_active(pfdev->base.dev);
+	pm_runtime_mark_last_busy(pfdev->base.dev);
+	pm_runtime_enable(pfdev->base.dev);
+	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
+	pm_runtime_use_autosuspend(pfdev->base.dev);
 
 	/*
 	 * Register the DRM device with the core and the connectors with
 	 * sysfs
 	 */
-	err = drm_dev_register(ddev, 0);
+	err = drm_dev_register(&pfdev->base, 0);
 	if (err < 0)
 		goto err_out1;
 
-	err = panfrost_gem_shrinker_init(ddev);
+	err = panfrost_gem_shrinker_init(&pfdev->base);
 	if (err)
 		goto err_out2;
 
 	return 0;
 
 err_out2:
-	drm_dev_unregister(ddev);
+	drm_dev_unregister(&pfdev->base);
 err_out1:
-	pm_runtime_disable(pfdev->dev);
+	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->dev);
+	pm_runtime_set_suspended(pfdev->base.dev);
 err_out0:
-	drm_dev_put(ddev);
 	return err;
 }
 
 static void panfrost_remove(struct platform_device *pdev)
 {
 	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
-	struct drm_device *ddev = pfdev->ddev;
 
-	drm_dev_unregister(ddev);
-	panfrost_gem_shrinker_cleanup(ddev);
+	drm_dev_unregister(&pfdev->base);
+	panfrost_gem_shrinker_cleanup(&pfdev->base);
 
-	pm_runtime_get_sync(pfdev->dev);
-	pm_runtime_disable(pfdev->dev);
+	pm_runtime_get_sync(pfdev->base.dev);
+	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->dev);
-
-	drm_dev_put(ddev);
+	pm_runtime_set_suspended(pfdev->base.dev);
 }
 
 static ssize_t profiling_show(struct device *dev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_dump.c b/drivers/gpu/drm/panfrost/panfrost_dump.c
index 47751302f1bc..ce704d077fa7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_dump.c
+++ b/drivers/gpu/drm/panfrost/panfrost_dump.c
@@ -163,7 +163,7 @@ void panfrost_core_dump(struct panfrost_job *job)
 	iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
 			__GFP_NORETRY);
 	if (!iter.start) {
-		dev_warn(pfdev->dev, "failed to allocate devcoredump file\n");
+		dev_warn(pfdev->base.dev, "failed to allocate devcoredump file\n");
 		return;
 	}
 
@@ -204,14 +204,14 @@ void panfrost_core_dump(struct panfrost_job *job)
 		mapping = job->mappings[i];
 
 		if (!bo->base.sgt) {
-			dev_err(pfdev->dev, "Panfrost Dump: BO has no sgt, cannot dump\n");
+			dev_err(pfdev->base.dev, "Panfrost Dump: BO has no sgt, cannot dump\n");
 			iter.hdr->bomap.valid = 0;
 			goto dump_header;
 		}
 
 		ret = drm_gem_vmap_unlocked(&bo->base.base, &map);
 		if (ret) {
-			dev_err(pfdev->dev, "Panfrost Dump: couldn't map Buffer Object\n");
+			dev_err(pfdev->base.dev, "Panfrost Dump: couldn't map Buffer Object\n");
 			iter.hdr->bomap.valid = 0;
 			goto dump_header;
 		}
@@ -237,5 +237,5 @@ dump_header:	panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_BO, iter.data +
 	}
 	panfrost_core_dump_header(&iter, PANFROSTDUMP_BUF_TRAILER, iter.data);
 
-	dev_coredumpv(pfdev->dev, iter.start, iter.data - iter.start, GFP_KERNEL);
+	dev_coredumpv(pfdev->base.dev, iter.start, iter.data - iter.start, GFP_KERNEL);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 8e0ff3efede7..768fed21c985 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -17,7 +17,7 @@
 static void panfrost_gem_free_object(struct drm_gem_object *obj)
 {
 	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
-	struct panfrost_device *pfdev = obj->dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 
 	/*
 	 * Make sure the BO is no longer inserted in the shrinker list before
@@ -41,7 +41,7 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
 
 		for (i = 0; i < n_sgt; i++) {
 			if (bo->sgts[i].sgl) {
-				dma_unmap_sgtable(pfdev->dev, &bo->sgts[i],
+				dma_unmap_sgtable(pfdev->base.dev, &bo->sgts[i],
 						  DMA_BIDIRECTIONAL, 0);
 				sg_free_table(&bo->sgts[i]);
 			}
@@ -249,7 +249,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
  */
 struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct panfrost_gem_object *obj;
 
 	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 3d9f51bd48b6..ee22777d06c8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -97,7 +97,7 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
  */
 int panfrost_gem_shrinker_init(struct drm_device *dev)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 
 	pfdev->shrinker = shrinker_alloc(0, "drm-panfrost");
 	if (!pfdev->shrinker)
@@ -120,7 +120,7 @@ int panfrost_gem_shrinker_init(struct drm_device *dev)
  */
 void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 
 	if (pfdev->shrinker)
 		shrinker_free(pfdev->shrinker);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index f5abde3866fb..69425104ca97 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -36,12 +36,12 @@ static irqreturn_t panfrost_gpu_irq_handler(int irq, void *data)
 		u64 address = (u64) gpu_read(pfdev, GPU_FAULT_ADDRESS_HI) << 32;
 		address |= gpu_read(pfdev, GPU_FAULT_ADDRESS_LO);
 
-		dev_warn(pfdev->dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
+		dev_warn(pfdev->base.dev, "GPU Fault 0x%08x (%s) at 0x%016llx\n",
 			 fault_status, panfrost_exception_name(fault_status & 0xFF),
 			 address);
 
 		if (state & GPU_IRQ_MULTIPLE_FAULT)
-			dev_warn(pfdev->dev, "There were multiple GPU faults - some have not been reported\n");
+			dev_warn(pfdev->base.dev, "There were multiple GPU faults - some have not been reported\n");
 
 		gpu_write(pfdev, GPU_INT_MASK, 0);
 	}
@@ -72,13 +72,13 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
 		val, val & GPU_IRQ_RESET_COMPLETED, 10, 10000);
 
 	if (ret) {
-		dev_err(pfdev->dev, "gpu soft reset timed out, attempting hard reset\n");
+		dev_err(pfdev->base.dev, "gpu soft reset timed out, attempting hard reset\n");
 
 		gpu_write(pfdev, GPU_CMD, GPU_CMD_HARD_RESET);
 		ret = readl_relaxed_poll_timeout(pfdev->iomem + GPU_INT_RAWSTAT, val,
 						 val & GPU_IRQ_RESET_COMPLETED, 100, 10000);
 		if (ret) {
-			dev_err(pfdev->dev, "gpu hard reset timed out\n");
+			dev_err(pfdev->base.dev, "gpu hard reset timed out\n");
 			return ret;
 		}
 	}
@@ -95,7 +95,7 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
 	 * All in-flight jobs should have released their cycle
 	 * counter references upon reset, but let us make sure
 	 */
-	if (drm_WARN_ON(pfdev->ddev, atomic_read(&pfdev->cycle_counter.use_count) != 0))
+	if (drm_WARN_ON(&pfdev->base, atomic_read(&pfdev->cycle_counter.use_count) != 0))
 		atomic_set(&pfdev->cycle_counter.use_count, 0);
 
 	return 0;
@@ -326,13 +326,13 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
 	bitmap_from_u64(pfdev->features.hw_features, hw_feat);
 	bitmap_from_u64(pfdev->features.hw_issues, hw_issues);
 
-	dev_info(pfdev->dev, "mali-%s id 0x%x major 0x%x minor 0x%x status 0x%x",
+	dev_info(pfdev->base.dev, "mali-%s id 0x%x major 0x%x minor 0x%x status 0x%x",
 		 name, gpu_id, major, minor, status);
-	dev_info(pfdev->dev, "features: %64pb, issues: %64pb",
+	dev_info(pfdev->base.dev, "features: %64pb, issues: %64pb",
 		 pfdev->features.hw_features,
 		 pfdev->features.hw_issues);
 
-	dev_info(pfdev->dev, "Features: L2:0x%08x Shader:0x%08x Tiler:0x%08x Mem:0x%0x MMU:0x%08x AS:0x%x JS:0x%x",
+	dev_info(pfdev->base.dev, "Features: L2:0x%08x Shader:0x%08x Tiler:0x%08x Mem:0x%0x MMU:0x%08x AS:0x%x JS:0x%x",
 		 pfdev->features.l2_features,
 		 pfdev->features.core_features,
 		 pfdev->features.tiler_features,
@@ -341,7 +341,7 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
 		 pfdev->features.as_present,
 		 pfdev->features.js_present);
 
-	dev_info(pfdev->dev, "shader_present=0x%0llx l2_present=0x%0llx",
+	dev_info(pfdev->base.dev, "shader_present=0x%0llx l2_present=0x%0llx",
 		 pfdev->features.shader_present, pfdev->features.l2_present);
 }
 
@@ -407,7 +407,7 @@ static u64 panfrost_get_core_mask(struct panfrost_device *pfdev)
 	 */
 	core_mask = ~(pfdev->features.l2_present - 1) &
 		     (pfdev->features.l2_present - 2);
-	dev_info_once(pfdev->dev, "using only 1st core group (%lu cores from %lu)\n",
+	dev_info_once(pfdev->base.dev, "using only 1st core group (%lu cores from %lu)\n",
 		      hweight64(core_mask),
 		      hweight64(pfdev->features.shader_present));
 
@@ -428,7 +428,7 @@ void panfrost_gpu_power_on(struct panfrost_device *pfdev)
 		val, val == (pfdev->features.l2_present & core_mask),
 		10, 20000);
 	if (ret)
-		dev_err(pfdev->dev, "error powering up gpu L2");
+		dev_err(pfdev->base.dev, "error powering up gpu L2");
 
 	gpu_write(pfdev, SHADER_PWRON_LO,
 		  pfdev->features.shader_present & core_mask);
@@ -436,13 +436,13 @@ void panfrost_gpu_power_on(struct panfrost_device *pfdev)
 		val, val == (pfdev->features.shader_present & core_mask),
 		10, 20000);
 	if (ret)
-		dev_err(pfdev->dev, "error powering up gpu shader");
+		dev_err(pfdev->base.dev, "error powering up gpu shader");
 
 	gpu_write(pfdev, TILER_PWRON_LO, pfdev->features.tiler_present);
 	ret = readl_relaxed_poll_timeout(pfdev->iomem + TILER_READY_LO,
 		val, val == pfdev->features.tiler_present, 10, 1000);
 	if (ret)
-		dev_err(pfdev->dev, "error powering up gpu tiler");
+		dev_err(pfdev->base.dev, "error powering up gpu tiler");
 }
 
 void panfrost_gpu_power_off(struct panfrost_device *pfdev)
@@ -454,19 +454,19 @@ void panfrost_gpu_power_off(struct panfrost_device *pfdev)
 	ret = readl_relaxed_poll_timeout(pfdev->iomem + SHADER_PWRTRANS_LO,
 					 val, !val, 1, 2000);
 	if (ret)
-		dev_err(pfdev->dev, "shader power transition timeout");
+		dev_err(pfdev->base.dev, "shader power transition timeout");
 
 	gpu_write(pfdev, TILER_PWROFF_LO, pfdev->features.tiler_present);
 	ret = readl_relaxed_poll_timeout(pfdev->iomem + TILER_PWRTRANS_LO,
 					 val, !val, 1, 2000);
 	if (ret)
-		dev_err(pfdev->dev, "tiler power transition timeout");
+		dev_err(pfdev->base.dev, "tiler power transition timeout");
 
 	gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present);
 	ret = readl_poll_timeout(pfdev->iomem + L2_PWRTRANS_LO,
 				 val, !val, 0, 2000);
 	if (ret)
-		dev_err(pfdev->dev, "l2 power transition timeout");
+		dev_err(pfdev->base.dev, "l2 power transition timeout");
 }
 
 void panfrost_gpu_suspend_irq(struct panfrost_device *pfdev)
@@ -487,21 +487,22 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 
 	panfrost_gpu_init_features(pfdev);
 
-	err = dma_set_mask_and_coherent(pfdev->dev,
-		DMA_BIT_MASK(FIELD_GET(0xff00, pfdev->features.mmu_features)));
+	err = dma_set_mask_and_coherent(pfdev->base.dev,
+					DMA_BIT_MASK(FIELD_GET(0xff00,
+							       pfdev->features.mmu_features)));
 	if (err)
 		return err;
 
-	dma_set_max_seg_size(pfdev->dev, UINT_MAX);
+	dma_set_max_seg_size(pfdev->base.dev, UINT_MAX);
 
-	pfdev->gpu_irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "gpu");
+	pfdev->gpu_irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "gpu");
 	if (pfdev->gpu_irq < 0)
 		return pfdev->gpu_irq;
 
-	err = devm_request_irq(pfdev->dev, pfdev->gpu_irq, panfrost_gpu_irq_handler,
+	err = devm_request_irq(pfdev->base.dev, pfdev->gpu_irq, panfrost_gpu_irq_handler,
 			       IRQF_SHARED, KBUILD_MODNAME "-gpu", pfdev);
 	if (err) {
-		dev_err(pfdev->dev, "failed to request gpu irq");
+		dev_err(pfdev->base.dev, "failed to request gpu irq");
 		return err;
 	}
 
@@ -521,9 +522,9 @@ u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev)
 
 	if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION)) {
 		/* Flush reduction only makes sense when the GPU is kept powered on between jobs */
-		if (pm_runtime_get_if_in_use(pfdev->dev)) {
+		if (pm_runtime_get_if_in_use(pfdev->base.dev)) {
 			flush_id = gpu_read(pfdev, GPU_LATEST_FLUSH_ID);
-			pm_runtime_put(pfdev->dev);
+			pm_runtime_put(pfdev->base.dev);
 			return flush_id;
 		}
 	}
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 9b8e82fb8bc4..f640d211cc3a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -94,7 +94,7 @@ static struct dma_fence *panfrost_fence_create(struct panfrost_device *pfdev, in
 	if (!fence)
 		return ERR_PTR(-ENOMEM);
 
-	fence->dev = pfdev->ddev;
+	fence->dev = &pfdev->base;
 	fence->queue = js_num;
 	fence->seqno = ++js->queue[js_num].emit_seqno;
 	dma_fence_init(&fence->base, &panfrost_fence_ops, &js->job_lock,
@@ -205,7 +205,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 
 	panfrost_devfreq_record_busy(&pfdev->pfdevfreq);
 
-	ret = pm_runtime_get_sync(pfdev->dev);
+	ret = pm_runtime_get_sync(pfdev->base.dev);
 	if (ret < 0)
 		return;
 
@@ -256,7 +256,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 		}
 
 		job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
-		dev_dbg(pfdev->dev,
+		dev_dbg(pfdev->base.dev,
 			"JS: Submitting atom %p to js[%d][%d] with head=0x%llx AS %d",
 			job, js, subslot, jc_head, cfg & 0xf);
 	}
@@ -437,12 +437,12 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 	bool signal_fence = true;
 
 	if (!panfrost_exception_is_fault(js_status)) {
-		dev_dbg(pfdev->dev, "js event, js=%d, status=%s, head=0x%x, tail=0x%x",
+		dev_dbg(pfdev->base.dev, "js event, js=%d, status=%s, head=0x%x, tail=0x%x",
 			js, exception_name,
 			job_read(pfdev, JS_HEAD_LO(js)),
 			job_read(pfdev, JS_TAIL_LO(js)));
 	} else {
-		dev_err(pfdev->dev, "js fault, js=%d, status=%s, head=0x%x, tail=0x%x",
+		dev_err(pfdev->base.dev, "js fault, js=%d, status=%s, head=0x%x, tail=0x%x",
 			js, exception_name,
 			job_read(pfdev, JS_HEAD_LO(js)),
 			job_read(pfdev, JS_TAIL_LO(js)));
@@ -474,7 +474,7 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 	if (signal_fence)
 		dma_fence_signal_locked(job->done_fence);
 
-	pm_runtime_put_autosuspend(pfdev->dev);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 
 	if (panfrost_exception_needs_reset(pfdev, js_status)) {
 		atomic_set(&pfdev->reset.pending, 1);
@@ -493,7 +493,7 @@ static void panfrost_job_handle_done(struct panfrost_device *pfdev,
 	panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 
 	dma_fence_signal_locked(job->done_fence);
-	pm_runtime_put_autosuspend(pfdev->dev);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static void panfrost_job_handle_irq(struct panfrost_device *pfdev, u32 status)
@@ -602,7 +602,7 @@ static void panfrost_job_handle_irqs(struct panfrost_device *pfdev)
 	u32 status = job_read(pfdev, JOB_INT_RAWSTAT);
 
 	while (status) {
-		pm_runtime_mark_last_busy(pfdev->dev);
+		pm_runtime_mark_last_busy(pfdev->base.dev);
 
 		spin_lock(&pfdev->js->job_lock);
 		panfrost_job_handle_irq(pfdev, status);
@@ -683,7 +683,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 				 10, 10000);
 
 	if (ret)
-		dev_err(pfdev->dev, "Soft-stop failed\n");
+		dev_err(pfdev->base.dev, "Soft-stop failed\n");
 
 	/* Handle the remaining interrupts before we reset. */
 	panfrost_job_handle_irqs(pfdev);
@@ -701,7 +701,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
 			    pfdev->jobs[i][j]->is_profiled)
 				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
-			pm_runtime_put_noidle(pfdev->dev);
+			pm_runtime_put_noidle(pfdev->base.dev);
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 		}
 	}
@@ -769,11 +769,11 @@ static enum drm_gpu_sched_stat panfrost_job_timedout(struct drm_sched_job
 	synchronize_irq(pfdev->js->irq);
 
 	if (dma_fence_is_signaled(job->done_fence)) {
-		dev_warn(pfdev->dev, "unexpectedly high interrupt latency\n");
+		dev_warn(pfdev->base.dev, "unexpectedly high interrupt latency\n");
 		return DRM_GPU_SCHED_STAT_NOMINAL;
 	}
 
-	dev_err(pfdev->dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
+	dev_err(pfdev->base.dev, "gpu sched timeout, js=%d, config=0x%x, status=0x%x, head=0x%x, tail=0x%x, sched_job=%p",
 		js,
 		job_read(pfdev, JS_CONFIG(js)),
 		job_read(pfdev, JS_STATUS(js)),
@@ -847,24 +847,25 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 	if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_JOBCHAIN_DISAMBIGUATION))
 		nentries = 1;
 
-	pfdev->js = js = devm_kzalloc(pfdev->dev, sizeof(*js), GFP_KERNEL);
+	js = devm_kzalloc(pfdev->base.dev, sizeof(*js), GFP_KERNEL);
 	if (!js)
 		return -ENOMEM;
+	pfdev->js = js;
 
 	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
 	spin_lock_init(&js->job_lock);
 
-	js->irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "job");
+	js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "job");
 	if (js->irq < 0)
 		return js->irq;
 
-	ret = devm_request_threaded_irq(pfdev->dev, js->irq,
+	ret = devm_request_threaded_irq(pfdev->base.dev, js->irq,
 					panfrost_job_irq_handler,
 					panfrost_job_irq_handler_thread,
 					IRQF_SHARED, KBUILD_MODNAME "-job",
 					pfdev);
 	if (ret) {
-		dev_err(pfdev->dev, "failed to request job irq");
+		dev_err(pfdev->base.dev, "failed to request job irq");
 		return ret;
 	}
 
@@ -881,9 +882,9 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 				     nentries, 0,
 				     msecs_to_jiffies(JOB_TIMEOUT_MS),
 				     pfdev->reset.wq,
-				     NULL, "pan_js", pfdev->dev);
+				     NULL, "pan_js", pfdev->base.dev);
 		if (ret) {
-			dev_err(pfdev->dev, "Failed to create scheduler: %d.", ret);
+			dev_err(pfdev->base.dev, "Failed to create scheduler: %d.", ret);
 			goto err_sched;
 		}
 	}
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index b91019cd5acb..2189e42d2bfa 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -39,7 +39,7 @@ static int wait_ready(struct panfrost_device *pfdev, u32 as_nr)
 	if (ret) {
 		/* The GPU hung, let's trigger a reset */
 		panfrost_device_schedule_reset(pfdev);
-		dev_err(pfdev->dev, "AS_ACTIVE bit stuck\n");
+		dev_err(pfdev->base.dev, "AS_ACTIVE bit stuck\n");
 	}
 
 	return ret;
@@ -212,7 +212,9 @@ u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
 	atomic_set(&mmu->as_count, 1);
 	list_add(&mmu->list, &pfdev->as_lru_list);
 
-	dev_dbg(pfdev->dev, "Assigned AS%d to mmu %p, alloc_mask=%lx", as, mmu, pfdev->as_alloc_mask);
+	dev_dbg(pfdev->base.dev,
+		"Assigned AS%d to mmu %p, alloc_mask=%lx",
+		as, mmu, pfdev->as_alloc_mask);
 
 	panfrost_mmu_enable(pfdev, mmu);
 
@@ -278,13 +280,13 @@ static void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
 	if (mmu->as < 0)
 		return;
 
-	pm_runtime_get_noresume(pfdev->dev);
+	pm_runtime_get_noresume(pfdev->base.dev);
 
 	/* Flush the PTs only if we're already awake */
-	if (pm_runtime_active(pfdev->dev))
+	if (pm_runtime_active(pfdev->base.dev))
 		mmu_hw_do_operation(pfdev, mmu, iova, size, AS_COMMAND_FLUSH_PT);
 
-	pm_runtime_put_autosuspend(pfdev->dev);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
@@ -299,7 +301,9 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 		unsigned long paddr = sg_dma_address(sgl);
 		size_t len = sg_dma_len(sgl);
 
-		dev_dbg(pfdev->dev, "map: as=%d, iova=%llx, paddr=%lx, len=%zx", mmu->as, iova, paddr, len);
+		dev_dbg(pfdev->base.dev,
+			"map: as=%d, iova=%llx, paddr=%lx, len=%zx",
+			mmu->as, iova, paddr, len);
 
 		while (len) {
 			size_t pgcount, mapped = 0;
@@ -359,7 +363,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping)
 	if (WARN_ON(!mapping->active))
 		return;
 
-	dev_dbg(pfdev->dev, "unmap: as=%d, iova=%llx, len=%zx",
+	dev_dbg(pfdev->base.dev, "unmap: as=%d, iova=%llx, len=%zx",
 		mapping->mmu->as, iova, len);
 
 	while (unmapped_len < len) {
@@ -456,7 +460,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 
 	bo = bomapping->obj;
 	if (!bo->is_heap) {
-		dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
+		dev_WARN(pfdev->base.dev, "matching BO is not heap type (GPU VA = %llx)",
 			 bomapping->mmnode.start << PAGE_SHIFT);
 		ret = -EINVAL;
 		goto err_bo;
@@ -523,7 +527,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	if (ret)
 		goto err_unlock;
 
-	ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0);
+	ret = dma_map_sgtable(pfdev->base.dev, sgt, DMA_BIDIRECTIONAL, 0);
 	if (ret)
 		goto err_map;
 
@@ -533,7 +537,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	bomapping->active = true;
 	bo->heap_rss_size += SZ_2M;
 
-	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
+	dev_dbg(pfdev->base.dev, "mapped page fault @ AS%d %llx", as, addr);
 
 out:
 	dma_resv_unlock(obj->resv);
@@ -559,10 +563,10 @@ static void panfrost_mmu_release_ctx(struct kref *kref)
 
 	spin_lock(&pfdev->as_lock);
 	if (mmu->as >= 0) {
-		pm_runtime_get_noresume(pfdev->dev);
-		if (pm_runtime_active(pfdev->dev))
+		pm_runtime_get_noresume(pfdev->base.dev);
+		if (pm_runtime_active(pfdev->base.dev))
 			panfrost_mmu_disable(pfdev, mmu->as);
-		pm_runtime_put_autosuspend(pfdev->dev);
+		pm_runtime_put_autosuspend(pfdev->base.dev);
 
 		clear_bit(mmu->as, &pfdev->as_alloc_mask);
 		clear_bit(mmu->as, &pfdev->as_in_use_mask);
@@ -637,7 +641,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
 		.oas		= FIELD_GET(0xff00, pfdev->features.mmu_features),
 		.coherent_walk	= pfdev->coherent,
 		.tlb		= &mmu_tlb_ops,
-		.iommu_dev	= pfdev->dev,
+		.iommu_dev	= pfdev->base.dev,
 	};
 
 	mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
@@ -720,7 +724,7 @@ static irqreturn_t panfrost_mmu_irq_handler_thread(int irq, void *data)
 
 		if (ret) {
 			/* terminal fault, print info about the fault */
-			dev_err(pfdev->dev,
+			dev_err(pfdev->base.dev,
 				"Unhandled Page fault in AS%d at VA 0x%016llX\n"
 				"Reason: %s\n"
 				"raw fault status: 0x%X\n"
@@ -768,18 +772,18 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 {
 	int err;
 
-	pfdev->mmu_irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "mmu");
+	pfdev->mmu_irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "mmu");
 	if (pfdev->mmu_irq < 0)
 		return pfdev->mmu_irq;
 
-	err = devm_request_threaded_irq(pfdev->dev, pfdev->mmu_irq,
+	err = devm_request_threaded_irq(pfdev->base.dev, pfdev->mmu_irq,
 					panfrost_mmu_irq_handler,
 					panfrost_mmu_irq_handler_thread,
 					IRQF_SHARED, KBUILD_MODNAME "-mmu",
 					pfdev);
 
 	if (err) {
-		dev_err(pfdev->dev, "failed to request mmu irq");
+		dev_err(pfdev->base.dev, "failed to request mmu irq");
 		return err;
 	}
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index ba9b6e2b2636..f30817bcf8ba 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -84,11 +84,11 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	else if (perfcnt->user)
 		return -EBUSY;
 
-	ret = pm_runtime_get_sync(pfdev->dev);
+	ret = pm_runtime_get_sync(pfdev->base.dev);
 	if (ret < 0)
 		goto err_put_pm;
 
-	bo = drm_gem_shmem_create(pfdev->ddev, perfcnt->bosize);
+	bo = drm_gem_shmem_create(&pfdev->base, perfcnt->bosize);
 	if (IS_ERR(bo)) {
 		ret = PTR_ERR(bo);
 		goto err_put_pm;
@@ -173,7 +173,7 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 err_put_bo:
 	drm_gem_object_put(&bo->base);
 err_put_pm:
-	pm_runtime_put(pfdev->dev);
+	pm_runtime_put(pfdev->base.dev);
 	return ret;
 }
 
@@ -201,8 +201,8 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
 	panfrost_gem_mapping_put(perfcnt->mapping);
 	perfcnt->mapping = NULL;
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
+	pm_runtime_mark_last_busy(pfdev->base.dev);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 
 	return 0;
 }
@@ -210,7 +210,7 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 	struct drm_panfrost_perfcnt_enable *req = data;
 	int ret;
@@ -237,7 +237,7 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 				struct drm_file *file_priv)
 {
-	struct panfrost_device *pfdev = dev->dev_private;
+	struct panfrost_device *pfdev = to_panfrost_device(dev);
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 	struct drm_panfrost_perfcnt_dump *req = data;
 	void __user *user_ptr = (void __user *)(uintptr_t)req->buf_ptr;
@@ -272,13 +272,13 @@ void panfrost_perfcnt_close(struct drm_file *file_priv)
 	struct panfrost_device *pfdev = pfile->pfdev;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
 
-	pm_runtime_get_sync(pfdev->dev);
+	pm_runtime_get_sync(pfdev->base.dev);
 	mutex_lock(&perfcnt->lock);
 	if (perfcnt->user == pfile)
 		panfrost_perfcnt_disable_locked(pfdev, file_priv);
 	mutex_unlock(&perfcnt->lock);
-	pm_runtime_mark_last_busy(pfdev->dev);
-	pm_runtime_put_autosuspend(pfdev->dev);
+	pm_runtime_mark_last_busy(pfdev->base.dev);
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
 int panfrost_perfcnt_init(struct panfrost_device *pfdev)
@@ -316,7 +316,7 @@ int panfrost_perfcnt_init(struct panfrost_device *pfdev)
 		       COUNTERS_PER_BLOCK * BYTES_PER_COUNTER;
 	}
 
-	perfcnt = devm_kzalloc(pfdev->dev, sizeof(*perfcnt), GFP_KERNEL);
+	perfcnt = devm_kzalloc(pfdev->base.dev, sizeof(*perfcnt), GFP_KERNEL);
 	if (!perfcnt)
 		return -ENOMEM;
 
-- 
2.47.0

[PATCH v2 2/8] drm/panfrost: Handle inexistent GPU during probe

[Adrián Larumbe]

Just in case we're dealing with a yet not recognised device.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_gpu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 69425104ca97..da55dc7c0552 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -236,9 +236,10 @@ static const struct panfrost_model gpu_models[] = {
 	 */
 	GPU_MODEL(g57, 0x9003,
 		GPU_REV(g57, 0, 0)),
+	{0},
 };
 
-static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
+static int panfrost_gpu_init_features(struct panfrost_device *pfdev)
 {
 	u32 gpu_id, num_js, major, minor, status, rev;
 	const char *name = "unknown";
@@ -323,6 +324,12 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
 		break;
 	}
 
+	if (!model->name) {
+		dev_err(pfdev->base.dev, "GPU model not found: mali-%s id rev %#x %#x\n",
+			name, gpu_id, rev);
+		return -ENODEV;
+	}
+
 	bitmap_from_u64(pfdev->features.hw_features, hw_feat);
 	bitmap_from_u64(pfdev->features.hw_issues, hw_issues);
 
@@ -343,6 +350,8 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
 
 	dev_info(pfdev->base.dev, "shader_present=0x%0llx l2_present=0x%0llx",
 		 pfdev->features.shader_present, pfdev->features.l2_present);
+
+	return 0;
 }
 
 void panfrost_cycle_counter_get(struct panfrost_device *pfdev)
@@ -485,7 +494,9 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 	if (err)
 		return err;
 
-	panfrost_gpu_init_features(pfdev);
+	err = panfrost_gpu_init_features(pfdev);
+	if (err)
+		return err;
 
 	err = dma_set_mask_and_coherent(pfdev->base.dev,
 					DMA_BIT_MASK(FIELD_GET(0xff00,
-- 
2.47.0

[PATCH v2 3/8] drm/panfrost: Handle job HW submit errors

[Adrián Larumbe]

Avoid waiting for the DRM scheduler job timedout handler, and instead, let
the DRM scheduler core signal the error fence immediately when HW job
submission fails.

That means we must also decrement the runtime-PM refcnt for the device,
because the job will never be enqueued or inflight.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index f640d211cc3a..3f4f0682d69d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -195,7 +195,7 @@ panfrost_enqueue_job(struct panfrost_device *pfdev, int slot,
 	return 1;
 }
 
-static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
+static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
 {
 	struct panfrost_device *pfdev = job->pfdev;
 	unsigned int subslot;
@@ -207,10 +207,11 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 
 	ret = pm_runtime_get_sync(pfdev->base.dev);
 	if (ret < 0)
-		return;
+		goto err_hwsubmit;
 
 	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) {
-		return;
+		ret = -EINVAL;
+		goto err_hwsubmit;
 	}
 
 	cfg = panfrost_mmu_as_get(pfdev, job->mmu);
@@ -261,6 +262,12 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 			job, js, subslot, jc_head, cfg & 0xf);
 	}
 	spin_unlock(&pfdev->js->job_lock);
+
+	return 0;
+
+err_hwsubmit:
+	pm_runtime_put_autosuspend(pfdev->base.dev);
+	return ret;
 }
 
 static int panfrost_acquire_object_fences(struct drm_gem_object **bos,
@@ -382,6 +389,7 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	struct panfrost_device *pfdev = job->pfdev;
 	int slot = panfrost_job_get_slot(job);
 	struct dma_fence *fence = NULL;
+	int ret;
 
 	if (unlikely(job->base.s_fence->finished.error))
 		return NULL;
@@ -400,7 +408,11 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 		dma_fence_put(job->done_fence);
 	job->done_fence = dma_fence_get(fence);
 
-	panfrost_job_hw_submit(job, slot);
+	ret = panfrost_job_hw_submit(job, slot);
+	if (ret) {
+		dma_fence_put(job->done_fence);
+		return ERR_PTR(ret);
+	}
 
 	return fence;
 }
-- 
2.47.0

Re: [PATCH v2 3/8] drm/panfrost: Handle job HW submit errors

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:18 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Avoid waiting for the DRM scheduler job timedout handler, and instead, let
> the DRM scheduler core signal the error fence immediately when HW job
> submission fails.
> 
> That means we must also decrement the runtime-PM refcnt for the device,
> because the job will never be enqueued or inflight.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_job.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index f640d211cc3a..3f4f0682d69d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -195,7 +195,7 @@ panfrost_enqueue_job(struct panfrost_device *pfdev, int slot,
>  	return 1;
>  }
>  
> -static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> +static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  {
>  	struct panfrost_device *pfdev = job->pfdev;
>  	unsigned int subslot;
> @@ -207,10 +207,11 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  
>  	ret = pm_runtime_get_sync(pfdev->base.dev);
>  	if (ret < 0)
> -		return;
> +		goto err_hwsubmit;
>  
>  	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) {
> -		return;
> +		ret = -EINVAL;
> +		goto err_hwsubmit;
>  	}
>  
>  	cfg = panfrost_mmu_as_get(pfdev, job->mmu);
> @@ -261,6 +262,12 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  			job, js, subslot, jc_head, cfg & 0xf);
>  	}
>  	spin_unlock(&pfdev->js->job_lock);
> +
> +	return 0;
> +
> +err_hwsubmit:
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
> +	return ret;
>  }
>  
>  static int panfrost_acquire_object_fences(struct drm_gem_object **bos,
> @@ -382,6 +389,7 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>  	struct panfrost_device *pfdev = job->pfdev;
>  	int slot = panfrost_job_get_slot(job);
>  	struct dma_fence *fence = NULL;
> +	int ret;
>  
>  	if (unlikely(job->base.s_fence->finished.error))
>  		return NULL;
> @@ -400,7 +408,11 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>  		dma_fence_put(job->done_fence);
>  	job->done_fence = dma_fence_get(fence);
>  
> -	panfrost_job_hw_submit(job, slot);
> +	ret = panfrost_job_hw_submit(job, slot);
> +	if (ret) {
> +		dma_fence_put(job->done_fence);

If you call dma_fence_put() here, you need to set job->done_fence to
NULL, otherwise dma_fence_put() will be called again on an already
freed fence in panfrost_job_cleanup(). Question is, do we really need
to call dma_fence_put(job->done_fence) here? Can't we let the job
destructor take care of that?

> +		return ERR_PTR(ret);
> +	}
>  
>  	return fence;
>  }

Re: [PATCH v2 3/8] drm/panfrost: Handle job HW submit errors

[Boris Brezillon]

On Mon, 2 Dec 2024 10:21:51 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 28 Nov 2024 21:06:18 +0000
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > Avoid waiting for the DRM scheduler job timedout handler, and instead, let
> > the DRM scheduler core signal the error fence immediately when HW job
> > submission fails.
> > 
> > That means we must also decrement the runtime-PM refcnt for the device,
> > because the job will never be enqueued or inflight.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_job.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index f640d211cc3a..3f4f0682d69d 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -195,7 +195,7 @@ panfrost_enqueue_job(struct panfrost_device *pfdev, int slot,
> >  	return 1;
> >  }
> >  
> > -static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> > +static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
> >  {
> >  	struct panfrost_device *pfdev = job->pfdev;
> >  	unsigned int subslot;
> > @@ -207,10 +207,11 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> >  
> >  	ret = pm_runtime_get_sync(pfdev->base.dev);
> >  	if (ret < 0)
> > -		return;
> > +		goto err_hwsubmit;
> >  
> >  	if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js)))) {
> > -		return;
> > +		ret = -EINVAL;
> > +		goto err_hwsubmit;
> >  	}
> >  
> >  	cfg = panfrost_mmu_as_get(pfdev, job->mmu);
> > @@ -261,6 +262,12 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
> >  			job, js, subslot, jc_head, cfg & 0xf);
> >  	}
> >  	spin_unlock(&pfdev->js->job_lock);
> > +
> > +	return 0;
> > +
> > +err_hwsubmit:
> > +	pm_runtime_put_autosuspend(pfdev->base.dev);
> > +	return ret;
> >  }
> >  
> >  static int panfrost_acquire_object_fences(struct drm_gem_object **bos,
> > @@ -382,6 +389,7 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >  	struct panfrost_device *pfdev = job->pfdev;
> >  	int slot = panfrost_job_get_slot(job);
> >  	struct dma_fence *fence = NULL;
> > +	int ret;
> >  
> >  	if (unlikely(job->base.s_fence->finished.error))
> >  		return NULL;
> > @@ -400,7 +408,11 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >  		dma_fence_put(job->done_fence);
> >  	job->done_fence = dma_fence_get(fence);
> >  
> > -	panfrost_job_hw_submit(job, slot);
> > +	ret = panfrost_job_hw_submit(job, slot);
> > +	if (ret) {
> > +		dma_fence_put(job->done_fence);  
> 
> If you call dma_fence_put() here, you need to set job->done_fence to
> NULL, otherwise dma_fence_put() will be called again on an already
> freed fence in panfrost_job_cleanup(). Question is, do we really need
> to call dma_fence_put(job->done_fence) here? Can't we let the job
> destructor take care of that?

My bad, it's the ref you've taken to return to drm_sched you're
releasing here. I'd go for `dma_fence_put(fence)` to make that clear.

With this minor change, the patch is

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
> > +		return ERR_PTR(ret);
> > +	}
> >  
> >  	return fence;
> >  }  
> 

[PATCH v2 4/8] drm/panfrost: Handle error when allocating AS number

[Adrián Larumbe]

If we reach the beginning of the LRU AS list, then return an error.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c     | 6 +++++-
 drivers/gpu/drm/panfrost/panfrost_mmu.c     | 5 +++--
 drivers/gpu/drm/panfrost/panfrost_mmu.h     | 2 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 9 ++++++++-
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 3f4f0682d69d..fba1a376f593 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -214,7 +214,11 @@ static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
 		goto err_hwsubmit;
 	}
 
-	cfg = panfrost_mmu_as_get(pfdev, job->mmu);
+	ret = panfrost_mmu_as_get(pfdev, job->mmu);
+	if (ret < 0)
+		goto err_hwsubmit;
+
+	cfg = (u32)ret;
 
 	job_write(pfdev, JS_HEAD_NEXT_LO(js), lower_32_bits(jc_head));
 	job_write(pfdev, JS_HEAD_NEXT_HI(js), upper_32_bits(jc_head));
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 2189e42d2bfa..03ac527b35e7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -155,7 +155,7 @@ static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
 	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
 }
 
-u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
+int panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
 {
 	int as;
 
@@ -197,7 +197,8 @@ u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
 			if (!atomic_read(&lru_mmu->as_count))
 				break;
 		}
-		WARN_ON(&lru_mmu->list == &pfdev->as_lru_list);
+		if (WARN_ON(&lru_mmu->list == &pfdev->as_lru_list))
+			return -EBUSY;
 
 		list_del_init(&lru_mmu->list);
 		as = lru_mmu->as;
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
index 022a9a74a114..e6e6966a0cca 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
@@ -16,7 +16,7 @@ void panfrost_mmu_fini(struct panfrost_device *pfdev);
 void panfrost_mmu_reset(struct panfrost_device *pfdev);
 void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev);
 
-u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
+int panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
 void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
 
 struct panfrost_mmu *panfrost_mmu_ctx_get(struct panfrost_mmu *mmu);
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index f30817bcf8ba..2145f9707dde 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -130,7 +130,12 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 
 	perfcnt->user = user;
 
-	as = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
+	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
+	if (ret)
+		goto err_unsetuser;
+
+	as = (u32)ret;
+
 	cfg = GPU_PERFCNT_CFG_AS(as) |
 	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
 
@@ -164,6 +169,8 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 
 	return 0;
 
+err_unsetuser:
+	perfcnt->user = NULL;
 err_vunmap:
 	drm_gem_vunmap_unlocked(&bo->base, &map);
 err_put_mapping:
-- 
2.47.0

Re: [PATCH v2 4/8] drm/panfrost: Handle error when allocating AS number

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:19 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> If we reach the beginning of the LRU AS list, then return an error.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_job.c     | 6 +++++-
>  drivers/gpu/drm/panfrost/panfrost_mmu.c     | 5 +++--
>  drivers/gpu/drm/panfrost/panfrost_mmu.h     | 2 +-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 9 ++++++++-
>  4 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 3f4f0682d69d..fba1a376f593 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -214,7 +214,11 @@ static int panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  		goto err_hwsubmit;
>  	}
>  
> -	cfg = panfrost_mmu_as_get(pfdev, job->mmu);
> +	ret = panfrost_mmu_as_get(pfdev, job->mmu);
> +	if (ret < 0)
> +		goto err_hwsubmit;
> +
> +	cfg = (u32)ret;
>  
>  	job_write(pfdev, JS_HEAD_NEXT_LO(js), lower_32_bits(jc_head));
>  	job_write(pfdev, JS_HEAD_NEXT_HI(js), upper_32_bits(jc_head));
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 2189e42d2bfa..03ac527b35e7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -155,7 +155,7 @@ static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>  	write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>  }
>  
> -u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
> +int panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>  {
>  	int as;
>  
> @@ -197,7 +197,8 @@ u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>  			if (!atomic_read(&lru_mmu->as_count))
>  				break;
>  		}
> -		WARN_ON(&lru_mmu->list == &pfdev->as_lru_list);
> +		if (WARN_ON(&lru_mmu->list == &pfdev->as_lru_list))
> +			return -EBUSY;
>  
>  		list_del_init(&lru_mmu->list);
>  		as = lru_mmu->as;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> index 022a9a74a114..e6e6966a0cca 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> @@ -16,7 +16,7 @@ void panfrost_mmu_fini(struct panfrost_device *pfdev);
>  void panfrost_mmu_reset(struct panfrost_device *pfdev);
>  void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev);
>  
> -u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
> +int panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
>  void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu);
>  
>  struct panfrost_mmu *panfrost_mmu_ctx_get(struct panfrost_mmu *mmu);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index f30817bcf8ba..2145f9707dde 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -130,7 +130,12 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  
>  	perfcnt->user = user;
>  
> -	as = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> +	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> +	if (ret)
> +		goto err_unsetuser;
> +
> +	as = (u32)ret;

I don't think the explicit cast is needed, since you've already checked
that the result is positive.

Looks good otherwise.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> +
>  	cfg = GPU_PERFCNT_CFG_AS(as) |
>  	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
>  
> @@ -164,6 +169,8 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  
>  	return 0;
>  
> +err_unsetuser:
> +	perfcnt->user = NULL;
>  err_vunmap:
>  	drm_gem_vunmap_unlocked(&bo->base, &map);
>  err_put_mapping:

[PATCH v2 5/8] drm/panfrost: Handle page mapping failure

[Adrián Larumbe]

When mapping the pages of a BO, either a heap type at page fault time or
else a non-heap BO at object creation time, if the ARM page table mapping
function fails, we unmap what had been mapped so far and bail out.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 44 ++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 03ac527b35e7..5e30888bea0e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -290,13 +290,31 @@ static void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
 	pm_runtime_put_autosuspend(pfdev->base.dev);
 }
 
+static void mmu_unmap_range(size_t len, u64 iova, struct io_pgtable_ops *ops)
+{
+	size_t pgsize, unmapped_len = 0;
+	size_t unmapped_page, pgcount;
+
+	while (unmapped_len < len) {
+		pgsize = get_pgsize(iova, len - unmapped_len, &pgcount);
+
+		unmapped_page = ops->unmap_pages(ops, iova, pgsize, pgcount, NULL);
+		WARN_ON(unmapped_page != pgsize * pgcount);
+
+		iova += pgsize * pgcount;
+		unmapped_len += pgsize * pgcount;
+	}
+}
+
 static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 		      u64 iova, int prot, struct sg_table *sgt)
 {
 	unsigned int count;
 	struct scatterlist *sgl;
 	struct io_pgtable_ops *ops = mmu->pgtbl_ops;
+	size_t total_mapped = 0;
 	u64 start_iova = iova;
+	int ret = 0;
 
 	for_each_sgtable_dma_sg(sgt, sgl, count) {
 		unsigned long paddr = sg_dma_address(sgl);
@@ -310,10 +328,15 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
 			size_t pgcount, mapped = 0;
 			size_t pgsize = get_pgsize(iova | paddr, len, &pgcount);
 
-			ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
+			ret = ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
 				       GFP_KERNEL, &mapped);
+			if (ret) {
+				mmu_unmap_range(total_mapped, start_iova, ops);
+				return ret;
+			}
 			/* Don't get stuck if things have gone wrong */
 			mapped = max(mapped, pgsize);
+			total_mapped += mapped;
 			iova += mapped;
 			paddr += mapped;
 			len -= mapped;
@@ -333,6 +356,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 	struct sg_table *sgt;
 	int prot = IOMMU_READ | IOMMU_WRITE;
+	int ret;
 
 	if (WARN_ON(mapping->active))
 		return 0;
@@ -344,8 +368,13 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	if (WARN_ON(IS_ERR(sgt)))
 		return PTR_ERR(sgt);
 
-	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
-		   prot, sgt);
+	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
+			 prot, sgt);
+	if (ret) {
+		drm_gem_shmem_put_pages(shmem);
+		return ret;
+	}
+
 	mapping->active = true;
 
 	return 0;
@@ -532,8 +561,10 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	if (ret)
 		goto err_map;
 
-	mmu_map_sg(pfdev, bomapping->mmu, addr,
-		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
+	ret = mmu_map_sg(pfdev, bomapping->mmu, addr,
+			 IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
+	if (ret)
+		goto err_mmu_map_sg;
 
 	bomapping->active = true;
 	bo->heap_rss_size += SZ_2M;
@@ -547,6 +578,9 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 
 	return 0;
 
+err_mmu_map_sg:
+	dma_unmap_sgtable(pfdev->base.dev, sgt,
+			  DMA_BIDIRECTIONAL, 0);
 err_map:
 	sg_free_table(sgt);
 err_unlock:
-- 
2.47.0

Re: [PATCH v2 5/8] drm/panfrost: Handle page mapping failure

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:20 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> When mapping the pages of a BO, either a heap type at page fault time or
> else a non-heap BO at object creation time, if the ARM page table mapping
> function fails, we unmap what had been mapped so far and bail out.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 44 ++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 03ac527b35e7..5e30888bea0e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -290,13 +290,31 @@ static void panfrost_mmu_flush_range(struct panfrost_device *pfdev,
>  	pm_runtime_put_autosuspend(pfdev->base.dev);
>  }
>  
> +static void mmu_unmap_range(size_t len, u64 iova, struct io_pgtable_ops *ops)

Can we use the following prototype instead?

static void mmu_unmap_range(struct panfrost_mmu *mmu,
			    u64 iova, size_t len)

> +{
> +	size_t pgsize, unmapped_len = 0;
> +	size_t unmapped_page, pgcount;
> +
> +	while (unmapped_len < len) {
> +		pgsize = get_pgsize(iova, len - unmapped_len, &pgcount);
> +
> +		unmapped_page = ops->unmap_pages(ops, iova, pgsize, pgcount, NULL);
> +		WARN_ON(unmapped_page != pgsize * pgcount);
> +
> +		iova += pgsize * pgcount;
> +		unmapped_len += pgsize * pgcount;
> +	}
> +}
> +
>  static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  		      u64 iova, int prot, struct sg_table *sgt)
>  {
>  	unsigned int count;
>  	struct scatterlist *sgl;
>  	struct io_pgtable_ops *ops = mmu->pgtbl_ops;
> +	size_t total_mapped = 0;
>  	u64 start_iova = iova;
> +	int ret = 0;
>  
>  	for_each_sgtable_dma_sg(sgt, sgl, count) {
>  		unsigned long paddr = sg_dma_address(sgl);
> @@ -310,10 +328,15 @@ static int mmu_map_sg(struct panfrost_device *pfdev, struct panfrost_mmu *mmu,
>  			size_t pgcount, mapped = 0;
>  			size_t pgsize = get_pgsize(iova | paddr, len, &pgcount);
>  
> -			ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
> +			ret = ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot,
>  				       GFP_KERNEL, &mapped);
> +			if (ret) {
> +				mmu_unmap_range(total_mapped, start_iova, ops);
> +				return ret;

I general, I'm not a huge fan of those error paths where things are
undone manually, because they tend to be overlooked when new steps are
added. I'd rather have a  "goto error_unmap_pages;" here, and the
according label defined at the end of the function.

> +			}
>  			/* Don't get stuck if things have gone wrong */
>  			mapped = max(mapped, pgsize);
> +			total_mapped += mapped;
>  			iova += mapped;
>  			paddr += mapped;
>  			len -= mapped;
> @@ -333,6 +356,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
>  	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
>  	struct sg_table *sgt;
>  	int prot = IOMMU_READ | IOMMU_WRITE;
> +	int ret;
>  
>  	if (WARN_ON(mapping->active))
>  		return 0;
> @@ -344,8 +368,13 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
>  	if (WARN_ON(IS_ERR(sgt)))
>  		return PTR_ERR(sgt);
>  
> -	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
> -		   prot, sgt);
> +	ret = mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
> +			 prot, sgt);
> +	if (ret) {
> +		drm_gem_shmem_put_pages(shmem);

Same here.

> +		return ret;
> +	}
> +
>  	mapping->active = true;
>  
>  	return 0;
> @@ -532,8 +561,10 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  	if (ret)
>  		goto err_map;
>  
> -	mmu_map_sg(pfdev, bomapping->mmu, addr,
> -		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
> +	ret = mmu_map_sg(pfdev, bomapping->mmu, addr,
> +			 IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
> +	if (ret)
> +		goto err_mmu_map_sg;
>  
>  	bomapping->active = true;
>  	bo->heap_rss_size += SZ_2M;
> @@ -547,6 +578,9 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  
>  	return 0;
>  
> +err_mmu_map_sg:
> +	dma_unmap_sgtable(pfdev->base.dev, sgt,
> +			  DMA_BIDIRECTIONAL, 0);
>  err_map:
>  	sg_free_table(sgt);
>  err_unlock:

[PATCH v2 6/8] drm/panfrost: Make re-enabling job interrupts at device reset optional

[Adrián Larumbe]

Rather than remasking interrupts after a device reset in the main reset
path, allow selecting whether to do this with an additional bool parameter.

To this end, split reenabling job interrupts into two functions, one that
clears the interrupts and another one which unmasks them conditionally.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
 drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
 drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 4fe29286bbe3..7e5d39699982 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 	return false;
 }
 
-void panfrost_device_reset(struct panfrost_device *pfdev)
+void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
+	u32 irq_mask;
+
 	panfrost_gpu_soft_reset(pfdev);
 
 	panfrost_gpu_power_on(pfdev);
 	panfrost_mmu_reset(pfdev);
-	panfrost_job_enable_interrupts(pfdev);
+
+	irq_mask = panfrost_job_reset_interrupts(pfdev);
+	if (enable_job_int)
+		panfrost_enable_interrupts(pfdev, irq_mask);
 }
 
 static int panfrost_device_runtime_resume(struct device *dev)
 {
 	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 
-	panfrost_device_reset(pfdev);
+	panfrost_device_reset(pfdev, true);
 	panfrost_devfreq_resume(pfdev);
 
 	return 0;
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index d9aea2c2cbe5..fc83d5e104a3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
 
 int panfrost_device_init(struct panfrost_device *pfdev);
 void panfrost_device_fini(struct panfrost_device *pfdev);
-void panfrost_device_reset(struct panfrost_device *pfdev);
+void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
 
 extern const struct dev_pm_ops panfrost_pm_ops;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index fba1a376f593..79d2ee3625b2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -27,6 +27,10 @@
 #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
 #define job_read(dev, reg) readl(dev->iomem + (reg))
 
+#define JOB_INT_ENABLE_MASK			\
+	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
+	 GENMASK(NUM_JOB_SLOTS - 1, 0))
+
 struct panfrost_queue_state {
 	struct drm_gpu_scheduler sched;
 	u64 fence_context;
@@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
 	return fence;
 }
 
-void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
+u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
 {
 	int j;
 	u32 irq_mask = 0;
 
-	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
-
 	for (j = 0; j < NUM_JOB_SLOTS; j++) {
 		irq_mask |= MK_JS_MASK(j);
 	}
 
 	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
+
+	return irq_mask;
+}
+
+void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
+{
+	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
 	job_write(pfdev, JOB_INT_MASK, irq_mask);
 }
 
@@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 	spin_unlock(&pfdev->js->job_lock);
 
 	/* Proceed with reset now. */
-	panfrost_device_reset(pfdev);
-
-	/* panfrost_device_reset() unmasks job interrupts, but we want to
-	 * keep them masked a bit longer.
-	 */
-	job_write(pfdev, JOB_INT_MASK, 0);
+	panfrost_device_reset(pfdev, false);
 
 	/* GPU has been reset, we can clear the reset pending bit. */
 	atomic_set(&pfdev->reset.pending, 0);
@@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
 		drm_sched_start(&pfdev->js->queue[i].sched, 0);
 
 	/* Re-enable job interrupts now that everything has been restarted. */
-	job_write(pfdev, JOB_INT_MASK,
-		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
-		  GENMASK(NUM_JOB_SLOTS - 1, 0));
+	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
 
 	dma_fence_end_signalling(cookie);
 }
@@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
 
 	/* Enable interrupts only if we're not about to get suspended */
 	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
-		job_write(pfdev, JOB_INT_MASK,
-			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
-			  GENMASK(NUM_JOB_SLOTS - 1, 0));
+		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
 
 	return IRQ_HANDLED;
 }
@@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 {
 	struct panfrost_job_slot *js;
 	unsigned int nentries = 2;
+	u32 irq_mask;
 	int ret, j;
 
 	/* All GPUs have two entries per queue, but without jobchain
@@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 		}
 	}
 
-	panfrost_job_enable_interrupts(pfdev);
+	irq_mask = panfrost_job_reset_interrupts(pfdev);
+	panfrost_enable_interrupts(pfdev, irq_mask);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index ec581b97852b..c09d42ee5039 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
 int panfrost_job_get_slot(struct panfrost_job *job);
 int panfrost_job_push(struct panfrost_job *job);
 void panfrost_job_put(struct panfrost_job *job);
-void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
+u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
+void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
 void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
 int panfrost_job_is_idle(struct panfrost_device *pfdev);
 
-- 
2.47.0

Re: [PATCH v2 6/8] drm/panfrost: Make re-enabling job interrupts at device reset optional

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:21 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Rather than remasking interrupts after a device reset in the main reset
> path, allow selecting whether to do this with an additional bool parameter.
> 
> To this end, split reenabling job interrupts into two functions, one that
> clears the interrupts and another one which unmasks them conditionally.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
>  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
>  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
>  4 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 4fe29286bbe3..7e5d39699982 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  	return false;
>  }
>  
> -void panfrost_device_reset(struct panfrost_device *pfdev)
> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	u32 irq_mask;
> +
>  	panfrost_gpu_soft_reset(pfdev);
>  
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);
> -	panfrost_job_enable_interrupts(pfdev);
> +
> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> +	if (enable_job_int)
> +		panfrost_enable_interrupts(pfdev, irq_mask);
>  }
>  
>  static int panfrost_device_runtime_resume(struct device *dev)
>  {
>  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
>  
> -	panfrost_device_reset(pfdev);
> +	panfrost_device_reset(pfdev, true);
>  	panfrost_devfreq_resume(pfdev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index d9aea2c2cbe5..fc83d5e104a3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
>  
>  int panfrost_device_init(struct panfrost_device *pfdev);
>  void panfrost_device_fini(struct panfrost_device *pfdev);
> -void panfrost_device_reset(struct panfrost_device *pfdev);
> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
>  
>  extern const struct dev_pm_ops panfrost_pm_ops;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index fba1a376f593..79d2ee3625b2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -27,6 +27,10 @@
>  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
>  #define job_read(dev, reg) readl(dev->iomem + (reg))
>  
> +#define JOB_INT_ENABLE_MASK			\
> +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
> +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
> +
>  struct panfrost_queue_state {
>  	struct drm_gpu_scheduler sched;
>  	u64 fence_context;
> @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>  	return fence;
>  }
>  
> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
>  {
>  	int j;
>  	u32 irq_mask = 0;
>  
> -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> -
>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>  		irq_mask |= MK_JS_MASK(j);
>  	}
>  
>  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
> +
> +	return irq_mask;
> +}

Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
can avoid returning a value that's constant.

> +
> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)

Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
function panfrost_job_enable_interrupts() instead.

> +{
> +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>  	job_write(pfdev, JOB_INT_MASK, irq_mask);
>  }
>  
> @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>  	spin_unlock(&pfdev->js->job_lock);
>  
>  	/* Proceed with reset now. */
> -	panfrost_device_reset(pfdev);
> -
> -	/* panfrost_device_reset() unmasks job interrupts, but we want to
> -	 * keep them masked a bit longer.
> -	 */
> -	job_write(pfdev, JOB_INT_MASK, 0);
> +	panfrost_device_reset(pfdev, false);
>  
>  	/* GPU has been reset, we can clear the reset pending bit. */
>  	atomic_set(&pfdev->reset.pending, 0);
> @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
>  
>  	/* Re-enable job interrupts now that everything has been restarted. */
> -	job_write(pfdev, JOB_INT_MASK,
> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
>  
>  	dma_fence_end_signalling(cookie);
>  }
> @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>  
>  	/* Enable interrupts only if we're not about to get suspended */
>  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> -		job_write(pfdev, JOB_INT_MASK,
> -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  {
>  	struct panfrost_job_slot *js;
>  	unsigned int nentries = 2;
> +	u32 irq_mask;
>  	int ret, j;
>  
>  	/* All GPUs have two entries per queue, but without jobchain
> @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  		}
>  	}
>  
> -	panfrost_job_enable_interrupts(pfdev);
> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> +	panfrost_enable_interrupts(pfdev, irq_mask);
>  
>  	return 0;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index ec581b97852b..c09d42ee5039 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
>  int panfrost_job_get_slot(struct panfrost_job *job);
>  int panfrost_job_push(struct panfrost_job *job);
>  void panfrost_job_put(struct panfrost_job *job);
> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
>  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
>  int panfrost_job_is_idle(struct panfrost_device *pfdev);
>  

Re: [PATCH v2 6/8] drm/panfrost: Make re-enabling job interrupts at device reset optional

[Adrián Larumbe]

Hi Boris,

On 02.12.2024 12:20, Boris Brezillon wrote:
> On Thu, 28 Nov 2024 21:06:21 +0000
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > Rather than remasking interrupts after a device reset in the main reset
> > path, allow selecting whether to do this with an additional bool parameter.
> > 
> > To this end, split reenabling job interrupts into two functions, one that
> > clears the interrupts and another one which unmasks them conditionally.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
> >  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
> >  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
> >  4 files changed, 29 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index 4fe29286bbe3..7e5d39699982 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
> >  	return false;
> >  }
> >  
> > -void panfrost_device_reset(struct panfrost_device *pfdev)
> > +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
> >  {
> > +	u32 irq_mask;
> > +
> >  	panfrost_gpu_soft_reset(pfdev);
> >  
> >  	panfrost_gpu_power_on(pfdev);
> >  	panfrost_mmu_reset(pfdev);
> > -	panfrost_job_enable_interrupts(pfdev);
> > +
> > +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> > +	if (enable_job_int)
> > +		panfrost_enable_interrupts(pfdev, irq_mask);
> >  }
> >  
> >  static int panfrost_device_runtime_resume(struct device *dev)
> >  {
> >  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> >  
> > -	panfrost_device_reset(pfdev);
> > +	panfrost_device_reset(pfdev, true);
> >  	panfrost_devfreq_resume(pfdev);
> >  
> >  	return 0;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> > index d9aea2c2cbe5..fc83d5e104a3 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> > @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
> >  
> >  int panfrost_device_init(struct panfrost_device *pfdev);
> >  void panfrost_device_fini(struct panfrost_device *pfdev);
> > -void panfrost_device_reset(struct panfrost_device *pfdev);
> > +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
> >  
> >  extern const struct dev_pm_ops panfrost_pm_ops;
> >  
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index fba1a376f593..79d2ee3625b2 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -27,6 +27,10 @@
> >  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
> >  #define job_read(dev, reg) readl(dev->iomem + (reg))
> >  
> > +#define JOB_INT_ENABLE_MASK			\
> > +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
> > +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
> > +
> >  struct panfrost_queue_state {
> >  	struct drm_gpu_scheduler sched;
> >  	u64 fence_context;
> > @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
> >  	return fence;
> >  }
> >  
> > -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
> > +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
> >  {
> >  	int j;
> >  	u32 irq_mask = 0;
> >  
> > -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> > -
> >  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
> >  		irq_mask |= MK_JS_MASK(j);
> >  	}
> >  
> >  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
> > +
> > +	return irq_mask;
> > +}
> 
> Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
> can avoid returning a value that's constant.

Because ALL_JS_INT_MASK would be defined as an OR of MK_JS_MASK() applications,
and this macro is defined in panfrost_regs.h, I can't think of a nice location
for it that would be suitable for all the files where it would be called.

Maybe I could implement the same loop inside panfrost_job_init() where it would be
called only once, and store the result in a const struct panfrost_job_slot field?

> > +
> > +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
> 
> Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
> function panfrost_job_enable_interrupts() instead.

I made a mistake here naming this function, it should've been panfrost_job_enable_interrupts().

Other than that, I decided to keep the irq_mask argument because I'm also calling it from
the very end of panfrost_reset() with JOB_INT_ENABLE_MASK, where I defined it to be

(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | GENMASK(NUM_JOB_SLOTS - 1, 0))

That raises the question, what is the functional difference between writing either
this or MK_JS_MASK(0) | MK_JS_MASK(1) | MK_JS_MASK(2) into the JOB_INT_MASK register?

It's also being done in panfrost_job_irq_handler_thread() so I'm not quite sure of this.

> > +{
> > +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
> >  	job_write(pfdev, JOB_INT_MASK, irq_mask);
> >  }
> >  
> > @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
> >  	spin_unlock(&pfdev->js->job_lock);
> >  
> >  	/* Proceed with reset now. */
> > -	panfrost_device_reset(pfdev);
> > -
> > -	/* panfrost_device_reset() unmasks job interrupts, but we want to
> > -	 * keep them masked a bit longer.
> > -	 */
> > -	job_write(pfdev, JOB_INT_MASK, 0);
> > +	panfrost_device_reset(pfdev, false);
> >  
> >  	/* GPU has been reset, we can clear the reset pending bit. */
> >  	atomic_set(&pfdev->reset.pending, 0);
> > @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
> >  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
> >  
> >  	/* Re-enable job interrupts now that everything has been restarted. */
> > -	job_write(pfdev, JOB_INT_MASK,
> > -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
> >  
> >  	dma_fence_end_signalling(cookie);
> >  }
> > @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
> >  
> >  	/* Enable interrupts only if we're not about to get suspended */
> >  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
> > -		job_write(pfdev, JOB_INT_MASK,
> > -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
> > -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
> > +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
> >  
> >  	return IRQ_HANDLED;
> >  }
> > @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> >  {
> >  	struct panfrost_job_slot *js;
> >  	unsigned int nentries = 2;
> > +	u32 irq_mask;
> >  	int ret, j;
> >  
> >  	/* All GPUs have two entries per queue, but without jobchain
> > @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> >  		}
> >  	}
> >  
> > -	panfrost_job_enable_interrupts(pfdev);
> > +	irq_mask = panfrost_job_reset_interrupts(pfdev);
> > +	panfrost_enable_interrupts(pfdev, irq_mask);
> >  
> >  	return 0;
> >  
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> > index ec581b97852b..c09d42ee5039 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> > @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
> >  int panfrost_job_get_slot(struct panfrost_job *job);
> >  int panfrost_job_push(struct panfrost_job *job);
> >  void panfrost_job_put(struct panfrost_job *job);
> > -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
> > +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
> > +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
> >  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
> >  int panfrost_job_is_idle(struct panfrost_device *pfdev);
> >  

Re: [PATCH v2 6/8] drm/panfrost: Make re-enabling job interrupts at device reset optional

[Steven Price]

On 04/12/2024 15:34, Adrián Larumbe wrote:
> Hi Boris,
> 
> On 02.12.2024 12:20, Boris Brezillon wrote:
>> On Thu, 28 Nov 2024 21:06:21 +0000
>> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
>>
>>> Rather than remasking interrupts after a device reset in the main reset
>>> path, allow selecting whether to do this with an additional bool parameter.
>>>
>>> To this end, split reenabling job interrupts into two functions, one that
>>> clears the interrupts and another one which unmasks them conditionally.
>>>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++--
>>>  drivers/gpu/drm/panfrost/panfrost_device.h |  2 +-
>>>  drivers/gpu/drm/panfrost/panfrost_job.c    | 34 ++++++++++++----------
>>>  drivers/gpu/drm/panfrost/panfrost_job.h    |  3 +-
>>>  4 files changed, 29 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> index 4fe29286bbe3..7e5d39699982 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> @@ -395,20 +395,25 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>>>  	return false;
>>>  }
>>>  
>>> -void panfrost_device_reset(struct panfrost_device *pfdev)
>>> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>>>  {
>>> +	u32 irq_mask;
>>> +
>>>  	panfrost_gpu_soft_reset(pfdev);
>>>  
>>>  	panfrost_gpu_power_on(pfdev);
>>>  	panfrost_mmu_reset(pfdev);
>>> -	panfrost_job_enable_interrupts(pfdev);
>>> +
>>> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
>>> +	if (enable_job_int)
>>> +		panfrost_enable_interrupts(pfdev, irq_mask);
>>>  }
>>>  
>>>  static int panfrost_device_runtime_resume(struct device *dev)
>>>  {
>>>  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
>>>  
>>> -	panfrost_device_reset(pfdev);
>>> +	panfrost_device_reset(pfdev, true);
>>>  	panfrost_devfreq_resume(pfdev);
>>>  
>>>  	return 0;
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> index d9aea2c2cbe5..fc83d5e104a3 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>>> @@ -205,7 +205,7 @@ int panfrost_unstable_ioctl_check(void);
>>>  
>>>  int panfrost_device_init(struct panfrost_device *pfdev);
>>>  void panfrost_device_fini(struct panfrost_device *pfdev);
>>> -void panfrost_device_reset(struct panfrost_device *pfdev);
>>> +void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
>>>  
>>>  extern const struct dev_pm_ops panfrost_pm_ops;
>>>  
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> index fba1a376f593..79d2ee3625b2 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>>> @@ -27,6 +27,10 @@
>>>  #define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
>>>  #define job_read(dev, reg) readl(dev->iomem + (reg))
>>>  
>>> +#define JOB_INT_ENABLE_MASK			\
>>> +	(GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |	\
>>> +	 GENMASK(NUM_JOB_SLOTS - 1, 0))
>>> +
>>>  struct panfrost_queue_state {
>>>  	struct drm_gpu_scheduler sched;
>>>  	u64 fence_context;
>>> @@ -421,18 +425,23 @@ static struct dma_fence *panfrost_job_run(struct drm_sched_job *sched_job)
>>>  	return fence;
>>>  }
>>>  
>>> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev)
>>> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev)
>>>  {
>>>  	int j;
>>>  	u32 irq_mask = 0;
>>>  
>>> -	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>>> -
>>>  	for (j = 0; j < NUM_JOB_SLOTS; j++) {
>>>  		irq_mask |= MK_JS_MASK(j);
>>>  	}
>>>  
>>>  	job_write(pfdev, JOB_INT_CLEAR, irq_mask);
>>> +
>>> +	return irq_mask;
>>> +}
>>
>> Let's define an ALL_JS_INT_MASK so we can get rid of the for loop and
>> can avoid returning a value that's constant.
> 
> Because ALL_JS_INT_MASK would be defined as an OR of MK_JS_MASK() applications,
> and this macro is defined in panfrost_regs.h, I can't think of a nice location
> for it that would be suitable for all the files where it would be called.

I can't speak for Boris, but I'd be quite happy with a:

#define ALL_JS_INT_MASK 0x70007

We know NUM_JOB_SLOTS is a (hardware) constant so we can compute the
value once. That or MK_JS_MASK(0)|MK_JS_MASK(1)|MK_JS_MASK(2) if you
prefer, or of course the GENMASK() variant below.

> Maybe I could implement the same loop inside panfrost_job_init() where it would be
> called only once, and store the result in a const struct panfrost_job_slot field?

It seems silly wasting memory on a compile time constant...

>>> +
>>> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask)
>>
>> Let's drop the irq_mask mask (use ALL_JS_INT_MASK), and call the
>> function panfrost_job_enable_interrupts() instead.
> 
> I made a mistake here naming this function, it should've been panfrost_job_enable_interrupts().
> 
> Other than that, I decided to keep the irq_mask argument because I'm also calling it from
> the very end of panfrost_reset() with JOB_INT_ENABLE_MASK, where I defined it to be
> 
> (GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | GENMASK(NUM_JOB_SLOTS - 1, 0))
> 
> That raises the question, what is the functional difference between writing either
> this or MK_JS_MASK(0) | MK_JS_MASK(1) | MK_JS_MASK(2) into the JOB_INT_MASK register?

Hopefully none - the two values should be the same. Indeed the GENMASK
variant is possibly best because it encodes using NUM_JOB_SLOTS.
Although I have to admit I have to think harder to decode the GENMASK()
version than either of the other alternatives above.

Steve

> It's also being done in panfrost_job_irq_handler_thread() so I'm not quite sure of this.
> 
>>> +{
>>> +	clear_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
>>>  	job_write(pfdev, JOB_INT_MASK, irq_mask);
>>>  }
>>>  
>>> @@ -725,12 +734,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>>>  	spin_unlock(&pfdev->js->job_lock);
>>>  
>>>  	/* Proceed with reset now. */
>>> -	panfrost_device_reset(pfdev);
>>> -
>>> -	/* panfrost_device_reset() unmasks job interrupts, but we want to
>>> -	 * keep them masked a bit longer.
>>> -	 */
>>> -	job_write(pfdev, JOB_INT_MASK, 0);
>>> +	panfrost_device_reset(pfdev, false);
>>>  
>>>  	/* GPU has been reset, we can clear the reset pending bit. */
>>>  	atomic_set(&pfdev->reset.pending, 0);
>>> @@ -752,9 +756,7 @@ panfrost_reset(struct panfrost_device *pfdev,
>>>  		drm_sched_start(&pfdev->js->queue[i].sched, 0);
>>>  
>>>  	/* Re-enable job interrupts now that everything has been restarted. */
>>> -	job_write(pfdev, JOB_INT_MASK,
>>> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>>> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
>>> +	panfrost_enable_interrupts(pfdev, JOB_INT_ENABLE_MASK);
>>>  
>>>  	dma_fence_end_signalling(cookie);
>>>  }
>>> @@ -827,9 +829,7 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>>>  
>>>  	/* Enable interrupts only if we're not about to get suspended */
>>>  	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
>>> -		job_write(pfdev, JOB_INT_MASK,
>>> -			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>>> -			  GENMASK(NUM_JOB_SLOTS - 1, 0));
>>> +		job_write(pfdev, JOB_INT_MASK, JOB_INT_ENABLE_MASK);
>>>  
>>>  	return IRQ_HANDLED;
>>>  }
>>> @@ -854,6 +854,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>>  {
>>>  	struct panfrost_job_slot *js;
>>>  	unsigned int nentries = 2;
>>> +	u32 irq_mask;
>>>  	int ret, j;
>>>  
>>>  	/* All GPUs have two entries per queue, but without jobchain
>>> @@ -905,7 +906,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>>  		}
>>>  	}
>>>  
>>> -	panfrost_job_enable_interrupts(pfdev);
>>> +	irq_mask = panfrost_job_reset_interrupts(pfdev);
>>> +	panfrost_enable_interrupts(pfdev, irq_mask);
>>>  
>>>  	return 0;
>>>  
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
>>> index ec581b97852b..c09d42ee5039 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
>>> @@ -46,7 +46,8 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv);
>>>  int panfrost_job_get_slot(struct panfrost_job *job);
>>>  int panfrost_job_push(struct panfrost_job *job);
>>>  void panfrost_job_put(struct panfrost_job *job);
>>> -void panfrost_job_enable_interrupts(struct panfrost_device *pfdev);
>>> +u32 panfrost_job_reset_interrupts(struct panfrost_device *pfdev);
>>> +void panfrost_enable_interrupts(struct panfrost_device *pfdev, u32 irq_mask);
>>>  void panfrost_job_suspend_irq(struct panfrost_device *pfdev);
>>>  int panfrost_job_is_idle(struct panfrost_device *pfdev);
>>>  

[PATCH v2 7/8] drm/panfrost: Add forward declaration and types header

[Adrián Larumbe]

This is to make LLVM syntactic analysers happy.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
index e6e6966a0cca..27c3c65ed074 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
@@ -4,6 +4,7 @@
 #ifndef __PANFROST_MMU_H__
 #define __PANFROST_MMU_H__
 
+struct panfrost_device;
 struct panfrost_gem_mapping;
 struct panfrost_file_priv;
 struct panfrost_mmu;
-- 
2.47.0

Re: [PATCH v2 7/8] drm/panfrost: Add forward declaration and types header

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:22 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> This is to make LLVM syntactic analysers happy.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> index e6e6966a0cca..27c3c65ed074 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
> @@ -4,6 +4,7 @@
>  #ifndef __PANFROST_MMU_H__
>  #define __PANFROST_MMU_H__
>  
> +struct panfrost_device;
>  struct panfrost_gem_mapping;
>  struct panfrost_file_priv;
>  struct panfrost_mmu;

[PATCH v2 8/8] drm/panfrost: Remove unused device property

[Adrián Larumbe]

The as_in_use_mask device state variable is no longer in use.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h | 1 -
 drivers/gpu/drm/panfrost/panfrost_mmu.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index fc83d5e104a3..b91957f886ea 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -117,7 +117,6 @@ struct panfrost_device {
 	DECLARE_BITMAP(is_suspended, PANFROST_COMP_BIT_MAX);
 
 	spinlock_t as_lock;
-	unsigned long as_in_use_mask;
 	unsigned long as_alloc_mask;
 	unsigned long as_faulty_mask;
 	struct list_head as_lru_list;
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 5e30888bea0e..95df39b463d8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -604,7 +604,6 @@ static void panfrost_mmu_release_ctx(struct kref *kref)
 		pm_runtime_put_autosuspend(pfdev->base.dev);
 
 		clear_bit(mmu->as, &pfdev->as_alloc_mask);
-		clear_bit(mmu->as, &pfdev->as_in_use_mask);
 		list_del(&mmu->list);
 	}
 	spin_unlock(&pfdev->as_lock);
-- 
2.47.0

Re: [PATCH v2 8/8] drm/panfrost: Remove unused device property

[Boris Brezillon]

On Thu, 28 Nov 2024 21:06:23 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> The as_in_use_mask device state variable is no longer in use.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.h | 1 -
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index fc83d5e104a3..b91957f886ea 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -117,7 +117,6 @@ struct panfrost_device {
>  	DECLARE_BITMAP(is_suspended, PANFROST_COMP_BIT_MAX);
>  
>  	spinlock_t as_lock;
> -	unsigned long as_in_use_mask;
>  	unsigned long as_alloc_mask;
>  	unsigned long as_faulty_mask;
>  	struct list_head as_lru_list;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 5e30888bea0e..95df39b463d8 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -604,7 +604,6 @@ static void panfrost_mmu_release_ctx(struct kref *kref)
>  		pm_runtime_put_autosuspend(pfdev->base.dev);
>  
>  		clear_bit(mmu->as, &pfdev->as_alloc_mask);
> -		clear_bit(mmu->as, &pfdev->as_in_use_mask);
>  		list_del(&mmu->list);
>  	}
>  	spin_unlock(&pfdev->as_lock);