Re: [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 7:35 PM Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > With the rework of how the __string() handles dynamic strings where it > saves off the source string in field in the helper structure[1], the > assignment of that value to the trace event field is stored in the helper > value and does not need to be passed in again. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) Acked-by: Rafael J. Wysocki # for thermal
Re: [Freedreno] [PATCH v5 03/11] PM / QoS: Fix constraints alloc vs reclaim locking
+0x10c/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
> The issue is that dev_pm_qos_mtx is held in the runpm suspend/resume (or
> freq change) path, but it is also held across allocations that could
> recurse into shrinker.
>
> Solve this by changing dev_pm_qos_constraints_allocate() into a function
> that can be called unconditionally before the device qos object is
> needed and before aquiring dev_pm_qos_mtx. This way the allocations can
acquiring
> be done without holding the mutex. In the case that we raced with
> another thread to allocate the qos object, detect this *after* acquiring
> the dev_pm_qos_mtx and simply free the redundant allocations.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Rob Clark
Please feel free to add
Acked-by: Rafael J. Wysocki
to this patch and the next 2 PM QoS ones in this series.
Thanks!
> ---
> drivers/base/power/qos.c | 76 +---
> 1 file changed, 56 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 8e93167f1783..7e95760d16dc 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -185,27 +185,33 @@ static int apply_constraint(struct dev_pm_qos_request
> *req,
> }
>
> /*
> - * dev_pm_qos_constraints_allocate
> + * dev_pm_qos_constraints_allocate: Allocate and initializes qos constraints
> * @dev: device to allocate data for
> *
> - * Called at the first call to add_request, for constraint data allocation
> - * Must be called with the dev_pm_qos_mtx mutex held
> + * Called to allocate constraints before dev_pm_qos_mtx mutex is held.
> Should
> + * be matched with a call to dev_pm_qos_constraints_set() once dev_pm_qos_mtx
> + * is held.
> */
> -static int dev_pm_qos_constraints_allocate(struct device *dev)
> +static struct dev_pm_qos *dev_pm_qos_constraints_allocate(struct device *dev)
> {
> struct dev_pm_qos *qos;
> struct pm_qos_constraints *c;
> struct blocking_notifier_head *n;
>
> - qos = kzalloc(sizeof(*qos), GFP_KERNEL);
> + /*
> +* If constraints are already allocated, we can skip speculatively
> +* allocating a new one, as we don't have to work about qos
> transitioning
> +* from non-null to null. The constraints are only freed on device
> +* removal.
> +*/
> + if (dev->power.qos)
> + return NULL;
> +
> + qos = kzalloc(sizeof(*qos) + 3 * sizeof(*n), GFP_KERNEL);
> if (!qos)
> - return -ENOMEM;
> + return NULL;
>
> - n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
> - if (!n) {
> - kfree(qos);
> - return -ENOMEM;
> - }
> + n = (struct blocking_notifier_head *)(qos + 1);
>
> c = >resume_latency;
> plist_head_init(>list);
> @@ -227,11 +233,29 @@ static int dev_pm_qos_constraints_allocate(struct
> device *dev)
>
> INIT_LIST_HEAD(>flags.list);
>
> + return qos;
> +}
> +
> +/*
> + * dev_pm_qos_constraints_set: Ensure dev->power.qos is set
> + *
> + * If dev->power.qos is already set, free the newly allocated qos
> constraints.
> + * Otherwise set dev->power.qos. Must be called with dev_pm_qos_mtx held.
> + *
> + * This split unsynchronized allocation and synchronized set moves allocation
> + * out from under dev_pm_qos_mtx, so that lockdep does does not get angry
> about
> + * drivers which use dev_pm_qos in paths related to shrinker/reclaim.
> + */
> +static void dev_pm_qos_constraints_set(struct device *dev, struct dev_pm_qos
> *qos)
> +{
> + if (dev->power.qos) {
> + kfree(qos);
> + return;
> + }
> +
> spin_lock_irq(>power.lock);
> dev->power.qos = qos;
> spin_unlock_irq(>power.lock);
> -
> - return 0;
> }
>
> static void __dev_pm_qos_hide_latency_limit(struct device *dev);
> @@ -309,7 +333,6 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
> dev->power.qos = ERR_PTR(-ENODEV);
> spin_unlock_irq(>power.lock);
>
> - kfree(qos->resume_latency.notifiers);
> kfree(qos);
>
> out:
> @@ -341,7 +364,7 @@ static int __dev_pm_qos_add_request(struct device *dev,
> if (IS_ERR(dev->power.qos))
> ret = -ENODEV;
> else if (!dev->power.qos)
> - ret = dev_pm_qos_constraints_allocate(dev);
> + ret = -ENOMEM;
>
> trace_dev_pm_qos_add_req
Re: [Freedreno] [RFC] PM / QoS: Decouple request alloc from dev_pm_qos_mtx (alternative solution)
On Fri, Aug 4, 2023 at 11:41 PM Rob Clark wrote:
>
> From: Rob Clark
>
> Similar to the previous patch, move the allocation out from under
> dev_pm_qos_mtx, by speculatively doing the allocation and handle
> any race after acquiring dev_pm_qos_mtx by freeing the redundant
> allocation.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Rob Clark
> ---
> This is an alternative to
> https://patchwork.freedesktop.org/patch/551417/?series=115028=4
>
> So, this does _slightly_ change error paths, for ex
> dev_pm_qos_update_user_latency_tolerance() will now allocate
> dev->power.qos in some error cases. But this seems harmless?
It is harmless AFAICS.
> A slightly more complicated version of this could conserve the
> previous error path behavior, but I figured I'd try the simpler
> thing first.
Good choice!
> drivers/base/power/qos.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 1b73a704aac1..c7ba85e89c42 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -920,8 +920,12 @@ s32 dev_pm_qos_get_user_latency_tolerance(struct device
> *dev)
> int dev_pm_qos_update_user_latency_tolerance(struct device *dev, s32 val)
> {
> struct dev_pm_qos *qos = dev_pm_qos_constraints_allocate();
> + struct dev_pm_qos_request *req = NULL;
> int ret = 0;
>
> + if (!dev->power.qos->latency_tolerance_req)
> + req = kzalloc(sizeof(*req), GFP_KERNEL);
> +
> mutex_lock(_pm_qos_mtx);
>
> dev_pm_qos_constraints_set(dev, qos);
> @@ -935,8 +939,6 @@ int dev_pm_qos_update_user_latency_tolerance(struct
> device *dev, s32 val)
> goto out;
>
> if (!dev->power.qos->latency_tolerance_req) {
> - struct dev_pm_qos_request *req;
> -
> if (val < 0) {
> if (val == PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT)
> ret = 0;
> @@ -944,17 +946,15 @@ int dev_pm_qos_update_user_latency_tolerance(struct
> device *dev, s32 val)
> ret = -EINVAL;
> goto out;
> }
> - req = kzalloc(sizeof(*req), GFP_KERNEL);
> if (!req) {
> ret = -ENOMEM;
> goto out;
> }
> ret = __dev_pm_qos_add_request(dev, req,
> DEV_PM_QOS_LATENCY_TOLERANCE, val);
> - if (ret < 0) {
> - kfree(req);
> + if (ret < 0)
> goto out;
> - }
> dev->power.qos->latency_tolerance_req = req;
> + req = NULL;
> } else {
> if (val < 0) {
> __dev_pm_qos_drop_user_request(dev,
> DEV_PM_QOS_LATENCY_TOLERANCE);
> @@ -966,6 +966,7 @@ int dev_pm_qos_update_user_latency_tolerance(struct
> device *dev, s32 val)
>
> out:
> mutex_unlock(_pm_qos_mtx);
> + kfree(req);
> return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_qos_update_user_latency_tolerance);
> --
Yes, something like this, please!
Re: [Freedreno] [PATCH v3 3/9] PM / QoS: Fix constraints alloc vs reclaim locking
On Fri, Aug 4, 2023 at 10:38 PM Rob Clark wrote:
>
> On Fri, Aug 4, 2023 at 12:11 PM Rafael J. Wysocki wrote:
> >
> > On Fri, Aug 4, 2023 at 8:38 PM Rob Clark wrote:
> > >
> > > On Fri, Aug 4, 2023 at 10:07 AM Rafael J. Wysocki
> > > wrote:
> > > >
> > > > On Fri, Aug 4, 2023 at 12:02 AM Rob Clark wrote:
> > > > >
> > > > > From: Rob Clark
> > > > >
> > > > > In the process of adding lockdep annotation for drm GPU scheduler's
> > > > > job_run() to detect potential deadlock against shrinker/reclaim, I hit
> > > > > this lockdep splat:
> > > > >
> > > > >==
> > > > >WARNING: possible circular locking dependency detected
> > > > >6.2.0-rc8-debug+ #558 Tainted: GW
> > > > >--
> > > > >ring0/125 is trying to acquire lock:
> > > > >ffd6d6ce0f28 (dev_pm_qos_mtx){+.+.}-{3:3}, at:
> > > > > dev_pm_qos_update_request+0x38/0x68
> > > > >
> > > > >but task is already holding lock:
> > > > >ff8087239208 (>active_lock){+.+.}-{3:3}, at:
> > > > > msm_gpu_submit+0xec/0x178
> > > > >
> > > > >which lock already depends on the new lock.
> > > > >
> > > > >the existing dependency chain (in reverse order) is:
> > > > >
> > > > >-> #4 (>active_lock){+.+.}-{3:3}:
> > > > > __mutex_lock+0xcc/0x3c8
> > > > > mutex_lock_nested+0x30/0x44
> > > > > msm_gpu_submit+0xec/0x178
> > > > > msm_job_run+0x78/0x150
> > > > > drm_sched_main+0x290/0x370
> > > > > kthread+0xf0/0x100
> > > > > ret_from_fork+0x10/0x20
> > > > >
> > > > >-> #3 (dma_fence_map){}-{0:0}:
> > > > > __dma_fence_might_wait+0x74/0xc0
> > > > > dma_resv_lockdep+0x1f4/0x2f4
> > > > > do_one_initcall+0x104/0x2bc
> > > > > kernel_init_freeable+0x344/0x34c
> > > > > kernel_init+0x30/0x134
> > > > > ret_from_fork+0x10/0x20
> > > > >
> > > > >-> #2 (mmu_notifier_invalidate_range_start){+.+.}-{0:0}:
> > > > > fs_reclaim_acquire+0x80/0xa8
> > > > > slab_pre_alloc_hook.constprop.0+0x40/0x25c
> > > > > __kmem_cache_alloc_node+0x60/0x1cc
> > > > > __kmalloc+0xd8/0x100
> > > > > topology_parse_cpu_capacity+0x8c/0x178
> > > > > get_cpu_for_node+0x88/0xc4
> > > > > parse_cluster+0x1b0/0x28c
> > > > > parse_cluster+0x8c/0x28c
> > > > > init_cpu_topology+0x168/0x188
> > > > > smp_prepare_cpus+0x24/0xf8
> > > > > kernel_init_freeable+0x18c/0x34c
> > > > > kernel_init+0x30/0x134
> > > > > ret_from_fork+0x10/0x20
> > > > >
> > > > >-> #1 (fs_reclaim){+.+.}-{0:0}:
> > > > > __fs_reclaim_acquire+0x3c/0x48
> > > > > fs_reclaim_acquire+0x54/0xa8
> > > > > slab_pre_alloc_hook.constprop.0+0x40/0x25c
> > > > > __kmem_cache_alloc_node+0x60/0x1cc
> > > > > kmalloc_trace+0x50/0xa8
> > > > > dev_pm_qos_constraints_allocate+0x38/0x100
> > > > > __dev_pm_qos_add_request+0xb0/0x1e8
> > > > > dev_pm_qos_add_request+0x58/0x80
> > > > > dev_pm_qos_expose_latency_limit+0x60/0x13c
> > > > > register_cpu+0x12c/0x130
> > > > > topology_init+0xac/0xbc
> > > > > do_one_initcall+0x104/0x2bc
> > > > > kernel_init_freeable+0x344/0x34c
> > > > > kernel_init+0x30/0x134
> > > > > ret_from_fork+0x10/0x20
> > > > >
> > > > >-> #0 (dev_pm_qos_mtx){+.+.}-{3:3}:
> > > > > __lock_acquire+0xe00/0x1060
> > > > > lock_acquire+0x1e0/0x2f8
> > > > > __mutex_lock+0xcc/0x3c8
> > >
Re: [Freedreno] [PATCH v3 3/9] PM / QoS: Fix constraints alloc vs reclaim locking
On Fri, Aug 4, 2023 at 8:38 PM Rob Clark wrote:
>
> On Fri, Aug 4, 2023 at 10:07 AM Rafael J. Wysocki wrote:
> >
> > On Fri, Aug 4, 2023 at 12:02 AM Rob Clark wrote:
> > >
> > > From: Rob Clark
> > >
> > > In the process of adding lockdep annotation for drm GPU scheduler's
> > > job_run() to detect potential deadlock against shrinker/reclaim, I hit
> > > this lockdep splat:
> > >
> > >==
> > >WARNING: possible circular locking dependency detected
> > >6.2.0-rc8-debug+ #558 Tainted: GW
> > >--
> > >ring0/125 is trying to acquire lock:
> > >ffd6d6ce0f28 (dev_pm_qos_mtx){+.+.}-{3:3}, at:
> > > dev_pm_qos_update_request+0x38/0x68
> > >
> > >but task is already holding lock:
> > >ff8087239208 (>active_lock){+.+.}-{3:3}, at:
> > > msm_gpu_submit+0xec/0x178
> > >
> > >which lock already depends on the new lock.
> > >
> > >the existing dependency chain (in reverse order) is:
> > >
> > >-> #4 (>active_lock){+.+.}-{3:3}:
> > > __mutex_lock+0xcc/0x3c8
> > > mutex_lock_nested+0x30/0x44
> > > msm_gpu_submit+0xec/0x178
> > > msm_job_run+0x78/0x150
> > > drm_sched_main+0x290/0x370
> > > kthread+0xf0/0x100
> > > ret_from_fork+0x10/0x20
> > >
> > >-> #3 (dma_fence_map){}-{0:0}:
> > > __dma_fence_might_wait+0x74/0xc0
> > > dma_resv_lockdep+0x1f4/0x2f4
> > > do_one_initcall+0x104/0x2bc
> > > kernel_init_freeable+0x344/0x34c
> > > kernel_init+0x30/0x134
> > > ret_from_fork+0x10/0x20
> > >
> > >-> #2 (mmu_notifier_invalidate_range_start){+.+.}-{0:0}:
> > > fs_reclaim_acquire+0x80/0xa8
> > > slab_pre_alloc_hook.constprop.0+0x40/0x25c
> > > __kmem_cache_alloc_node+0x60/0x1cc
> > > __kmalloc+0xd8/0x100
> > > topology_parse_cpu_capacity+0x8c/0x178
> > > get_cpu_for_node+0x88/0xc4
> > > parse_cluster+0x1b0/0x28c
> > > parse_cluster+0x8c/0x28c
> > > init_cpu_topology+0x168/0x188
> > > smp_prepare_cpus+0x24/0xf8
> > > kernel_init_freeable+0x18c/0x34c
> > > kernel_init+0x30/0x134
> > > ret_from_fork+0x10/0x20
> > >
> > >-> #1 (fs_reclaim){+.+.}-{0:0}:
> > > __fs_reclaim_acquire+0x3c/0x48
> > > fs_reclaim_acquire+0x54/0xa8
> > > slab_pre_alloc_hook.constprop.0+0x40/0x25c
> > > __kmem_cache_alloc_node+0x60/0x1cc
> > > kmalloc_trace+0x50/0xa8
> > > dev_pm_qos_constraints_allocate+0x38/0x100
> > > __dev_pm_qos_add_request+0xb0/0x1e8
> > > dev_pm_qos_add_request+0x58/0x80
> > > dev_pm_qos_expose_latency_limit+0x60/0x13c
> > > register_cpu+0x12c/0x130
> > > topology_init+0xac/0xbc
> > > do_one_initcall+0x104/0x2bc
> > > kernel_init_freeable+0x344/0x34c
> > > kernel_init+0x30/0x134
> > > ret_from_fork+0x10/0x20
> > >
> > >-> #0 (dev_pm_qos_mtx){+.+.}-{3:3}:
> > > __lock_acquire+0xe00/0x1060
> > > lock_acquire+0x1e0/0x2f8
> > > __mutex_lock+0xcc/0x3c8
> > > mutex_lock_nested+0x30/0x44
> > > dev_pm_qos_update_request+0x38/0x68
> > > msm_devfreq_boost+0x40/0x70
> > > msm_devfreq_active+0xc0/0xf0
> > > msm_gpu_submit+0x10c/0x178
> > > msm_job_run+0x78/0x150
> > > drm_sched_main+0x290/0x370
> > > kthread+0xf0/0x100
> > > ret_from_fork+0x10/0x20
> > >
> > >other info that might help us debug this:
> > >
> > >Chain exists of:
> > > dev_pm_qos_mtx --> dma_fence_map --> >active_lock
> > >
> > > Possible unsafe locking scenario:
> > >
> > > CPU0CPU1
> > >
> > > lock(>active_lock);
> > > lock(dma_fence_ma
Re: [Freedreno] [PATCH v3 3/9] PM / QoS: Fix constraints alloc vs reclaim locking
On Fri, Aug 4, 2023 at 12:02 AM Rob Clark wrote:
>
> From: Rob Clark
>
> In the process of adding lockdep annotation for drm GPU scheduler's
> job_run() to detect potential deadlock against shrinker/reclaim, I hit
> this lockdep splat:
>
>==
>WARNING: possible circular locking dependency detected
>6.2.0-rc8-debug+ #558 Tainted: GW
>--
>ring0/125 is trying to acquire lock:
>ffd6d6ce0f28 (dev_pm_qos_mtx){+.+.}-{3:3}, at:
> dev_pm_qos_update_request+0x38/0x68
>
>but task is already holding lock:
>ff8087239208 (>active_lock){+.+.}-{3:3}, at:
> msm_gpu_submit+0xec/0x178
>
>which lock already depends on the new lock.
>
>the existing dependency chain (in reverse order) is:
>
>-> #4 (>active_lock){+.+.}-{3:3}:
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> msm_gpu_submit+0xec/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
>-> #3 (dma_fence_map){}-{0:0}:
> __dma_fence_might_wait+0x74/0xc0
> dma_resv_lockdep+0x1f4/0x2f4
> do_one_initcall+0x104/0x2bc
> kernel_init_freeable+0x344/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #2 (mmu_notifier_invalidate_range_start){+.+.}-{0:0}:
> fs_reclaim_acquire+0x80/0xa8
> slab_pre_alloc_hook.constprop.0+0x40/0x25c
> __kmem_cache_alloc_node+0x60/0x1cc
> __kmalloc+0xd8/0x100
> topology_parse_cpu_capacity+0x8c/0x178
> get_cpu_for_node+0x88/0xc4
> parse_cluster+0x1b0/0x28c
> parse_cluster+0x8c/0x28c
> init_cpu_topology+0x168/0x188
> smp_prepare_cpus+0x24/0xf8
> kernel_init_freeable+0x18c/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #1 (fs_reclaim){+.+.}-{0:0}:
> __fs_reclaim_acquire+0x3c/0x48
> fs_reclaim_acquire+0x54/0xa8
> slab_pre_alloc_hook.constprop.0+0x40/0x25c
> __kmem_cache_alloc_node+0x60/0x1cc
> kmalloc_trace+0x50/0xa8
> dev_pm_qos_constraints_allocate+0x38/0x100
> __dev_pm_qos_add_request+0xb0/0x1e8
> dev_pm_qos_add_request+0x58/0x80
> dev_pm_qos_expose_latency_limit+0x60/0x13c
> register_cpu+0x12c/0x130
> topology_init+0xac/0xbc
> do_one_initcall+0x104/0x2bc
> kernel_init_freeable+0x344/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #0 (dev_pm_qos_mtx){+.+.}-{3:3}:
> __lock_acquire+0xe00/0x1060
> lock_acquire+0x1e0/0x2f8
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> dev_pm_qos_update_request+0x38/0x68
> msm_devfreq_boost+0x40/0x70
> msm_devfreq_active+0xc0/0xf0
> msm_gpu_submit+0x10c/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
>other info that might help us debug this:
>
>Chain exists of:
> dev_pm_qos_mtx --> dma_fence_map --> >active_lock
>
> Possible unsafe locking scenario:
>
> CPU0CPU1
>
> lock(>active_lock);
> lock(dma_fence_map);
> lock(>active_lock);
> lock(dev_pm_qos_mtx);
>
> *** DEADLOCK ***
>
>3 locks held by ring0/123:
> #0: ff8087251170 (>lock){+.+.}-{3:3}, at: msm_job_run+0x64/0x150
> #1: ffd00b0e57e8 (dma_fence_map){}-{0:0}, at:
> msm_job_run+0x68/0x150
> #2: ff8087251208 (>active_lock){+.+.}-{3:3}, at:
> msm_gpu_submit+0xec/0x178
>
>stack backtrace:
>CPU: 6 PID: 123 Comm: ring0 Not tainted 6.2.0-rc8-debug+ #559
>Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>Call trace:
> dump_backtrace.part.0+0xb4/0xf8
> show_stack+0x20/0x38
> dump_stack_lvl+0x9c/0xd0
> dump_stack+0x18/0x34
> print_circular_bug+0x1b4/0x1f0
> check_noncircular+0x78/0xac
> __lock_acquire+0xe00/0x1060
> lock_acquire+0x1e0/0x2f8
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> dev_pm_qos_update_request+0x38/0x68
> msm_devfreq_boost+0x40/0x70
> msm_devfreq_active+0xc0/0xf0
> msm_gpu_submit+0x10c/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
> The issue is that dev_pm_qos_mtx is held in the runpm suspend/resume (or
> freq change) path, but it is also held across allocations that could
> recurse into shrinker.
>
> Solve this by changing dev_pm_qos_constraints_allocate() into a function
> that can be called unconditionally before
Re: [Freedreno] [PATCH v2 17/23] PM / QoS: Fix constraints alloc vs reclaim locking
On Mon, Mar 20, 2023 at 3:45 PM Rob Clark wrote:
>
> From: Rob Clark
>
> In the process of adding lockdep annotation for drm GPU scheduler's
> job_run() to detect potential deadlock against shrinker/reclaim, I hit
> this lockdep splat:
>
>==
>WARNING: possible circular locking dependency detected
>6.2.0-rc8-debug+ #558 Tainted: GW
>--
>ring0/125 is trying to acquire lock:
>ffd6d6ce0f28 (dev_pm_qos_mtx){+.+.}-{3:3}, at:
> dev_pm_qos_update_request+0x38/0x68
>
>but task is already holding lock:
>ff8087239208 (>active_lock){+.+.}-{3:3}, at:
> msm_gpu_submit+0xec/0x178
>
>which lock already depends on the new lock.
>
>the existing dependency chain (in reverse order) is:
>
>-> #4 (>active_lock){+.+.}-{3:3}:
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> msm_gpu_submit+0xec/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
>-> #3 (dma_fence_map){}-{0:0}:
> __dma_fence_might_wait+0x74/0xc0
> dma_resv_lockdep+0x1f4/0x2f4
> do_one_initcall+0x104/0x2bc
> kernel_init_freeable+0x344/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #2 (mmu_notifier_invalidate_range_start){+.+.}-{0:0}:
> fs_reclaim_acquire+0x80/0xa8
> slab_pre_alloc_hook.constprop.0+0x40/0x25c
> __kmem_cache_alloc_node+0x60/0x1cc
> __kmalloc+0xd8/0x100
> topology_parse_cpu_capacity+0x8c/0x178
> get_cpu_for_node+0x88/0xc4
> parse_cluster+0x1b0/0x28c
> parse_cluster+0x8c/0x28c
> init_cpu_topology+0x168/0x188
> smp_prepare_cpus+0x24/0xf8
> kernel_init_freeable+0x18c/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #1 (fs_reclaim){+.+.}-{0:0}:
> __fs_reclaim_acquire+0x3c/0x48
> fs_reclaim_acquire+0x54/0xa8
> slab_pre_alloc_hook.constprop.0+0x40/0x25c
> __kmem_cache_alloc_node+0x60/0x1cc
> kmalloc_trace+0x50/0xa8
> dev_pm_qos_constraints_allocate+0x38/0x100
> __dev_pm_qos_add_request+0xb0/0x1e8
> dev_pm_qos_add_request+0x58/0x80
> dev_pm_qos_expose_latency_limit+0x60/0x13c
> register_cpu+0x12c/0x130
> topology_init+0xac/0xbc
> do_one_initcall+0x104/0x2bc
> kernel_init_freeable+0x344/0x34c
> kernel_init+0x30/0x134
> ret_from_fork+0x10/0x20
>
>-> #0 (dev_pm_qos_mtx){+.+.}-{3:3}:
> __lock_acquire+0xe00/0x1060
> lock_acquire+0x1e0/0x2f8
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> dev_pm_qos_update_request+0x38/0x68
> msm_devfreq_boost+0x40/0x70
> msm_devfreq_active+0xc0/0xf0
> msm_gpu_submit+0x10c/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
>other info that might help us debug this:
>
>Chain exists of:
> dev_pm_qos_mtx --> dma_fence_map --> >active_lock
>
> Possible unsafe locking scenario:
>
> CPU0CPU1
>
> lock(>active_lock);
> lock(dma_fence_map);
> lock(>active_lock);
> lock(dev_pm_qos_mtx);
>
> *** DEADLOCK ***
>
>3 locks held by ring0/123:
> #0: ff8087251170 (>lock){+.+.}-{3:3}, at: msm_job_run+0x64/0x150
> #1: ffd00b0e57e8 (dma_fence_map){}-{0:0}, at:
> msm_job_run+0x68/0x150
> #2: ff8087251208 (>active_lock){+.+.}-{3:3}, at:
> msm_gpu_submit+0xec/0x178
>
>stack backtrace:
>CPU: 6 PID: 123 Comm: ring0 Not tainted 6.2.0-rc8-debug+ #559
>Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>Call trace:
> dump_backtrace.part.0+0xb4/0xf8
> show_stack+0x20/0x38
> dump_stack_lvl+0x9c/0xd0
> dump_stack+0x18/0x34
> print_circular_bug+0x1b4/0x1f0
> check_noncircular+0x78/0xac
> __lock_acquire+0xe00/0x1060
> lock_acquire+0x1e0/0x2f8
> __mutex_lock+0xcc/0x3c8
> mutex_lock_nested+0x30/0x44
> dev_pm_qos_update_request+0x38/0x68
> msm_devfreq_boost+0x40/0x70
> msm_devfreq_active+0xc0/0xf0
> msm_gpu_submit+0x10c/0x178
> msm_job_run+0x78/0x150
> drm_sched_main+0x290/0x370
> kthread+0xf0/0x100
> ret_from_fork+0x10/0x20
>
> The issue is that dev_pm_qos_mtx is held in the runpm suspend/resume (or
> freq change) path, but it is also held across allocations that could
> recurse into shrinker.
>
> Solve this by changing dev_pm_qos_constraints_allocate() into a function
> that can be called unconditionally before
Re: [Freedreno] [PATCH] device property: Fix recent breakage of fwnode_get_next_parent_dev()
On Sun, May 1, 2022 at 9:50 AM Andy Shevchenko
wrote:
>
> On Sat, Apr 30, 2022 at 3:00 PM Douglas Anderson
> wrote:
> >
> > Due to a subtle typo, instead of commit 87ffea09470d ("device
> > property: Introduce fwnode_for_each_parent_node()") being a no-op
> > change, it ended up causing the display on my sc7180-trogdor-lazor
> > device from coming up unless I added "fw_devlink=off" to my kernel
> > command line. Fix the typo.
>
> Sorry and merci pour la fix!
> Reviewed-by: Andy Shevchenko
Applied, thanks!
> > Fixes: 87ffea09470d ("device property: Introduce
> > fwnode_for_each_parent_node()")
> > Signed-off-by: Douglas Anderson
> > ---
> >
> > drivers/base/property.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 36401cfe432c..52e85dcb20b5 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -600,7 +600,7 @@ struct device *fwnode_get_next_parent_dev(struct
> > fwnode_handle *fwnode)
> > struct device *dev;
> >
> > fwnode_for_each_parent_node(fwnode, parent) {
> > - dev = get_dev_from_fwnode(fwnode);
> > + dev = get_dev_from_fwnode(parent);
> > if (dev) {
> > fwnode_handle_put(parent);
> > return dev;
> > --
> > 2.36.0.464.gb9c8b46e94-goog
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Mon, Jul 23, 2018 at 7:59 AM, Marek Szyprowski wrote: > Hi Rafael, > > On 2018-07-11 22:36, Rafael J. Wysocki wrote: >> On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski >> wrote: [cut] >>> Frankly, if there are no other reasons I suggest to wire system >>> suspend/resume to pm_runtime_force_* helpers: >>> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume). >> Not a good idea at all IMO. >> >> Use PM driver flags rather I'd say. > > Frankly, till now I wasn't aware of the DPM_FLAG_* in struct dev_pm_info > 'driver_flags'. They are a relatively recent addition. > I've briefly checked them but I don't see the equivalent > of using SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume): keep device suspend if it was runtime suspended > AND really call pm_runtime_suspend if it was not runtime suspended on > system suspend. DPM_FLAG_SMART_SUSPEND is expected to cause that to happen. If you want the device to remain in suspend after the system-wide resume from sleep (if possible), you can set DPM_FLAG_LEAVE_SUSPENDED for it too. Currently a caveat is that genpd still doesn't support the flags. I have patches for that, but I haven't got to posting them yet. If you are interested, I can push this work forward relatively quickly, so please let me know. >>> This way you will have everything related to suspending and resuming in >>> one place and you would not need to bother about all possible cases (like >>> suspending from runtime pm active and suspending from runtime pm suspended >>> cases as well as restoring proper device state on resume). This is >>> especially important in recent kernel releases, where devices are >>> system-suspended regardless their runtime pm states (in older kernels >>> devices were first runtime resumed for system suspend, what made code >>> simpler, but wasn't best from power consumption perspective). >>> >>> If you go this way, You only need to ensure that runtime resume will also >>> restore proper device state besides enabling all the clocks. This will >>> also prepare your driver to properly operate inside power domain, where it >>> is possible for device to loose its internal state after runtime suspend >>> when respective power domain has been turned off. >> I'm not sure if you are aware of the pm_runtime_force_* limitations, though. > > What are those limitations? First off, they don't work with middle-layer code implementing its own PM callbacks that actually operate on devices (like the PCI bus type or the generic ACPI PM domain). This means that drivers using them may not work on systems with ACPI, for example. Second, pm_runtime_force_resume() will always try to leave the device in suspend during system-wide resume from sleep which may not be desirable. Finally, they expect the runtime PM status to be updated by system-wide PM callbacks of devices below the one using them (eg. its children and their children etc) which generally is not required and may not take place unless the drivers of those devices use pm_runtime_force_* themselves. So careful there. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam wrote: > > > On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >> wrote: >>> >>> Hi Rafael, >>> >>> >>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>> wrote: >>>> >>>> Hi Rafael, >>>> >>>> >>>> >>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>> >>>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>>>> >>>>>> From: Sricharan R >>>>>> >>>>>> Finally add the device link between the master device and >>>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>>> master needs it. This is done from add_device callback which gets >>>>>> called once when the master is added to the smmu. >>>>>> >>>>>> Signed-off-by: Sricharan R >>>>>> Signed-off-by: Vivek Gautam >>>>>> Reviewed-by: Tomasz Figa >>>>>> Cc: Rafael J. Wysocki >>>>>> Cc: Lukas Wunner >>>>>> --- >>>>>> >>>>>>- Change since v11 >>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>> >>>>>>drivers/iommu/arm-smmu.c | 12 >>>>>>1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>> *dev) >>>>>> iommu_device_link(>iommu, dev); >>>>>>+ if (pm_runtime_enabled(smmu->dev) && >>>>> >>>>> Why does the creation of the link depend on whether or not runtime PM >>>>> is enabled for the MMU device? >>>> >>>> >>>> The main purpose of this device link is to handle the runtime PM >>>> synchronization >>>> between the supplier (iommu) and consumer (client devices, such as >>>> GPU/display). >>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>> support >>>> such [1]. >>> >>> Is there something you would like me to modify in this patch? >> >> Not really, as long as you are sure that it is correct. :-) >> >> You need to remember, however, that if you add system-wide PM >> callbacks to the driver, the ordering between them and the client >> device callbacks during system-wide suspend matters as well. Don't >> you need the link the ensure the correct system-wide suspend ordering >> too? > > > The fact that currently we handle clocks only through runtime pm callbacks, > would it be better to call runtime pm put/get in system-wide PM callbacks. > This would be same as i mentioned in the other thread. Well, my point is that there's no reason for system-wide suspend to depend directly on runtime PM (which can be effectively disabled by user space as mentioned for multiple times in this thread). It simply is not efficient to let the clock run while the system as a whole is sleeping (even if power/control has been set to "on" for this particular device) and it should not be too hard to prevent that from happening. You may need an additional flag in the driver for that or similar, but it definitely should be doable. Now, that's my advice and I'm not the maintainer of that code, so it is your call (as long as the maintainer agrees with it). Thanks, Rafael ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
Hi,
On Mon, Jul 16, 2018 at 12:11 PM, Vivek Gautam
wrote:
> HI Rafael,
>
>
>
> On 7/16/2018 2:21 PM, Rafael J. Wysocki wrote:
>>
>> On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam
>> wrote:
[cut]
>>>> Although, given the PM
>>>> subsystem internals, the suspend function wouldn't be called on SMMU
>>>> implementation needed power control (since they would have runtime PM
>>>> enabled) and on others, it would be called but do nothing (since no
>>>> clocks).
>>>>
>>>>> Honestly, I just don't know. :-)
>>>>>
>>>>> It just looks odd the way it is done. I think the clock should be
>>>>> gated during system-wide suspend too, because the system can spend
>>>>> much more time in a sleep state than in the working state, on average.
>>>>>
>>>>> And note that you cannot rely on runtime PM to always do it for you,
>>>>> because it may be disabled at a client device or even blocked by user
>>>>> space via power/control in sysfs and that shouldn't matter for
>>>>> system-wide PM.
>>>>
>>>> User space blocking runtime PM through sysfs is a good point. I'm not
>>>> 100% sure how the PM subsystem deals with that in case of system-wide
>>>> suspend. I guess for consistency and safety, we should have the
>>>> suspend callback.
>>>
>>> Will add the following suspend callback (same as
>>> arm_smmu_runtime_suspend):
>>>
>>> static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
>>> {
>>> struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>>
>>> clk_bulk_disable(smmu->num_clks, smmu->clks);
>>>
>>> return 0;
>>> }
>>
>> I think you also need to check if the clock has already been disabled
>> by runtime PM. Otherwise you may end up disabling it twice in a row.
>
>
> Should I rather call a pm_runtime_put() in suspend callback?
That wouldn't work as runtime PM may be effectively disabled by user
space via sysfs. That's one of the reasons why you need the extra
system-wide suspend callback in the first place. :-)
> Or an expanded form something similar to:
> https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/slimbus/qcom-ctrl.c#L695
Yes, you can do something like that, but be careful to make sure that
the state of the device after system-wide resume is consistent with
its runtime PM status in all cases.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam > wrote: >> Hi Rafael, >> >> >> >> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>> >>>> From: Sricharan R >>>> >>>> Finally add the device link between the master device and >>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>> master needs it. This is done from add_device callback which gets >>>> called once when the master is added to the smmu. >>>> >>>> Signed-off-by: Sricharan R >>>> Signed-off-by: Vivek Gautam >>>> Reviewed-by: Tomasz Figa >>>> Cc: Rafael J. Wysocki >>>> Cc: Lukas Wunner >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>> iommu_device_link(>iommu, dev); >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >> >> >> The main purpose of this device link is to handle the runtime PM >> synchronization >> between the supplier (iommu) and consumer (client devices, such as >> GPU/display). >> Moreover, the runtime pm is conditionally enabled for smmu devices that >> support >> such [1]. > > Is there something you would like me to modify in this patch? Not really, as long as you are sure that it is correct. :-) You need to remember, however, that if you add system-wide PM callbacks to the driver, the ordering between them and the client device callbacks during system-wide suspend matters as well. Don't you need the link the ensure the correct system-wide suspend ordering too? ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam
wrote:
> Hi,
>
>
> On Wed, Jul 11, 2018 at 6:21 PM, Tomasz Figa wrote:
>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki wrote:
>>>
>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
>>> wrote:
>>> > Hi Rafael,
>>> >
>>> >
>>> > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki
>>> > wrote:
>>> >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>> >>> From: Sricharan R
>>> >>>
>>> >>> The smmu needs to be functional only when the respective
>>> >>> master's using it are active. The device_link feature
>>> >>> helps to track such functional dependencies, so that the
>>> >>> iommu gets powered when the master device enables itself
>>> >>> using pm_runtime. So by adapting the smmu driver for
>>> >>> runtime pm, above said dependency can be addressed.
>>> >>>
>>> >>> This patch adds the pm runtime/sleep callbacks to the
>>> >>> driver and also the functions to parse the smmu clocks
>>> >>> from DT and enable them in resume/suspend.
>>> >>>
>>> >>> Signed-off-by: Sricharan R
>>> >>> Signed-off-by: Archit Taneja
>>> >>> [vivek: Clock rework to request bulk of clocks]
>>> >>> Signed-off-by: Vivek Gautam
>>> >>> Reviewed-by: Tomasz Figa
>>> >>> ---
>>> >>>
>>> >>> - No change since v11.
>>> >>>
>>> >>> drivers/iommu/arm-smmu.c | 60
>>> >>> ++--
>>> >>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>> >>>
>>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> >>> index f7a96bcf94a6..a01d0dde21dd 100644
>>> >>> --- a/drivers/iommu/arm-smmu.c
>>> >>> +++ b/drivers/iommu/arm-smmu.c
>>> >>> @@ -48,6 +48,7 @@
>>> >>> #include
>>> >>> #include
>>> >>> #include
>>> >>> +#include
>>> >>> #include
>>> >>> #include
>>> >>>
>>> >>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>> >>> u32 num_global_irqs;
>>> >>> u32 num_context_irqs;
>>> >>> unsigned int*irqs;
>>> >>> + struct clk_bulk_data*clks;
>>> >>> + int num_clks;
>>> >>>
>>> >>> u32 cavium_id_base; /* Specific to
>>> >>> Cavium */
>>> >>>
>>> >>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>> >>> arm_smmu_device *smmu)
>>> >>> struct arm_smmu_match_data {
>>> >>> enum arm_smmu_arch_version version;
>>> >>> enum arm_smmu_implementation model;
>>> >>> + const char * const *clks;
>>> >>> + int num_clks;
>>> >>> };
>>> >>>
>>> >>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>> >>> -static struct arm_smmu_match_data name = { .version = ver, .model =
>>> >>> imp }
>>> >>> +static const struct arm_smmu_match_data name = { .version = ver,
>>> >>> .model = imp }
>>> >>>
>>> >>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> >>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> >>> @@ -1919,6 +1924,23 @@ static const struct of_device_id
>>> >>> arm_smmu_of_match[] = {
>>> >>> };
>>> >>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>> >>>
>>> >>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>> >>> +const char * const *clks)
>>> >>> +{
>>> >>> + int i;
>>> >>> +
>>> >>> + if (smmu->num_clks < 1)
>>> >>> + return;
>>> >>> +
>>> >>> + smmu->
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski
wrote:
> Hi Tomasz,
>
> On 2018-07-11 14:51, Tomasz Figa wrote:
>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki wrote:
>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
>>> wrote:
>>>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki
>>>> wrote:
>>>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>>>>> From: Sricharan R
>>>>>>
>>>>>> The smmu needs to be functional only when the respective
>>>>>> master's using it are active. The device_link feature
>>>>>> helps to track such functional dependencies, so that the
>>>>>> iommu gets powered when the master device enables itself
>>>>>> using pm_runtime. So by adapting the smmu driver for
>>>>>> runtime pm, above said dependency can be addressed.
>>>>>>
>>>>>> This patch adds the pm runtime/sleep callbacks to the
>>>>>> driver and also the functions to parse the smmu clocks
>>>>>> from DT and enable them in resume/suspend.
>>>>>>
>>>>>> Signed-off-by: Sricharan R
>>>>>> Signed-off-by: Archit Taneja
>>>>>> [vivek: Clock rework to request bulk of clocks]
>>>>>> Signed-off-by: Vivek Gautam
>>>>>> Reviewed-by: Tomasz Figa
>>>>>> ---
>>>>>>
>>>>>> - No change since v11.
>>>>>>
>>>>>> drivers/iommu/arm-smmu.c | 60
>>>>>> ++--
>>>>>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>>>>> index f7a96bcf94a6..a01d0dde21dd 100644
>>>>>> --- a/drivers/iommu/arm-smmu.c
>>>>>> +++ b/drivers/iommu/arm-smmu.c
>>>>>> @@ -48,6 +48,7 @@
>>>>>> #include
>>>>>> #include
>>>>>> #include
>>>>>> +#include
>>>>>> #include
>>>>>> #include
>>>>>>
>>>>>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>>>>>u32 num_global_irqs;
>>>>>>u32 num_context_irqs;
>>>>>>unsigned int*irqs;
>>>>>> + struct clk_bulk_data*clks;
>>>>>> + int num_clks;
>>>>>>
>>>>>>u32 cavium_id_base; /* Specific to
>>>>>> Cavium */
>>>>>>
>>>>>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>>>>> arm_smmu_device *smmu)
>>>>>> struct arm_smmu_match_data {
>>>>>>enum arm_smmu_arch_version version;
>>>>>>enum arm_smmu_implementation model;
>>>>>> + const char * const *clks;
>>>>>> + int num_clks;
>>>>>> };
>>>>>>
>>>>>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>>>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp
>>>>>> }
>>>>>> +static const struct arm_smmu_match_data name = { .version = ver, .model
>>>>>> = imp }
>>>>>>
>>>>>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>>>>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>>>>> @@ -1919,6 +1924,23 @@ static const struct of_device_id
>>>>>> arm_smmu_of_match[] = {
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>>>>
>>>>>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>>>>> +const char * const *clks)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + if (smmu->num_clks < 1)
>>>>>> + return;
>>>>>> +
>>>>>> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
>>>>>> + sizeof(*smmu->clks), GFP_KERNEL);
>>>
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
wrote:
> Hi Rafael,
>
>
> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki wrote:
>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>> From: Sricharan R
>>>
>>> The smmu needs to be functional only when the respective
>>> master's using it are active. The device_link feature
>>> helps to track such functional dependencies, so that the
>>> iommu gets powered when the master device enables itself
>>> using pm_runtime. So by adapting the smmu driver for
>>> runtime pm, above said dependency can be addressed.
>>>
>>> This patch adds the pm runtime/sleep callbacks to the
>>> driver and also the functions to parse the smmu clocks
>>> from DT and enable them in resume/suspend.
>>>
>>> Signed-off-by: Sricharan R
>>> Signed-off-by: Archit Taneja
>>> [vivek: Clock rework to request bulk of clocks]
>>> Signed-off-by: Vivek Gautam
>>> Reviewed-by: Tomasz Figa
>>> ---
>>>
>>> - No change since v11.
>>>
>>> drivers/iommu/arm-smmu.c | 60
>>> ++--
>>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index f7a96bcf94a6..a01d0dde21dd 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -48,6 +48,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>>
>>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>> u32 num_global_irqs;
>>> u32 num_context_irqs;
>>> unsigned int*irqs;
>>> + struct clk_bulk_data*clks;
>>> + int num_clks;
>>>
>>> u32 cavium_id_base; /* Specific to Cavium
>>> */
>>>
>>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>> arm_smmu_device *smmu)
>>> struct arm_smmu_match_data {
>>> enum arm_smmu_arch_version version;
>>> enum arm_smmu_implementation model;
>>> + const char * const *clks;
>>> + int num_clks;
>>> };
>>>
>>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>>> +static const struct arm_smmu_match_data name = { .version = ver, .model =
>>> imp }
>>>
>>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> @@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[]
>>> = {
>>> };
>>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>
>>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>> +const char * const *clks)
>>> +{
>>> + int i;
>>> +
>>> + if (smmu->num_clks < 1)
>>> + return;
>>> +
>>> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
>>> + sizeof(*smmu->clks), GFP_KERNEL);
>>> + if (!smmu->clks)
>>> + return;
>>> +
>>> + for (i = 0; i < smmu->num_clks; i++)
>>> + smmu->clks[i].id = clks[i];
>>> +}
>>> +
>>> #ifdef CONFIG_ACPI
>>> static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
>>> {
>>> @@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct
>>> platform_device *pdev,
>>> data = of_device_get_match_data(dev);
>>> smmu->version = data->version;
>>> smmu->model = data->model;
>>> + smmu->num_clks = data->num_clks;
>>> +
>>> + arm_smmu_fill_clk_data(smmu, data->clks);
>>>
>>> parse_driver_options(smmu);
>>>
>>> @@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct
>>> platform_device *pdev)
>>> smmu->irqs[i] = irq;
>>> }
>>>
>>> + err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
>>> + if (err)
>>> + return err;
>&g
Re: [Freedreno] [PATCH v12 2/4] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Wed, Jul 11, 2018 at 12:05 PM, Tomasz Figa wrote:
> Hi Rafael,
>
> Thanks for review.
>
> On Wed, Jul 11, 2018 at 6:53 PM Rafael J. Wysocki wrote:
>>
>> On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote:
>> > From: Sricharan R
>> >
>> > The smmu device probe/remove and add/remove master device callbacks
>> > gets called when the smmu is not linked to its master, that is without
>> > the context of the master device. So calling runtime apis in those places
>> > separately.
>> >
>> > Signed-off-by: Sricharan R
>> > [vivek: Cleanup pm runtime calls]
>> > Signed-off-by: Vivek Gautam
>> > Reviewed-by: Tomasz Figa
>> > ---
>> >
>> > - Change since v11
>> >* Replaced pm_runtime_disable() with pm_runtime_force_suspend()
>> > to avoid warning about " Unpreparing enabled clock".
>> > Full warning text mentioned in cover patch.
>> >
>> > drivers/iommu/arm-smmu.c | 92
>> > +++-
>> > 1 file changed, 84 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> > index a01d0dde21dd..09265e206e2d 100644
>> > --- a/drivers/iommu/arm-smmu.c
>> > +++ b/drivers/iommu/arm-smmu.c
>> > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[]
>> > = {
>> > { 0, NULL},
>> > };
>> >
>> > +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
>> > +{
>> > + if (pm_runtime_enabled(smmu->dev))
>>
>> Why do you need the pm_runtime_enabled() checks here and below?
>>
>> pm_runtime_get_sync() and pm_runtime_put() should work just fine if
>> runtime PM is not enabled.
>
> Because pm_runtime_get_sync() acquires a spin lock, even if only for
> the short time of checking if runtime PM is enabled and SMMU driver
> maintainers didn't want any spin locks in certain IOMMU API code paths
> on hardware implementations that don't need runtime PM, while we still
> need to be able to control runtime PM there on hardware
> implementations that need so.
OK, so it is an optimization. It would be good to put a comment in
there to that effect.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> Finally add the device link between the master device and
> smmu, so that the smmu gets runtime enabled/disabled only when the
> master needs it. This is done from add_device callback which gets
> called once when the master is added to the smmu.
>
> Signed-off-by: Sricharan R
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> Cc: Rafael J. Wysocki
> Cc: Lukas Wunner
> ---
>
> - Change since v11
>* Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER.
>
> drivers/iommu/arm-smmu.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 09265e206e2d..916cde4954d2 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev)
>
> iommu_device_link(>iommu, dev);
>
> + if (pm_runtime_enabled(smmu->dev) &&
Why does the creation of the link depend on whether or not runtime PM
is enabled for the MMU device?
What about system-wide PM and system shutdown? Are they always guaranteed
to happen in the right order without the link?
> + !device_link_add(dev, smmu->dev,
> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) {
> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n",
> + dev_name(dev));
> + ret = -ENODEV;
> + goto out_unlink;
> + }
> +
> return 0;
>
> +out_unlink:
> + iommu_device_unlink(>iommu, dev);
> + arm_smmu_master_free_smes(fwspec);
> out_cfg_free:
> kfree(cfg);
> out_free:
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v12 2/4] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
>
> Signed-off-by: Sricharan R
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> ---
>
> - Change since v11
>* Replaced pm_runtime_disable() with pm_runtime_force_suspend()
> to avoid warning about " Unpreparing enabled clock".
> Full warning text mentioned in cover patch.
>
> drivers/iommu/arm-smmu.c | 92
> +++-
> 1 file changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a01d0dde21dd..09265e206e2d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> { 0, NULL},
> };
>
> +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> +{
> + if (pm_runtime_enabled(smmu->dev))
Why do you need the pm_runtime_enabled() checks here and below?
pm_runtime_get_sync() and pm_runtime_put() should work just fine if
runtime PM is not enabled.
> + return pm_runtime_get_sync(smmu->dev);
> +
> + return 0;
> +}
> +
> +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> +{
> + if (pm_runtime_enabled(smmu->dev))
> + pm_runtime_put(smmu->dev);
> +}
> +
> static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> {
> return container_of(dom, struct arm_smmu_domain, domain);
> @@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct
> iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = _domain->cfg;
> - int irq;
> + int ret, irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> return;
>
> + ret = arm_smmu_rpm_get(smmu);
> + if (ret < 0)
> + return;
> +
> /*
>* Disable the context bank and free the page tables before freeing
>* it.
> @@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct
> iommu_domain *domain)
>
> free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> +
> + arm_smmu_rpm_put(smmu);
> }
>
> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> @@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
> return -ENODEV;
>
> smmu = fwspec_smmu(fwspec);
> +
> + ret = arm_smmu_rpm_get(smmu);
> + if (ret < 0)
> + return ret;
> +
> /* Ensure that the domain is finalised */
> ret = arm_smmu_init_domain_context(domain, smmu);
> if (ret < 0)
> - return ret;
> + goto rpm_put;
>
> /*
>* Sanity check the domain. We don't support domains across
> @@ -1226,33 +1251,50 @@ static int arm_smmu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
> dev_err(dev,
> "cannot attach to SMMU %s whilst already attached to
> domain on SMMU %s\n",
> dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
> - return -EINVAL;
> + ret = -EINVAL;
> + goto rpm_put;
> }
>
> /* Looks ok, so add the device to the domain */
> - return arm_smmu_domain_add_master(smmu_domain, fwspec);
> + ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
> +
> +rpm_put:
> + arm_smmu_rpm_put(smmu);
> + return ret;
> }
>
> static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t paddr, size_t size, int prot)
> {
> struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> + int ret;
>
> if (!ops)
> return -ENODEV;
>
> - return ops->map(ops, iova, paddr, size, prot);
> + arm_smmu_rpm_get(smmu);
> + ret = ops->map(ops, iova, paddr, size, prot);
> + arm_smmu_rpm_put(smmu);
> +
> + return ret;
> }
>
> static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>size_t size)
> {
> struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> + size_t ret;
>
> if (!ops)
> return 0;
>
> - return ops->unmap(ops, iova, size);
> + arm_smmu_rpm_get(smmu);
> + ret = ops->unmap(ops, iova, size);
> + arm_smmu_rpm_put(smmu);
> +
> +
Re: [Freedreno] [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> The smmu needs to be functional only when the respective
> master's using it are active. The device_link feature
> helps to track such functional dependencies, so that the
> iommu gets powered when the master device enables itself
> using pm_runtime. So by adapting the smmu driver for
> runtime pm, above said dependency can be addressed.
>
> This patch adds the pm runtime/sleep callbacks to the
> driver and also the functions to parse the smmu clocks
> from DT and enable them in resume/suspend.
>
> Signed-off-by: Sricharan R
> Signed-off-by: Archit Taneja
> [vivek: Clock rework to request bulk of clocks]
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> ---
>
> - No change since v11.
>
> drivers/iommu/arm-smmu.c | 60
> ++--
> 1 file changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7a96bcf94a6..a01d0dde21dd 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,6 +48,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -205,6 +206,8 @@ struct arm_smmu_device {
> u32 num_global_irqs;
> u32 num_context_irqs;
> unsigned int*irqs;
> + struct clk_bulk_data*clks;
> + int num_clks;
>
> u32 cavium_id_base; /* Specific to Cavium */
>
> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
> struct arm_smmu_match_data {
> enum arm_smmu_arch_version version;
> enum arm_smmu_implementation model;
> + const char * const *clks;
> + int num_clks;
> };
>
> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +static const struct arm_smmu_match_data name = { .version = ver, .model =
> imp }
>
> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> @@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[] =
> {
> };
> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
> +const char * const *clks)
> +{
> + int i;
> +
> + if (smmu->num_clks < 1)
> + return;
> +
> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
> + sizeof(*smmu->clks), GFP_KERNEL);
> + if (!smmu->clks)
> + return;
> +
> + for (i = 0; i < smmu->num_clks; i++)
> + smmu->clks[i].id = clks[i];
> +}
> +
> #ifdef CONFIG_ACPI
> static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> {
> @@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct
> platform_device *pdev,
> data = of_device_get_match_data(dev);
> smmu->version = data->version;
> smmu->model = data->model;
> + smmu->num_clks = data->num_clks;
> +
> + arm_smmu_fill_clk_data(smmu, data->clks);
>
> parse_driver_options(smmu);
>
> @@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
> smmu->irqs[i] = irq;
> }
>
> + err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
> + if (err)
> + return err;
> +
> + err = clk_bulk_prepare(smmu->num_clks, smmu->clks);
> + if (err)
> + return err;
> +
> err = arm_smmu_device_cfg_probe(smmu);
> if (err)
> return err;
> @@ -2181,6 +2214,9 @@ static int arm_smmu_device_remove(struct
> platform_device *pdev)
>
> /* Turn the thing off */
> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +
> + clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> +
> return 0;
> }
>
> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct
> device *dev)
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> +{
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + return clk_bulk_enable(smmu->num_clks, smmu->clks);
> +}
> +
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(smmu->num_clks, smmu->clks);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
This is suspicious.
If you need a runtime suspend method, why do you think that it is not necessary
to suspend the device during system-wide transitions?
> +
Re: [Freedreno] [PATCH v5 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers
On Tue, Jan 9, 2018 at 11:01 AM, Vivek Gautam
<vivek.gau...@codeaurora.org> wrote:
> The device link allows the pm framework to tie the supplier and
> consumer. So, whenever the consumer is powered-on the supplier
> is powered-on first.
>
> There are however cases in which the consumer wants to power-on
> the supplier, but not itself.
> E.g., A Graphics or multimedia driver wants to power-on the SMMU
> to unmap a buffer and finish the TLB operations without powering
> on itself. Some of these unmap requests are coming from the
> user space when the controller itself is not powered-up, and it
> can be huge penalty in terms of power and latency to power-up
> the graphics/mm controllers.
> There can be an argument that the supplier should handle this case
> on its own and there should not be a need for the consumer to
> power-on the supplier. But as discussed on the thread [1] about
> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
> in atomic path in arm_smmu_unmap.
>
> [1] https://patchwork.kernel.org/patch/9827825/
>
> Signed-off-by: Vivek Gautam <vivek.gau...@codeaurora.org>
Acked-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Please feel free to route this along with the rest of the series.
Thanks!
> ---
>
> * This is v2 of the patch [1]. Adding it to this patch series.
>[1] https://patchwork.kernel.org/patch/10102447/
>
> drivers/base/power/runtime.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 6e89b51ea3d9..06a2a88fe866 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1579,6 +1579,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>
> device_links_read_unlock(idx);
> }
> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>
> /**
> * pm_runtime_put_suppliers - Drop references to supplier devices.
> @@ -1597,6 +1598,7 @@ void pm_runtime_put_suppliers(struct device *dev)
>
> device_links_read_unlock(idx);
> }
> +EXPORT_SYMBOL_GPL(pm_runtime_put_suppliers);
>
> void pm_runtime_new_link(struct device *dev)
> {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5 0/6] iommu/arm-smmu: Add runtime pm/sleep support
On Tuesday, January 9, 2018 11:01:43 AM CET Vivek Gautam wrote:
> This series provides the support for turning on the arm-smmu's
> clocks/power domains using runtime pm. This is done using the
> recently introduced device links patches, which lets the smmu's
> runtime to follow the master's runtime pm, so the smmu remains
> powered only when the masters use it.
>
> It also adds support for Qcom's arm-smmu-v2 variant that
> has different clocks and power requirements.
>
> Took some reference from the exynos runtime patches [2].
>
> Previous version of the patchset [1].
>
> After much discussion [4] over the use of pm_runtime_get/put() in
> .unmap op path for the arm-smmu, and after disussing over more than
> a couple of approaches to address this, we are putting forward the
> changes *without* using pm_runtime APIs in 'unmap'. Rather, letting
> the client device take the control of powering on/off the connected
> iommu through pm_runtime_get(put)_suppliers() APIs for the scnerios
> when the iommu power can't be directly controlled by clients through
> device links.
> Rafael has agreed to export the suppliers APIs [5].
>
> [V5]
>* Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over
> the list [4] for the last patch series.
>* Added a patch to export pm_runtime_get/put_suppliers() APIs to the
> series as agreed with Rafael [5].
>* Added the related patch for msm drm iommu layer to use
> pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs.
>* Dropped arm-mmu500 clock patch since that would break existing
> platforms.
>* Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to reflect
> the IP version rather than the platform on which it is used.
> The same IP is used across multiple platforms including msm8996,
> and sdm845 etc.
>* Using clock bulk APIs to handle the clocks available to the IP as
> suggested by Stephen Boyd.
>* The first patch in v4 version of the patch-series:
> ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has
> already made it to mainline.
>
> [V4]
>* Reworked the clock handling part. We now take clock names as data
> in the driver for supported compatible versions, and loop over them
> to get, enable, and disable the clocks.
>* Using qcom,msm8996 based compatibles for bindings instead of a generic
> qcom compatible.
>* Refactor MMU500 patch to just add the necessary clock names data and
> corresponding bindings.
>* Added the pm_runtime_get/put() calls in .unmap iommu op (fix added by
> Stanimir on top of previous patch version.
>* Added a patch to fix error path in arm_smmu_add_device()
>* Removed patch 3/5 of V3 patch series that added qcom,smmu-v2 bindings.
>
> [V3]
>* Reworked the patches to keep the clocks init/enabling function
> separately for each compatible.
>
>* Added clocks bindings for MMU40x/500.
>
>* Added a new compatible for qcom,smmu-v2 implementation and
> the clock bindings for the same.
>
>* Rebased on top of 4.11-rc1
>
> [V2]
>* Split the patches little differently.
>
>* Addressed comments.
>
>* Removed the patch #4 [3] from previous post
> for arm-smmu context save restore. Planning to
> post this separately after reworking/addressing Robin's
> feedback.
>
>* Reversed the sequence to disable clocks than enabling.
> This was required for those cases where the
> clocks are populated in a dependent order from DT.
>
> [1] https://www.spinics.net/lists/arm-kernel/msg567488.html
> [2] https://lkml.org/lkml/2016/10/20/70
> [3] https://patchwork.kernel.org/patch/9389717/
> [4] https://patchwork.kernel.org/patch/9827825/
> [5] https://patchwork.kernel.org/patch/10102445/
>
> Sricharan R (3):
> iommu/arm-smmu: Add pm_runtime/sleep ops
> iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
> iommu/arm-smmu: Add the device_link between masters and smmu
>
> Vivek Gautam (3):
> base: power: runtime: Export pm_runtime_get/put_suppliers
> iommu/arm-smmu: Add support for qcom,smmu-v2 variant
> drm/msm: iommu: Replace runtime calls with runtime suppliers
>
> .../devicetree/bindings/iommu/arm,smmu.txt | 35 ++
> drivers/base/power/runtime.c | 2 +
> drivers/gpu/drm/msm/msm_iommu.c| 16 +--
> drivers/iommu/arm-smmu.c | 124
> -
> 4 files changed, 163 insertions(+), 14 deletions(-)
I need some time to review the series.
Thanks,
Rafael
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
