On 10/05/2020 17:55, Clément Péron wrote:

This declaration can be avoided so change it.

Signed-off-by: Clément Péron peron.clem@gmail.com

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 38 ++++++++++----------- 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 413987038fbf..1b560b903ea6 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -14,7 +14,24 @@ #include "panfrost_gpu.h" #include "panfrost_regs.h"

-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev); +static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev) +{

• ktime_t now;
• ktime_t last;
• if (!pfdev->devfreq.devfreq)

• return;
  

• now = ktime_get();
• last = pfdev->devfreq.time_last_update;
• if (atomic_read(&pfdev->devfreq.busy_count) > 0)

• pfdev->devfreq.busy_time += ktime_sub(now, last);
  

• else

• pfdev->devfreq.idle_time += ktime_sub(now, last);
  

• pfdev->devfreq.time_last_update = now;

+}

static int panfrost_devfreq_target(struct device *dev, unsigned long *freq, u32 flags) @@ -139,25 +156,6 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev) devfreq_suspend_device(pfdev->devfreq.devfreq); }

-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev) -{

• ktime_t now;
• ktime_t last;
• if (!pfdev->devfreq.devfreq)

• return;
  

• now = ktime_get();
• last = pfdev->devfreq.time_last_update;
• if (atomic_read(&pfdev->devfreq.busy_count) > 0)

• pfdev->devfreq.busy_time += ktime_sub(now, last);
  

• else

• pfdev->devfreq.idle_time += ktime_sub(now, last);
  

• pfdev->devfreq.time_last_update = now;

-}

• void panfrost_devfreq_record_busy(struct panfrost_device *pfdev) { panfrost_devfreq_update_utilization(pfdev);

On 10/05/2020 17:55, Clément Péron wrote:

Don't include not required headers and sort them.

Signed-off-by: Clément Péron peron.clem@gmail.com

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 1b560b903ea6..df7b71da9a84 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -1,18 +1,14 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright 2019 Collabora ltd. */

+#include <linux/clk.h> #include <linux/devfreq.h> #include <linux/devfreq_cooling.h> #include <linux/platform_device.h> #include <linux/pm_opp.h> -#include <linux/clk.h> -#include <linux/regulator/consumer.h>

#include "panfrost_device.h" #include "panfrost_devfreq.h" -#include "panfrost_features.h" -#include "panfrost_issues.h" -#include "panfrost_gpu.h" -#include "panfrost_regs.h"

static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev) {

On 10/05/2020 17:55, Clément Péron wrote:

This use devfreq variable that will be lock with spinlock in future patches. We should either introduce a function to access this one but as devfreq is optional let's just remove it.

Signed-off-by: Clément Péron peron.clem@gmail.com

As far as I can tell this should be safe. As you note this wouldn't work without devfreq anyway.

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_job.c | 4 ---- 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 7914b1570841..63e32a9f2749 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -581,10 +581,6 @@ 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))

On 10/05/2020 17:55, Clément Péron wrote:

Introduce a proper panfrost_devfreq to deal with devfreq variables.

Signed-off-by: Clément Péron peron.clem@gmail.com

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 76 ++++++++++++--------- drivers/gpu/drm/panfrost/panfrost_devfreq.h | 20 +++++- drivers/gpu/drm/panfrost/panfrost_device.h | 11 +-- drivers/gpu/drm/panfrost/panfrost_job.c | 6 +- 4 files changed, 66 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index df7b71da9a84..962550363391 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -10,23 +10,23 @@ #include "panfrost_device.h" #include "panfrost_devfreq.h"

-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev) +static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq) { ktime_t now; ktime_t last;

• if (!pfdev->devfreq.devfreq)

• if (!pfdevfreq->devfreq) return;

  now = ktime_get();

• last = pfdev->devfreq.time_last_update;

• last = pfdevfreq->time_last_update;

• if (atomic_read(&pfdev->devfreq.busy_count) > 0)

• pfdev->devfreq.busy_time += ktime_sub(now, last);
  

• if (atomic_read(&pfdevfreq->busy_count) > 0)

• pfdevfreq->busy_time += ktime_sub(now, last);
  

  else

• pfdev->devfreq.idle_time += ktime_sub(now, last);
  

• pfdevfreq->idle_time += ktime_sub(now, last);
  
  

• pfdev->devfreq.time_last_update = now;

• pfdevfreq->time_last_update = now; }

  static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,

@@ -47,30 +47,31 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq, return 0; }

-static void panfrost_devfreq_reset(struct panfrost_device *pfdev) +static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq) {

• pfdev->devfreq.busy_time = 0;
• pfdev->devfreq.idle_time = 0;
• pfdev->devfreq.time_last_update = ktime_get();

• pfdevfreq->busy_time = 0;

• pfdevfreq->idle_time = 0;

• pfdevfreq->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);

• struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

• panfrost_devfreq_update_utilization(pfdev);

• panfrost_devfreq_update_utilization(pfdevfreq);

  status->current_frequency = clk_get_rate(pfdev->clock);

• status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.busy_time,

• 				   pfdev->devfreq.idle_time));
  

• status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,

• 				   pfdevfreq->idle_time));
  
  

• status->busy_time = ktime_to_ns(pfdev->devfreq.busy_time);

• status->busy_time = ktime_to_ns(pfdevfreq->busy_time);

• panfrost_devfreq_reset(pfdev);

• panfrost_devfreq_reset(pfdevfreq);

• dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n", status->busy_time,

• status->total_time,
  

• dev_dbg(pfdev->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);

@@ -91,6 +92,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) struct device *dev = &pfdev->pdev->dev; struct devfreq *devfreq; struct thermal_cooling_device *cooling;

• struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

  ret = dev_pm_opp_of_add_table(dev); if (ret == -ENODEV) /* Optional, continue without devfreq */

@@ -98,7 +100,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) else if (ret) return ret;

• panfrost_devfreq_reset(pfdev);

• panfrost_devfreq_reset(pfdevfreq);

  cur_freq = clk_get_rate(pfdev->clock);

@@ -116,53 +118,59 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) dev_pm_opp_of_remove_table(dev); return PTR_ERR(devfreq); }

• pfdev->devfreq.devfreq = devfreq;

• pfdevfreq->devfreq = devfreq;

  cooling = of_devfreq_cooling_register(dev->of_node, devfreq); if (IS_ERR(cooling)) DRM_DEV_INFO(dev, "Failed to register cooling device\n"); else

• pfdev->devfreq.cooling = cooling;
  

• pfdevfreq->cooling = cooling;
  
  

  return 0; }

  void panfrost_devfreq_fini(struct panfrost_device *pfdev) {

• if (pfdev->devfreq.cooling)

• devfreq_cooling_unregister(pfdev->devfreq.cooling);
  

• struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

• if (pfdevfreq->cooling)

• devfreq_cooling_unregister(pfdevfreq->cooling);
  

  dev_pm_opp_of_remove_table(&pfdev->pdev->dev); }

  void panfrost_devfreq_resume(struct panfrost_device *pfdev) {

• if (!pfdev->devfreq.devfreq)

• struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
• if (!pfdevfreq->devfreq) return;

• panfrost_devfreq_reset(pfdev);

• panfrost_devfreq_reset(pfdevfreq);

• devfreq_resume_device(pfdev->devfreq.devfreq);

• devfreq_resume_device(pfdevfreq->devfreq); }

  void panfrost_devfreq_suspend(struct panfrost_device *pfdev) {

• if (!pfdev->devfreq.devfreq)

• struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
• if (!pfdevfreq->devfreq) return;

• devfreq_suspend_device(pfdev->devfreq.devfreq);

• devfreq_suspend_device(pfdevfreq->devfreq); }

-void panfrost_devfreq_record_busy(struct panfrost_device *pfdev) +void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq) {

• panfrost_devfreq_update_utilization(pfdev);
• atomic_inc(&pfdev->devfreq.busy_count);

• panfrost_devfreq_update_utilization(pfdevfreq);
• atomic_inc(&pfdevfreq->busy_count); }

-void panfrost_devfreq_record_idle(struct panfrost_device *pfdev) +void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq) { int count;

• panfrost_devfreq_update_utilization(pfdev);
• count = atomic_dec_if_positive(&pfdev->devfreq.busy_count);

• panfrost_devfreq_update_utilization(pfdevfreq);
• count = atomic_dec_if_positive(&pfdevfreq->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 0611beffc8d0..0697f8d5aa34 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -4,13 +4,29 @@ #ifndef __PANFROST_DEVFREQ_H__ #define __PANFROST_DEVFREQ_H__

+#include <linux/ktime.h>

+struct devfreq; +struct thermal_cooling_device;

+struct panfrost_device;

+struct panfrost_devfreq {

• struct devfreq *devfreq;
• struct thermal_cooling_device *cooling;
• ktime_t busy_time;
• ktime_t idle_time;
• ktime_t time_last_update;
• atomic_t busy_count;

+};

• int panfrost_devfreq_init(struct panfrost_device *pfdev); 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_busy(struct panfrost_device *pfdev); -void panfrost_devfreq_record_idle(struct panfrost_device *pfdev); +void panfrost_devfreq_record_busy(struct panfrost_devfreq *devfreq); +void panfrost_devfreq_record_idle(struct panfrost_devfreq *devfreq);

#endif /* __PANFROST_DEVFREQ_H__ */ diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index c30c719a8059..2efa59c9d1c5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -13,6 +13,8 @@ #include <drm/drm_mm.h> #include <drm/gpu_scheduler.h>

+#include "panfrost_devfreq.h"

• struct panfrost_device; struct panfrost_mmu; struct panfrost_job_slot;

@@ -107,14 +109,7 @@ struct panfrost_device { struct list_head shrinker_list; struct shrinker shrinker;

• struct {

• struct devfreq *devfreq;
  

• struct thermal_cooling_device *cooling;
  

• ktime_t busy_time;
  

• ktime_t idle_time;
  

• ktime_t time_last_update;
  

• atomic_t busy_count;
  

• } devfreq;

• struct panfrost_devfreq pfdevfreq; };

  struct panfrost_mmu {

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 63e32a9f2749..a67f3eac6a58 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_busy(pfdev);

• panfrost_devfreq_record_busy(&pfdev->pfdevfreq);

  job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF); job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);

@@ -415,7 +415,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) } spin_unlock_irqrestore(&pfdev->js->job_lock, flags);

• panfrost_devfreq_record_idle(pfdev);

• panfrost_devfreq_record_idle(&pfdev->pfdevfreq); panfrost_device_reset(pfdev);

  for (i = 0; i < NUM_JOB_SLOTS; i++)

@@ -478,7 +478,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_idle(pfdev);
  

• 		panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
  
  	dma_fence_signal_locked(job->done_fence);
  	pm_runtime_put_autosuspend(pfdev->dev);
  
  

On 10/05/2020 17:55, Clément Péron wrote:

Convert busy_count to a simple int protected by spinlock.

Signed-off-by: Clément Péron peron.clem@gmail.com

Looks like a fairly mechanical cleanup.

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 43 +++++++++++++++------ drivers/gpu/drm/panfrost/panfrost_devfreq.h | 10 ++++- 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 962550363391..78753cfb59fb 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -12,16 +12,12 @@

static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq) {

• ktime_t now;
• ktime_t last;
• if (!pfdevfreq->devfreq)

• return;
  

• ktime_t now, last;

  now = ktime_get(); last = pfdevfreq->time_last_update;

• if (atomic_read(&pfdevfreq->busy_count) > 0)

• if (pfdevfreq->busy_count > 0) pfdevfreq->busy_time += ktime_sub(now, last); else pfdevfreq->idle_time += ktime_sub(now, last);

@@ -59,10 +55,14 @@ static int panfrost_devfreq_get_dev_status(struct device *dev, { struct panfrost_device *pfdev = dev_get_drvdata(dev); struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

• unsigned long irqflags;

• status->current_frequency = clk_get_rate(pfdev->clock);

• spin_lock_irqsave(&pfdevfreq->lock, irqflags);

  panfrost_devfreq_update_utilization(pfdevfreq);

• status->current_frequency = clk_get_rate(pfdev->clock); status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time, pfdevfreq->idle_time));

@@ -70,6 +70,8 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,

panfrost_devfreq_reset(pfdevfreq);

• spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
• dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n", status->busy_time, status->total_time, status->busy_time / (status->total_time / 100),

@@ -100,6 +102,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) else if (ret) return ret;

• spin_lock_init(&pfdevfreq->lock);

• panfrost_devfreq_reset(pfdevfreq);

  cur_freq = clk_get_rate(pfdev->clock);

@@ -162,15 +166,32 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)

void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq) {

• unsigned long irqflags;
• if (!pfdevfreq->devfreq)

• return;
  

• spin_lock_irqsave(&pfdevfreq->lock, irqflags);
• panfrost_devfreq_update_utilization(pfdevfreq);

• atomic_inc(&pfdevfreq->busy_count);

• pfdevfreq->busy_count++;

• spin_unlock_irqrestore(&pfdevfreq->lock, irqflags); }

  void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq) {

• int count;

• unsigned long irqflags;

• if (!pfdevfreq->devfreq)

• return;
  

• spin_lock_irqsave(&pfdevfreq->lock, irqflags);

  panfrost_devfreq_update_utilization(pfdevfreq);

• count = atomic_dec_if_positive(&pfdevfreq->busy_count);
• WARN_ON(count < 0);

• WARN_ON(--pfdevfreq->busy_count < 0);
• spin_unlock_irqrestore(&pfdevfreq->lock, irqflags); }

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h index 0697f8d5aa34..e6629900a618 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -4,6 +4,7 @@ #ifndef __PANFROST_DEVFREQ_H__ #define __PANFROST_DEVFREQ_H__

+#include <linux/spinlock.h> #include <linux/ktime.h>

struct devfreq; @@ -14,10 +15,17 @@ struct panfrost_device; struct panfrost_devfreq { struct devfreq *devfreq; struct thermal_cooling_device *cooling;

• ktime_t busy_time; ktime_t idle_time; ktime_t time_last_update;

• atomic_t busy_count;

• int busy_count;

• /*

• * Protect busy_time, idle_time, time_last_update and busy_count
  

• * because these can be updated concurrently, for example by the GP
  

• * and PP interrupts.
  

• */
  

• spinlock_t lock; };

  int panfrost_devfreq_init(struct panfrost_device *pfdev);

Hi Robin,

On Fri, 29 May 2020 at 14:20, Robin Murphy robin.murphy@arm.com wrote:

On 2020-05-10 17:55, Clément Péron wrote:

...

Convert busy_count to a simple int protected by spinlock.

A little more reasoning might be nice.

I have follow the modification requested for lima devfreq and clearly don't have any argument to switch to spinlock.

The Lima Maintainer asked to change witht the following reason : "Better make this count a normal int which is also protected by the spinlock, because current implementation can't protect atomic ops for state change and busy idle check and we are using spinlock already"

...

Signed-off-by: Clément Péron peron.clem@gmail.com

[...]

...

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h index 0697f8d5aa34..e6629900a618 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -4,6 +4,7 @@ #ifndef __PANFROST_DEVFREQ_H__ #define __PANFROST_DEVFREQ_H__

+#include <linux/spinlock.h> #include <linux/ktime.h>

struct devfreq; @@ -14,10 +15,17 @@ struct panfrost_device; struct panfrost_devfreq { struct devfreq *devfreq; struct thermal_cooling_device *cooling;

• ktime_t busy_time;
  ktime_t idle_time;
  ktime_t time_last_update;
  

• atomic_t busy_count;
  

• int busy_count;
  

• /*
  

•  * Protect busy_time, idle_time, time_last_update and busy_count
  

•  * because these can be updated concurrently, for example by the GP
  

•  * and PP interrupts.
  

•  */
  

Nit: this comment is clearly wrong, since we only have Job, GPU and MMU interrupts here. I guess if there is a race it would be between submission/completion/timeout on different job slots.

It's copy/paste from lima I will update it,

Given that, should this actually be considered a fix for 9e62b885f715 ("drm/panfrost: Simplify devfreq utilisation tracking")?

I can't say if it can be considered as a fix, I didn't see any improvement on my board before and after this patch. I'm still facing some issue and didn't have time to fully investigate it.

Thanks for you review,

Robin.

Hi Steven,

On Thu, 28 May 2020 at 15:22, Steven Price steven.price@arm.com wrote:

On 10/05/2020 17:55, Clément Péron wrote:

...

Instead of expecting an error from dev_pm_opp_of_add_table() do a simple device_property_present() check.

Signed-off-by: Clément Péron peron.clem@gmail.com

I'm not sure I understand why this is better. We seem to have more code to do roughly the same thing just with the hard-coded "operating-points-v2" name (if there's ever a 'v3' we'll then have to update this).

Is the desire just to get an error on probe if the table is malformed? Have you hit this situation? If so this sounds like something which would be better fixed in the generic OPP code rather than Panfrost itself.

The idea was to avoid calling devfreq if there is no opp table. But I think you're right we don't have to check for malformed device-tree in the driver.

I will drop this patch,

Regards, Clement

Steve

...

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index d9007f44b772..fce21c682414 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) struct thermal_cooling_device *cooling; struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

• ret = dev_pm_opp_of_add_table(dev);
  

• if (ret == -ENODEV) /* Optional, continue without devfreq */
  

• if (!device_property_present(dev, "operating-points-v2"))
  

•         /* Optional, continue without devfreq */
          return 0;
  

• else if (ret)
  

•         return ret;
  

• pfdevfreq->opp_of_table_added = true;
  
  spin_lock_init(&pfdevfreq->lock);
  
  

• ret = dev_pm_opp_of_add_table(dev);
  

• if (ret) {
  

•         DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
  

•         goto err_fini;
  

• }
  

• pfdevfreq->opp_of_table_added = true;
  

• panfrost_devfreq_reset(pfdevfreq);
  
  cur_freq = clk_get_rate(pfdev->clock);
  
  

On 10/05/2020 17:55, Clément Péron wrote:

Later we will introduce devfreq probing regulator if they are present. As regulator should be probe only one time we need to get this logic in the device_init().

panfrost_device is already taking care of devfreq_resume() and devfreq_suspend(), so it's not totally illogic to move the devfreq_init() and devfreq_fini() here.

Signed-off-by: Clément Péron peron.clem@gmail.com

drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++-------- drivers/gpu/drm/panfrost/panfrost_drv.c | 15 ++------- 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 8136babd3ba9..f480127205d6 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev) return err; }

• err = panfrost_devfreq_init(pfdev);
• if (err) {

• dev_err(pfdev->dev, "devfreq init failed %d\n", err);
  

• goto err_out0;
  

• }
• err = panfrost_regulator_init(pfdev); if (err) { dev_err(pfdev->dev, "regulator init failed %d\n", err);

• goto err_out0;
  

• goto err_out1;
  

NIT: Rather than just renumbering these can we give them sensible names so we don't have this sort of refactoring in future?

}

err = panfrost_reset_init(pfdev); if (err) { dev_err(pfdev->dev, "reset init failed %d\n", err);

• goto err_out1;
  

• goto err_out2;
  

  }

  err = panfrost_pm_domain_init(pfdev); if (err)

• goto err_out2;
  

• goto err_out3;
  
  

  res = platform_get_resource(pfdev->pdev, IORESOURCE_MEM, 0); pfdev->iomem = devm_ioremap_resource(pfdev->dev, res); if (IS_ERR(pfdev->iomem)) { dev_err(pfdev->dev, "failed to ioremap iomem\n"); err = PTR_ERR(pfdev->iomem);

• goto err_out3;
  

• goto err_out4;
  

  }

  err = panfrost_gpu_init(pfdev); if (err)

• goto err_out3;
  

• goto err_out4;
  
  

  err = panfrost_mmu_init(pfdev); if (err)

• goto err_out4;
  

• goto err_out5;
  
  

  err = panfrost_job_init(pfdev); if (err)

• goto err_out5;
  

• goto err_out6;
  
  

  err = panfrost_perfcnt_init(pfdev); if (err)

• goto err_out6;
  

• goto err_out7;
  
  

  return 0;

-err_out6: +err_out7: panfrost_job_fini(pfdev); -err_out5: +err_out6: panfrost_mmu_fini(pfdev); -err_out4: +err_out5: panfrost_gpu_fini(pfdev); -err_out3: +err_out4: panfrost_pm_domain_fini(pfdev); -err_out2: +err_out3: panfrost_reset_fini(pfdev); -err_out1: +err_out2: panfrost_regulator_fini(pfdev); +err_out1:

• panfrost_devfreq_fini(pfdev); err_out0: panfrost_clk_fini(pfdev); return err;

@@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev) panfrost_gpu_fini(pfdev); panfrost_pm_domain_fini(pfdev); panfrost_reset_fini(pfdev);

• panfrost_devfreq_fini(pfdev); panfrost_regulator_fini(pfdev); panfrost_clk_fini(pfdev); }

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 882fecc33fdb..4dda68689015 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -14,7 +14,6 @@ #include <drm/drm_utils.h>

#include "panfrost_device.h" -#include "panfrost_devfreq.h" #include "panfrost_gem.h" #include "panfrost_mmu.h" #include "panfrost_job.h" @@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev) goto err_out0; }

• err = panfrost_devfreq_init(pfdev);
• if (err) {

• if (err != -EPROBE_DEFER)
  

• 	dev_err(&pdev->dev, "Fatal error during devfreq init\n");
  

You seem to have lost the check for EPROBE_DEFER during the move.

• goto err_out1;
  

• }
• pm_runtime_set_active(pfdev->dev); pm_runtime_mark_last_busy(pfdev->dev); pm_runtime_enable(pfdev->dev);

@@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev) */ err = drm_dev_register(ddev, 0); if (err < 0)

• goto err_out2;
  

• goto err_out1;
  
  

  panfrost_gem_shrinker_init(ddev);

  return 0;

-err_out2:

• pm_runtime_disable(pfdev->dev);
• panfrost_devfreq_fini(pfdev); err_out1:

• pm_runtime_disable(pfdev->dev); panfrost_device_fini(pfdev); err_out0: drm_dev_put(ddev);

@@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev) panfrost_gem_shrinker_cleanup(ddev);

pm_runtime_get_sync(pfdev->dev);

• panfrost_devfreq_fini(pfdev); panfrost_device_fini(pfdev); pm_runtime_put_sync_suspend(pfdev->dev); pm_runtime_disable(pfdev->dev);

On 5/29/20 2:38 PM, Clément Péron wrote:

Hi Steven

On Thu, 28 May 2020 at 15:22, Steven Price steven.price@arm.com wrote:

...

On 10/05/2020 17:55, Clément Péron wrote:

...

Later we will introduce devfreq probing regulator if they are present. As regulator should be probe only one time we need to get this logic in the device_init().

panfrost_device is already taking care of devfreq_resume() and devfreq_suspend(), so it's not totally illogic to move the devfreq_init() and devfreq_fini() here.

Signed-off-by: Clément Péron peron.clem@gmail.com

drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++-------- drivers/gpu/drm/panfrost/panfrost_drv.c | 15 ++------- 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 8136babd3ba9..f480127205d6 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev) return err; }

• err = panfrost_devfreq_init(pfdev);
  

• if (err) {
  

•         dev_err(pfdev->dev, "devfreq init failed %d\n", err);
  

•         goto err_out0;
  

• }
  

•  err = panfrost_regulator_init(pfdev);
   if (err) {
           dev_err(pfdev->dev, "regulator init failed %d\n", err);
  

•         goto err_out0;
  

•         goto err_out1;
  

NIT: Rather than just renumbering these can we give them sensible names so we don't have this sort of refactoring in future?

Agree, I will change that in v2

FWIW, there's a sensible approach described in Documentation/process/coding-style.rst ("7) Centralized exiting of functions").

Regards,

Tomeu

...

...

   }

   err = panfrost_reset_init(pfdev);
   if (err) {
           dev_err(pfdev->dev, "reset init failed %d\n", err);

•         goto err_out1;
  

•         goto err_out2;
   }
  
   err = panfrost_pm_domain_init(pfdev);
   if (err)
  

•         goto err_out2;
  

•         goto err_out3;
  
   res = platform_get_resource(pfdev->pdev, IORESOURCE_MEM, 0);
   pfdev->iomem = devm_ioremap_resource(pfdev->dev, res);
   if (IS_ERR(pfdev->iomem)) {
           dev_err(pfdev->dev, "failed to ioremap iomem\n");
           err = PTR_ERR(pfdev->iomem);
  

•         goto err_out3;
  

•         goto err_out4;
   }
  
   err = panfrost_gpu_init(pfdev);
   if (err)
  

•         goto err_out3;
  

•         goto err_out4;
  
   err = panfrost_mmu_init(pfdev);
   if (err)
  

•         goto err_out4;
  

•         goto err_out5;
  
   err = panfrost_job_init(pfdev);
   if (err)
  

•         goto err_out5;
  

•         goto err_out6;
  
   err = panfrost_perfcnt_init(pfdev);
   if (err)
  

•         goto err_out6;
  

•         goto err_out7;
  
   return 0;
  

-err_out6: +err_out7: panfrost_job_fini(pfdev); -err_out5: +err_out6: panfrost_mmu_fini(pfdev); -err_out4: +err_out5: panfrost_gpu_fini(pfdev); -err_out3: +err_out4: panfrost_pm_domain_fini(pfdev); -err_out2: +err_out3: panfrost_reset_fini(pfdev); -err_out1: +err_out2: panfrost_regulator_fini(pfdev); +err_out1:

• panfrost_devfreq_fini(pfdev);
  

  err_out0: panfrost_clk_fini(pfdev); return err;

@@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev) panfrost_gpu_fini(pfdev); panfrost_pm_domain_fini(pfdev); panfrost_reset_fini(pfdev);

• panfrost_devfreq_fini(pfdev);
   panfrost_regulator_fini(pfdev);
   panfrost_clk_fini(pfdev);
  

  }

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 882fecc33fdb..4dda68689015 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -14,7 +14,6 @@ #include <drm/drm_utils.h>

#include "panfrost_device.h" -#include "panfrost_devfreq.h" #include "panfrost_gem.h" #include "panfrost_mmu.h" #include "panfrost_job.h" @@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev) goto err_out0; }

• err = panfrost_devfreq_init(pfdev);
  

• if (err) {
  

•         if (err != -EPROBE_DEFER)
  

•                 dev_err(&pdev->dev, "Fatal error during devfreq init\n");
  

You seem to have lost the check for EPROBE_DEFER during the move.

Correct, sorry for that, I will fix it in v2.

Thanks for your review, Clement

...

...

•         goto err_out1;
  

• }
  

•  pm_runtime_set_active(pfdev->dev);
   pm_runtime_mark_last_busy(pfdev->dev);
   pm_runtime_enable(pfdev->dev);
  

@@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev) */ err = drm_dev_register(ddev, 0); if (err < 0)

•         goto err_out2;
  

•         goto err_out1;
  
   panfrost_gem_shrinker_init(ddev);
  
   return 0;
  
  

-err_out2:

• pm_runtime_disable(pfdev->dev);
  

• panfrost_devfreq_fini(pfdev);
  

  err_out1:

• pm_runtime_disable(pfdev->dev);
   panfrost_device_fini(pfdev);
  

  err_out0: drm_dev_put(ddev);

@@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev) panfrost_gem_shrinker_cleanup(ddev);

   pm_runtime_get_sync(pfdev->dev);

• panfrost_devfreq_fini(pfdev);
   panfrost_device_fini(pfdev);
   pm_runtime_put_sync_suspend(pfdev->dev);
   pm_runtime_disable(pfdev->dev);
  
  

On 10/05/2020 17:55, Clément Péron wrote:

We will later introduce regulators managed by OPP.

Only alloc regulators when it's needed. This also help use to release the regulators only when they are allocated.

Signed-off-by: Clément Péron peron.clem@gmail.com

LGTM:

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_device.c | 14 +++++++++----- drivers/gpu/drm/panfrost/panfrost_device.h | 3 +-- 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index f480127205d6..67eedf64e82d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -90,9 +90,11 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev) { int ret, i;

• if (WARN(pfdev->comp->num_supplies > ARRAY_SIZE(pfdev->regulators),

• 	"Too many supplies in compatible structure.\n"))
  

• return -EINVAL;
  

• pfdev->regulators = devm_kcalloc(pfdev->dev, pfdev->comp->num_supplies,

• 			 sizeof(*pfdev->regulators),
  

• 			 GFP_KERNEL);
  

• if (!pfdev->regulators)

• return -ENOMEM;
  
  

  for (i = 0; i < pfdev->comp->num_supplies; i++) pfdev->regulators[i].supply = pfdev->comp->supply_names[i];

@@ -117,8 +119,10 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)

static void panfrost_regulator_fini(struct panfrost_device *pfdev) {

• regulator_bulk_disable(pfdev->comp->num_supplies,

• 	pfdev->regulators);
  

• if (!pfdev->regulators)

• return;
  

• regulator_bulk_disable(pfdev->comp->num_supplies, pfdev->regulators); }

  static void panfrost_pm_domain_fini(struct panfrost_device *pfdev)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index 2efa59c9d1c5..953f7536a773 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -22,7 +22,6 @@ struct panfrost_job; struct panfrost_perfcnt;

#define NUM_JOB_SLOTS 3 -#define MAX_REGULATORS 2 #define MAX_PM_DOMAINS 3

struct panfrost_features { @@ -81,7 +80,7 @@ struct panfrost_device { void __iomem *iomem; struct clk *clock; struct clk *bus_clock;

• struct regulator_bulk_data regulators[MAX_REGULATORS];

• struct regulator_bulk_data *regulators; struct reset_control *rstc; /* pm_domains for devices with more than one. */ struct device *pm_domain_devs[MAX_PM_DOMAINS];

On 10/05/2020 17:55, Clément Péron wrote:

Some OPP tables specify voltage for each frequency. Devfreq can handle these regulators but they should be get only 1 time to avoid issue and know who is in charge.

If OPP table is probe don't init regulator.

Signed-off-by: Clément Péron peron.clem@gmail.com

This looks like it should work - thanks for doing this!

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 19 +++++++++++++++++++ drivers/gpu/drm/panfrost/panfrost_devfreq.h | 2 ++ drivers/gpu/drm/panfrost/panfrost_device.c | 11 +++++++---- 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index fce21c682414..9ffea0d4a087 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -93,6 +93,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) unsigned long cur_freq; struct device *dev = &pfdev->pdev->dev; struct devfreq *devfreq;

• struct opp_table *opp_table; struct thermal_cooling_device *cooling; struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;

@@ -102,6 +103,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)

spin_lock_init(&pfdevfreq->lock);

• opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,

• 			      pfdev->comp->num_supplies);
  

• if (IS_ERR(opp_table)) {

• ret = PTR_ERR(opp_table);
  

• /* Continue if the optional regulator is missing */
  

• if (ret != -ENODEV) {
  

• 	DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
  

• 	goto err_fini;
  

• }
  

• } else {

• pfdevfreq->regulators_opp_table = opp_table;
  

• }
• ret = dev_pm_opp_of_add_table(dev); if (ret) { DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");

@@ -157,6 +171,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev) dev_pm_opp_of_remove_table(&pfdev->pdev->dev); pfdevfreq->opp_of_table_added = false; }

• if (pfdevfreq->regulators_opp_table) {

• dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
  

• pfdevfreq->regulators_opp_table = NULL;
  

• } }

  void panfrost_devfreq_resume(struct panfrost_device *pfdev)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h index add203cb00c2..347cde4786cf 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -8,12 +8,14 @@ #include <linux/ktime.h>

struct devfreq; +struct opp_table; struct thermal_cooling_device;

struct panfrost_device;

struct panfrost_devfreq { struct devfreq *devfreq;

• struct opp_table *regulators_opp_table; struct thermal_cooling_device *cooling; bool opp_of_table_added;

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 67eedf64e82d..8b17fb2e3369 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -222,10 +222,13 @@ int panfrost_device_init(struct panfrost_device *pfdev) goto err_out0; }

• err = panfrost_regulator_init(pfdev);
• if (err) {

• dev_err(pfdev->dev, "regulator init failed %d\n", err);
  

• goto err_out1;
  

• /* OPP will handle regulators */

• if (!pfdev->pfdevfreq.opp_of_table_added) {

• err = panfrost_regulator_init(pfdev);
  

• if (err) {
  

• 	dev_err(pfdev->dev, "regulator init failed %d\n", err);
  

• 	goto err_out1;
  

• }
  

  }

  err = panfrost_reset_init(pfdev);

Hi Steven,

On Thu, 28 May 2020 at 15:23, Steven Price steven.price@arm.com wrote:

On 10/05/2020 17:55, Clément Péron wrote:

...

Some SoCs have several clocks defined and the OPP core needs to know the exact name of the clk to use.

Set the clock name to "core".

Signed-off-by: Clément Péron peron.clem@gmail.com

This is unfortunately a regression for the RK3288. The device tree binding doesn't require "clock-names", and for the RK3288 it currently isn't specified. So this breaks the platform.

Adding the "clock-names" to the device tree 'fixes' it, but we really need to keep backwards compatibility.

Yes you're right, thanks for cathing this.

Regards, Clement

Steve

...

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 +++++++++++++ drivers/gpu/drm/panfrost/panfrost_devfreq.h | 1 + 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 9ffea0d4a087..6bf3541b4d53 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -103,6 +103,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)

  spin_lock_init(&pfdevfreq->lock);

• opp_table = dev_pm_opp_set_clkname(dev, "core");
  

• if (IS_ERR(opp_table)) {
  

•         ret = PTR_ERR(opp_table);
  

•         goto err_fini;
  

• }
  

• pfdevfreq->clkname_opp_table = opp_table;
  

• opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
                                        pfdev->comp->num_supplies);
  if (IS_ERR(opp_table)) {
  

@@ -176,6 +184,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev) dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table); pfdevfreq->regulators_opp_table = NULL; }

• if (pfdevfreq->clkname_opp_table) {
  

•         dev_pm_opp_put_clkname(pfdevfreq->clkname_opp_table);
  

•         pfdevfreq->clkname_opp_table = NULL;
  

• }
  

  }

  void panfrost_devfreq_resume(struct panfrost_device *pfdev)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h index 347cde4786cf..1f2475e1d034 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h @@ -16,6 +16,7 @@ struct panfrost_device; struct panfrost_devfreq { struct devfreq *devfreq; struct opp_table *regulators_opp_table;

• struct opp_table *clkname_opp_table;
  struct thermal_cooling_device *cooling;
  bool opp_of_table_added;