From: Lina Iyer <ilina@codeaurora.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Linux PM <linux-pm@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
Date: Mon, 19 Oct 2020 11:02:15 -0600 [thread overview]
Message-ID: <20201019170215.GE16756@codeaurora.org> (raw)
In-Reply-To: <CAJZ5v0gbXwhaMkFd1MdYPE2APTxQqd8Kv-MMhGTU6eQdJuAZnw@mail.gmail.com>
On Mon, Oct 19 2020 at 04:21 -0600, Rafael J. Wysocki wrote:
>On Mon, Oct 19, 2020 at 12:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Fri, 16 Oct 2020 at 18:55, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >
>> > On Thu, Oct 15, 2020 at 9:38 PM Lina Iyer <ilina@codeaurora.org> wrote:
>> > >
>> > > Some devices may have a predictable interrupt pattern while executing
>> > > usecases. An example would be the VSYNC interrupt associated with
>> > > display devices. A 60 Hz display could cause a interrupt every 16 ms. If
>> > > the device were in a PM domain, the domain would need to be powered up
>> > > for device to resume and handle the interrupt.
>> > >
>> > > Entering a domain idle state saves power, only if the residency of the
>> > > idle state is met. Without knowing the idle duration of the domain, the
>> > > governor would just choose the deepest idle state that matches the QoS
>> > > requirements. The domain might be powered off just as the device is
>> > > expecting to wake up. If devices could inform runtime PM of their next
>> > > event, the parent PM domain's idle duration can be determined.
>> > >
>> > > So let's add the pm_runtime_set_next_wake() API for the device to notify
>> > > runtime PM of the impending wakeup and document it's usage.
>> > >
>> > > Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> > > ---
>> > > Changes in v2:
>> > > - Update documentation
>> > > - Remove runtime PM enabled check
>> > > - Update commit text
>> > > ---
>> > > Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
>> > > drivers/base/power/runtime.c | 24 ++++++++++++++++++++++++
>> > > include/linux/pm.h | 2 ++
>> > > include/linux/pm_runtime.h | 1 +
>> > > 4 files changed, 44 insertions(+)
>> > >
>> > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
>> > > index 0553008b6279..f6aaef15a511 100644
>> > > --- a/Documentation/power/runtime_pm.rst
>> > > +++ b/Documentation/power/runtime_pm.rst
>> > > @@ -515,6 +515,12 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>> > > power.use_autosuspend isn't set, otherwise returns the expiration time
>> > > in jiffies
>> > >
>> > > + `int pm_runtime_set_next_event(struct device *dev, ktime_t next);`
>> > > + - inform runtime PM of the next event on the device. Devices that are
>> > > + sensitive to their domain idle enter/exit latencies may provide this
>> > > + information for use by the PM domain governor. The domain governor would
>> > > + use this information to calculate it's sleep length.
>> > > +
>> > > It is safe to execute the following helper functions from interrupt context:
>> > >
>> > > - pm_request_idle()
>> > > @@ -545,6 +551,7 @@ functions may also be used in interrupt context:
>> > > - pm_runtime_put_sync()
>> > > - pm_runtime_put_sync_suspend()
>> > > - pm_runtime_put_sync_autosuspend()
>> > > +- pm_runtime_set_next_event()
>> > >
>> > > 5. Runtime PM Initialization, Device Probing and Removal
>> > > ========================================================
>> > > @@ -639,6 +646,16 @@ suspend routine). It may be necessary to resume the device and suspend it again
>> > > in order to do so. The same is true if the driver uses different power levels
>> > > or other settings for runtime suspend and system sleep.
>> > >
>> > > +When a device enters idle at runtime, it may trigger the runtime PM up the
>> > > +hierarchy and if device has a predictable interrupt pattern, we can even do a
>> > > +better job at determining the parent's idle state. For example, a display
>> > > +device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
>> > > +domain is powering down and happens to be at the boundary of the VSYNC
>> > > +interrupt, it may not be efficient to power off the domain. Knowing the next
>> > > +wake up (when available) for devices in the domain we can determine the idle
>> > > +duration of the domain. By comparing idle duration with the residencies of the
>> > > +domain idle states, we can be efficient in both power and performance.
>> > > +
>> > > During system resume, the simplest approach is to bring all devices back to full
>> > > power, even if they had been suspended before the system suspend began. There
>> > > are several reasons for this, including:
>> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> > > index 8143210a5c54..5d2ebacfd35e 100644
>> > > --- a/drivers/base/power/runtime.c
>> > > +++ b/drivers/base/power/runtime.c
>> > > @@ -122,6 +122,27 @@ u64 pm_runtime_suspended_time(struct device *dev)
>> > > }
>> > > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>> > >
>> > > +/**
>> > > + * pm_runtime_set_next_wakeup_event - Notify PM framework of an impending event.
>> > > + * @dev: Device to handle
>> > > + * @next: impending interrupt/wakeup for the device
>> > > + */
>> > > +int pm_runtime_set_next_event(struct device *dev, ktime_t next)
>> > > +{
>> > > + unsigned long flags;
>> > > + int ret = -EINVAL;
>> > > +
>> > > + spin_lock_irqsave(&dev->power.lock, flags);
>> > > + if (ktime_before(ktime_get(), next)) {
>> > > + dev->power.next_event = next;
>> > > + ret = 0;
>> > > + }
>> > > + spin_unlock_irqrestore(&dev->power.lock, flags);
>> > > +
>> > > + return ret;
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(pm_runtime_set_next_event);
>> > > +
>> > > /**
>> > > * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
>> > > * @dev: Device to handle.
>> > > @@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
>> > > "Enabling runtime PM for inactive device (%s) with active children\n",
>> > > dev_name(dev));
>> > >
>> > > + /* Reset the next wakeup for the device */
>> > > + dev->power.next_event = KTIME_MAX;
>> > > +
>> > > spin_unlock_irqrestore(&dev->power.lock, flags);
>> > > }
>> > > EXPORT_SYMBOL_GPL(pm_runtime_enable);
>> > > diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > > index a30a4b54df52..9051658674a4 100644
>> > > --- a/include/linux/pm.h
>> > > +++ b/include/linux/pm.h
>> > > @@ -8,6 +8,7 @@
>> > > #ifndef _LINUX_PM_H
>> > > #define _LINUX_PM_H
>> > >
>> > > +#include <linux/ktime.h>
>> > > #include <linux/list.h>
>> > > #include <linux/workqueue.h>
>> > > #include <linux/spinlock.h>
>> > > @@ -616,6 +617,7 @@ struct dev_pm_info {
>> > > u64 active_time;
>> > > u64 suspended_time;
>> > > u64 accounting_timestamp;
>> > > + ktime_t next_event;
>> >
>> > While there are some cosmetic changes to be made, this particular bit
>> > is fundamentally questionable IMV, because next_event (which BTW would
>> > better be called next_wakeup IMO) is not used by PM-runtime.
>> >
>> > The only user of it will be genpd AFAICS, so I don't quite see a
>> > reason to inflict this extra memory cost on everybody, even if they
>> > don't care about genpd and may not even compile it in.
>>
>> That's a good point!
>>
>> May I suggest that the new data is put into the "struct
>> generic_pm_domain_data" instead, which means it will be allocated when
>> a device is attached to a genpd.
>
>Yes, something like that.
>
>> Moreover, we should probably rename the API (and move the
>> implementation of it accordingly) from pm_runtime_set_next_event() to
>> dev_pm_genpd_set_next_wakeup().
>
>Right.
>
Thanks, both of you for the suggestions. I will send an update soon.
--Lina
>> Unless we believe the interface could
>> be useful for other PM domain types (ACPI ?), then we could consider
>> adding a ->set_next_wakeup() callback to the struct dev_pm_domain and
>> implement the interface through a common
>> dev_pm_domain_set_next_wakeup() API.
>
>Maybe.
>
>That would depend on who the other user would be and I wouldn't worry
>about that upfront.
>
>Cheers!
next prev parent reply other threads:[~2020-10-19 17:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-15 19:38 [PATCH v3 0/2] Better domain idle from device wakeup patterns Lina Iyer
2020-10-15 19:38 ` [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup Lina Iyer
2020-10-16 16:55 ` Rafael J. Wysocki
2020-10-19 10:00 ` Ulf Hansson
2020-10-19 10:21 ` Rafael J. Wysocki
2020-10-19 17:02 ` Lina Iyer [this message]
2020-10-15 19:38 ` [PATCH v3 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201019170215.GE16756@codeaurora.org \
--to=ilina@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).