* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 16:39 [PATCH] drm/simpledrm: Add support for multiple "power-domains" Janne Grunau via B4 Relay
@ 2023-09-10 18:28 ` kernel test robot
2023-09-10 18:58 ` Janne Grunau
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-09-10 18:28 UTC (permalink / raw)
To: Janne Grunau via B4 Relay, Thomas Zimmermann,
Javier Martinez Canillas, David Airlie, Daniel Vetter
Cc: oe-kbuild-all, dri-devel, linux-kernel, asahi
Hi Janne,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 15d30b46573d75f5cb58cfacded8ebab9c76a2b0]
url: https://github.com/intel-lab-lkp/linux/commits/Janne-Grunau-via-B4-Relay/drm-simpledrm-Add-support-for-multiple-power-domains/20230911-004026
base: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
patch link: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685%40jannau.net
patch subject: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230911/202309110206.wDXP9YXl-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230911/202309110206.wDXP9YXl-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309110206.wDXP9YXl-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14,
from include/linux/i2c.h:13,
from include/uapi/linux/fb.h:6,
from include/linux/fb.h:7,
from include/linux/platform_data/simplefb.h:12,
from drivers/gpu/drm/tiny/simpledrm.c:7:
drivers/gpu/drm/tiny/simpledrm.c: In function 'simpledrm_device_detach_genpd':
>> include/drm/drm_print.h:456:39: warning: ' ' flag used with '%p' gnu_printf format [-Wformat=]
456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
include/drm/drm_print.h:456:9: note: in expansion of macro 'dev_err'
456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~~
include/drm/drm_print.h:469:9: note: in expansion of macro '__drm_printk'
469 | __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~
drivers/gpu/drm/tiny/simpledrm.c:506:9: note: in expansion of macro 'drm_err'
506 | drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
| ^~~~~~~
vim +456 include/drm/drm_print.h
e820f52577b14c6 Jim Cromie 2022-09-11 416
02c9656b2f0d699 Haneen Mohammed 2017-10-17 417 /**
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 418 * DRM_DEV_DEBUG() - Debug output for generic drm code
02c9656b2f0d699 Haneen Mohammed 2017-10-17 419 *
306589856399e18 Douglas Anderson 2021-09-21 420 * NOTE: this is deprecated in favor of drm_dbg_core().
306589856399e18 Douglas Anderson 2021-09-21 421 *
091756bbb1a9613 Haneen Mohammed 2017-10-17 422 * @dev: device pointer
091756bbb1a9613 Haneen Mohammed 2017-10-17 423 * @fmt: printf() like format string.
02c9656b2f0d699 Haneen Mohammed 2017-10-17 424 */
db87086492581c8 Joe Perches 2018-03-16 425 #define DRM_DEV_DEBUG(dev, fmt, ...) \
db87086492581c8 Joe Perches 2018-03-16 426 drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 427 /**
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 428 * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 429 *
306589856399e18 Douglas Anderson 2021-09-21 430 * NOTE: this is deprecated in favor of drm_dbg() or dev_dbg().
306589856399e18 Douglas Anderson 2021-09-21 431 *
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 432 * @dev: device pointer
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 433 * @fmt: printf() like format string.
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 434 */
db87086492581c8 Joe Perches 2018-03-16 435 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...) \
db87086492581c8 Joe Perches 2018-03-16 436 drm_dev_dbg(dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 437 /**
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 438 * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 439 *
306589856399e18 Douglas Anderson 2021-09-21 440 * NOTE: this is deprecated in favor of drm_dbg_kms().
306589856399e18 Douglas Anderson 2021-09-21 441 *
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 442 * @dev: device pointer
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 443 * @fmt: printf() like format string.
b52817e9de06a3a Mauro Carvalho Chehab 2020-10-27 444 */
db87086492581c8 Joe Perches 2018-03-16 445 #define DRM_DEV_DEBUG_KMS(dev, fmt, ...) \
db87086492581c8 Joe Perches 2018-03-16 446 drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
a18b21929453af7 Lyude Paul 2018-07-16 447
fb6c7ab8718eb25 Jani Nikula 2019-12-10 448 /*
fb6c7ab8718eb25 Jani Nikula 2019-12-10 449 * struct drm_device based logging
fb6c7ab8718eb25 Jani Nikula 2019-12-10 450 *
fb6c7ab8718eb25 Jani Nikula 2019-12-10 451 * Prefer drm_device based logging over device or prink based logging.
fb6c7ab8718eb25 Jani Nikula 2019-12-10 452 */
fb6c7ab8718eb25 Jani Nikula 2019-12-10 453
fb6c7ab8718eb25 Jani Nikula 2019-12-10 454 /* Helper for struct drm_device based logging. */
fb6c7ab8718eb25 Jani Nikula 2019-12-10 455 #define __drm_printk(drm, level, type, fmt, ...) \
fb6c7ab8718eb25 Jani Nikula 2019-12-10 @456 dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
fb6c7ab8718eb25 Jani Nikula 2019-12-10 457
fb6c7ab8718eb25 Jani Nikula 2019-12-10 458
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 16:39 [PATCH] drm/simpledrm: Add support for multiple "power-domains" Janne Grunau via B4 Relay
2023-09-10 18:28 ` kernel test robot
@ 2023-09-10 18:58 ` Janne Grunau
2023-09-10 20:16 ` Christophe JAILLET
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Janne Grunau @ 2023-09-10 18:58 UTC (permalink / raw)
To: Thomas Zimmermann, Javier Martinez Canillas, David Airlie,
Daniel Vetter
Cc: dri-devel, linux-kernel, asahi
On 2023-09-10 18:39:39 +0200, Janne Grunau via B4 Relay wrote:
> From: Janne Grunau <j@jannau.net>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 106 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..efedede57d42 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
commented cast, removed locally for v2
> +
> +
> + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
broken log statement as pointed out by kernel test robot, not ment to be
included in this change. removed locally for v2
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return PTR_ERR(sdev->pwr_dom_devs[i]);
> + }
> + drm_err(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_err(&sdev->dev, "failed to link power-domain %u\n", i);
wrong format specifier for int, fixed locally for v2
Janne
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 16:39 [PATCH] drm/simpledrm: Add support for multiple "power-domains" Janne Grunau via B4 Relay
2023-09-10 18:28 ` kernel test robot
2023-09-10 18:58 ` Janne Grunau
@ 2023-09-10 20:16 ` Christophe JAILLET
2023-09-12 6:44 ` Janne Grunau
2023-09-11 12:26 ` Thomas Zimmermann
2023-09-12 14:33 ` kernel test robot
4 siblings, 1 reply; 9+ messages in thread
From: Christophe JAILLET @ 2023-09-10 20:16 UTC (permalink / raw)
To: j, Thomas Zimmermann, Javier Martinez Canillas, David Airlie,
Daniel Vetter
Cc: dri-devel, linux-kernel, asahi
Le 10/09/2023 à 18:39, Janne Grunau via B4 Relay a écrit :
> From: Janne Grunau <j@jannau.net>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 106 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..efedede57d42 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> +
> +
> + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return PTR_ERR(sdev->pwr_dom_devs[i]);
> + }
> + drm_err(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> + }
> +
sdev->pwr_dom_devs[i] can be an ERR_PTR here.
Maybe a break or a continue missing after drm_err() above?
CJ
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_err(&sdev->dev, "failed to link power-domain %u\n", i);
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +754,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 20:16 ` Christophe JAILLET
@ 2023-09-12 6:44 ` Janne Grunau
0 siblings, 0 replies; 9+ messages in thread
From: Janne Grunau @ 2023-09-12 6:44 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Thomas Zimmermann, Javier Martinez Canillas, David Airlie,
Daniel Vetter, dri-devel, linux-kernel, asahi
On 2023-09-10 22:16:05 +0200, Christophe JAILLET wrote:
> Le 10/09/2023 à 18:39, Janne Grunau via B4 Relay a écrit :
> > From: Janne Grunau <j@jannau.net>
> >
> > Multiple power domains need to be handled explicitly in each driver. The
> > driver core can not handle it automatically since it is not aware of
> > power sequencing requirements the hardware might have. This is not a
> > problem for simpledrm since everything is expected to be powered on by
> > the bootloader. simpledrm has just ensure it remains powered on during
> > its lifetime.
> > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > systems. The HDMI output initialized by the bootloader requires keeping
> > the display controller and a DP phy power domain on.
> >
> > Signed-off-by: Janne Grunau <j@jannau.net>
> > ---
> > drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index ff86ba1ae1b8..efedede57d42 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -6,6 +6,7 @@
> > #include <linux/of_address.h>
> > #include <linux/platform_data/simplefb.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/regulator/consumer.h>
> > #include <drm/drm_aperture.h>
> > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > unsigned int regulator_count;
> > struct regulator **regulators;
> > #endif
> > + /* power-domains */
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + int pwr_dom_count;
> > + struct device **pwr_dom_devs;
> > + struct device_link **pwr_dom_links;
> > +#endif
> > /* simplefb settings */
> > struct drm_display_mode mode;
> > @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > }
> > #endif
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +/*
> > + * Generic power domain handling code.
> > + *
> > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > + * dt node. This is only necessary if there is more than one power-domain.
> > + * A single power-domains is handled automatically by the driver core. Multiple
> > + * power-domains have to be handled by drivers since the driver core can't know
> > + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> > + * since the bootloader has put the power domains already in the correct state.
> > + * simpledrm has only to ensure they remain active for its lifetime.
> > + *
> > + * When the driver unloads, we detach from the power-domains.
> > + *
> > + * We only complain about errors here, no action is taken as the most likely
> > + * error can only happen due to a mismatch between the bootloader which set
> > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > + * device tree. Chances are that there are no adverse effects, and if there are,
> > + * a clean teardown of the fb probe will not help us much either. So just
> > + * complain and carry on, and hope that the user actually gets a working fb at
> > + * the end of things.
> > + */
> > +static void simpledrm_device_detach_genpd(void *res)
> > +{
> > + int i;
> > + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> > +
> > +
> > + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
> > + if (sdev->pwr_dom_count <= 1)
> > + return;
> > +
> > + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> > + if (!sdev->pwr_dom_links[i])
> > + device_link_del(sdev->pwr_dom_links[i]);
> > + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> > + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> > + }
> > +}
> > +
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + struct device *dev = sdev->dev.dev;
> > + int i;
> > +
> > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> > + "#power-domain-cells");
> > + /*
> > + * Single power-domain devices are handled by driver core nothing to do
> > + * here. The same for device nodes without "power-domains" property.
> > + */
> > + if (sdev->pwr_dom_count <= 1)
> > + return 0;
> > +
> > + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_devs),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_devs)
> > + return -ENOMEM;
> > +
> > + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_links),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_links)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < sdev->pwr_dom_count; i++) {
> > + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> > + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> > + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> > + if (ret == -EPROBE_DEFER) {
> > + simpledrm_device_detach_genpd(sdev);
> > + return PTR_ERR(sdev->pwr_dom_devs[i]);
> > + }
> > + drm_err(&sdev->dev,
> > + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
> > + }
> > +
>
> sdev->pwr_dom_devs[i] can be an ERR_PTR here.
> Maybe a break or a continue missing after drm_err() above?
yes, a continue is missing, added, locally for v2.
thanks
Janne
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 16:39 [PATCH] drm/simpledrm: Add support for multiple "power-domains" Janne Grunau via B4 Relay
` (2 preceding siblings ...)
2023-09-10 20:16 ` Christophe JAILLET
@ 2023-09-11 12:26 ` Thomas Zimmermann
2023-09-11 14:43 ` Jani Nikula
2023-09-12 6:47 ` Janne Grunau
2023-09-12 14:33 ` kernel test robot
4 siblings, 2 replies; 9+ messages in thread
From: Thomas Zimmermann @ 2023-09-11 12:26 UTC (permalink / raw)
To: j, Javier Martinez Canillas, David Airlie, Daniel Vetter
Cc: linux-kernel, dri-devel, asahi
[-- Attachment #1.1: Type: text/plain, Size: 6486 bytes --]
Hi
Am 10.09.23 um 18:39 schrieb Janne Grunau via B4 Relay:
> From: Janne Grunau <j@jannau.net>
>
> Multiple power domains need to be handled explicitly in each driver. The
> driver core can not handle it automatically since it is not aware of
> power sequencing requirements the hardware might have. This is not a
> problem for simpledrm since everything is expected to be powered on by
> the bootloader. simpledrm has just ensure it remains powered on during
> its lifetime.
> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> systems. The HDMI output initialized by the bootloader requires keeping
> the display controller and a DP phy power domain on.
>
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
> drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 106 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index ff86ba1ae1b8..efedede57d42 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -6,6 +6,7 @@
> #include <linux/of_address.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
>
> #include <drm/drm_aperture.h>
> @@ -227,6 +228,12 @@ struct simpledrm_device {
> unsigned int regulator_count;
> struct regulator **regulators;
> #endif
> + /* power-domains */
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> + int pwr_dom_count;
> + struct device **pwr_dom_devs;
> + struct device_link **pwr_dom_links;
> +#endif
>
> /* simplefb settings */
> struct drm_display_mode mode;
> @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> +/*
> + * Generic power domain handling code.
> + *
> + * Here we handle the power-domains properties of our "simple-framebuffer"
> + * dt node. This is only necessary if there is more than one power-domain.
> + * A single power-domains is handled automatically by the driver core. Multiple
> + * power-domains have to be handled by drivers since the driver core can't know
> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> + * since the bootloader has put the power domains already in the correct state.
> + * simpledrm has only to ensure they remain active for its lifetime.
> + *
> + * When the driver unloads, we detach from the power-domains.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> + * device tree. Chances are that there are no adverse effects, and if there are,
> + * a clean teardown of the fb probe will not help us much either. So just
> + * complain and carry on, and hope that the user actually gets a working fb at
> + * the end of things.
> + */
> +static void simpledrm_device_detach_genpd(void *res)
> +{
> + int i;
> + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> +
> +
> + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
If anything, drm_dbg()
> + if (sdev->pwr_dom_count <= 1)
> + return;
> +
> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> + if (!sdev->pwr_dom_links[i])
> + device_link_del(sdev->pwr_dom_links[i]);
> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> + }
> +}
> +
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->dev.dev;
> + int i;
> +
> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells");
> + /*
> + * Single power-domain devices are handled by driver core nothing to do
> + * here. The same for device nodes without "power-domains" property.
> + */
> + if (sdev->pwr_dom_count <= 1)
> + return 0;
> +
> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_devs),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_devs)
> + return -ENOMEM;
> +
> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> + sizeof(*sdev->pwr_dom_links),
> + GFP_KERNEL);
> + if (!sdev->pwr_dom_links)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->pwr_dom_count; i++) {
> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> + if (ret == -EPROBE_DEFER) {
> + simpledrm_device_detach_genpd(sdev);
> + return PTR_ERR(sdev->pwr_dom_devs[i]);
> + }
> + drm_err(&sdev->dev,
> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
The driver's not really failing to initialize AFAICT. CAlling drm_warn()
might be more appropriate.
> + }
> +
> + sdev->pwr_dom_links[i] = device_link_add(dev,
> + sdev->pwr_dom_devs[i],
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_RPM_ACTIVE);
> + if (!sdev->pwr_dom_links[i])
> + drm_err(&sdev->dev, "failed to link power-domain %u\n", i);
Also drm_warn() ?
Best regards
Thomas
> + }
> +
> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
> +}
> +#else
> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * Modesetting
> */
> @@ -651,6 +754,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> if (ret)
> return ERR_PTR(ret);
> ret = simpledrm_device_init_regulators(sdev);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = simpledrm_device_attach_genpd(sdev);
> if (ret)
> return ERR_PTR(ret);
>
>
> ---
> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>
> Best regards,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-11 12:26 ` Thomas Zimmermann
@ 2023-09-11 14:43 ` Jani Nikula
2023-09-12 6:47 ` Janne Grunau
1 sibling, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2023-09-11 14:43 UTC (permalink / raw)
To: Thomas Zimmermann, j, Javier Martinez Canillas, David Airlie,
Daniel Vetter
Cc: linux-kernel, dri-devel, asahi
On Mon, 11 Sep 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 10.09.23 um 18:39 schrieb Janne Grunau via B4 Relay:
>> From: Janne Grunau <j@jannau.net>
>>
>> Multiple power domains need to be handled explicitly in each driver. The
>> driver core can not handle it automatically since it is not aware of
>> power sequencing requirements the hardware might have. This is not a
>> problem for simpledrm since everything is expected to be powered on by
>> the bootloader. simpledrm has just ensure it remains powered on during
>> its lifetime.
>> This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
>> systems. The HDMI output initialized by the bootloader requires keeping
>> the display controller and a DP phy power domain on.
>>
>> Signed-off-by: Janne Grunau <j@jannau.net>
>> ---
>> drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 106 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>> index ff86ba1ae1b8..efedede57d42 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -6,6 +6,7 @@
>> #include <linux/of_address.h>
>> #include <linux/platform_data/simplefb.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> #include <linux/regulator/consumer.h>
>>
>> #include <drm/drm_aperture.h>
>> @@ -227,6 +228,12 @@ struct simpledrm_device {
>> unsigned int regulator_count;
>> struct regulator **regulators;
>> #endif
>> + /* power-domains */
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> + int pwr_dom_count;
>> + struct device **pwr_dom_devs;
>> + struct device_link **pwr_dom_links;
>> +#endif
>>
>> /* simplefb settings */
>> struct drm_display_mode mode;
>> @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>> }
>> #endif
>>
>> +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
>> +/*
>> + * Generic power domain handling code.
>> + *
>> + * Here we handle the power-domains properties of our "simple-framebuffer"
>> + * dt node. This is only necessary if there is more than one power-domain.
>> + * A single power-domains is handled automatically by the driver core. Multiple
>> + * power-domains have to be handled by drivers since the driver core can't know
>> + * the correct power sequencing. Power sequencing is not an issue for simpledrm
>> + * since the bootloader has put the power domains already in the correct state.
>> + * simpledrm has only to ensure they remain active for its lifetime.
>> + *
>> + * When the driver unloads, we detach from the power-domains.
>> + *
>> + * We only complain about errors here, no action is taken as the most likely
>> + * error can only happen due to a mismatch between the bootloader which set
>> + * up the "simple-framebuffer" dt node, and the PM domain providers in the
>> + * device tree. Chances are that there are no adverse effects, and if there are,
>> + * a clean teardown of the fb probe will not help us much either. So just
>> + * complain and carry on, and hope that the user actually gets a working fb at
>> + * the end of things.
>> + */
>> +static void simpledrm_device_detach_genpd(void *res)
>> +{
>> + int i;
>> + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
>> +
>> +
>> + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
>
> If anything, drm_dbg()
Drive-by comment, drm_dbg() already prints the function, there's no need
to use __func__.
BR,
Jani.
>
>> + if (sdev->pwr_dom_count <= 1)
>> + return;
>> +
>> + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
>> + if (!sdev->pwr_dom_links[i])
>> + device_link_del(sdev->pwr_dom_links[i]);
>> + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
>> + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
>> + }
>> +}
>> +
>> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
>> +{
>> + struct device *dev = sdev->dev.dev;
>> + int i;
>> +
>> + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
>> + "#power-domain-cells");
>> + /*
>> + * Single power-domain devices are handled by driver core nothing to do
>> + * here. The same for device nodes without "power-domains" property.
>> + */
>> + if (sdev->pwr_dom_count <= 1)
>> + return 0;
>> +
>> + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
>> + sizeof(*sdev->pwr_dom_devs),
>> + GFP_KERNEL);
>> + if (!sdev->pwr_dom_devs)
>> + return -ENOMEM;
>> +
>> + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
>> + sizeof(*sdev->pwr_dom_links),
>> + GFP_KERNEL);
>> + if (!sdev->pwr_dom_links)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < sdev->pwr_dom_count; i++) {
>> + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
>> + if (IS_ERR(sdev->pwr_dom_devs[i])) {
>> + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
>> + if (ret == -EPROBE_DEFER) {
>> + simpledrm_device_detach_genpd(sdev);
>> + return PTR_ERR(sdev->pwr_dom_devs[i]);
>> + }
>> + drm_err(&sdev->dev,
>> + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
>
> The driver's not really failing to initialize AFAICT. CAlling drm_warn()
> might be more appropriate.
>
>> + }
>> +
>> + sdev->pwr_dom_links[i] = device_link_add(dev,
>> + sdev->pwr_dom_devs[i],
>> + DL_FLAG_STATELESS |
>> + DL_FLAG_PM_RUNTIME |
>> + DL_FLAG_RPM_ACTIVE);
>> + if (!sdev->pwr_dom_links[i])
>> + drm_err(&sdev->dev, "failed to link power-domain %u\n", i);
>
> Also drm_warn() ?
>
> Best regards
> Thomas
>
>> + }
>> +
>> + return devm_add_action_or_reset(dev, simpledrm_device_detach_genpd, sdev);
>> +}
>> +#else
>> +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> /*
>> * Modesetting
>> */
>> @@ -651,6 +754,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>> if (ret)
>> return ERR_PTR(ret);
>> ret = simpledrm_device_init_regulators(sdev);
>> + if (ret)
>> + return ERR_PTR(ret);
>> + ret = simpledrm_device_attach_genpd(sdev);
>> if (ret)
>> return ERR_PTR(ret);
>>
>>
>> ---
>> base-commit: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
>> change-id: 20230910-simpledrm-multiple-power-domains-f41efa6ad9bc
>>
>> Best regards,
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-11 12:26 ` Thomas Zimmermann
2023-09-11 14:43 ` Jani Nikula
@ 2023-09-12 6:47 ` Janne Grunau
1 sibling, 0 replies; 9+ messages in thread
From: Janne Grunau @ 2023-09-12 6:47 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Javier Martinez Canillas, David Airlie, Daniel Vetter,
linux-kernel, dri-devel, asahi
On 2023-09-11 14:26:10 +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 10.09.23 um 18:39 schrieb Janne Grunau via B4 Relay:
> > From: Janne Grunau <j@jannau.net>
> >
> > Multiple power domains need to be handled explicitly in each driver. The
> > driver core can not handle it automatically since it is not aware of
> > power sequencing requirements the hardware might have. This is not a
> > problem for simpledrm since everything is expected to be powered on by
> > the bootloader. simpledrm has just ensure it remains powered on during
> > its lifetime.
> > This is required on Apple silicon M2 and M2 Pro/Max/Ultra desktop
> > systems. The HDMI output initialized by the bootloader requires keeping
> > the display controller and a DP phy power domain on.
> >
> > Signed-off-by: Janne Grunau <j@jannau.net>
> > ---
> > drivers/gpu/drm/tiny/simpledrm.c | 106 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index ff86ba1ae1b8..efedede57d42 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -6,6 +6,7 @@
> > #include <linux/of_address.h>
> > #include <linux/platform_data/simplefb.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/regulator/consumer.h>
> > #include <drm/drm_aperture.h>
> > @@ -227,6 +228,12 @@ struct simpledrm_device {
> > unsigned int regulator_count;
> > struct regulator **regulators;
> > #endif
> > + /* power-domains */
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > + int pwr_dom_count;
> > + struct device **pwr_dom_devs;
> > + struct device_link **pwr_dom_links;
> > +#endif
> > /* simplefb settings */
> > struct drm_display_mode mode;
> > @@ -468,6 +475,102 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > }
> > #endif
> > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> > +/*
> > + * Generic power domain handling code.
> > + *
> > + * Here we handle the power-domains properties of our "simple-framebuffer"
> > + * dt node. This is only necessary if there is more than one power-domain.
> > + * A single power-domains is handled automatically by the driver core. Multiple
> > + * power-domains have to be handled by drivers since the driver core can't know
> > + * the correct power sequencing. Power sequencing is not an issue for simpledrm
> > + * since the bootloader has put the power domains already in the correct state.
> > + * simpledrm has only to ensure they remain active for its lifetime.
> > + *
> > + * When the driver unloads, we detach from the power-domains.
> > + *
> > + * We only complain about errors here, no action is taken as the most likely
> > + * error can only happen due to a mismatch between the bootloader which set
> > + * up the "simple-framebuffer" dt node, and the PM domain providers in the
> > + * device tree. Chances are that there are no adverse effects, and if there are,
> > + * a clean teardown of the fb probe will not help us much either. So just
> > + * complain and carry on, and hope that the user actually gets a working fb at
> > + * the end of things.
> > + */
> > +static void simpledrm_device_detach_genpd(void *res)
> > +{
> > + int i;
> > + struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
> > +
> > +
> > + drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
>
> If anything, drm_dbg()
see my own reply, was never supposed to be there, removed locally
>
> > + if (sdev->pwr_dom_count <= 1)
> > + return;
> > +
> > + for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
> > + if (!sdev->pwr_dom_links[i])
> > + device_link_del(sdev->pwr_dom_links[i]);
> > + if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
> > + dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
> > + }
> > +}
> > +
> > +static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
> > +{
> > + struct device *dev = sdev->dev.dev;
> > + int i;
> > +
> > + sdev->pwr_dom_count = of_count_phandle_with_args(dev->of_node, "power-domains",
> > + "#power-domain-cells");
> > + /*
> > + * Single power-domain devices are handled by driver core nothing to do
> > + * here. The same for device nodes without "power-domains" property.
> > + */
> > + if (sdev->pwr_dom_count <= 1)
> > + return 0;
> > +
> > + sdev->pwr_dom_devs = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_devs),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_devs)
> > + return -ENOMEM;
> > +
> > + sdev->pwr_dom_links = devm_kcalloc(dev, sdev->pwr_dom_count,
> > + sizeof(*sdev->pwr_dom_links),
> > + GFP_KERNEL);
> > + if (!sdev->pwr_dom_links)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < sdev->pwr_dom_count; i++) {
> > + sdev->pwr_dom_devs[i] = dev_pm_domain_attach_by_id(dev, i);
> > + if (IS_ERR(sdev->pwr_dom_devs[i])) {
> > + int ret = PTR_ERR(sdev->pwr_dom_devs[i]);
> > + if (ret == -EPROBE_DEFER) {
> > + simpledrm_device_detach_genpd(sdev);
> > + return PTR_ERR(sdev->pwr_dom_devs[i]);
> > + }
> > + drm_err(&sdev->dev,
> > + "pm_domain_attach_by_id(%u) failed: %d\n", i, ret);
>
> The driver's not really failing to initialize AFAICT. CAlling drm_warn()
> might be more appropriate.
copied from simpledrm_device_init_regulators() but I agree that
drm_warn() is more appropiate. change locally for v2.
> > + }
> > +
> > + sdev->pwr_dom_links[i] = device_link_add(dev,
> > + sdev->pwr_dom_devs[i],
> > + DL_FLAG_STATELESS |
> > + DL_FLAG_PM_RUNTIME |
> > + DL_FLAG_RPM_ACTIVE);
> > + if (!sdev->pwr_dom_links[i])
> > + drm_err(&sdev->dev, "failed to link power-domain %u\n", i);
>
> Also drm_warn() ?
changed
Thanks,
Janne
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
2023-09-10 16:39 [PATCH] drm/simpledrm: Add support for multiple "power-domains" Janne Grunau via B4 Relay
` (3 preceding siblings ...)
2023-09-11 12:26 ` Thomas Zimmermann
@ 2023-09-12 14:33 ` kernel test robot
4 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-09-12 14:33 UTC (permalink / raw)
To: Janne Grunau via B4 Relay, Thomas Zimmermann,
Javier Martinez Canillas, David Airlie, Daniel Vetter
Cc: llvm, oe-kbuild-all, dri-devel, linux-kernel, asahi
Hi Janne,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 15d30b46573d75f5cb58cfacded8ebab9c76a2b0]
url: https://github.com/intel-lab-lkp/linux/commits/Janne-Grunau-via-B4-Relay/drm-simpledrm-Add-support-for-multiple-power-domains/20230911-004026
base: 15d30b46573d75f5cb58cfacded8ebab9c76a2b0
patch link: https://lore.kernel.org/r/20230910-simpledrm-multiple-power-domains-v1-1-f8718aefc685%40jannau.net
patch subject: [PATCH] drm/simpledrm: Add support for multiple "power-domains"
config: arm64-randconfig-r003-20230912 (https://download.01.org/0day-ci/archive/20230912/202309122212.MetCn4UK-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230912/202309122212.MetCn4UK-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309122212.MetCn4UK-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/tiny/simpledrm.c:506:24: warning: flag ' ' results in undefined behavior with 'p' conversion specifier [-Wformat]
506 | drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
| ~^~
include/drm/drm_print.h:469:39: note: expanded from macro 'drm_err'
469 | __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
| ^~~
include/drm/drm_print.h:456:41: note: expanded from macro '__drm_printk'
456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:144:57: note: expanded from macro 'dev_err'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:19:22: note: expanded from macro 'dev_fmt'
19 | #define dev_fmt(fmt) fmt
| ^~~
include/linux/dev_printk.h:110:16: note: expanded from macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
1 warning generated.
vim +506 drivers/gpu/drm/tiny/simpledrm.c
477
478 #if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
479 /*
480 * Generic power domain handling code.
481 *
482 * Here we handle the power-domains properties of our "simple-framebuffer"
483 * dt node. This is only necessary if there is more than one power-domain.
484 * A single power-domains is handled automatically by the driver core. Multiple
485 * power-domains have to be handled by drivers since the driver core can't know
486 * the correct power sequencing. Power sequencing is not an issue for simpledrm
487 * since the bootloader has put the power domains already in the correct state.
488 * simpledrm has only to ensure they remain active for its lifetime.
489 *
490 * When the driver unloads, we detach from the power-domains.
491 *
492 * We only complain about errors here, no action is taken as the most likely
493 * error can only happen due to a mismatch between the bootloader which set
494 * up the "simple-framebuffer" dt node, and the PM domain providers in the
495 * device tree. Chances are that there are no adverse effects, and if there are,
496 * a clean teardown of the fb probe will not help us much either. So just
497 * complain and carry on, and hope that the user actually gets a working fb at
498 * the end of things.
499 */
500 static void simpledrm_device_detach_genpd(void *res)
501 {
502 int i;
503 struct simpledrm_device *sdev = /*(struct simpledrm_device *)*/res;
504
505
> 506 drm_err(&sdev->dev, "% power-domains count:%d\n", __func__, sdev->pwr_dom_count);
507 if (sdev->pwr_dom_count <= 1)
508 return;
509
510 for (i = sdev->pwr_dom_count - 1; i >= 0; i--) {
511 if (!sdev->pwr_dom_links[i])
512 device_link_del(sdev->pwr_dom_links[i]);
513 if (!IS_ERR_OR_NULL(sdev->pwr_dom_devs[i]))
514 dev_pm_domain_detach(sdev->pwr_dom_devs[i], true);
515 }
516 }
517
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread