Re: [PATCH 01/26] drm: modeset-helper: Export dev_pm_ops for simple drivers

[Paul Cercueil]

Hi Thomas,

Le mar. 8 nov. 2022 à 13:10:49 +0100, Thomas Zimmermann 
<tzimmermann@xxxxxxx> 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@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)
> #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