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 <paul@xxxxxxxxxxxxxxx> --- 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) #endifDrivers 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
