Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pm: optimize resources #39397

Merged
merged 7 commits into from Nov 19, 2021
Merged

Conversation

gmarull
Copy link
Collaborator

@gmarull gmarull commented Oct 13, 2021

It is well known that PM subsystem has never been optimized in terms of
resource usage. The situation is particularly bad in case the PM runtime
API is enabled. What this patch does is to move the responsibility of PM
resource definition to the device driver like this:

  • Device is responsible to define PM resources, using a new set of
    macros: PM_DEVICE_*DEFINE().
  • DEVICE_*DEFINE macro accepts a reference to the device PM state, which
    can be obtained using PM_DEVICE_*REF() set of macros. This
    allows device to initialize the dev->pm reference.

This method decouples a bit more PM from devices since devices just keep
a reference to the device PM state. It also means that future PM changes
will have less chances to impact all devices, but only devices that
support PM.

Stats for west build -b nrf52840dk_nrf52840 samples/hello_world -- -DCONFIG_PM=y -DCONFIG_PM_DEVICE=y -DCONFIG_PM_DEVICE_RUNTIME=y

main:

Memory region         Used Size  Region Size  %age Used
           FLASH:       19632 B         1 MB      1.87%
            SRAM:        7360 B       256 KB      2.81%
        IDT_LIST:          0 GB         2 KB      0.00%

This PR:

Memory region         Used Size  Region Size  %age Used
           FLASH:       19188 B         1 MB      1.83%
            SRAM:        6912 B       256 KB      2.64%
        IDT_LIST:          0 GB         2 KB      0.00%

@gmarull gmarull mentioned this pull request Oct 13, 2021
@gmarull gmarull added the RFC label Oct 13, 2021
@gmarull gmarull requested a review from carlocaione Oct 13, 2021
@ceolin
Copy link
Member

@ceolin ceolin commented Oct 13, 2021

I think get rid of the pm struct with the when PM_DISABLE is passed instead of the callback is a good thing. The other change I have concerns, I think it makes the code more complicated, harder to read and maintain. Instead of jump in optimizations I think we should review the async usage and implementation.

This is not a 👎 but I have doubts if the benefits are greater than the cons.

@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Oct 14, 2021

I think get rid of the pm struct with the when PM_DISABLE is passed instead of the callback is a good thing. The other change I have concerns, I think it makes the code more complicated, harder to read and maintain. Instead of jump in optimizations I think we should review the async usage and implementation.

This is not a -1 but I have doubts if the benefits are greater than the cons.

Since an API change has not been discarded yet (usage of PM_DISABLE), I think it's worth considering all options to try avoiding yet another change in the future. In some other proposals, like pinctrl, optimizing memory usage has been a critical factor on the API design. If savings are considerable, increased complexity could be justified up to a certain point. I think Zephyr should be consistent on this aspect. I'm currently reviewing the runtime API to see if it can be simplified somehow, so this option may become less relevant. Let's see.

yperess
yperess previously requested changes Oct 19, 2021
Copy link
Collaborator

@yperess yperess left a comment

Finally, if we were to add a binding API, we would need a few changes to subsys/pm I would add the following to the build:

zephyr_sources_ifdef(CONFIG_PM    pm.c)

Here I would add an initializer function that will run (probably post kernel). It would iterate through the new section added for the struct pm_device and set the .pm pointers for all the devices which had PM bindings.

(these are just my thoughts/suggestions, please implement as you see fit)

include/device.h Outdated
#define SYS_DEVICE_DEFINE(drv_name, init_fn, pm_enable, level, prio) \
DEVICE_DEFINE(Z_SYS_NAME(init_fn), drv_name, init_fn, \
pm_control_fn, \
pm_enable, \
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from meeting: leave this as argument, but ignore

include/device.h Outdated
#define DEVICE_DEFINE(dev_name, drv_name, init_fn, pm_enable, \
data_ptr, cfg_ptr, level, prio, api_ptr) \
Z_DEVICE_DEFINE(DT_INVALID_NODE, dev_name, drv_name, init_fn, \
pm_control_fn, \
pm_enable, \
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from meeting: leave this as argument, but ignore

include/device.h Outdated
#define DEVICE_DT_DEFINE(node_id, init_fn, pm_enable, \
data_ptr, cfg_ptr, level, prio, \
api_ptr, ...) \
Z_DEVICE_DEFINE(node_id, Z_DEVICE_DT_DEV_NAME(node_id), \
DEVICE_DT_NAME(node_id), init_fn, \
pm_control_fn, \
pm_enable, \
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from meeting: leave this as argument, but ignore

include/device.h Outdated
Z_DEVICE_DEFINE_PM_INIT(dev_name, pm_control_fn)
#define Z_DEVICE_DEFINE_INIT(node_id, dev_name, pm_enable) \
.handles = Z_DEVICE_HANDLE_NAME(node_id, dev_name), \
COND_CODE_1(pm_enable, (Z_DEVICE_DEFINE_PM_INIT(dev_name)), ())
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.pm will always be NULL by default until the subsystem runs so no need for COND_CODE_1

#define Z_PM_DEVICE_STATE_DEFINE(node_id, dev_name, pm_control, async) \
COND_CODE_1(async, (Z_PM_DEVICE_RUNTIME_ASYNC_DEFINE(dev_name);), ()) \
static struct pm_device Z_PM_DEVICE_STATE_NAME(dev_name) = \
Z_PM_DEVICE_INIT(Z_PM_DEVICE_STATE_NAME(dev_name), dev_name, node_id, \
pm_control, async)

/**
* Define device PM state for the given device name.
*
* @param dev_name Device name.
* @param pm_control PM control callback.
* @param async #PM_NO_ASYNC or #PM_ASYNC.
*/
#define PM_DEVICE_STATE_DEFINE(dev_name, pm_control, async) \
Z_PM_DEVICE_STATE_DEFINE(DT_INVALID_NODE, dev_name, pm_control, async)

/**
* Define device PM state for the given node identifier.
*
* @param node_id Node identifier.
* @param pm_control PM control callback.
* @param async #PM_NO_ASYNC or #PM_ASYNC.
*/
#define PM_DEVICE_STATE_DT_DEFINE(node_id, pm_control, async) \
Z_PM_DEVICE_STATE_DEFINE(node_id, Z_DEVICE_DT_DEV_NAME(node_id), \
pm_control, async)

/**
* Define device PM state for the given instance.
*
* @param idx Instance index.
* @param pm_control PM control callback.
* @param async #PM_NO_ASYNC or #PM_ASYNC.
*/
#define PM_DEVICE_STATE_DT_INST_DEFINE(idx, pm_control, async) \
Z_PM_DEVICE_STATE_DEFINE(DT_DRV_INST(idx), \
Z_DEVICE_DT_DEV_NAME(DT_DRV_INST(idx)), \
pm_control, async)
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would now be: PM_DEVICE_DT_BIND(dev_node_id, pm_control) along with the other permutations (I believe async will be added later to keep the PR simple)

#define Z_PM_DEVICE_INIT(obj, dev_name, node_id, pm_control_, async) \
{ \
INIT_PM_DEVICE_RUNTIME(obj) \
INIT_PM_DEVICE_RUNTIME(obj, dev_name, async) \
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now include a const struct device *dev field so we can map from the struct pm_device back up.

*/
#define Z_PM_DEVICE_STATE_DEFINE(node_id, dev_name, pm_control, async) \
COND_CODE_1(async, (Z_PM_DEVICE_RUNTIME_ASYNC_DEFINE(dev_name);), ()) \
static struct pm_device Z_PM_DEVICE_STATE_NAME(dev_name) = \
Copy link
Collaborator

@yperess yperess Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in its own linker section with a start/end pointer so we can iterate

@gmarull gmarull force-pushed the pm-optimize-runtime branch 3 times, most recently from 42feee5 to 1d4e455 Compare Oct 21, 2021
@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Oct 21, 2021

@yperess I have updated the PR with a simpler approach: a reference to the state is passed to the device definition macros (I think @carlescufi mentioned this option), making the change less impactful (NULL can continue to be used). I'll continue exploring the bind option and see if I can come up with something, or just feel free to work on it/contact me.

Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

You also need to update subsys/pm/device.c to change all if (pm->action_cb == NULL) checks to if (pm == NULL)

include/device.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added the area: Tests label Oct 22, 2021
include/pm/device.h Outdated Show resolved Hide resolved
include/pm/device.h Outdated Show resolved Hide resolved
include/pm/device.h Outdated Show resolved Hide resolved
ceolin
ceolin approved these changes Oct 26, 2021
Copy link
Member

@ceolin ceolin left a comment

That is the right direction IMHO. I think the late binding is better but that can be done on top of this one.

yperess added a commit to yperess/zephyr that referenced this issue Nov 4, 2021
This change takes the following approach:
1. Create a new `struct device_subsystem_hooks` which will be extended
   anytime that a new subsystem requires direct access from a device.
   This will add `sizeof(void*)` to the device struct (less than adding
   `struct pm_device` per instance. Then an additional pointer per
   enabled subsystem that hooks to the device struct.
2. The subsystem is responsible for creating the device-to-pm pointer
   by adding a `const struct device*` to the `struct pm_device` (again
   a cost of 1 pointer per enabled PM device). The subsystem then
   registers a `SYS_INIT` which will iterate through the PM structs
   and set the pointers from the device back to the PM struct.

This commit demonstrates a tradeoff. There's a slight increased cost
in memory over PR zephyrproject-rtos#39397 in favor of an extensible API that allows
device declaration to be separate from subsystems.

Signed-off-by: Yuval Peress <peress@google.com>
@yperess
Copy link
Collaborator

@yperess yperess commented Nov 4, 2021

@gmarull @keith-zephyr I finally got things working, see yperess@067f701 for an example. This builds:

$ west build -p -b nrf52840dk_nrf52840 samples/subsys/pm/device_pm/

include/device.h Outdated Show resolved Hide resolved
include/pm/device.h Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
@gmarull gmarull requested review from andyross and rlubos as code owners Nov 16, 2021
@gmarull gmarull requested review from keith-zephyr and removed request for rlubos and alwa-nordic Nov 16, 2021
@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Nov 17, 2021

@yperess can you re-review?

@gmarull gmarull requested a review from GTLin08 as a code owner Nov 17, 2021
gmarull added 7 commits Nov 18, 2021
Define the device using DEVICE_DEFINE macro, so that a single option can
be used regardless of PM being enabled or not.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
The macro already mentions in the docstrings that PM is not supported:

"Invokes DEVICE_DEFINE() with no power management support".

This patch removed the PM entry from the macro and ajusts its uses.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
It is well known that PM subsystem has never been optimized in terms of
resource usage. The situation is particularly bad in case the PM runtime
API is enabled. What this patch does is to move the responsability of PM
resource definition to the device like this:

- Device is responsible to define PM resources, using a new set of
  macros: PM_DEVICE_*DEFINE().
- DEVICE_*DEFINE macro accepts a reference to the device PM state, which
  can be obtained using PM_DEVICE_*REF() set of macros. This
  allows device to initialize the dev->pm reference.

This method decouples a bit more PM from devices since devices just keep
a reference to the device PM state. It also means that future PM changes
will have less chances to impact all devices, but only devices that
support PM.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Port some drivers to the recently introduced macros to showcase its
usage and be able to do some initial testing (nRF52840).

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Use PM_DEVICE_STATE_DEFINE to define PM state.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Use recently introduced PM macros to define device PM resources.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Increase main stack size (in 128 bytes) to make these tests run
successfully on qemu_x86 platform.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
@gmarull gmarull added the DNM label Nov 18, 2021
ceolin
ceolin approved these changes Nov 18, 2021
@gmarull gmarull dismissed yperess’s stale review Nov 19, 2021

Raised concerns do not apply to the current approach (bind option has not been implemented at this stage). I'll investigate if it is feasible using some sort of post-processing as we do in other areas.

@gmarull gmarull removed the DNM label Nov 19, 2021
@gmarull
Copy link
Collaborator Author

@gmarull gmarull commented Nov 19, 2021

Confirmed CI passes, dropped buildkite resorce increase patch.

@carlescufi carlescufi merged commit a6a296d into zephyrproject-rtos:main Nov 19, 2021
25 of 26 checks passed
@gmarull gmarull deleted the pm-optimize-runtime branch Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API area: I2C area: Networking area: Power Management area: PWM area: Tests area: Timer platform: NXP platform: STM32 RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants