Export a dev_pm_ops meant to be used with simple drivers, which have
their struct drm_device registered as their struct device's drvdata, and
only call drm_mode_config_pm_{suspend,resume}.
The symbol is conditionally exported if IS_ENABLED(CONFIG_PM_SLEEP), and
therefore it should always be referenced using the pm_sleep_ptr() macro.
Signed-off-by: Paul Cercueil <[email protected]>
---
drivers/gpu/drm/drm_modeset_helper.c | 32 ++++++++++++++++++++++++++++
include/drm/drm_modeset_helper.h | 4 ++++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index f858dfedf2cf..0bc9f9228a60 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -20,6 +20,9 @@
* OF THIS SOFTWARE.
*/
+#include <linux/device.h>
+#include <linux/pm.h>
+
#include <drm/drm_atomic_helper.h>
#include <drm/drm_fb_helper.h>
#include <drm/drm_fourcc.h>
@@ -244,3 +247,32 @@ int drm_mode_config_helper_resume(struct drm_device *dev)
return ret;
}
EXPORT_SYMBOL(drm_mode_config_helper_resume);
+
+static int drm_mode_config_pm_suspend(struct device *dev)
+{
+ struct drm_device *drm = dev_get_drvdata(dev);
+
+ return drm_mode_config_helper_suspend(drm);
+}
+
+static int drm_mode_config_pm_resume(struct device *dev)
+{
+ struct drm_device *drm = dev_get_drvdata(dev);
+
+ return drm_mode_config_helper_resume(drm);
+}
+
+/**
+ * drm_mode_config_pm_ops - Exported dev_pm_ops helper for simple drivers
+ *
+ * This dev_pm_ops can be used for simple drivers that would otherwise only call
+ * drm_mode_config_helper_suspend / drm_mode_config_helper_resume in their PM
+ * callbacks. It is only valid if the driver's drm_device has been registered as
+ * the struct device's drvdata.
+ *
+ * The exported symbol must always be used with the pm_sleep_ptr() macro, like
+ * this:
+ * .pm = pm_sleep_ptr(&drm_mode_config_pm_ops),
+ */
+EXPORT_SIMPLE_DEV_PM_OPS(drm_mode_config_pm_ops,
+ drm_mode_config_pm_suspend, drm_mode_config_pm_resume);
diff --git a/include/drm/drm_modeset_helper.h b/include/drm/drm_modeset_helper.h
index 995fd981cab0..85f29637e9c1 100644
--- a/include/drm/drm_modeset_helper.h
+++ b/include/drm/drm_modeset_helper.h
@@ -23,6 +23,8 @@
#ifndef __DRM_KMS_HELPER_H__
#define __DRM_KMS_HELPER_H__
+#include <linux/pm.h>
+
struct drm_crtc;
struct drm_crtc_funcs;
struct drm_device;
@@ -41,4 +43,6 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
int drm_mode_config_helper_suspend(struct drm_device *dev);
int drm_mode_config_helper_resume(struct drm_device *dev);
+extern const struct dev_pm_ops drm_mode_config_pm_ops;
+
#endif
--
2.35.1
Hi Paul,
thanks for cleaning up. Please see my comments below.
Am 07.11.22 um 18:50 schrieb Paul Cercueil:
> Export a dev_pm_ops meant to be used with simple drivers, which have
> their struct drm_device registered as their struct device's drvdata, and
> only call drm_mode_config_pm_{suspend,resume}.
>
> The symbol is conditionally exported if IS_ENABLED(CONFIG_PM_SLEEP), and
> therefore it should always be referenced using the pm_sleep_ptr() macro.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
> drivers/gpu/drm/drm_modeset_helper.c | 32 ++++++++++++++++++++++++++++
> include/drm/drm_modeset_helper.h | 4 ++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index f858dfedf2cf..0bc9f9228a60 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -20,6 +20,9 @@
> * OF THIS SOFTWARE.
> */
>
> +#include <linux/device.h>
> +#include <linux/pm.h>
> +
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_fourcc.h>
> @@ -244,3 +247,32 @@ int drm_mode_config_helper_resume(struct drm_device *dev)
> return ret;
> }
> EXPORT_SYMBOL(drm_mode_config_helper_resume);
> +
> +static int drm_mode_config_pm_suspend(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + return drm_mode_config_helper_suspend(drm);
> +}
> +
> +static int drm_mode_config_pm_resume(struct device *dev)
> +{
> + struct drm_device *drm = dev_get_drvdata(dev);
> +
> + return drm_mode_config_helper_resume(drm);
> +}
> +
> +/**
> + * drm_mode_config_pm_ops - Exported dev_pm_ops helper for simple drivers
> + *
> + * This dev_pm_ops can be used for simple drivers that would otherwise only call
> + * drm_mode_config_helper_suspend / drm_mode_config_helper_resume in their PM
> + * callbacks. It is only valid if the driver's drm_device has been registered as
> + * the struct device's drvdata.
> + *
> + * The exported symbol must always be used with the pm_sleep_ptr() macro, like
> + * this:
> + * .pm = pm_sleep_ptr(&drm_mode_config_pm_ops),
> + */
> +EXPORT_SIMPLE_DEV_PM_OPS(drm_mode_config_pm_ops,
> + drm_mode_config_pm_suspend, drm_mode_config_pm_resume);
> diff --git a/include/drm/drm_modeset_helper.h b/include/drm/drm_modeset_helper.h
> index 995fd981cab0..85f29637e9c1 100644
> --- a/include/drm/drm_modeset_helper.h
> +++ b/include/drm/drm_modeset_helper.h
> @@ -23,6 +23,8 @@
> #ifndef __DRM_KMS_HELPER_H__
> #define __DRM_KMS_HELPER_H__
I like that you clean up the driver, but not how it's done TBH.
>
> +#include <linux/pm.h>
> +
Half of DRM somehow incudes drm_kms_helper.h. So this include statements
affects more or less everything.
> struct drm_crtc;
> struct drm_crtc_funcs;
> struct drm_device;
> @@ -41,4 +43,6 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> int drm_mode_config_helper_suspend(struct drm_device *dev);
> int drm_mode_config_helper_resume(struct drm_device *dev);
>
> +extern const struct dev_pm_ops drm_mode_config_pm_ops;
> +
That's maybe subjective, but I don't like exporting such _funcs and _ops
instances. They are like blackboxes. And they pollute the symbol
namespace unnecessarily.
I propose a solution similar to DEFINE_DRM_GEM_FOPS [1] or
drm_module_pci_driver. [2]
Define a macro in the header to create the _ops instance, such as
#if defined(CONFIG_PM)
#define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name) \
static __ ## _name ## _suspend() { \
call drm_mode_config_helper_suspend() \
} \
static __ ## _name ## _resume() { \
call drm_mode_config_helper_resume() \
} \
static SIMPLE_DEV_PM_OPS(_name, __ ## _name ## _suspend, __ ## _name
## _resume);
#else
#define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name)
#endif
Drivers can then keep the instance and the include for pm.h internal
within their _drv.c file. The small callback functions are within the
source file as well. If CONFIG_PM has been disabled, nothing is being
generated.
Best regards
Thomas
[1]
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_gem.h#L396
[2]
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_module.h#L58
> #endif
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi Thomas,
Le mar. 8 nov. 2022 ? 13:10:49 +0100, Thomas Zimmermann
<[email protected]> a ?crit :
> Hi Paul,
>
> thanks for cleaning up. Please see my comments below.
>
> Am 07.11.22 um 18:50 schrieb Paul Cercueil:
>> Export a dev_pm_ops meant to be used with simple drivers, which have
>> their struct drm_device registered as their struct device's drvdata,
>> and
>> only call drm_mode_config_pm_{suspend,resume}.
>>
>> The symbol is conditionally exported if IS_ENABLED(CONFIG_PM_SLEEP),
>> and
>> therefore it should always be referenced using the pm_sleep_ptr()
>> macro.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>> drivers/gpu/drm/drm_modeset_helper.c | 32
>> ++++++++++++++++++++++++++++
>> include/drm/drm_modeset_helper.h | 4 ++++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modeset_helper.c
>> b/drivers/gpu/drm/drm_modeset_helper.c
>> index f858dfedf2cf..0bc9f9228a60 100644
>> --- a/drivers/gpu/drm/drm_modeset_helper.c
>> +++ b/drivers/gpu/drm/drm_modeset_helper.c
>> @@ -20,6 +20,9 @@
>> * OF THIS SOFTWARE.
>> */
>>
>> +#include <linux/device.h>
>> +#include <linux/pm.h>
>> +
>> #include <drm/drm_atomic_helper.h>
>> #include <drm/drm_fb_helper.h>
>> #include <drm/drm_fourcc.h>
>> @@ -244,3 +247,32 @@ int drm_mode_config_helper_resume(struct
>> drm_device *dev)
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_mode_config_helper_resume);
>> +
>> +static int drm_mode_config_pm_suspend(struct device *dev)
>> +{
>> + struct drm_device *drm = dev_get_drvdata(dev);
>> +
>> + return drm_mode_config_helper_suspend(drm);
>> +}
>> +
>> +static int drm_mode_config_pm_resume(struct device *dev)
>> +{
>> + struct drm_device *drm = dev_get_drvdata(dev);
>> +
>> + return drm_mode_config_helper_resume(drm);
>> +}
>> +
>> +/**
>> + * drm_mode_config_pm_ops - Exported dev_pm_ops helper for simple
>> drivers
>> + *
>> + * This dev_pm_ops can be used for simple drivers that would
>> otherwise only call
>> + * drm_mode_config_helper_suspend / drm_mode_config_helper_resume
>> in their PM
>> + * callbacks. It is only valid if the driver's drm_device has been
>> registered as
>> + * the struct device's drvdata.
>> + *
>> + * The exported symbol must always be used with the pm_sleep_ptr()
>> macro, like
>> + * this:
>> + * .pm = pm_sleep_ptr(&drm_mode_config_pm_ops),
>> + */
>> +EXPORT_SIMPLE_DEV_PM_OPS(drm_mode_config_pm_ops,
>> + drm_mode_config_pm_suspend, drm_mode_config_pm_resume);
>> diff --git a/include/drm/drm_modeset_helper.h
>> b/include/drm/drm_modeset_helper.h
>> index 995fd981cab0..85f29637e9c1 100644
>> --- a/include/drm/drm_modeset_helper.h
>> +++ b/include/drm/drm_modeset_helper.h
>> @@ -23,6 +23,8 @@
>> #ifndef __DRM_KMS_HELPER_H__
>> #define __DRM_KMS_HELPER_H__
>
> I like that you clean up the driver, but not how it's done TBH.
>
>>
>> +#include <linux/pm.h>
>> +
>
> Half of DRM somehow incudes drm_kms_helper.h. So this include
> statements
> affects more or less everything.
>
>> struct drm_crtc;
>> struct drm_crtc_funcs;
>> struct drm_device;
>> @@ -41,4 +43,6 @@ int drm_crtc_init(struct drm_device *dev, struct
>> drm_crtc *crtc,
>> int drm_mode_config_helper_suspend(struct drm_device *dev);
>> int drm_mode_config_helper_resume(struct drm_device *dev);
>>
>> +extern const struct dev_pm_ops drm_mode_config_pm_ops;
>> +
>
> That's maybe subjective, but I don't like exporting such _funcs and
> _ops
> instances. They are like blackboxes. And they pollute the symbol
> namespace unnecessarily.
>
> I propose a solution similar to DEFINE_DRM_GEM_FOPS [1] or
> drm_module_pci_driver. [2]
>
> Define a macro in the header to create the _ops instance, such as
>
> #if defined(CONFIG_PM)
> #define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name) \
> static __ ## _name ## _suspend() { \
> call drm_mode_config_helper_suspend() \
> } \
> static __ ## _name ## _resume() { \
> call drm_mode_config_helper_resume() \
> } \
> static SIMPLE_DEV_PM_OPS(_name, __ ## _name ## _suspend, __ ## _name
> ## _resume);
> #else
> #define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name)
> #endif
>
> Drivers can then keep the instance and the include for pm.h internal
> within their _drv.c file. The small callback functions are within the
> source file as well. If CONFIG_PM has been disabled, nothing is being
> generated.
Another alternative would be to make the exported
"drm_mode_config_pm_ops" a pointer. Then it can be opaque and you
wouldn't need the <linux/pm.h> include in the header.
Your solution works too, I guess. I find it inelegant, but I won't
fight you hard on this.
Cheers,
-Paul
>
> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_gem.h#L396
> [2]
> https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_module.h#L58
>
>
>> #endif
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 N?rnberg, Germany
> (HRB 36809, AG N?rnberg)
> Gesch?ftsf?hrer: Ivo Totev