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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux