All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/7] drm: add drmm_encoder_alloc()
@ 2020-09-11 13:57 Philipp Zabel
  2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

Add an alternative to drm_encoder_init() that allocates and initializes
an encoder and registers drm_encoder_cleanup() with
drmm_add_action_or_reset().

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - call va_start() / va_end() unconditionally
---
 drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
 include/drm/drm_encoder.h     |  30 ++++++++++
 2 files changed, 108 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index e555281f43d4..f5414705f9ad 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -26,6 +26,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_managed.h>
 
 #include "drm_crtc_internal.h"
 
@@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
 	}
 }
 
-/**
- * drm_encoder_init - Init a preallocated encoder
- * @dev: drm device
- * @encoder: the encoder to init
- * @funcs: callbacks for this encoder
- * @encoder_type: user visible type of the encoder
- * @name: printf style format string for the encoder name, or NULL for default name
- *
- * Initialises a preallocated encoder. Encoder should be subclassed as part of
- * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
- * called from the driver's &drm_encoder_funcs.destroy hook.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_encoder_init(struct drm_device *dev,
-		     struct drm_encoder *encoder,
-		     const struct drm_encoder_funcs *funcs,
-		     int encoder_type, const char *name, ...)
+__printf(5, 0)
+static int __drm_encoder_init(struct drm_device *dev,
+			      struct drm_encoder *encoder,
+			      const struct drm_encoder_funcs *funcs,
+			      int encoder_type, const char *name, va_list ap)
 {
 	int ret;
 
@@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
 	encoder->encoder_type = encoder_type;
 	encoder->funcs = funcs;
 	if (name) {
-		va_list ap;
-
-		va_start(ap, name);
 		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
-		va_end(ap);
 	} else {
 		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
 					  drm_encoder_enum_list[encoder_type].name,
@@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
 
 	return ret;
 }
+
+/**
+ * drm_encoder_init - Init a preallocated encoder
+ * @dev: drm device
+ * @encoder: the encoder to init
+ * @funcs: callbacks for this encoder
+ * @encoder_type: user visible type of the encoder
+ * @name: printf style format string for the encoder name, or NULL for default name
+ *
+ * Initializes a preallocated encoder. Encoder should be subclassed as part of
+ * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
+ * called from the driver's &drm_encoder_funcs.destroy hook.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_encoder_init(struct drm_device *dev,
+		     struct drm_encoder *encoder,
+		     const struct drm_encoder_funcs *funcs,
+		     int encoder_type, const char *name, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, name);
+	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
+	va_end(ap);
+
+	return ret;
+}
 EXPORT_SYMBOL(drm_encoder_init);
 
 /**
@@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
+static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
+{
+	struct drm_encoder *encoder = ptr;
+
+	if (WARN_ON(!encoder->dev))
+		return;
+
+	drm_encoder_cleanup(encoder);
+}
+
+void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
+			   const struct drm_encoder_funcs *funcs,
+			   int encoder_type, const char *name, ...)
+{
+	void *container;
+	struct drm_encoder *encoder;
+	va_list ap;
+	int ret;
+
+	if (WARN_ON(!funcs || funcs->destroy))
+		return ERR_PTR(-EINVAL);
+
+	container = drmm_kzalloc(dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-EINVAL);
+
+	encoder = container + offset;
+
+	va_start(ap, name);
+	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
+	va_end(ap);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return container;
+}
+EXPORT_SYMBOL(__drmm_encoder_alloc);
+
 static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
 {
 	struct drm_connector *connector;
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index a60f5f1555ac..4ecad1260ff7 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev,
 		     const struct drm_encoder_funcs *funcs,
 		     int encoder_type, const char *name, ...);
 
+__printf(6, 7)
+void *__drmm_encoder_alloc(struct drm_device *dev,
+			   size_t size, size_t offset,
+			   const struct drm_encoder_funcs *funcs,
+			   int encoder_type,
+			   const char *name, ...);
+
+/**
+ * drmm_encoder_alloc - Allocate and initialize an encoder
+ * @dev: drm device
+ * @type: the type of the struct which contains struct &drm_encoder
+ * @member: the name of the &drm_encoder within @type.
+ * @funcs: callbacks for this encoder
+ * @encoder_type: user visible type of the encoder
+ * @name: printf style format string for the encoder name, or NULL for default name
+ *
+ * Allocates and initializes an encoder. Encoder should be subclassed as part of
+ * driver encoder objects. Cleanup is automatically handled through registering
+ * drm_encoder_cleanup() with drmm_add_action().
+ *
+ * The @drm_encoder_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Pointer to new encoder, or ERR_PTR on failure.
+ */
+#define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \
+	((type *)__drmm_encoder_alloc(dev, sizeof(type), \
+				      offsetof(type, member), funcs, \
+				      encoder_type, name, ##__VA_ARGS__))
+
 /**
  * drm_encoder_index - find the index of a registered encoder
  * @encoder: encoder to find index for
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-12-04  9:19   ` Laurent Pinchart
  2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

Add an alternative to drm_simple_encoder_init() that allocates and
initializes a simple encoder and registers drm_encoder_cleanup() with
drmm_add_action_or_reset().

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++
 include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 74946690aba4..3cbbfb0f9b51 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -9,6 +9,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_simple_encoder_init);
 
+static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { };
+
+void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
+				  size_t offset, int encoder_type)
+{
+	return __drmm_encoder_alloc(dev, size, offset,
+				    &drmm_simple_encoder_funcs_empty,
+				    encoder_type, NULL);
+}
+EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
+
 static enum drm_mode_status
 drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
 			       const struct drm_display_mode *mode)
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index a026375464ff..e6dbf3161c2f 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -185,4 +185,28 @@ int drm_simple_encoder_init(struct drm_device *dev,
 			    struct drm_encoder *encoder,
 			    int encoder_type);
 
+void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
+				  size_t offset, int encoder_type);
+
+/**
+ * drmm_simple_encoder_alloc - Allocate and initialize an encoder with basic
+ *                             functionality.
+ * @dev: drm device
+ * @type: the type of the struct which contains struct &drm_encoder
+ * @member: the name of the &drm_encoder within @type.
+ * @encoder_type: user visible type of the encoder
+ *
+ * Allocates and initializes an encoder that has no further functionality.
+ * Settings for possible CRTC and clones are left to their initial values.
+ * Cleanup is automatically handled through registering drm_encoder_cleanup()
+ * with drmm_add_action().
+ *
+ * Returns:
+ * Pointer to new encoder, or ERR_PTR on failure.
+ */
+#define drmm_simple_encoder_alloc(dev, type, member, encoder_type) \
+	((type *)__drmm_simple_encoder_alloc(dev, sizeof(type), \
+					     offsetof(type, member), \
+					     encoder_type))
+
 #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
  2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-12-04  9:22   ` Laurent Pinchart
  2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

Add an alternative to drm_universal_plane_init() that allocates
and initializes a plane and registers drm_plane_cleanup() with
drmm_add_action_or_reset().

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - call va_start() / va_end() unconditionally
---
 drivers/gpu/drm/drm_plane.c | 126 +++++++++++++++++++++++++++---------
 include/drm/drm_plane.h     |  42 ++++++++++++
 2 files changed, 139 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index affe1cfed009..0081f6bb76b2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -30,6 +30,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_vblank.h>
 
 #include "drm_crtc_internal.h"
@@ -152,31 +153,16 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 	return 0;
 }
 
-/**
- * drm_universal_plane_init - Initialize a new universal plane object
- * @dev: DRM device
- * @plane: plane object to init
- * @possible_crtcs: bitmask of possible CRTCs
- * @funcs: callbacks for the new plane
- * @formats: array of supported formats (DRM_FORMAT\_\*)
- * @format_count: number of elements in @formats
- * @format_modifiers: array of struct drm_format modifiers terminated by
- *                    DRM_FORMAT_MOD_INVALID
- * @type: type of plane (overlay, primary, cursor)
- * @name: printf style format string for the plane name, or NULL for default name
- *
- * Initializes a plane object of type @type.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
-			     uint32_t possible_crtcs,
-			     const struct drm_plane_funcs *funcs,
-			     const uint32_t *formats, unsigned int format_count,
-			     const uint64_t *format_modifiers,
-			     enum drm_plane_type type,
-			     const char *name, ...)
+__printf(9, 0)
+static int __drm_universal_plane_init(struct drm_device *dev,
+				      struct drm_plane *plane,
+				      uint32_t possible_crtcs,
+				      const struct drm_plane_funcs *funcs,
+				      const uint32_t *formats,
+				      unsigned int format_count,
+				      const uint64_t *format_modifiers,
+				      enum drm_plane_type type,
+				      const char *name, va_list ap)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	unsigned int format_modifier_count = 0;
@@ -237,11 +223,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	}
 
 	if (name) {
-		va_list ap;
-
-		va_start(ap, name);
 		plane->name = kvasprintf(GFP_KERNEL, name, ap);
-		va_end(ap);
 	} else {
 		plane->name = kasprintf(GFP_KERNEL, "plane-%d",
 					drm_num_planes(dev));
@@ -286,8 +268,94 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	return 0;
 }
+
+/**
+ * drm_universal_plane_init - Initialize a new universal plane object
+ * @dev: DRM device
+ * @plane: plane object to init
+ * @possible_crtcs: bitmask of possible CRTCs
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ *                    DRM_FORMAT_MOD_INVALID
+ * @type: type of plane (overlay, primary, cursor)
+ * @name: printf style format string for the plane name, or NULL for default name
+ *
+ * Initializes a plane object of type @type.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
+			     uint32_t possible_crtcs,
+			     const struct drm_plane_funcs *funcs,
+			     const uint32_t *formats, unsigned int format_count,
+			     const uint64_t *format_modifiers,
+			     enum drm_plane_type type,
+			     const char *name, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, name);
+	ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+					 formats, format_count, format_modifiers,
+					 type, name, ap);
+	va_end(ap);
+	return ret;
+}
 EXPORT_SYMBOL(drm_universal_plane_init);
 
+static void drmm_universal_plane_alloc_release(struct drm_device *dev, void *ptr)
+{
+	struct drm_plane *plane = ptr;
+
+	if (WARN_ON(!plane->dev))
+		return;
+
+	drm_plane_cleanup(plane);
+}
+
+void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
+				   size_t offset, uint32_t possible_crtcs,
+				   const struct drm_plane_funcs *funcs,
+				   const uint32_t *formats, unsigned int format_count,
+				   const uint64_t *format_modifiers,
+				   enum drm_plane_type type,
+				   const char *name, ...)
+{
+	void *container;
+	struct drm_plane *plane;
+	va_list ap;
+	int ret;
+
+	if (!funcs || funcs->destroy)
+		return ERR_PTR(-EINVAL);
+
+	container = drmm_kzalloc(dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-ENOMEM);
+
+	plane = container + offset;
+
+	va_start(ap, name);
+	ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+					 formats, format_count, format_modifiers,
+					 type, name, ap);
+	va_end(ap);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = drmm_add_action_or_reset(dev, drmm_universal_plane_alloc_release,
+				       plane);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return container;
+}
+EXPORT_SYMBOL(__drmm_universal_plane_alloc);
+
 int drm_plane_register_all(struct drm_device *dev)
 {
 	unsigned int num_planes = 0;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3f396d94afe4..82bd63710a39 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -746,6 +746,48 @@ int drm_plane_init(struct drm_device *dev,
 		   bool is_primary);
 void drm_plane_cleanup(struct drm_plane *plane);
 
+__printf(10, 11)
+void *__drmm_universal_plane_alloc(struct drm_device *dev,
+				   size_t size, size_t offset,
+				   uint32_t possible_crtcs,
+				   const struct drm_plane_funcs *funcs,
+				   const uint32_t *formats,
+				   unsigned int format_count,
+				   const uint64_t *format_modifiers,
+				   enum drm_plane_type plane_type,
+				   const char *name, ...);
+
+/**
+ * drmm_universal_plane_alloc - Allocate and initialize an universal plane object
+ * @dev: DRM device
+ * @type: the type of the struct which contains struct &drm_plane
+ * @member: the name of the &drm_plane within @type
+ * @possible_crtcs: bitmask of possible CRTCs
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ *                    DRM_FORMAT_MOD_INVALID
+ * @plane_type: type of plane (overlay, primary, cursor)
+ * @name: printf style format string for the plane name, or NULL for default name
+ *
+ * Allocates and initializes a plane object of type @type. Cleanup is
+ * automatically handled through registering drm_plane_cleanup() with
+ * drmm_add_action().
+ *
+ * The @drm_plane_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Pointer to new plane, or ERR_PTR on failure.
+ */
+#define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
+				   format_count, format_modifiers, plane_type, name, ...) \
+	((type *)__drmm_universal_plane_alloc(dev, sizeof(type), \
+					      offsetof(type, member), \
+					      possible_crtcs, funcs, formats, \
+					      format_count, format_modifiers, \
+					      plane_type, name, ##__VA_ARGS__))
+
 /**
  * drm_plane_index - find the index of a registered plane
  * @plane: plane to find index for
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
  2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
  2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-12-04  9:23   ` Laurent Pinchart
  2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

Add an alternative to drm_crtc_init_with_planes() that allocates
and initializes a crtc and registers drm_crtc_cleanup() with
drmm_add_action_or_reset().

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - call va_start() / va_end() unconditionally
---
 drivers/gpu/drm/drm_crtc.c | 116 ++++++++++++++++++++++++++++---------
 include/drm/drm_crtc.h     |  33 +++++++++++
 2 files changed, 121 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index aecdd7ea26dc..b9e7c11a76b7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -38,6 +38,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_lock.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
@@ -231,30 +232,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
  * 	Setting MODE_ID to 0 will release reserved resources for the CRTC.
  */
 
-/**
- * drm_crtc_init_with_planes - Initialise a new CRTC object with
- *    specified primary and cursor planes.
- * @dev: DRM device
- * @crtc: CRTC object to init
- * @primary: Primary plane for CRTC
- * @cursor: Cursor plane for CRTC
- * @funcs: callbacks for the new CRTC
- * @name: printf style format string for the CRTC name, or NULL for default name
- *
- * Inits a new object created as base part of a driver crtc object. Drivers
- * should use this function instead of drm_crtc_init(), which is only provided
- * for backwards compatibility with drivers which do not yet support universal
- * planes). For really simple hardware which has only 1 plane look at
- * drm_simple_display_pipe_init() instead.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
-			      struct drm_plane *primary,
-			      struct drm_plane *cursor,
-			      const struct drm_crtc_funcs *funcs,
-			      const char *name, ...)
+__printf(6, 0)
+static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
+				       struct drm_plane *primary,
+				       struct drm_plane *cursor,
+				       const struct drm_crtc_funcs *funcs,
+				       const char *name, va_list ap)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret;
@@ -282,11 +265,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 		return ret;
 
 	if (name) {
-		va_list ap;
-
-		va_start(ap, name);
 		crtc->name = kvasprintf(GFP_KERNEL, name, ap);
-		va_end(ap);
 	} else {
 		crtc->name = kasprintf(GFP_KERNEL, "crtc-%d",
 				       drm_num_crtcs(dev));
@@ -330,8 +309,89 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 
 	return 0;
 }
+
+/**
+ * drm_crtc_init_with_planes - Initialise a new CRTC object with
+ *    specified primary and cursor planes.
+ * @dev: DRM device
+ * @crtc: CRTC object to init
+ * @primary: Primary plane for CRTC
+ * @cursor: Cursor plane for CRTC
+ * @funcs: callbacks for the new CRTC
+ * @name: printf style format string for the CRTC name, or NULL for default name
+ *
+ * Inits a new object created as base part of a driver crtc object. Drivers
+ * should use this function instead of drm_crtc_init(), which is only provided
+ * for backwards compatibility with drivers which do not yet support universal
+ * planes). For really simple hardware which has only 1 plane look at
+ * drm_simple_display_pipe_init() instead.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
+			      struct drm_plane *primary,
+			      struct drm_plane *cursor,
+			      const struct drm_crtc_funcs *funcs,
+			      const char *name, ...)
+{
+	va_list ap;
+	int ret;
+
+	va_start(ap, name);
+	ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
+					  name, ap);
+	va_end(ap);
+
+	return ret;
+}
 EXPORT_SYMBOL(drm_crtc_init_with_planes);
 
+static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev,
+						void *ptr)
+{
+	struct drm_crtc *crtc = ptr;
+
+	drm_crtc_cleanup(crtc);
+}
+
+void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
+				    size_t size, size_t offset,
+				    struct drm_plane *primary,
+				    struct drm_plane *cursor,
+				    const struct drm_crtc_funcs *funcs,
+				    const char *name, ...)
+{
+	void *container;
+	struct drm_crtc *crtc;
+	va_list ap;
+	int ret;
+
+	if (!funcs || funcs->destroy)
+		return ERR_PTR(-EINVAL);
+
+	container = drmm_kzalloc(dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-ENOMEM);
+
+	crtc = container + offset;
+
+	va_start(ap, name);
+	ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
+					  name, ap);
+	va_end(ap);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = drmm_add_action_or_reset(dev, drmm_crtc_alloc_with_planes_cleanup,
+				       crtc);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return container;
+}
+EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes);
+
 /**
  * drm_crtc_cleanup - Clean up the core crtc usage
  * @crtc: CRTC to cleanup
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 59b51a09cae6..b71c0a3d4126 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1210,6 +1210,39 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
 			      const char *name, ...);
 void drm_crtc_cleanup(struct drm_crtc *crtc);
 
+__printf(7, 8)
+void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
+				    size_t size, size_t offset,
+				    struct drm_plane *primary,
+				    struct drm_plane *cursor,
+				    const struct drm_crtc_funcs *funcs,
+				    const char *name, ...);
+
+/**
+ * drm_crtc_alloc_with_planes - Allocate and initialize a new CRTC object with
+ *    specified primary and cursor planes.
+ * @dev: DRM device
+ * @type: the type of the struct which contains struct &drm_crtc
+ * @member: the name of the &drm_crtc within @type.
+ * @primary: Primary plane for CRTC
+ * @cursor: Cursor plane for CRTC
+ * @funcs: callbacks for the new CRTC
+ * @name: printf style format string for the CRTC name, or NULL for default name
+ *
+ * Allocates and initializes a new crtc object. Cleanup is automatically
+ * handled through registering drmm_crtc_cleanup() with drmm_add_action().
+ *
+ * The @drm_crtc_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Pointer to new crtc, or ERR_PTR on failure.
+ */
+#define drmm_crtc_alloc_with_planes(dev, type, member, primary, cursor, funcs, name, ...) \
+	((type *)__drmm_crtc_alloc_with_planes(dev, sizeof(type), \
+					       offsetof(type, member), \
+					       primary, cursor, funcs, \
+					       name, ##__VA_ARGS__))
+
 /**
  * drm_crtc_index - find the index of a registered CRTC
  * @crtc: CRTC to find index for
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
                   ` (2 preceding siblings ...)
  2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-09-16  9:08   ` Daniel Vetter
  2020-09-11 13:57 ` [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc() Philipp Zabel
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

This allows to drop the custom drm_encoder_cleanup() actions.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_simple_encoder_init() users.

This and the following patches depend on the drm/imx conversion to use managed
resources [1].

[1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      | 19 ++++---------------
 drivers/gpu/drm/imx/imx-ldb.c          | 20 ++++----------------
 drivers/gpu/drm/imx/imx-tve.c          | 22 ++++------------------
 drivers/gpu/drm/imx/parallel-display.c | 22 ++++------------------
 4 files changed, 16 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 16be8bd92653..87428fb23d9f 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -188,13 +188,6 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
 
-static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data)
-{
-	struct drm_encoder *encoder = data;
-
-	drm_encoder_cleanup(encoder);
-}
-
 static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 			    void *data)
 {
@@ -203,9 +196,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder;
 	int ret;
 
-	hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL);
-	if (!hdmi_encoder)
-		return -ENOMEM;
+	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct imx_hdmi_encoder,
+						 encoder, DRM_MODE_ENCODER_TMDS);
+	if (IS_ERR(hdmi_encoder))
+		return PTR_ERR(hdmi_encoder);
 
 	hdmi_encoder->hdmi = dev_get_drvdata(dev);
 	encoder = &hdmi_encoder->encoder;
@@ -215,11 +209,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 		return ret;
 
 	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
-
-	ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder);
-	if (ret)
-		return ret;
 
 	return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
 }
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index d4beb58f509d..dbfe39e2f7f6 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -414,13 +414,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
 	return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
 }
 
-static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data)
-{
-	struct drm_encoder *encoder = data;
-
-	drm_encoder_cleanup(encoder);
-}
-
 static int imx_ldb_register(struct drm_device *drm,
 	struct imx_ldb_channel *imx_ldb_ch)
 {
@@ -430,20 +423,15 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct drm_encoder *encoder;
 	int ret;
 
-	ldb_encoder = drmm_kzalloc(drm, sizeof(*ldb_encoder), GFP_KERNEL);
-	if (!ldb_encoder)
-		return -ENOMEM;
+	ldb_encoder = drmm_simple_encoder_alloc(drm, struct imx_ldb_encoder,
+						encoder, DRM_MODE_ENCODER_LVDS);
+	if (IS_ERR(ldb_encoder))
+		return PTR_ERR(ldb_encoder);
 
 	ldb_encoder->channel = imx_ldb_ch;
 	connector = &ldb_encoder->connector;
 	encoder = &ldb_encoder->encoder;
 
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
-
-	ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder);
-	if (ret)
-		return ret;
-
 	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index bac025eafa1f..0746f0b425df 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -433,13 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
 	return 0;
 }
 
-static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr)
-{
-	struct drm_encoder *encoder = ptr;
-
-	drm_encoder_cleanup(encoder);
-}
-
 static void imx_tve_disable_regulator(void *data)
 {
 	struct imx_tve *tve = data;
@@ -498,22 +491,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	encoder_type = tve->mode == TVE_MODE_VGA ?
 		       DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
 
-	tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL);
-	if (!tvee)
-		return -ENOMEM;
+	tvee = drmm_simple_encoder_alloc(drm, struct imx_tve_encoder, encoder,
+					 encoder_type);
+	if (IS_ERR(tvee))
+		return PTR_ERR(tvee);
 
 	tvee->tve = tve;
 	encoder = &tvee->encoder;
 	connector = &tvee->connector;
 
-	ret = drm_simple_encoder_init(drm, encoder, encoder_type);
-	if (ret)
-		return ret;
-
-	ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder);
-	if (ret)
-		return ret;
-
 	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 50b5b89c2db2..9b1ec7e73c30 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -258,13 +258,6 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
 	.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
 };
 
-static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr)
-{
-	struct drm_encoder *encoder = ptr;
-
-	drm_encoder_cleanup(encoder);
-}
-
 static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
@@ -275,23 +268,16 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	struct drm_bridge *bridge;
 	int ret;
 
-	imxpd_encoder = drmm_kzalloc(drm, sizeof(*imxpd_encoder), GFP_KERNEL);
-	if (!imxpd_encoder)
-		return -ENOMEM;
+	imxpd_encoder = drmm_simple_encoder_alloc(drm, struct imx_parallel_display_encoder,
+						  encoder, DRM_MODE_ENCODER_NONE);
+	if (IS_ERR(imxpd_encoder))
+		return PTR_ERR(imxpd_encoder);
 
 	imxpd_encoder->pd = imxpd;
 	connector = &imxpd_encoder->connector;
 	encoder = &imxpd_encoder->encoder;
 	bridge = &imxpd_encoder->bridge;
 
-	ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
-	if (ret)
-		return ret;
-
-	ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
-	if (ret)
-		return ret;
-
 	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
 	if (ret)
 		return ret;
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
                   ` (3 preceding siblings ...)
  2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-09-11 13:57 ` [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes() Philipp Zabel
  2020-12-04  9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
  6 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

This allows to drop the custom drm_plane_cleanup action.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_universal_plane_init() user.
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 34 ++++++++-----------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 38b959aa3564..075508051b5f 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -815,13 +815,6 @@ int ipu_planes_assign_pre(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(ipu_planes_assign_pre);
 
-static void ipu_plane_cleanup(struct drm_device *dev, void *data)
-{
-	struct ipu_plane *ipu_plane = data;
-
-	drm_plane_cleanup(&ipu_plane->base);
-}
-
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 				 int dma, int dp, unsigned int possible_crtcs,
 				 enum drm_plane_type type)
@@ -834,10 +827,15 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 	DRM_DEBUG_KMS("channel %d, dp flow %d, possible_crtcs=0x%x\n",
 		      dma, dp, possible_crtcs);
 
-	ipu_plane = drmm_kzalloc(dev, sizeof(*ipu_plane), GFP_KERNEL);
-	if (!ipu_plane) {
-		DRM_ERROR("failed to allocate plane\n");
-		return ERR_PTR(-ENOMEM);
+	ipu_plane = drmm_universal_plane_alloc(dev, struct ipu_plane, base,
+					       possible_crtcs, &ipu_plane_funcs,
+					       ipu_plane_formats,
+					       ARRAY_SIZE(ipu_plane_formats),
+					       modifiers, type, NULL);
+	if (IS_ERR(ipu_plane)) {
+		DRM_ERROR("failed to allocate and initialize %s plane\n",
+			  zpos ? "overlay" : "primary");
+		return ipu_plane;
 	}
 
 	ipu_plane->ipu = ipu;
@@ -847,20 +845,6 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 	if (ipu_prg_present(ipu))
 		modifiers = pre_format_modifiers;
 
-	ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
-				       &ipu_plane_funcs, ipu_plane_formats,
-				       ARRAY_SIZE(ipu_plane_formats),
-				       modifiers, type, NULL);
-	if (ret) {
-		DRM_ERROR("failed to initialize %s plane\n",
-			  zpos ? "overlay" : "primary");
-		return ERR_PTR(ret);
-	}
-
-	ret = drmm_add_action_or_reset(dev, ipu_plane_cleanup, ipu_plane);
-	if (ret)
-		return ERR_PTR(ret);
-
 	drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
 
 	if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG)
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
                   ` (4 preceding siblings ...)
  2020-09-11 13:57 ` [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
  2020-12-04  9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
  6 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
  To: dri-devel; +Cc: kernel

Merge ipu_crtc_init into ipu_drm_bind and use
drmm_crtc_alloc_with_planes().
This allows to drop the custom drm_crtc_cleanup action.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_crtc_init_with_planes() user.
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 71 ++++++++++++--------------------
 1 file changed, 26 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index f284e4ea6d5f..982ef79653ae 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -349,47 +349,43 @@ static int ipu_get_resources(struct drm_device *dev, struct ipu_crtc *ipu_crtc,
 	return 0;
 }
 
-static void ipu_crtc_cleanup(struct drm_device *drm, void *ptr)
-{
-	struct drm_crtc *crtc = ptr;
-
-	drm_crtc_cleanup(crtc);
-}
-
-static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
-	struct ipu_client_platformdata *pdata, struct drm_device *drm)
+static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 {
-	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
-	struct drm_crtc *crtc = &ipu_crtc->base;
+	struct ipu_client_platformdata *pdata = dev->platform_data;
+	struct ipu_soc *ipu = dev_get_drvdata(dev->parent);
+	struct drm_device *drm = data;
+	struct ipu_plane *primary_plane;
+	struct ipu_crtc *ipu_crtc;
+	struct drm_crtc *crtc;
 	int dp = -EINVAL;
 	int ret;
 
-	ret = ipu_get_resources(drm, ipu_crtc, pdata);
-	if (ret) {
-		dev_err(ipu_crtc->dev, "getting resources failed with %d.\n",
-				ret);
-		return ret;
-	}
-
 	if (pdata->dp >= 0)
 		dp = IPU_DP_FLOW_SYNC_BG;
-	ipu_crtc->plane[0] = ipu_plane_init(drm, ipu, pdata->dma[0], dp, 0,
-					    DRM_PLANE_TYPE_PRIMARY);
-	if (IS_ERR(ipu_crtc->plane[0])) {
-		ret = PTR_ERR(ipu_crtc->plane[0]);
-		return ret;
-	}
+	primary_plane = ipu_plane_init(drm, ipu, pdata->dma[0], dp, 0,
+				       DRM_PLANE_TYPE_PRIMARY);
+	if (IS_ERR(primary_plane))
+		return PTR_ERR(primary_plane);
 
+	ipu_crtc = drmm_crtc_alloc_with_planes(drm, struct ipu_crtc, base,
+					       &primary_plane->base, NULL,
+					       &ipu_crtc_funcs, NULL);
+	if (IS_ERR(ipu_crtc))
+		return PTR_ERR(ipu_crtc);
+
+	ipu_crtc->dev = dev;
+	ipu_crtc->plane[0] = primary_plane;
+
+	crtc = &ipu_crtc->base;
 	crtc->port = pdata->of_node;
 	drm_crtc_helper_add(crtc, &ipu_helper_funcs);
-	ret = drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base,
-					NULL, &ipu_crtc_funcs, NULL);
-	if (ret)
-		return ret;
 
-	ret = drmm_add_action_or_reset(drm, ipu_crtc_cleanup, crtc);
-	if (ret)
+	ret = ipu_get_resources(drm, ipu_crtc, pdata);
+	if (ret) {
+		dev_err(ipu_crtc->dev, "getting resources failed with %d.\n",
+			ret);
 		return ret;
+	}
 
 	/* If this crtc is using the DP, add an overlay plane */
 	if (pdata->dp >= 0 && pdata->dma[1] > 0) {
@@ -414,21 +410,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 	return 0;
 }
 
-static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
-{
-	struct ipu_client_platformdata *pdata = dev->platform_data;
-	struct drm_device *drm = data;
-	struct ipu_crtc *ipu_crtc;
-
-	ipu_crtc = drmm_kzalloc(drm, sizeof(*ipu_crtc), GFP_KERNEL);
-	if (!ipu_crtc)
-		return -ENOMEM;
-
-	ipu_crtc->dev = dev;
-
-	return ipu_crtc_init(ipu_crtc, pdata, drm);
-}
-
 static const struct component_ops ipu_crtc_ops = {
 	.bind = ipu_drm_bind,
 };
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
  2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-16  9:08   ` Daniel Vetter
  2020-09-16 10:22     ` Philipp Zabel
  0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2020-09-16  9:08 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
> This allows to drop the custom drm_encoder_cleanup() actions.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> New in v3, example conversion of drm_simple_encoder_init() users.
> 
> This and the following patches depend on the drm/imx conversion to use managed
> resources [1].
> 
> [1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
> ---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 19 ++++---------------
>  drivers/gpu/drm/imx/imx-ldb.c          | 20 ++++----------------
>  drivers/gpu/drm/imx/imx-tve.c          | 22 ++++------------------
>  drivers/gpu/drm/imx/parallel-display.c | 22 ++++------------------
>  4 files changed, 16 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index 16be8bd92653..87428fb23d9f 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -188,13 +188,6 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
>  
> -static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> -	struct drm_encoder *encoder = data;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  			    void *data)
>  {
> @@ -203,9 +196,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder;
>  	int ret;
>  
> -	hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL);

Where does this apply to? Neither upstream nor linux-next seems to have
the drmm_ conversion for imx already applied, and that's kinda the juicy
part I'd like to look at a bit. The patches here are just mechanical
conversion. Can you pls include the drmm_ conversion too (maybe even
squash these patches here in, I think that would be more readable)?

Or am I looking at the wrong tree?

Anyway I think it looks all neat.
-Daniel

> -	if (!hdmi_encoder)
> -		return -ENOMEM;
> +	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct imx_hdmi_encoder,
> +						 encoder, DRM_MODE_ENCODER_TMDS);
> +	if (IS_ERR(hdmi_encoder))
> +		return PTR_ERR(hdmi_encoder);
>  
>  	hdmi_encoder->hdmi = dev_get_drvdata(dev);
>  	encoder = &hdmi_encoder->encoder;
> @@ -215,11 +209,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  		return ret;
>  
>  	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> -
> -	ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
>  
>  	return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
>  }
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index d4beb58f509d..dbfe39e2f7f6 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -414,13 +414,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
>  	return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
>  }
>  
> -static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> -	struct drm_encoder *encoder = data;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int imx_ldb_register(struct drm_device *drm,
>  	struct imx_ldb_channel *imx_ldb_ch)
>  {
> @@ -430,20 +423,15 @@ static int imx_ldb_register(struct drm_device *drm,
>  	struct drm_encoder *encoder;
>  	int ret;
>  
> -	ldb_encoder = drmm_kzalloc(drm, sizeof(*ldb_encoder), GFP_KERNEL);
> -	if (!ldb_encoder)
> -		return -ENOMEM;
> +	ldb_encoder = drmm_simple_encoder_alloc(drm, struct imx_ldb_encoder,
> +						encoder, DRM_MODE_ENCODER_LVDS);
> +	if (IS_ERR(ldb_encoder))
> +		return PTR_ERR(ldb_encoder);
>  
>  	ldb_encoder->channel = imx_ldb_ch;
>  	connector = &ldb_encoder->connector;
>  	encoder = &ldb_encoder->encoder;
>  
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
> -
> -	ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index bac025eafa1f..0746f0b425df 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -433,13 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
>  	return 0;
>  }
>  
> -static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> -	struct drm_encoder *encoder = ptr;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static void imx_tve_disable_regulator(void *data)
>  {
>  	struct imx_tve *tve = data;
> @@ -498,22 +491,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  	encoder_type = tve->mode == TVE_MODE_VGA ?
>  		       DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
>  
> -	tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL);
> -	if (!tvee)
> -		return -ENOMEM;
> +	tvee = drmm_simple_encoder_alloc(drm, struct imx_tve_encoder, encoder,
> +					 encoder_type);
> +	if (IS_ERR(tvee))
> +		return PTR_ERR(tvee);
>  
>  	tvee->tve = tve;
>  	encoder = &tvee->encoder;
>  	connector = &tvee->connector;
>  
> -	ret = drm_simple_encoder_init(drm, encoder, encoder_type);
> -	if (ret)
> -		return ret;
> -
> -	ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 50b5b89c2db2..9b1ec7e73c30 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,13 +258,6 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
>  	.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
>  };
>  
> -static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> -	struct drm_encoder *encoder = ptr;
> -
> -	drm_encoder_cleanup(encoder);
> -}
> -
>  static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct drm_device *drm = data;
> @@ -275,23 +268,16 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  	struct drm_bridge *bridge;
>  	int ret;
>  
> -	imxpd_encoder = drmm_kzalloc(drm, sizeof(*imxpd_encoder), GFP_KERNEL);
> -	if (!imxpd_encoder)
> -		return -ENOMEM;
> +	imxpd_encoder = drmm_simple_encoder_alloc(drm, struct imx_parallel_display_encoder,
> +						  encoder, DRM_MODE_ENCODER_NONE);
> +	if (IS_ERR(imxpd_encoder))
> +		return PTR_ERR(imxpd_encoder);
>  
>  	imxpd_encoder->pd = imxpd;
>  	connector = &imxpd_encoder->connector;
>  	encoder = &imxpd_encoder->encoder;
>  	bridge = &imxpd_encoder->bridge;
>  
> -	ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
> -	if (ret)
> -		return ret;
> -
> -	ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
> -	if (ret)
> -		return ret;
> -
>  	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
>  	if (ret)
>  		return ret;
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
  2020-09-16  9:08   ` Daniel Vetter
@ 2020-09-16 10:22     ` Philipp Zabel
  0 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-16 10:22 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: kernel, dri-devel

Hi Daniel,

On Wed, 2020-09-16 at 11:08 +0200, Daniel Vetter wrote:
> On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
> > This allows to drop the custom drm_encoder_cleanup() actions.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > New in v3, example conversion of drm_simple_encoder_init() users.
> > 
> > This and the following patches depend on the drm/imx conversion to use managed
> > resources [1].
> > 
> > [1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
[...]
> Where does this apply to? Neither upstream nor linux-next seems to have
> the drmm_ conversion for imx already applied, and that's kinda the juicy
> part I'd like to look at a bit.

Oh, ok. I did (re)send the basic drmm_ conversion separately at [1],
mentioned above.

> The patches here are just mechanical conversion. Can you pls include
> the drmm_ conversion too (maybe even squash these patches here in, I
> think that would be more readable)?

I'll squash and resend as a single series.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
  2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
                   ` (5 preceding siblings ...)
  2020-09-11 13:57 ` [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-12-04  9:17 ` Laurent Pinchart
  2020-12-04 10:12   ` Philipp Zabel
  6 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04  9:17 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

Thank you for the patch.

On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_encoder_init() that allocates and initializes
> an encoder and registers drm_encoder_cleanup() with
> drmm_add_action_or_reset().
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
>  - call va_start() / va_end() unconditionally
> ---
>  drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
>  include/drm/drm_encoder.h     |  30 ++++++++++
>  2 files changed, 108 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..f5414705f9ad 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -26,6 +26,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_managed.h>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
>  	}
>  }
>  
> -/**
> - * drm_encoder_init - Init a preallocated encoder
> - * @dev: drm device
> - * @encoder: the encoder to init
> - * @funcs: callbacks for this encoder
> - * @encoder_type: user visible type of the encoder
> - * @name: printf style format string for the encoder name, or NULL for default name
> - *
> - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> - * called from the driver's &drm_encoder_funcs.destroy hook.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_encoder_init(struct drm_device *dev,
> -		     struct drm_encoder *encoder,
> -		     const struct drm_encoder_funcs *funcs,
> -		     int encoder_type, const char *name, ...)
> +__printf(5, 0)
> +static int __drm_encoder_init(struct drm_device *dev,
> +			      struct drm_encoder *encoder,
> +			      const struct drm_encoder_funcs *funcs,
> +			      int encoder_type, const char *name, va_list ap)
>  {
>  	int ret;
>  
> @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
>  	encoder->encoder_type = encoder_type;
>  	encoder->funcs = funcs;
>  	if (name) {
> -		va_list ap;
> -
> -		va_start(ap, name);
>  		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> -		va_end(ap);
>  	} else {
>  		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
>  					  drm_encoder_enum_list[encoder_type].name,
> @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
>  
>  	return ret;
>  }
> +
> +/**
> + * drm_encoder_init - Init a preallocated encoder
> + * @dev: drm device
> + * @encoder: the encoder to init
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for default name
> + *
> + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> + * called from the driver's &drm_encoder_funcs.destroy hook.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_encoder_init(struct drm_device *dev,
> +		     struct drm_encoder *encoder,
> +		     const struct drm_encoder_funcs *funcs,
> +		     int encoder_type, const char *name, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	va_start(ap, name);
> +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> +	va_end(ap);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL(drm_encoder_init);
>  
>  /**
> @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  }
>  EXPORT_SYMBOL(drm_encoder_cleanup);
>  
> +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> +{
> +	struct drm_encoder *encoder = ptr;
> +
> +	if (WARN_ON(!encoder->dev))
> +		return;
> +
> +	drm_encoder_cleanup(encoder);
> +}
> +
> +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> +			   const struct drm_encoder_funcs *funcs,
> +			   int encoder_type, const char *name, ...)
> +{
> +	void *container;
> +	struct drm_encoder *encoder;
> +	va_list ap;
> +	int ret;
> +
> +	if (WARN_ON(!funcs || funcs->destroy))
> +		return ERR_PTR(-EINVAL);
> +
> +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!container)
> +		return ERR_PTR(-EINVAL);
> +
> +	encoder = container + offset;
> +
> +	va_start(ap, name);
> +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> +	va_end(ap);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> +	if (ret)
> +		return ERR_PTR(ret);

Given that drmm_encoder_alloc_release() will be called right before the
kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
to replace drmm_kzalloc() with kzalloc() and add a kfree() in
drmm_encoder_alloc_release() ? That will save one context allocation.

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	return container;
> +}
> +EXPORT_SYMBOL(__drmm_encoder_alloc);
> +
>  static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
>  {
>  	struct drm_connector *connector;
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index a60f5f1555ac..4ecad1260ff7 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev,
>  		     const struct drm_encoder_funcs *funcs,
>  		     int encoder_type, const char *name, ...);
>  
> +__printf(6, 7)
> +void *__drmm_encoder_alloc(struct drm_device *dev,
> +			   size_t size, size_t offset,
> +			   const struct drm_encoder_funcs *funcs,
> +			   int encoder_type,
> +			   const char *name, ...);
> +
> +/**
> + * drmm_encoder_alloc - Allocate and initialize an encoder
> + * @dev: drm device
> + * @type: the type of the struct which contains struct &drm_encoder
> + * @member: the name of the &drm_encoder within @type.
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for default name
> + *
> + * Allocates and initializes an encoder. Encoder should be subclassed as part of
> + * driver encoder objects. Cleanup is automatically handled through registering
> + * drm_encoder_cleanup() with drmm_add_action().
> + *
> + * The @drm_encoder_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new encoder, or ERR_PTR on failure.
> + */
> +#define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \
> +	((type *)__drmm_encoder_alloc(dev, sizeof(type), \
> +				      offsetof(type, member), funcs, \
> +				      encoder_type, name, ##__VA_ARGS__))
> +
>  /**
>   * drm_encoder_index - find the index of a registered encoder
>   * @encoder: encoder to find index for

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
  2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-12-04  9:19   ` Laurent Pinchart
  2020-12-04 10:13     ` Philipp Zabel
  0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04  9:19 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

Thank you for the patch.

On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_simple_encoder_init() that allocates and
> initializes a simple encoder and registers drm_encoder_cleanup() with
> drmm_add_action_or_reset().
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++
>  include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 74946690aba4..3cbbfb0f9b51 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -9,6 +9,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
> @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_simple_encoder_init);
>  
> +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { };
> +
> +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
> +				  size_t offset, int encoder_type)
> +{
> +	return __drmm_encoder_alloc(dev, size, offset,
> +				    &drmm_simple_encoder_funcs_empty,
> +				    encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);

Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
in the core (if we don't already) and use drmm_encoder_alloc() directly
in drivers ?

> +
>  static enum drm_mode_status
>  drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
>  			       const struct drm_display_mode *mode)
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index a026375464ff..e6dbf3161c2f 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -185,4 +185,28 @@ int drm_simple_encoder_init(struct drm_device *dev,
>  			    struct drm_encoder *encoder,
>  			    int encoder_type);
>  
> +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
> +				  size_t offset, int encoder_type);
> +
> +/**
> + * drmm_simple_encoder_alloc - Allocate and initialize an encoder with basic
> + *                             functionality.
> + * @dev: drm device
> + * @type: the type of the struct which contains struct &drm_encoder
> + * @member: the name of the &drm_encoder within @type.
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initializes an encoder that has no further functionality.
> + * Settings for possible CRTC and clones are left to their initial values.
> + * Cleanup is automatically handled through registering drm_encoder_cleanup()
> + * with drmm_add_action().
> + *
> + * Returns:
> + * Pointer to new encoder, or ERR_PTR on failure.
> + */
> +#define drmm_simple_encoder_alloc(dev, type, member, encoder_type) \
> +	((type *)__drmm_simple_encoder_alloc(dev, sizeof(type), \
> +					     offsetof(type, member), \
> +					     encoder_type))
> +
>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc()
  2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
@ 2020-12-04  9:22   ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04  9:22 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

Thank you for the patch.

On Fri, Sep 11, 2020 at 03:57:20PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_universal_plane_init() that allocates
> and initializes a plane and registers drm_plane_cleanup() with
> drmm_add_action_or_reset().
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
>  - call va_start() / va_end() unconditionally
> ---
>  drivers/gpu/drm/drm_plane.c | 126 +++++++++++++++++++++++++++---------
>  include/drm/drm_plane.h     |  42 ++++++++++++
>  2 files changed, 139 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index affe1cfed009..0081f6bb76b2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -30,6 +30,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_vblank.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -152,31 +153,16 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
>  	return 0;
>  }
>  
> -/**
> - * drm_universal_plane_init - Initialize a new universal plane object
> - * @dev: DRM device
> - * @plane: plane object to init
> - * @possible_crtcs: bitmask of possible CRTCs
> - * @funcs: callbacks for the new plane
> - * @formats: array of supported formats (DRM_FORMAT\_\*)
> - * @format_count: number of elements in @formats
> - * @format_modifiers: array of struct drm_format modifiers terminated by
> - *                    DRM_FORMAT_MOD_INVALID
> - * @type: type of plane (overlay, primary, cursor)
> - * @name: printf style format string for the plane name, or NULL for default name
> - *
> - * Initializes a plane object of type @type.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> -			     uint32_t possible_crtcs,
> -			     const struct drm_plane_funcs *funcs,
> -			     const uint32_t *formats, unsigned int format_count,
> -			     const uint64_t *format_modifiers,
> -			     enum drm_plane_type type,
> -			     const char *name, ...)
> +__printf(9, 0)
> +static int __drm_universal_plane_init(struct drm_device *dev,
> +				      struct drm_plane *plane,
> +				      uint32_t possible_crtcs,
> +				      const struct drm_plane_funcs *funcs,
> +				      const uint32_t *formats,
> +				      unsigned int format_count,
> +				      const uint64_t *format_modifiers,
> +				      enum drm_plane_type type,
> +				      const char *name, va_list ap)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	unsigned int format_modifier_count = 0;
> @@ -237,11 +223,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	}
>  
>  	if (name) {
> -		va_list ap;
> -
> -		va_start(ap, name);
>  		plane->name = kvasprintf(GFP_KERNEL, name, ap);
> -		va_end(ap);
>  	} else {
>  		plane->name = kasprintf(GFP_KERNEL, "plane-%d",
>  					drm_num_planes(dev));
> @@ -286,8 +268,94 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	return 0;
>  }
> +
> +/**
> + * drm_universal_plane_init - Initialize a new universal plane object
> + * @dev: DRM device
> + * @plane: plane object to init
> + * @possible_crtcs: bitmask of possible CRTCs
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (DRM_FORMAT\_\*)
> + * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + *                    DRM_FORMAT_MOD_INVALID
> + * @type: type of plane (overlay, primary, cursor)
> + * @name: printf style format string for the plane name, or NULL for default name
> + *
> + * Initializes a plane object of type @type.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> +			     uint32_t possible_crtcs,
> +			     const struct drm_plane_funcs *funcs,
> +			     const uint32_t *formats, unsigned int format_count,
> +			     const uint64_t *format_modifiers,
> +			     enum drm_plane_type type,
> +			     const char *name, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	va_start(ap, name);
> +	ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> +					 formats, format_count, format_modifiers,
> +					 type, name, ap);
> +	va_end(ap);
> +	return ret;
> +}
>  EXPORT_SYMBOL(drm_universal_plane_init);
>  
> +static void drmm_universal_plane_alloc_release(struct drm_device *dev, void *ptr)
> +{
> +	struct drm_plane *plane = ptr;
> +
> +	if (WARN_ON(!plane->dev))
> +		return;
> +
> +	drm_plane_cleanup(plane);
> +}
> +
> +void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
> +				   size_t offset, uint32_t possible_crtcs,
> +				   const struct drm_plane_funcs *funcs,
> +				   const uint32_t *formats, unsigned int format_count,
> +				   const uint64_t *format_modifiers,
> +				   enum drm_plane_type type,
> +				   const char *name, ...)
> +{
> +	void *container;
> +	struct drm_plane *plane;
> +	va_list ap;
> +	int ret;
> +
> +	if (!funcs || funcs->destroy)
> +		return ERR_PTR(-EINVAL);
> +
> +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!container)
> +		return ERR_PTR(-ENOMEM);
> +
> +	plane = container + offset;
> +
> +	va_start(ap, name);
> +	ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> +					 formats, format_count, format_modifiers,
> +					 type, name, ap);
> +	va_end(ap);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = drmm_add_action_or_reset(dev, drmm_universal_plane_alloc_release,
> +				       plane);

Similarly as in 1/9, how about using kzalloc() with a kfree() in
drmm_universal_plane_alloc_release() ? With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return container;
> +}
> +EXPORT_SYMBOL(__drmm_universal_plane_alloc);
> +
>  int drm_plane_register_all(struct drm_device *dev)
>  {
>  	unsigned int num_planes = 0;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 3f396d94afe4..82bd63710a39 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -746,6 +746,48 @@ int drm_plane_init(struct drm_device *dev,
>  		   bool is_primary);
>  void drm_plane_cleanup(struct drm_plane *plane);
>  
> +__printf(10, 11)
> +void *__drmm_universal_plane_alloc(struct drm_device *dev,
> +				   size_t size, size_t offset,
> +				   uint32_t possible_crtcs,
> +				   const struct drm_plane_funcs *funcs,
> +				   const uint32_t *formats,
> +				   unsigned int format_count,
> +				   const uint64_t *format_modifiers,
> +				   enum drm_plane_type plane_type,
> +				   const char *name, ...);
> +
> +/**
> + * drmm_universal_plane_alloc - Allocate and initialize an universal plane object
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct &drm_plane
> + * @member: the name of the &drm_plane within @type
> + * @possible_crtcs: bitmask of possible CRTCs
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (DRM_FORMAT\_\*)
> + * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + *                    DRM_FORMAT_MOD_INVALID
> + * @plane_type: type of plane (overlay, primary, cursor)
> + * @name: printf style format string for the plane name, or NULL for default name
> + *
> + * Allocates and initializes a plane object of type @type. Cleanup is
> + * automatically handled through registering drm_plane_cleanup() with
> + * drmm_add_action().
> + *
> + * The @drm_plane_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new plane, or ERR_PTR on failure.
> + */
> +#define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
> +				   format_count, format_modifiers, plane_type, name, ...) \
> +	((type *)__drmm_universal_plane_alloc(dev, sizeof(type), \
> +					      offsetof(type, member), \
> +					      possible_crtcs, funcs, formats, \
> +					      format_count, format_modifiers, \
> +					      plane_type, name, ##__VA_ARGS__))
> +
>  /**
>   * drm_plane_index - find the index of a registered plane
>   * @plane: plane to find index for

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes()
  2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-12-04  9:23   ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04  9:23 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

Thank you for the patch.

On Fri, Sep 11, 2020 at 03:57:21PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_crtc_init_with_planes() that allocates
> and initializes a crtc and registers drm_crtc_cleanup() with
> drmm_add_action_or_reset().
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
>  - call va_start() / va_end() unconditionally
> ---
>  drivers/gpu/drm/drm_crtc.c | 116 ++++++++++++++++++++++++++++---------
>  include/drm/drm_crtc.h     |  33 +++++++++++
>  2 files changed, 121 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index aecdd7ea26dc..b9e7c11a76b7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_modeset_lock.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_auth.h>
> @@ -231,30 +232,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
>   * 	Setting MODE_ID to 0 will release reserved resources for the CRTC.
>   */
>  
> -/**
> - * drm_crtc_init_with_planes - Initialise a new CRTC object with
> - *    specified primary and cursor planes.
> - * @dev: DRM device
> - * @crtc: CRTC object to init
> - * @primary: Primary plane for CRTC
> - * @cursor: Cursor plane for CRTC
> - * @funcs: callbacks for the new CRTC
> - * @name: printf style format string for the CRTC name, or NULL for default name
> - *
> - * Inits a new object created as base part of a driver crtc object. Drivers
> - * should use this function instead of drm_crtc_init(), which is only provided
> - * for backwards compatibility with drivers which do not yet support universal
> - * planes). For really simple hardware which has only 1 plane look at
> - * drm_simple_display_pipe_init() instead.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> -			      struct drm_plane *primary,
> -			      struct drm_plane *cursor,
> -			      const struct drm_crtc_funcs *funcs,
> -			      const char *name, ...)
> +__printf(6, 0)
> +static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> +				       struct drm_plane *primary,
> +				       struct drm_plane *cursor,
> +				       const struct drm_crtc_funcs *funcs,
> +				       const char *name, va_list ap)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
> @@ -282,11 +265,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  		return ret;
>  
>  	if (name) {
> -		va_list ap;
> -
> -		va_start(ap, name);
>  		crtc->name = kvasprintf(GFP_KERNEL, name, ap);
> -		va_end(ap);
>  	} else {
>  		crtc->name = kasprintf(GFP_KERNEL, "crtc-%d",
>  				       drm_num_crtcs(dev));
> @@ -330,8 +309,89 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  	return 0;
>  }
> +
> +/**
> + * drm_crtc_init_with_planes - Initialise a new CRTC object with
> + *    specified primary and cursor planes.
> + * @dev: DRM device
> + * @crtc: CRTC object to init
> + * @primary: Primary plane for CRTC
> + * @cursor: Cursor plane for CRTC
> + * @funcs: callbacks for the new CRTC
> + * @name: printf style format string for the CRTC name, or NULL for default name
> + *
> + * Inits a new object created as base part of a driver crtc object. Drivers
> + * should use this function instead of drm_crtc_init(), which is only provided
> + * for backwards compatibility with drivers which do not yet support universal
> + * planes). For really simple hardware which has only 1 plane look at
> + * drm_simple_display_pipe_init() instead.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> +			      struct drm_plane *primary,
> +			      struct drm_plane *cursor,
> +			      const struct drm_crtc_funcs *funcs,
> +			      const char *name, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	va_start(ap, name);
> +	ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
> +					  name, ap);
> +	va_end(ap);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL(drm_crtc_init_with_planes);
>  
> +static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev,
> +						void *ptr)
> +{
> +	struct drm_crtc *crtc = ptr;
> +
> +	drm_crtc_cleanup(crtc);
> +}
> +
> +void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
> +				    size_t size, size_t offset,
> +				    struct drm_plane *primary,
> +				    struct drm_plane *cursor,
> +				    const struct drm_crtc_funcs *funcs,
> +				    const char *name, ...)
> +{
> +	void *container;
> +	struct drm_crtc *crtc;
> +	va_list ap;
> +	int ret;
> +
> +	if (!funcs || funcs->destroy)
> +		return ERR_PTR(-EINVAL);
> +
> +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!container)
> +		return ERR_PTR(-ENOMEM);
> +
> +	crtc = container + offset;
> +
> +	va_start(ap, name);
> +	ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
> +					  name, ap);
> +	va_end(ap);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = drmm_add_action_or_reset(dev, drmm_crtc_alloc_with_planes_cleanup,
> +				       crtc);

Similarly as in 1/9, how about using kzalloc() with a kfree() in
drmm_crtc_alloc_with_planes_cleanup() ? With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return container;
> +}
> +EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes);
> +
>  /**
>   * drm_crtc_cleanup - Clean up the core crtc usage
>   * @crtc: CRTC to cleanup
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 59b51a09cae6..b71c0a3d4126 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1210,6 +1210,39 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
>  			      const char *name, ...);
>  void drm_crtc_cleanup(struct drm_crtc *crtc);
>  
> +__printf(7, 8)
> +void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
> +				    size_t size, size_t offset,
> +				    struct drm_plane *primary,
> +				    struct drm_plane *cursor,
> +				    const struct drm_crtc_funcs *funcs,
> +				    const char *name, ...);
> +
> +/**
> + * drm_crtc_alloc_with_planes - Allocate and initialize a new CRTC object with
> + *    specified primary and cursor planes.
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct &drm_crtc
> + * @member: the name of the &drm_crtc within @type.
> + * @primary: Primary plane for CRTC
> + * @cursor: Cursor plane for CRTC
> + * @funcs: callbacks for the new CRTC
> + * @name: printf style format string for the CRTC name, or NULL for default name
> + *
> + * Allocates and initializes a new crtc object. Cleanup is automatically
> + * handled through registering drmm_crtc_cleanup() with drmm_add_action().
> + *
> + * The @drm_crtc_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new crtc, or ERR_PTR on failure.
> + */
> +#define drmm_crtc_alloc_with_planes(dev, type, member, primary, cursor, funcs, name, ...) \
> +	((type *)__drmm_crtc_alloc_with_planes(dev, sizeof(type), \
> +					       offsetof(type, member), \
> +					       primary, cursor, funcs, \
> +					       name, ##__VA_ARGS__))
> +
>  /**
>   * drm_crtc_index - find the index of a registered CRTC
>   * @crtc: CRTC to find index for

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
  2020-12-04  9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
@ 2020-12-04 10:12   ` Philipp Zabel
  2020-12-05 18:57     ` Laurent Pinchart
  0 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-12-04 10:12 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: kernel, dri-devel

Hi Laurent,

On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> Hi Philipp,
> 
> Thank you for the patch.

Thank you for the review.

> On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > Add an alternative to drm_encoder_init() that allocates and initializes
> > an encoder and registers drm_encoder_cleanup() with
> > drmm_add_action_or_reset().
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v2:
> >  - call va_start() / va_end() unconditionally
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
> >  include/drm/drm_encoder.h     |  30 ++++++++++
> >  2 files changed, 108 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index e555281f43d4..f5414705f9ad 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -26,6 +26,7 @@
> >  #include <drm/drm_device.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_managed.h>
> >  
> >  #include "drm_crtc_internal.h"
> >  
> > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> >  	}
> >  }
> >  
> > -/**
> > - * drm_encoder_init - Init a preallocated encoder
> > - * @dev: drm device
> > - * @encoder: the encoder to init
> > - * @funcs: callbacks for this encoder
> > - * @encoder_type: user visible type of the encoder
> > - * @name: printf style format string for the encoder name, or NULL for default name
> > - *
> > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > - *
> > - * Returns:
> > - * Zero on success, error code on failure.
> > - */
> > -int drm_encoder_init(struct drm_device *dev,
> > -		     struct drm_encoder *encoder,
> > -		     const struct drm_encoder_funcs *funcs,
> > -		     int encoder_type, const char *name, ...)
> > +__printf(5, 0)
> > +static int __drm_encoder_init(struct drm_device *dev,
> > +			      struct drm_encoder *encoder,
> > +			      const struct drm_encoder_funcs *funcs,
> > +			      int encoder_type, const char *name, va_list ap)
> >  {
> >  	int ret;
> >  
> > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> >  	encoder->encoder_type = encoder_type;
> >  	encoder->funcs = funcs;
> >  	if (name) {
> > -		va_list ap;
> > -
> > -		va_start(ap, name);
> >  		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > -		va_end(ap);
> >  	} else {
> >  		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> >  					  drm_encoder_enum_list[encoder_type].name,
> > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> >  
> >  	return ret;
> >  }
> > +
> > +/**
> > + * drm_encoder_init - Init a preallocated encoder
> > + * @dev: drm device
> > + * @encoder: the encoder to init
> > + * @funcs: callbacks for this encoder
> > + * @encoder_type: user visible type of the encoder
> > + * @name: printf style format string for the encoder name, or NULL for default name
> > + *
> > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_encoder_init(struct drm_device *dev,
> > +		     struct drm_encoder *encoder,
> > +		     const struct drm_encoder_funcs *funcs,
> > +		     int encoder_type, const char *name, ...)
> > +{
> > +	va_list ap;
> > +	int ret;
> > +
> > +	va_start(ap, name);
> > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > +	va_end(ap);
> > +
> > +	return ret;
> > +}
> >  EXPORT_SYMBOL(drm_encoder_init);
> >  
> >  /**
> > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> >  }
> >  EXPORT_SYMBOL(drm_encoder_cleanup);
> >  
> > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > +{
> > +	struct drm_encoder *encoder = ptr;
> > +
> > +	if (WARN_ON(!encoder->dev))
> > +		return;
> > +
> > +	drm_encoder_cleanup(encoder);
> > +}
> > +
> > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > +			   const struct drm_encoder_funcs *funcs,
> > +			   int encoder_type, const char *name, ...)
> > +{
> > +	void *container;
> > +	struct drm_encoder *encoder;
> > +	va_list ap;
> > +	int ret;
> > +
> > +	if (WARN_ON(!funcs || funcs->destroy))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > +	if (!container)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	encoder = container + offset;
> > +
> > +	va_start(ap, name);
> > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > +	va_end(ap);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> 
> Given that drmm_encoder_alloc_release() will be called right before the
> kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> drmm_encoder_alloc_release() ? That will save one context allocation.

That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
the container pointer that must be passed to kfree(), nor the offset
between container and encoder.

> With this addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
  2020-12-04  9:19   ` Laurent Pinchart
@ 2020-12-04 10:13     ` Philipp Zabel
  2020-12-05 18:58       ` Laurent Pinchart
  0 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-12-04 10:13 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: kernel, dri-devel

Hi Laurent,

On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> > Add an alternative to drm_simple_encoder_init() that allocates and
> > initializes a simple encoder and registers drm_encoder_cleanup() with
> > drmm_add_action_or_reset().
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++
> >  include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index 74946690aba4..3cbbfb0f9b51 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -9,6 +9,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_managed.h>
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/drm_simple_kms_helper.h>
> > @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_simple_encoder_init);
> >  
> > +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { };
> > +
> > +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
> > +				  size_t offset, int encoder_type)
> > +{
> > +	return __drmm_encoder_alloc(dev, size, offset,
> > +				    &drmm_simple_encoder_funcs_empty,
> > +				    encoder_type, NULL);
> > +}
> > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
> 
> Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
> in the core (if we don't already) and use drmm_encoder_alloc() directly
> in drivers ?

I could change it to remove the empty drm_encoder_funcs, and prepend
something like this:

----------8<----------
From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Fri, 4 Dec 2020 10:49:41 +0100
Subject: [PATCH] drm/encoder: make encoder control functions optional

Simple managed encoders do not require the .destroy callback,
make the whole funcs structure optional.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/drm_encoder.c     | 4 ++--
 drivers/gpu/drm/drm_mode_config.c | 5 +++--
 include/drm/drm_encoder.h         | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index e555281f43d4..b0b86a3c08f5 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev)
 	int ret = 0;
 
 	drm_for_each_encoder(encoder, dev) {
-		if (encoder->funcs->late_register)
+		if (encoder->funcs && encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)
 			return ret;
@@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev)
 	struct drm_encoder *encoder;
 
 	drm_for_each_encoder(encoder, dev) {
-		if (encoder->funcs->early_unregister)
+		if (encoder->funcs && encoder->funcs->early_unregister)
 			encoder->funcs->early_unregister(encoder);
 	}
 }
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index f1affc1bb679..87e144155456 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev)
 			crtc->funcs->reset(crtc);
 
 	drm_for_each_encoder(encoder, dev)
-		if (encoder->funcs->reset)
+		if (encoder->funcs && encoder->funcs->reset)
 			encoder->funcs->reset(encoder);
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
@@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 
 	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
 				 head) {
-		encoder->funcs->destroy(encoder);
+		if (encoder->funcs)
+			encoder->funcs->destroy(encoder);
 	}
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5dfa5f7a80a7..833123637fbf 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -89,7 +89,7 @@ struct drm_encoder_funcs {
  * @head: list management
  * @base: base KMS object
  * @name: human readable name, can be overwritten by the driver
- * @funcs: control functions
+ * @funcs: control functions, can be NULL for simple managed encoders
  * @helper_private: mid-layer private data
  *
  * CRTCs drive pixels to encoders, which convert them into signals
-- 
2.20.1
---------->8----------

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
  2020-12-04 10:12   ` Philipp Zabel
@ 2020-12-05 18:57     ` Laurent Pinchart
  2020-12-09  0:22       ` Daniel Vetter
  0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-05 18:57 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

On Fri, Dec 04, 2020 at 11:12:20AM +0100, Philipp Zabel wrote:
> On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> > On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > > Add an alternative to drm_encoder_init() that allocates and initializes
> > > an encoder and registers drm_encoder_cleanup() with
> > > drmm_add_action_or_reset().
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > Changes since v2:
> > >  - call va_start() / va_end() unconditionally
> > > ---
> > >  drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
> > >  include/drm/drm_encoder.h     |  30 ++++++++++
> > >  2 files changed, 108 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > > index e555281f43d4..f5414705f9ad 100644
> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -26,6 +26,7 @@
> > >  #include <drm/drm_device.h>
> > >  #include <drm/drm_drv.h>
> > >  #include <drm/drm_encoder.h>
> > > +#include <drm/drm_managed.h>
> > >  
> > >  #include "drm_crtc_internal.h"
> > >  
> > > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> > >  	}
> > >  }
> > >  
> > > -/**
> > > - * drm_encoder_init - Init a preallocated encoder
> > > - * @dev: drm device
> > > - * @encoder: the encoder to init
> > > - * @funcs: callbacks for this encoder
> > > - * @encoder_type: user visible type of the encoder
> > > - * @name: printf style format string for the encoder name, or NULL for default name
> > > - *
> > > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > > - *
> > > - * Returns:
> > > - * Zero on success, error code on failure.
> > > - */
> > > -int drm_encoder_init(struct drm_device *dev,
> > > -		     struct drm_encoder *encoder,
> > > -		     const struct drm_encoder_funcs *funcs,
> > > -		     int encoder_type, const char *name, ...)
> > > +__printf(5, 0)
> > > +static int __drm_encoder_init(struct drm_device *dev,
> > > +			      struct drm_encoder *encoder,
> > > +			      const struct drm_encoder_funcs *funcs,
> > > +			      int encoder_type, const char *name, va_list ap)
> > >  {
> > >  	int ret;
> > >  
> > > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> > >  	encoder->encoder_type = encoder_type;
> > >  	encoder->funcs = funcs;
> > >  	if (name) {
> > > -		va_list ap;
> > > -
> > > -		va_start(ap, name);
> > >  		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > > -		va_end(ap);
> > >  	} else {
> > >  		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > >  					  drm_encoder_enum_list[encoder_type].name,
> > > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> > >  
> > >  	return ret;
> > >  }
> > > +
> > > +/**
> > > + * drm_encoder_init - Init a preallocated encoder
> > > + * @dev: drm device
> > > + * @encoder: the encoder to init
> > > + * @funcs: callbacks for this encoder
> > > + * @encoder_type: user visible type of the encoder
> > > + * @name: printf style format string for the encoder name, or NULL for default name
> > > + *
> > > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > > + *
> > > + * Returns:
> > > + * Zero on success, error code on failure.
> > > + */
> > > +int drm_encoder_init(struct drm_device *dev,
> > > +		     struct drm_encoder *encoder,
> > > +		     const struct drm_encoder_funcs *funcs,
> > > +		     int encoder_type, const char *name, ...)
> > > +{
> > > +	va_list ap;
> > > +	int ret;
> > > +
> > > +	va_start(ap, name);
> > > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > +	va_end(ap);
> > > +
> > > +	return ret;
> > > +}
> > >  EXPORT_SYMBOL(drm_encoder_init);
> > >  
> > >  /**
> > > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > >  }
> > >  EXPORT_SYMBOL(drm_encoder_cleanup);
> > >  
> > > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > > +{
> > > +	struct drm_encoder *encoder = ptr;
> > > +
> > > +	if (WARN_ON(!encoder->dev))
> > > +		return;
> > > +
> > > +	drm_encoder_cleanup(encoder);
> > > +}
> > > +
> > > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > > +			   const struct drm_encoder_funcs *funcs,
> > > +			   int encoder_type, const char *name, ...)
> > > +{
> > > +	void *container;
> > > +	struct drm_encoder *encoder;
> > > +	va_list ap;
> > > +	int ret;
> > > +
> > > +	if (WARN_ON(!funcs || funcs->destroy))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > > +	if (!container)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	encoder = container + offset;
> > > +
> > > +	va_start(ap, name);
> > > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > +	va_end(ap);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > 
> > Given that drmm_encoder_alloc_release() will be called right before the
> > kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> > to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> > drmm_encoder_alloc_release() ? That will save one context allocation.
> 
> That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
> the container pointer that must be passed to kfree(), nor the offset
> between container and encoder.

Indeed :-S Adding more context to the drmres when creating it with
drmm_add_action_or_reset() would solve this, but it's probably overkill
(although I think this would definitely be useful if/when we turn the
DRM resource manager to a more generic component usable by other
subsystems).

> > With this addressed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

You can add my tag to this patch and the other ones in the series where
I've made the same comment.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
  2020-12-04 10:13     ` Philipp Zabel
@ 2020-12-05 18:58       ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-05 18:58 UTC (permalink / raw)
  To: Philipp Zabel; +Cc: kernel, dri-devel

Hi Philipp,

On Fri, Dec 04, 2020 at 11:13:33AM +0100, Philipp Zabel wrote:
> On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote:
> > On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> > > Add an alternative to drm_simple_encoder_init() that allocates and
> > > initializes a simple encoder and registers drm_encoder_cleanup() with
> > > drmm_add_action_or_reset().
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++
> > >  include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
> > >  2 files changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > index 74946690aba4..3cbbfb0f9b51 100644
> > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > @@ -9,6 +9,7 @@
> > >  #include <drm/drm_atomic.h>
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_bridge.h>
> > > +#include <drm/drm_managed.h>
> > >  #include <drm/drm_plane_helper.h>
> > >  #include <drm/drm_probe_helper.h>
> > >  #include <drm/drm_simple_kms_helper.h>
> > > @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev,
> > >  }
> > >  EXPORT_SYMBOL(drm_simple_encoder_init);
> > >  
> > > +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { };
> > > +
> > > +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
> > > +				  size_t offset, int encoder_type)
> > > +{
> > > +	return __drmm_encoder_alloc(dev, size, offset,
> > > +				    &drmm_simple_encoder_funcs_empty,
> > > +				    encoder_type, NULL);
> > > +}
> > > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
> > 
> > Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
> > in the core (if we don't already) and use drmm_encoder_alloc() directly
> > in drivers ?
> 
> I could change it to remove the empty drm_encoder_funcs, and prepend
> something like this:
> 
> ----------8<----------
> From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Fri, 4 Dec 2020 10:49:41 +0100
> Subject: [PATCH] drm/encoder: make encoder control functions optional
> 
> Simple managed encoders do not require the .destroy callback,
> make the whole funcs structure optional.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

That's perfect.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_encoder.c     | 4 ++--
>  drivers/gpu/drm/drm_mode_config.c | 5 +++--
>  include/drm/drm_encoder.h         | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..b0b86a3c08f5 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	int ret = 0;
>  
>  	drm_for_each_encoder(encoder, dev) {
> -		if (encoder->funcs->late_register)
> +		if (encoder->funcs && encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
>  		if (ret)
>  			return ret;
> @@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev)
>  	struct drm_encoder *encoder;
>  
>  	drm_for_each_encoder(encoder, dev) {
> -		if (encoder->funcs->early_unregister)
> +		if (encoder->funcs && encoder->funcs->early_unregister)
>  			encoder->funcs->early_unregister(encoder);
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..87e144155456 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev)
>  			crtc->funcs->reset(crtc);
>  
>  	drm_for_each_encoder(encoder, dev)
> -		if (encoder->funcs->reset)
> +		if (encoder->funcs && encoder->funcs->reset)
>  			encoder->funcs->reset(encoder);
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
> @@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  
>  	list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
>  				 head) {
> -		encoder->funcs->destroy(encoder);
> +		if (encoder->funcs)
> +			encoder->funcs->destroy(encoder);
>  	}
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 5dfa5f7a80a7..833123637fbf 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -89,7 +89,7 @@ struct drm_encoder_funcs {
>   * @head: list management
>   * @base: base KMS object
>   * @name: human readable name, can be overwritten by the driver
> - * @funcs: control functions
> + * @funcs: control functions, can be NULL for simple managed encoders
>   * @helper_private: mid-layer private data
>   *
>   * CRTCs drive pixels to encoders, which convert them into signals

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
  2020-12-05 18:57     ` Laurent Pinchart
@ 2020-12-09  0:22       ` Daniel Vetter
  0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2020-12-09  0:22 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: dri-devel, kernel

On Sat, Dec 05, 2020 at 08:57:38PM +0200, Laurent Pinchart wrote:
> Hi Philipp,
> 
> On Fri, Dec 04, 2020 at 11:12:20AM +0100, Philipp Zabel wrote:
> > On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> > > On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > > > Add an alternative to drm_encoder_init() that allocates and initializes
> > > > an encoder and registers drm_encoder_cleanup() with
> > > > drmm_add_action_or_reset().
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > > Changes since v2:
> > > >  - call va_start() / va_end() unconditionally
> > > > ---
> > > >  drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
> > > >  include/drm/drm_encoder.h     |  30 ++++++++++
> > > >  2 files changed, 108 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > > > index e555281f43d4..f5414705f9ad 100644
> > > > --- a/drivers/gpu/drm/drm_encoder.c
> > > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include <drm/drm_device.h>
> > > >  #include <drm/drm_drv.h>
> > > >  #include <drm/drm_encoder.h>
> > > > +#include <drm/drm_managed.h>
> > > >  
> > > >  #include "drm_crtc_internal.h"
> > > >  
> > > > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> > > >  	}
> > > >  }
> > > >  
> > > > -/**
> > > > - * drm_encoder_init - Init a preallocated encoder
> > > > - * @dev: drm device
> > > > - * @encoder: the encoder to init
> > > > - * @funcs: callbacks for this encoder
> > > > - * @encoder_type: user visible type of the encoder
> > > > - * @name: printf style format string for the encoder name, or NULL for default name
> > > > - *
> > > > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > > > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > > > - *
> > > > - * Returns:
> > > > - * Zero on success, error code on failure.
> > > > - */
> > > > -int drm_encoder_init(struct drm_device *dev,
> > > > -		     struct drm_encoder *encoder,
> > > > -		     const struct drm_encoder_funcs *funcs,
> > > > -		     int encoder_type, const char *name, ...)
> > > > +__printf(5, 0)
> > > > +static int __drm_encoder_init(struct drm_device *dev,
> > > > +			      struct drm_encoder *encoder,
> > > > +			      const struct drm_encoder_funcs *funcs,
> > > > +			      int encoder_type, const char *name, va_list ap)
> > > >  {
> > > >  	int ret;
> > > >  
> > > > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> > > >  	encoder->encoder_type = encoder_type;
> > > >  	encoder->funcs = funcs;
> > > >  	if (name) {
> > > > -		va_list ap;
> > > > -
> > > > -		va_start(ap, name);
> > > >  		encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > > > -		va_end(ap);
> > > >  	} else {
> > > >  		encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > > >  					  drm_encoder_enum_list[encoder_type].name,
> > > > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> > > >  
> > > >  	return ret;
> > > >  }
> > > > +
> > > > +/**
> > > > + * drm_encoder_init - Init a preallocated encoder
> > > > + * @dev: drm device
> > > > + * @encoder: the encoder to init
> > > > + * @funcs: callbacks for this encoder
> > > > + * @encoder_type: user visible type of the encoder
> > > > + * @name: printf style format string for the encoder name, or NULL for default name
> > > > + *
> > > > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > > > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > > > + *
> > > > + * Returns:
> > > > + * Zero on success, error code on failure.
> > > > + */
> > > > +int drm_encoder_init(struct drm_device *dev,
> > > > +		     struct drm_encoder *encoder,
> > > > +		     const struct drm_encoder_funcs *funcs,
> > > > +		     int encoder_type, const char *name, ...)
> > > > +{
> > > > +	va_list ap;
> > > > +	int ret;
> > > > +
> > > > +	va_start(ap, name);
> > > > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > > +	va_end(ap);
> > > > +
> > > > +	return ret;
> > > > +}
> > > >  EXPORT_SYMBOL(drm_encoder_init);
> > > >  
> > > >  /**
> > > > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_encoder_cleanup);
> > > >  
> > > > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > > > +{
> > > > +	struct drm_encoder *encoder = ptr;
> > > > +
> > > > +	if (WARN_ON(!encoder->dev))
> > > > +		return;
> > > > +
> > > > +	drm_encoder_cleanup(encoder);
> > > > +}
> > > > +
> > > > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > > > +			   const struct drm_encoder_funcs *funcs,
> > > > +			   int encoder_type, const char *name, ...)
> > > > +{
> > > > +	void *container;
> > > > +	struct drm_encoder *encoder;
> > > > +	va_list ap;
> > > > +	int ret;
> > > > +
> > > > +	if (WARN_ON(!funcs || funcs->destroy))
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > > > +	if (!container)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	encoder = container + offset;
> > > > +
> > > > +	va_start(ap, name);
> > > > +	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > > +	va_end(ap);
> > > > +	if (ret)
> > > > +		return ERR_PTR(ret);
> > > > +
> > > > +	ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > > > +	if (ret)
> > > > +		return ERR_PTR(ret);
> > > 
> > > Given that drmm_encoder_alloc_release() will be called right before the
> > > kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> > > to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> > > drmm_encoder_alloc_release() ? That will save one context allocation.
> > 
> > That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
> > the container pointer that must be passed to kfree(), nor the offset
> > between container and encoder.
> 
> Indeed :-S Adding more context to the drmres when creating it with
> drmm_add_action_or_reset() would solve this, but it's probably overkill
> (although I think this would definitely be useful if/when we turn the
> DRM resource manager to a more generic component usable by other
> subsystems).

Internally it's even funnier, because the drmm_kzalloc doesn't even have a
callback - the allocation is freed as part of the drmm tracking structure.
Which means that drmm_kzalloc has a "free" release callback already.

This irked me to no end, but I couldn't come up with an interface to stuff
the release callback into the allocation which I deemed safe enough for
drmm, which is supposed to make life easy and bugs disappear, not greate
new surprises.

What I just realized is that drmm_add_action_or_reset could look at the
drmm cleanup action stack, and if the topmost one doesn't have a hook set
already, use that one instead of allocating a new tracker structure. This
way ordering is always guaranteed (e.g. if you mix up drmm_kzalloc calls
with add_action for different objects in strange orders) and there's no
problem ever, we just avoid a few small allocations.

Still needs a few special cases, so not sure it's actually a win.
-Daniel

> 
> > > With this addressed,
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> You can add my tag to this patch and the other ones in the series where
> I've made the same comment.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-12-09  0:22 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
2020-12-04  9:19   ` Laurent Pinchart
2020-12-04 10:13     ` Philipp Zabel
2020-12-05 18:58       ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
2020-12-04  9:22   ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
2020-12-04  9:23   ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
2020-09-16  9:08   ` Daniel Vetter
2020-09-16 10:22     ` Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc() Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes() Philipp Zabel
2020-12-04  9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
2020-12-04 10:12   ` Philipp Zabel
2020-12-05 18:57     ` Laurent Pinchart
2020-12-09  0:22       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.