| Message ID | 20221107175106.360578-2-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Get rid of #ifdef CONFIG_PM* guards | expand |
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@crapouillou.net> > --- > 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
Hi Thomas, Le mar. 8 nov. 2022 à 13:10:49 +0100, Thomas Zimmermann <tzimmermann@suse.de> 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 <paul@crapouillou.net> >> --- >> 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
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
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@crapouillou.net> --- drivers/gpu/drm/drm_modeset_helper.c | 32 ++++++++++++++++++++++++++++ include/drm/drm_modeset_helper.h | 4 ++++ 2 files changed, 36 insertions(+)