[PATCH 0/2] drm/panfrost: Tidy up the devfreq implementation

[PATCH 0/2] drm/panfrost: Tidy up the devfreq implementation

[Steven Price]

The devfreq implementation in panfrost is unnecessarily open coded. It
also tracks utilisation metrics per slot which isn't very useful. Let's
tidy it up!

This should be picked up along with Mark's change[1] to fix
regulator_get_optional() misuse. This also deletes the code changes from
52282163dfa6 and e21dd290881b which would otherwise need reverting, see
the previous discussion[2].

[1] https://lore.kernel.org/lkml/20190904123032.23263-1-broonie@kernel.org/
[2] https://lore.kernel.org/lkml/ccd81530-2dbd-3c02-ca0a-1085b00663b5@arm.com/

Steven Price (2):
  drm/panfrost: Use generic code for devfreq
  drm/panfrost: Simplify devfreq utilisation tracking

 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 126 ++++++--------------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |   3 +-
 drivers/gpu/drm/panfrost/panfrost_device.h  |  14 +--
 drivers/gpu/drm/panfrost/panfrost_job.c     |  14 +--
 4 files changed, 48 insertions(+), 109 deletions(-)

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] drm/panfrost: Tidy up the devfreq implementation

[Steven Price]

The devfreq implementation in panfrost is unnecessarily open coded. It
also tracks utilisation metrics per slot which isn't very useful. Let's
tidy it up!

This should be picked up along with Mark's change[1] to fix
regulator_get_optional() misuse. This also deletes the code changes from
52282163dfa6 and e21dd290881b which would otherwise need reverting, see
the previous discussion[2].

[1] https://lore.kernel.org/lkml/20190904123032.23263-1-broonie@kernel.org/
[2] https://lore.kernel.org/lkml/ccd81530-2dbd-3c02-ca0a-1085b00663b5@arm.com/

Steven Price (2):
  drm/panfrost: Use generic code for devfreq
  drm/panfrost: Simplify devfreq utilisation tracking

 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 126 ++++++--------------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |   3 +-
 drivers/gpu/drm/panfrost/panfrost_device.h  |  14 +--
 drivers/gpu/drm/panfrost/panfrost_job.c     |  14 +--
 4 files changed, 48 insertions(+), 109 deletions(-)

-- 
2.20.1

[PATCH 1/2] drm/panfrost: Use generic code for devfreq

[Steven Price]

Use dev_pm_opp_set_rate() instead of open coding the devfreq
integration, simplifying the code.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 62 ++++-----------------
 drivers/gpu/drm/panfrost/panfrost_device.h  |  2 -
 2 files changed, 10 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index a1f5fa6a742a..7ded282a5ca8 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -18,57 +18,14 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
 static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 				   u32 flags)
 {
-	struct panfrost_device *pfdev = platform_get_drvdata(to_platform_device(dev));
-	struct dev_pm_opp *opp;
-	unsigned long old_clk_rate = pfdev->devfreq.cur_freq;
-	unsigned long target_volt, target_rate;
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 	int err;
 
-	opp = devfreq_recommended_opp(dev, freq, flags);
-	if (IS_ERR(opp))
-		return PTR_ERR(opp);
-
-	target_rate = dev_pm_opp_get_freq(opp);
-	target_volt = dev_pm_opp_get_voltage(opp);
-	dev_pm_opp_put(opp);
-
-	if (old_clk_rate == target_rate)
-		return 0;
-
-	/*
-	 * If frequency scaling from low to high, adjust voltage first.
-	 * If frequency scaling from high to low, adjust frequency first.
-	 */
-	if (old_clk_rate < target_rate && pfdev->regulator) {
-		err = regulator_set_voltage(pfdev->regulator, target_volt,
-					    target_volt);
-		if (err) {
-			dev_err(dev, "Cannot set voltage %lu uV\n",
-				target_volt);
-			return err;
-		}
-	}
-
-	err = clk_set_rate(pfdev->clock, target_rate);
-	if (err) {
-		dev_err(dev, "Cannot set frequency %lu (%d)\n", target_rate,
-			err);
-		if (pfdev->regulator)
-			regulator_set_voltage(pfdev->regulator,
-					      pfdev->devfreq.cur_volt,
-					      pfdev->devfreq.cur_volt);
+	err = dev_pm_opp_set_rate(dev, *freq);
+	if (err)
 		return err;
-	}
 
-	if (old_clk_rate > target_rate && pfdev->regulator) {
-		err = regulator_set_voltage(pfdev->regulator, target_volt,
-					    target_volt);
-		if (err)
-			dev_err(dev, "Cannot set voltage %lu uV\n", target_volt);
-	}
-
-	pfdev->devfreq.cur_freq = target_rate;
-	pfdev->devfreq.cur_volt = target_volt;
+	*freq = clk_get_rate(pfdev->clock);
 
 	return 0;
 }
@@ -88,7 +45,7 @@ static void panfrost_devfreq_reset(struct panfrost_device *pfdev)
 static int panfrost_devfreq_get_dev_status(struct device *dev,
 					   struct devfreq_dev_status *status)
 {
-	struct panfrost_device *pfdev = platform_get_drvdata(to_platform_device(dev));
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 	int i;
 
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
@@ -121,7 +78,7 @@ static int panfrost_devfreq_get_cur_freq(struct device *dev, unsigned long *freq
 {
 	struct panfrost_device *pfdev = platform_get_drvdata(to_platform_device(dev));
 
-	*freq = pfdev->devfreq.cur_freq;
+	*freq = clk_get_rate(pfdev->clock);
 
 	return 0;
 }
@@ -137,6 +94,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 {
 	int ret;
 	struct dev_pm_opp *opp;
+	unsigned long cur_freq;
 
 	ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
 	if (ret == -ENODEV) /* Optional, continue without devfreq */
@@ -146,13 +104,13 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 
 	panfrost_devfreq_reset(pfdev);
 
-	pfdev->devfreq.cur_freq = clk_get_rate(pfdev->clock);
+	cur_freq = clk_get_rate(pfdev->clock);
 
-	opp = devfreq_recommended_opp(&pfdev->pdev->dev, &pfdev->devfreq.cur_freq, 0);
+	opp = devfreq_recommended_opp(&pfdev->pdev->dev, &cur_freq, 0);
 	if (IS_ERR(opp))
 		return PTR_ERR(opp);
 
-	panfrost_devfreq_profile.initial_freq = pfdev->devfreq.cur_freq;
+	panfrost_devfreq_profile.initial_freq = cur_freq;
 	dev_pm_opp_put(opp);
 
 	pfdev->devfreq.devfreq = devm_devfreq_add_device(&pfdev->pdev->dev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index f503c566e99f..4c2b3c84baac 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -95,8 +95,6 @@ struct panfrost_device {
 	struct {
 		struct devfreq *devfreq;
 		struct thermal_cooling_device *cooling;
-		unsigned long cur_freq;
-		unsigned long cur_volt;
 		struct panfrost_devfreq_slot slot[NUM_JOB_SLOTS];
 	} devfreq;
 };
-- 
2.20.1

Re: [PATCH 1/2] drm/panfrost: Use generic code for devfreq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 208 bytes --]

On Thu, Sep 12, 2019 at 12:28:03PM +0100, Steven Price wrote:
> Use dev_pm_opp_set_rate() instead of open coding the devfreq
> integration, simplifying the code.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] drm/panfrost: Simplify devfreq utilisation tracking

[Steven Price]

Instead of tracking per-slot utilisation track a single value for the
entire GPU. Ultimately it doesn't matter if the GPU is busy with only
vertex or a combination of vertex and fragment processing - if it's busy
then it's busy and devfreq should be scaling appropriately.

This also makes way for being able to submit multiple jobs per slot
which requires more values than the original boolean per slot.

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 64 ++++++++-------------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  3 +-
 drivers/gpu/drm/panfrost/panfrost_device.h  | 12 ++--
 drivers/gpu/drm/panfrost/panfrost_job.c     | 14 ++---
 4 files changed, 38 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 7ded282a5ca8..4c4e8a30a1ac 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -13,7 +13,7 @@
 #include "panfrost_gpu.h"
 #include "panfrost_regs.h"
 
-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, int slot);
+static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev);
 
 static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 				   u32 flags)
@@ -32,37 +32,23 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 
 static void panfrost_devfreq_reset(struct panfrost_device *pfdev)
 {
-	ktime_t now = ktime_get();
-	int i;
-
-	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		pfdev->devfreq.slot[i].busy_time = 0;
-		pfdev->devfreq.slot[i].idle_time = 0;
-		pfdev->devfreq.slot[i].time_last_update = now;
-	}
+	pfdev->devfreq.busy_time = 0;
+	pfdev->devfreq.idle_time = 0;
+	pfdev->devfreq.time_last_update = ktime_get();
 }
 
 static int panfrost_devfreq_get_dev_status(struct device *dev,
 					   struct devfreq_dev_status *status)
 {
 	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int i;
 
-	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		panfrost_devfreq_update_utilization(pfdev, i);
-	}
+	panfrost_devfreq_update_utilization(pfdev);
 
 	status->current_frequency = clk_get_rate(pfdev->clock);
-	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.slot[0].busy_time,
-						   pfdev->devfreq.slot[0].idle_time));
-
-	status->busy_time = 0;
-	for (i = 0; i < NUM_JOB_SLOTS; i++) {
-		status->busy_time += ktime_to_ns(pfdev->devfreq.slot[i].busy_time);
-	}
+	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.busy_time,
+						   pfdev->devfreq.idle_time));
 
-	/* We're scheduling only to one core atm, so don't divide for now */
-	/* status->busy_time /= NUM_JOB_SLOTS; */
+	status->busy_time = ktime_to_ns(pfdev->devfreq.busy_time);
 
 	panfrost_devfreq_reset(pfdev);
 
@@ -134,14 +120,10 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
 {
-	int i;
-
 	if (!pfdev->devfreq.devfreq)
 		return;
 
 	panfrost_devfreq_reset(pfdev);
-	for (i = 0; i < NUM_JOB_SLOTS; i++)
-		pfdev->devfreq.slot[i].busy = false;
 
 	devfreq_resume_device(pfdev->devfreq.devfreq);
 }
@@ -154,9 +136,8 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
 	devfreq_suspend_device(pfdev->devfreq.devfreq);
 }
 
-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, int slot)
+static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
 {
-	struct panfrost_devfreq_slot *devfreq_slot = &pfdev->devfreq.slot[slot];
 	ktime_t now;
 	ktime_t last;
 
@@ -164,22 +145,27 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
 		return;
 
 	now = ktime_get();
-	last = pfdev->devfreq.slot[slot].time_last_update;
+	last = pfdev->devfreq.time_last_update;
 
-	/* If we last recorded a transition to busy, we have been idle since */
-	if (devfreq_slot->busy)
-		pfdev->devfreq.slot[slot].busy_time += ktime_sub(now, last);
+	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
+		pfdev->devfreq.busy_time += ktime_sub(now, last);
 	else
-		pfdev->devfreq.slot[slot].idle_time += ktime_sub(now, last);
+		pfdev->devfreq.idle_time += ktime_sub(now, last);
+
+	pfdev->devfreq.time_last_update = now;
+}
 
-	pfdev->devfreq.slot[slot].time_last_update = now;
+void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
+{
+	panfrost_devfreq_update_utilization(pfdev);
+	atomic_inc(&pfdev->devfreq.busy_count);
 }
 
-/* The job scheduler is expected to call this at every transition busy <-> idle */
-void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
+void panfrost_devfreq_record_idle(struct panfrost_device *pfdev)
 {
-	struct panfrost_devfreq_slot *devfreq_slot = &pfdev->devfreq.slot[slot];
+	int count;
 
-	panfrost_devfreq_update_utilization(pfdev, slot);
-	devfreq_slot->busy = !devfreq_slot->busy;
+	panfrost_devfreq_update_utilization(pfdev);
+	count = atomic_dec_if_positive(&pfdev->devfreq.busy_count);
+	WARN_ON(count < 0);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index e3bc63e82843..0611beffc8d0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -10,6 +10,7 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev);
 void panfrost_devfreq_resume(struct panfrost_device *pfdev);
 void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
 
-void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
+void panfrost_devfreq_record_busy(struct panfrost_device *pfdev);
+void panfrost_devfreq_record_idle(struct panfrost_device *pfdev);
 
 #endif /* __PANFROST_DEVFREQ_H__ */
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 4c2b3c84baac..233957f88d77 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -51,13 +51,6 @@ struct panfrost_features {
 	unsigned long hw_issues[64 / BITS_PER_LONG];
 };
 
-struct panfrost_devfreq_slot {
-	ktime_t busy_time;
-	ktime_t idle_time;
-	ktime_t time_last_update;
-	bool busy;
-};
-
 struct panfrost_device {
 	struct device *dev;
 	struct drm_device *ddev;
@@ -95,7 +88,10 @@ struct panfrost_device {
 	struct {
 		struct devfreq *devfreq;
 		struct thermal_cooling_device *cooling;
-		struct panfrost_devfreq_slot slot[NUM_JOB_SLOTS];
+		ktime_t busy_time;
+		ktime_t idle_time;
+		ktime_t time_last_update;
+		atomic_t busy_count;
 	} devfreq;
 };
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 05c85f45a0de..47aeadb4f161 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -155,7 +155,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 
 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
 
-	panfrost_devfreq_record_transition(pfdev, js);
+	panfrost_devfreq_record_busy(pfdev);
 	spin_lock_irqsave(&pfdev->hwaccess_lock, flags);
 
 	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
@@ -396,7 +396,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
 
 	/* panfrost_core_dump(pfdev); */
 
-	panfrost_devfreq_record_transition(pfdev, js);
+	panfrost_devfreq_record_idle(pfdev);
 	panfrost_device_reset(pfdev);
 
 	for (i = 0; i < NUM_JOB_SLOTS; i++)
@@ -454,7 +454,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
 
 			pfdev->jobs[j] = NULL;
 			panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
-			panfrost_devfreq_record_transition(pfdev, j);
+			panfrost_devfreq_record_idle(pfdev);
 			dma_fence_signal(job->done_fence);
 		}
 
@@ -551,14 +551,14 @@ int panfrost_job_is_idle(struct panfrost_device *pfdev)
 	struct panfrost_job_slot *js = pfdev->js;
 	int i;
 
+	/* Check whether the hardware is idle */
+	if (atomic_read(&pfdev->devfreq.busy_count))
+		return false;
+
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		/* If there are any jobs in the HW queue, we're not idle */
 		if (atomic_read(&js->queue[i].sched.hw_rq_count))
 			return false;
-
-		/* Check whether the hardware is idle */
-		if (pfdev->devfreq.slot[i].busy)
-			return false;
 	}
 
 	return true;
-- 
2.20.1

Re: [PATCH 2/2] drm/panfrost: Simplify devfreq utilisation tracking

[Alyssa Rosenzweig]

[-- Attachment #1: Type: text/plain, Size: 9246 bytes --]

Patch 1 is:

	Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Patch 2 is:

	Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Good stuff as always!

On Thu, Sep 12, 2019 at 12:28:04PM +0100, Steven Price wrote:
> Instead of tracking per-slot utilisation track a single value for the
> entire GPU. Ultimately it doesn't matter if the GPU is busy with only
> vertex or a combination of vertex and fragment processing - if it's busy
> then it's busy and devfreq should be scaling appropriately.
> 
> This also makes way for being able to submit multiple jobs per slot
> which requires more values than the original boolean per slot.
> 
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 64 ++++++++-------------
>  drivers/gpu/drm/panfrost/panfrost_devfreq.h |  3 +-
>  drivers/gpu/drm/panfrost/panfrost_device.h  | 12 ++--
>  drivers/gpu/drm/panfrost/panfrost_job.c     | 14 ++---
>  4 files changed, 38 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 7ded282a5ca8..4c4e8a30a1ac 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -13,7 +13,7 @@
>  #include "panfrost_gpu.h"
>  #include "panfrost_regs.h"
>  
> -static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, int slot);
> +static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev);
>  
>  static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  				   u32 flags)
> @@ -32,37 +32,23 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  
>  static void panfrost_devfreq_reset(struct panfrost_device *pfdev)
>  {
> -	ktime_t now = ktime_get();
> -	int i;
> -
> -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		pfdev->devfreq.slot[i].busy_time = 0;
> -		pfdev->devfreq.slot[i].idle_time = 0;
> -		pfdev->devfreq.slot[i].time_last_update = now;
> -	}
> +	pfdev->devfreq.busy_time = 0;
> +	pfdev->devfreq.idle_time = 0;
> +	pfdev->devfreq.time_last_update = ktime_get();
>  }
>  
>  static int panfrost_devfreq_get_dev_status(struct device *dev,
>  					   struct devfreq_dev_status *status)
>  {
>  	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> -	int i;
>  
> -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		panfrost_devfreq_update_utilization(pfdev, i);
> -	}
> +	panfrost_devfreq_update_utilization(pfdev);
>  
>  	status->current_frequency = clk_get_rate(pfdev->clock);
> -	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.slot[0].busy_time,
> -						   pfdev->devfreq.slot[0].idle_time));
> -
> -	status->busy_time = 0;
> -	for (i = 0; i < NUM_JOB_SLOTS; i++) {
> -		status->busy_time += ktime_to_ns(pfdev->devfreq.slot[i].busy_time);
> -	}
> +	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.busy_time,
> +						   pfdev->devfreq.idle_time));
>  
> -	/* We're scheduling only to one core atm, so don't divide for now */
> -	/* status->busy_time /= NUM_JOB_SLOTS; */
> +	status->busy_time = ktime_to_ns(pfdev->devfreq.busy_time);
>  
>  	panfrost_devfreq_reset(pfdev);
>  
> @@ -134,14 +120,10 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>  
>  void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>  {
> -	int i;
> -
>  	if (!pfdev->devfreq.devfreq)
>  		return;
>  
>  	panfrost_devfreq_reset(pfdev);
> -	for (i = 0; i < NUM_JOB_SLOTS; i++)
> -		pfdev->devfreq.slot[i].busy = false;
>  
>  	devfreq_resume_device(pfdev->devfreq.devfreq);
>  }
> @@ -154,9 +136,8 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>  	devfreq_suspend_device(pfdev->devfreq.devfreq);
>  }
>  
> -static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, int slot)
> +static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
>  {
> -	struct panfrost_devfreq_slot *devfreq_slot = &pfdev->devfreq.slot[slot];
>  	ktime_t now;
>  	ktime_t last;
>  
> @@ -164,22 +145,27 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
>  		return;
>  
>  	now = ktime_get();
> -	last = pfdev->devfreq.slot[slot].time_last_update;
> +	last = pfdev->devfreq.time_last_update;
>  
> -	/* If we last recorded a transition to busy, we have been idle since */
> -	if (devfreq_slot->busy)
> -		pfdev->devfreq.slot[slot].busy_time += ktime_sub(now, last);
> +	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
> +		pfdev->devfreq.busy_time += ktime_sub(now, last);
>  	else
> -		pfdev->devfreq.slot[slot].idle_time += ktime_sub(now, last);
> +		pfdev->devfreq.idle_time += ktime_sub(now, last);
> +
> +	pfdev->devfreq.time_last_update = now;
> +}
>  
> -	pfdev->devfreq.slot[slot].time_last_update = now;
> +void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
> +{
> +	panfrost_devfreq_update_utilization(pfdev);
> +	atomic_inc(&pfdev->devfreq.busy_count);
>  }
>  
> -/* The job scheduler is expected to call this at every transition busy <-> idle */
> -void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
> +void panfrost_devfreq_record_idle(struct panfrost_device *pfdev)
>  {
> -	struct panfrost_devfreq_slot *devfreq_slot = &pfdev->devfreq.slot[slot];
> +	int count;
>  
> -	panfrost_devfreq_update_utilization(pfdev, slot);
> -	devfreq_slot->busy = !devfreq_slot->busy;
> +	panfrost_devfreq_update_utilization(pfdev);
> +	count = atomic_dec_if_positive(&pfdev->devfreq.busy_count);
> +	WARN_ON(count < 0);
>  }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index e3bc63e82843..0611beffc8d0 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -10,6 +10,7 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev);
>  void panfrost_devfreq_resume(struct panfrost_device *pfdev);
>  void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
>  
> -void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
> +void panfrost_devfreq_record_busy(struct panfrost_device *pfdev);
> +void panfrost_devfreq_record_idle(struct panfrost_device *pfdev);
>  
>  #endif /* __PANFROST_DEVFREQ_H__ */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 4c2b3c84baac..233957f88d77 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -51,13 +51,6 @@ struct panfrost_features {
>  	unsigned long hw_issues[64 / BITS_PER_LONG];
>  };
>  
> -struct panfrost_devfreq_slot {
> -	ktime_t busy_time;
> -	ktime_t idle_time;
> -	ktime_t time_last_update;
> -	bool busy;
> -};
> -
>  struct panfrost_device {
>  	struct device *dev;
>  	struct drm_device *ddev;
> @@ -95,7 +88,10 @@ struct panfrost_device {
>  	struct {
>  		struct devfreq *devfreq;
>  		struct thermal_cooling_device *cooling;
> -		struct panfrost_devfreq_slot slot[NUM_JOB_SLOTS];
> +		ktime_t busy_time;
> +		ktime_t idle_time;
> +		ktime_t time_last_update;
> +		atomic_t busy_count;
>  	} devfreq;
>  };
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 05c85f45a0de..47aeadb4f161 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -155,7 +155,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  
>  	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
>  
> -	panfrost_devfreq_record_transition(pfdev, js);
> +	panfrost_devfreq_record_busy(pfdev);
>  	spin_lock_irqsave(&pfdev->hwaccess_lock, flags);
>  
>  	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
> @@ -396,7 +396,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
>  
>  	/* panfrost_core_dump(pfdev); */
>  
> -	panfrost_devfreq_record_transition(pfdev, js);
> +	panfrost_devfreq_record_idle(pfdev);
>  	panfrost_device_reset(pfdev);
>  
>  	for (i = 0; i < NUM_JOB_SLOTS; i++)
> @@ -454,7 +454,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>  
>  			pfdev->jobs[j] = NULL;
>  			panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
> -			panfrost_devfreq_record_transition(pfdev, j);
> +			panfrost_devfreq_record_idle(pfdev);
>  			dma_fence_signal(job->done_fence);
>  		}
>  
> @@ -551,14 +551,14 @@ int panfrost_job_is_idle(struct panfrost_device *pfdev)
>  	struct panfrost_job_slot *js = pfdev->js;
>  	int i;
>  
> +	/* Check whether the hardware is idle */
> +	if (atomic_read(&pfdev->devfreq.busy_count))
> +		return false;
> +
>  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>  		/* If there are any jobs in the HW queue, we're not idle */
>  		if (atomic_read(&js->queue[i].sched.hw_rq_count))
>  			return false;
> -
> -		/* Check whether the hardware is idle */
> -		if (pfdev->devfreq.slot[i].busy)
> -			return false;
>  	}
>  
>  	return true;
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/panfrost: Simplify devfreq utilisation tracking

[Rob Herring]

On Fri, Sep 13, 2019 at 12:43 PM Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
>
> Patch 1 is:
>
>         Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>
> Patch 2 is:
>
>         Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

In the future, please reply to each patch with your tag. The reason
being is patchwork will automatically add them instead of me doing it
manually. For a tag on a whole series, replying to patch #0 is fine.
Patchwork doesn't handle that, but I view that as a patchwork
limitation.

Rob

Re: [PATCH 2/2] drm/panfrost: Simplify devfreq utilisation tracking

[Alyssa Rosenzweig]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

Alright; I'm not familiar with patchwork but sounds good.

On Mon, Sep 16, 2019 at 05:36:24PM -0500, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 12:43 PM Alyssa Rosenzweig
> <alyssa.rosenzweig@collabora.com> wrote:
> >
> > Patch 1 is:
> >
> >         Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> >
> > Patch 2 is:
> >
> >         Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> In the future, please reply to each patch with your tag. The reason
> being is patchwork will automatically add them instead of me doing it
> manually. For a tag on a whole series, replying to patch #0 is fine.
> Patchwork doesn't handle that, but I view that as a patchwork
> limitation.
> 
> Rob

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] drm/panfrost: Tidy up the devfreq implementation

[Tomeu Vizoso]

On 9/12/19 12:28 PM, Steven Price wrote:
> The devfreq implementation in panfrost is unnecessarily open coded. It
> also tracks utilisation metrics per slot which isn't very useful. Let's
> tidy it up!
> 
> This should be picked up along with Mark's change[1] to fix
> regulator_get_optional() misuse. This also deletes the code changes from
> 52282163dfa6 and e21dd290881b which would otherwise need reverting, see
> the previous discussion[2].

Both patches look great.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks!

Tomeu

> [1] https://lore.kernel.org/lkml/20190904123032.23263-1-broonie@kernel.org/
> [2] https://lore.kernel.org/lkml/ccd81530-2dbd-3c02-ca0a-1085b00663b5@arm.com/
> 
> Steven Price (2):
>    drm/panfrost: Use generic code for devfreq
>    drm/panfrost: Simplify devfreq utilisation tracking
> 
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 126 ++++++--------------
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h |   3 +-
>   drivers/gpu/drm/panfrost/panfrost_device.h  |  14 +--
>   drivers/gpu/drm/panfrost/panfrost_job.c     |  14 +--
>   4 files changed, 48 insertions(+), 109 deletions(-)
>