[PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Philipp Zabel]

Hi,

this is an update of the drmm_(simple_)encoder_alloc(),
drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
functions in v3 [1] together with the imx-drm managed allocation
conversion from [2] as an example usage.
a bit.

Changes since v3:
 - Allow encoder_funcs to be NULL and let drmm_encoder_alloc() accept a
   NULL value for the funcs parameter. This allows to
 - drop the now useless drmm_simple_encoder_funcs_empty structure.
 - Reorder and squash the imx-drm managed allocation conversion patches
   to use the new functions directly.
 - Fold functions into bind callbacks where they are the only remaining
   call.

[1] https://lore.kernel.org/dri-devel/20200911135724.25833-1-p.zabel@pengutronix.de/
[2] https://lore.kernel.org/dri-devel/20200911133855.29801-1-p.zabel@pengutronix.de/

regards
Philipp

Philipp Zabel (19):
  drm/encoder: make encoder control functions optional
  drm: add drmm_encoder_alloc()
  drm/simple_kms_helper: add drmm_simple_encoder_alloc()
  drm/plane: add drmm_universal_plane_alloc()
  drm/crtc: add drmm_crtc_alloc_with_planes()
  drm/imx: dw_hdmi-imx: move initialization into probe
  drm/imx: imx-ldb: use local connector variable
  drm/imx: imx-ldb: move initialization into probe
  drm/imx: imx-tve: use local encoder and connector variables
  drm/imx: imx-tve: move initialization into probe
  drm/imx: imx-tve: use devm_clk_register
  drm/imx: parallel-display: use local bridge and connector variables
  drm/imx: parallel-display: move initialization into probe
  drm/imx: dw_hdmi-imx: use drm managed resources
  drm/imx: imx-ldb: use drm managed resources
  drm/imx: imx-tve: use drm managed resources
  drm/imx: parallel-display: use drm managed resources
  drm/imx: ipuv3-plane: use drm managed resources
  drm/imx: ipuv3-crtc: use drm managed resources

 drivers/gpu/drm/drm_crtc.c              | 116 ++++++++++++++++-----
 drivers/gpu/drm/drm_encoder.c           | 105 ++++++++++++++-----
 drivers/gpu/drm/drm_mode_config.c       |   5 +-
 drivers/gpu/drm/drm_plane.c             | 126 +++++++++++++++++------
 drivers/gpu/drm/drm_simple_kms_helper.c |   9 ++
 drivers/gpu/drm/imx/dw_hdmi-imx.c       |  95 ++++++++---------
 drivers/gpu/drm/imx/imx-ldb.c           | 109 +++++++++++---------
 drivers/gpu/drm/imx/imx-tve.c           | 109 ++++++++++----------
 drivers/gpu/drm/imx/ipuv3-crtc.c        | 131 ++++++++----------------
 drivers/gpu/drm/imx/ipuv3-plane.c       |  69 ++++++-------
 drivers/gpu/drm/imx/ipuv3-plane.h       |   3 -
 drivers/gpu/drm/imx/parallel-display.c  |  91 ++++++++--------
 include/drm/drm_crtc.h                  |  33 ++++++
 include/drm/drm_encoder.h               |  32 +++++-
 include/drm/drm_plane.h                 |  42 ++++++++
 include/drm/drm_simple_kms_helper.h     |  24 +++++
 16 files changed, 684 insertions(+), 415 deletions(-)

-- 
2.20.1

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

[PATCH v4 01/19] drm/encoder: make encoder control functions optional

[Philipp Zabel]

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

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
New in v4.
---
 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

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

[PATCH v4 02/19] drm: add drmm_encoder_alloc()

[Philipp Zabel]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v3:
 - allow the funcs parameter to __drmm_encoder_alloc() to be NULL
---
 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 b0b86a3c08f5..cc0edb8361e8 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 833123637fbf..fb2f56c006db 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -194,6 +194,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

[PATCH v4 03/19] drm/simple_kms_helper: add drmm_simple_encoder_alloc()

[Philipp Zabel]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v3:
 - drop drmm_simple_encoder_funcs_empty, now that encoder control
   functions are optional
---
 drivers/gpu/drm/drm_simple_kms_helper.c |  9 +++++++++
 include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 743e57c1b44f..ccf0908eb82e 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,14 @@ int drm_simple_encoder_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_simple_encoder_init);
 
+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, NULL, 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

[PATCH v4 04/19] drm/plane: add drmm_universal_plane_alloc()

[Philipp Zabel]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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 e6231947f987..6b1261fe09fb 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 1d82b264e5e4..8ef06ee1c8eb 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -764,6 +764,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

[PATCH v4 05/19] drm/crtc: add drmm_crtc_alloc_with_planes()

[Philipp Zabel]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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 f927976eca50..668c31332230 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>
@@ -240,30 +241,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *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;
@@ -291,11 +274,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));
@@ -339,8 +318,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 ba839e5e357d..9053f80b613f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1223,6 +1223,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

[PATCH v4 06/19] drm/imx: dw_hdmi-imx: move initialization into probe

[Philipp Zabel]

Parts of the initialization that do not require the drm device can be
done once during probe instead of possibly multiple times during bind.
The bind function only creates the encoder and attaches the bridge.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c | 74 +++++++++++--------------------
 1 file changed, 26 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index d07b39b8afd2..bbd0a0cd7c3d 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -15,6 +15,7 @@
 
 #include <drm/bridge/dw_hdmi.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_of.h>
@@ -25,6 +26,7 @@
 struct imx_hdmi {
 	struct device *dev;
 	struct drm_encoder encoder;
+	struct drm_bridge *bridge;
 	struct dw_hdmi *hdmi;
 	struct regmap *regmap;
 };
@@ -98,19 +100,6 @@ static const struct dw_hdmi_phy_config imx_phy_config[] = {
 	{ ~0UL,      0x0000, 0x0000, 0x0000}
 };
 
-static int dw_hdmi_imx_parse_dt(struct imx_hdmi *hdmi)
-{
-	struct device_node *np = hdmi->dev->of_node;
-
-	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
-	if (IS_ERR(hdmi->regmap)) {
-		dev_err(hdmi->dev, "Unable to get gpr\n");
-		return PTR_ERR(hdmi->regmap);
-	}
-
-	return 0;
-}
-
 static void dw_hdmi_imx_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_hdmi *hdmi = enc_to_imx_hdmi(encoder);
@@ -195,65 +184,34 @@ MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
 static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 			    void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	const struct dw_hdmi_plat_data *plat_data;
-	const struct of_device_id *match;
 	struct drm_device *drm = data;
 	struct drm_encoder *encoder;
 	struct imx_hdmi *hdmi;
 	int ret;
 
-	if (!pdev->dev.of_node)
-		return -ENODEV;
-
 	hdmi = dev_get_drvdata(dev);
-	memset(hdmi, 0, sizeof(*hdmi));
+	memset(&hdmi->encoder, 0, sizeof(hdmi->encoder));
 
-	match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
-	plat_data = match->data;
-	hdmi->dev = &pdev->dev;
 	encoder = &hdmi->encoder;
 
 	ret = imx_drm_encoder_parse_of(drm, encoder, dev->of_node);
 	if (ret)
 		return ret;
 
-	ret = dw_hdmi_imx_parse_dt(hdmi);
-	if (ret < 0)
-		return ret;
-
 	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
 
-	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
-
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (IS_ERR(hdmi->hdmi)) {
-		ret = PTR_ERR(hdmi->hdmi);
-		drm_encoder_cleanup(encoder);
-	}
-
-	return ret;
-}
-
-static void dw_hdmi_imx_unbind(struct device *dev, struct device *master,
-			       void *data)
-{
-	struct imx_hdmi *hdmi = dev_get_drvdata(dev);
-
-	dw_hdmi_unbind(hdmi->hdmi);
+	return drm_bridge_attach(encoder, hdmi->bridge, NULL, 0);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
 	.bind	= dw_hdmi_imx_bind,
-	.unbind	= dw_hdmi_imx_unbind,
 };
 
 static int dw_hdmi_imx_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match = of_match_node(dw_hdmi_imx_dt_ids, np);
 	struct imx_hdmi *hdmi;
 
 	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
@@ -261,13 +219,33 @@ static int dw_hdmi_imx_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, hdmi);
+	hdmi->dev = &pdev->dev;
+
+	hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
+	if (IS_ERR(hdmi->regmap)) {
+		dev_err(hdmi->dev, "Unable to get gpr\n");
+		return PTR_ERR(hdmi->regmap);
+	}
+
+	hdmi->hdmi = dw_hdmi_probe(pdev, match->data);
+	if (IS_ERR(hdmi->hdmi))
+		return PTR_ERR(hdmi->hdmi);
+
+	hdmi->bridge = of_drm_find_bridge(np);
+	if (!hdmi->bridge) {
+		dev_err(hdmi->dev, "Unable to find bridge\n");
+		return -ENODEV;
+	}
 
 	return component_add(&pdev->dev, &dw_hdmi_imx_ops);
 }
 
 static int dw_hdmi_imx_remove(struct platform_device *pdev)
 {
+	struct imx_hdmi *hdmi = platform_get_drvdata(pdev);
+
 	component_del(&pdev->dev, &dw_hdmi_imx_ops);
+	dw_hdmi_remove(hdmi->hdmi);
 
 	return 0;
 }
-- 
2.20.1

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

[PATCH v4 07/19] drm/imx: imx-ldb: use local connector variable

[Philipp Zabel]

Use a local variable for the connector.
This simplifies the following commits.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 41e2978cb1eb..288a81f134fe 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -411,6 +411,7 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct imx_ldb_channel *imx_ldb_ch)
 {
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
+	struct drm_connector *connector = &imx_ldb_ch->connector;
 	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
 	int ret;
 
@@ -432,8 +433,7 @@ static int imx_ldb_register(struct drm_device *drm,
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
 
 	if (imx_ldb_ch->bridge) {
-		ret = drm_bridge_attach(&imx_ldb_ch->encoder,
-					imx_ldb_ch->bridge, NULL, 0);
+		ret = drm_bridge_attach(encoder, imx_ldb_ch->bridge, NULL, 0);
 		if (ret) {
 			DRM_ERROR("Failed to initialize bridge with drm\n");
 			return ret;
@@ -445,13 +445,13 @@ static int imx_ldb_register(struct drm_device *drm,
 		 * historical reasons, the ldb driver can also work without
 		 * a panel.
 		 */
-		drm_connector_helper_add(&imx_ldb_ch->connector,
-				&imx_ldb_connector_helper_funcs);
-		drm_connector_init_with_ddc(drm, &imx_ldb_ch->connector,
+		drm_connector_helper_add(connector,
+					 &imx_ldb_connector_helper_funcs);
+		drm_connector_init_with_ddc(drm, connector,
 					    &imx_ldb_connector_funcs,
 					    DRM_MODE_CONNECTOR_LVDS,
 					    imx_ldb_ch->ddc);
-		drm_connector_attach_encoder(&imx_ldb_ch->connector, encoder);
+		drm_connector_attach_encoder(connector, encoder);
 	}
 
 	return 0;
-- 
2.20.1

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

[PATCH v4 08/19] drm/imx: imx-ldb: move initialization into probe

[Philipp Zabel]

Parts of the initialization that do not require the drm device can be
done once during probe instead of possibly multiple times during bind.
The bind function only creates the encoders.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 72 ++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 288a81f134fe..c3639cc32ddf 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -415,6 +415,9 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
 	int ret;
 
+	memset(connector, 0, sizeof(*connector));
+	memset(encoder, 0, sizeof(*encoder));
+
 	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
 	if (ret)
 		return ret;
@@ -559,17 +562,42 @@ static int imx_ldb_panel_ddc(struct device *dev,
 static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
+	struct imx_ldb *imx_ldb = dev_get_drvdata(dev);
+	int ret;
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
+
+		if (!channel->ldb)
+			break;
+
+		ret = imx_ldb_register(drm, channel);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct component_ops imx_ldb_ops = {
+	.bind	= imx_ldb_bind,
+};
+
+static int imx_ldb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	const struct of_device_id *of_id =
-			of_match_device(imx_ldb_dt_ids, dev);
+	const struct of_device_id *of_id = of_match_device(imx_ldb_dt_ids, dev);
 	struct device_node *child;
 	struct imx_ldb *imx_ldb;
 	int dual;
 	int ret;
 	int i;
 
-	imx_ldb = dev_get_drvdata(dev);
-	memset(imx_ldb, 0, sizeof(*imx_ldb));
+	imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
+	if (!imx_ldb)
+		return -ENOMEM;
 
 	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
 	if (IS_ERR(imx_ldb->regmap)) {
@@ -669,25 +697,20 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		}
 		channel->bus_format = bus_format;
 		channel->child = child;
-
-		ret = imx_ldb_register(drm, channel);
-		if (ret) {
-			channel->child = NULL;
-			goto free_child;
-		}
 	}
 
-	return 0;
+	platform_set_drvdata(pdev, imx_ldb);
+
+	return component_add(&pdev->dev, &imx_ldb_ops);
 
 free_child:
 	of_node_put(child);
 	return ret;
 }
 
-static void imx_ldb_unbind(struct device *dev, struct device *master,
-	void *data)
+static int imx_ldb_remove(struct platform_device *pdev)
 {
-	struct imx_ldb *imx_ldb = dev_get_drvdata(dev);
+	struct imx_ldb *imx_ldb = platform_get_drvdata(pdev);
 	int i;
 
 	for (i = 0; i < 2; i++) {
@@ -696,28 +719,7 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 		kfree(channel->edid);
 		i2c_put_adapter(channel->ddc);
 	}
-}
-
-static const struct component_ops imx_ldb_ops = {
-	.bind	= imx_ldb_bind,
-	.unbind	= imx_ldb_unbind,
-};
 
-static int imx_ldb_probe(struct platform_device *pdev)
-{
-	struct imx_ldb *imx_ldb;
-
-	imx_ldb = devm_kzalloc(&pdev->dev, sizeof(*imx_ldb), GFP_KERNEL);
-	if (!imx_ldb)
-		return -ENOMEM;
-
-	platform_set_drvdata(pdev, imx_ldb);
-
-	return component_add(&pdev->dev, &imx_ldb_ops);
-}
-
-static int imx_ldb_remove(struct platform_device *pdev)
-{
 	component_del(&pdev->dev, &imx_ldb_ops);
 	return 0;
 }
-- 
2.20.1

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

[PATCH v4 09/19] drm/imx: imx-tve: use local encoder and connector variables

[Philipp Zabel]

Introduce local variables for encoder and connector.
This simplifies the following commits.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-tve.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 2a8d2e32e7b4..37771073a525 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -430,27 +430,28 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
 
 static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 {
+	struct drm_encoder *encoder = &tve->encoder;
+	struct drm_connector *connector = &tve->connector;
 	int encoder_type;
 	int ret;
 
 	encoder_type = tve->mode == TVE_MODE_VGA ?
 				DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
 
-	ret = imx_drm_encoder_parse_of(drm, &tve->encoder, tve->dev->of_node);
+	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
 	if (ret)
 		return ret;
 
-	drm_encoder_helper_add(&tve->encoder, &imx_tve_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, &tve->encoder, encoder_type);
+	drm_encoder_helper_add(encoder, &imx_tve_encoder_helper_funcs);
+	drm_simple_encoder_init(drm, encoder, encoder_type);
 
-	drm_connector_helper_add(&tve->connector,
-			&imx_tve_connector_helper_funcs);
-	drm_connector_init_with_ddc(drm, &tve->connector,
+	drm_connector_helper_add(connector, &imx_tve_connector_helper_funcs);
+	drm_connector_init_with_ddc(drm, connector,
 				    &imx_tve_connector_funcs,
 				    DRM_MODE_CONNECTOR_VGA,
 				    tve->ddc);
 
-	drm_connector_attach_encoder(&tve->connector, &tve->encoder);
+	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
 }
-- 
2.20.1

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

[PATCH v4 10/19] drm/imx: imx-tve: move initialization into probe

[Philipp Zabel]

Parts of the initialization that do not require the drm device can be
done once during probe instead of possibly multiple times during bind.
The bind function only creates the encoder.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-tve.c | 42 ++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 37771073a525..649e2f56a5da 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -438,6 +438,9 @@ static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 	encoder_type = tve->mode == TVE_MODE_VGA ?
 				DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
 
+	memset(connector, 0, sizeof(*connector));
+	memset(encoder, 0, sizeof(*encoder));
+
 	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
 	if (ret)
 		return ret;
@@ -503,8 +506,19 @@ static int of_get_tve_mode(struct device_node *np)
 
 static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
 	struct drm_device *drm = data;
+	struct imx_tve *tve = dev_get_drvdata(dev);
+
+	return imx_tve_register(drm, tve);
+}
+
+static const struct component_ops imx_tve_ops = {
+	.bind	= imx_tve_bind,
+};
+
+static int imx_tve_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *ddc_node;
 	struct imx_tve *tve;
@@ -514,8 +528,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	int irq;
 	int ret;
 
-	tve = dev_get_drvdata(dev);
-	memset(tve, 0, sizeof(*tve));
+	tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
+	if (!tve)
+		return -ENOMEM;
 
 	tve->dev = dev;
 
@@ -622,28 +637,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	ret = imx_tve_register(drm, tve);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
-static const struct component_ops imx_tve_ops = {
-	.bind	= imx_tve_bind,
-};
-
-static int imx_tve_probe(struct platform_device *pdev)
-{
-	struct imx_tve *tve;
-
-	tve = devm_kzalloc(&pdev->dev, sizeof(*tve), GFP_KERNEL);
-	if (!tve)
-		return -ENOMEM;
-
 	platform_set_drvdata(pdev, tve);
 
-	return component_add(&pdev->dev, &imx_tve_ops);
+	return component_add(dev, &imx_tve_ops);
 }
 
 static int imx_tve_remove(struct platform_device *pdev)
-- 
2.20.1

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

[PATCH v4 11/19] drm/imx: imx-tve: use devm_clk_register

[Philipp Zabel]

Avoid leaking the clock provider when the driver is unbound.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-tve.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 649e2f56a5da..3ef71f688f79 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -418,7 +418,7 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
 	init.parent_names = (const char **)&tve_di_parent;
 
 	tve->clk_hw_di.init = &init;
-	tve->di_clk = clk_register(tve->dev, &tve->clk_hw_di);
+	tve->di_clk = devm_clk_register(tve->dev, &tve->clk_hw_di);
 	if (IS_ERR(tve->di_clk)) {
 		dev_err(tve->dev, "failed to register TVE output clock: %ld\n",
 			PTR_ERR(tve->di_clk));
-- 
2.20.1

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

[PATCH v4 12/19] drm/imx: parallel-display: use local bridge and connector variables

[Philipp Zabel]

Use local variables for bridge and connector.
This simplifies the following commits.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 2eb8df4697df..16e576f8ee83 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -256,7 +256,9 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
 static int imx_pd_register(struct drm_device *drm,
 	struct imx_parallel_display *imxpd)
 {
+	struct drm_connector *connector = &imxpd->connector;
 	struct drm_encoder *encoder = &imxpd->encoder;
+	struct drm_bridge *bridge = &imxpd->bridge;
 	int ret;
 
 	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
@@ -268,31 +270,29 @@ static int imx_pd_register(struct drm_device *drm,
 	 * immediately since the current state is ON
 	 * at this point.
 	 */
-	imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
+	connector->dpms = DRM_MODE_DPMS_OFF;
 
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
 
-	imxpd->bridge.funcs = &imx_pd_bridge_funcs;
-	drm_bridge_attach(encoder, &imxpd->bridge, NULL, 0);
+	bridge->funcs = &imx_pd_bridge_funcs;
+	drm_bridge_attach(encoder, bridge, NULL, 0);
 
 	if (!imxpd->next_bridge) {
-		drm_connector_helper_add(&imxpd->connector,
-				&imx_pd_connector_helper_funcs);
-		drm_connector_init(drm, &imxpd->connector,
-				   &imx_pd_connector_funcs,
+		drm_connector_helper_add(connector,
+					 &imx_pd_connector_helper_funcs);
+		drm_connector_init(drm, connector, &imx_pd_connector_funcs,
 				   DRM_MODE_CONNECTOR_DPI);
 	}
 
 	if (imxpd->next_bridge) {
-		ret = drm_bridge_attach(encoder, imxpd->next_bridge,
-					&imxpd->bridge, 0);
+		ret = drm_bridge_attach(encoder, imxpd->next_bridge, bridge, 0);
 		if (ret < 0) {
 			dev_err(imxpd->dev, "failed to attach bridge: %d\n",
 				ret);
 			return ret;
 		}
 	} else {
-		drm_connector_attach_encoder(&imxpd->connector, encoder);
+		drm_connector_attach_encoder(connector, encoder);
 	}
 
 	return 0;
-- 
2.20.1

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

[PATCH v4 13/19] drm/imx: parallel-display: move initialization into probe

[Philipp Zabel]

Parts of the initialization that do not require the drm device can be
done once during probe instead of possibly multiple times during bind.
The bind function only creates the encoder.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 42 ++++++++++++--------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 16e576f8ee83..42b44dbf45f5 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -261,6 +261,10 @@ static int imx_pd_register(struct drm_device *drm,
 	struct drm_bridge *bridge = &imxpd->bridge;
 	int ret;
 
+	memset(connector, 0, sizeof(*connector));
+	memset(encoder, 0, sizeof(*encoder));
+	memset(bridge, 0, sizeof(*bridge));
+
 	ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
 	if (ret)
 		return ret;
@@ -301,6 +305,18 @@ static int imx_pd_register(struct drm_device *drm,
 static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
+	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
+
+	return imx_pd_register(drm, imxpd);
+}
+
+static const struct component_ops imx_pd_ops = {
+	.bind	= imx_pd_bind,
+};
+
+static int imx_pd_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	const u8 *edidp;
 	struct imx_parallel_display *imxpd;
@@ -309,8 +325,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	u32 bus_format = 0;
 	const char *fmt;
 
-	imxpd = dev_get_drvdata(dev);
-	memset(imxpd, 0, sizeof(*imxpd));
+	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
+	if (!imxpd)
+		return -ENOMEM;
 
 	/* port@1 is the output port */
 	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel,
@@ -337,28 +354,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 
 	imxpd->dev = dev;
 
-	ret = imx_pd_register(drm, imxpd);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
-static const struct component_ops imx_pd_ops = {
-	.bind	= imx_pd_bind,
-};
-
-static int imx_pd_probe(struct platform_device *pdev)
-{
-	struct imx_parallel_display *imxpd;
-
-	imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL);
-	if (!imxpd)
-		return -ENOMEM;
-
 	platform_set_drvdata(pdev, imxpd);
 
-	return component_add(&pdev->dev, &imx_pd_ops);
+	return component_add(dev, &imx_pd_ops);
 }
 
 static int imx_pd_remove(struct platform_device *pdev)
-- 
2.20.1

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

[PATCH v4 14/19] drm/imx: dw_hdmi-imx: use drm managed resources

[Philipp Zabel]

Use drmm_simple_encoder_alloc() to align encoder memory lifetime with
the drm device. drm_encoder_cleanup() is called automatically.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_simple_encoder_alloc()
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index bbd0a0cd7c3d..87428fb23d9f 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -18,14 +18,21 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_of.h>
 #include <drm/drm_simple_kms_helper.h>
 
 #include "imx-drm.h"
 
+struct imx_hdmi;
+
+struct imx_hdmi_encoder {
+	struct drm_encoder encoder;
+	struct imx_hdmi *hdmi;
+};
+
 struct imx_hdmi {
 	struct device *dev;
-	struct drm_encoder encoder;
 	struct drm_bridge *bridge;
 	struct dw_hdmi *hdmi;
 	struct regmap *regmap;
@@ -33,7 +40,7 @@ struct imx_hdmi {
 
 static inline struct imx_hdmi *enc_to_imx_hdmi(struct drm_encoder *e)
 {
-	return container_of(e, struct imx_hdmi, encoder);
+	return container_of(e, struct imx_hdmi_encoder, encoder)->hdmi;
 }
 
 static const struct dw_hdmi_mpll_config imx_mpll_cfg[] = {
@@ -185,23 +192,25 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 			    void *data)
 {
 	struct drm_device *drm = data;
+	struct imx_hdmi_encoder *hdmi_encoder;
 	struct drm_encoder *encoder;
-	struct imx_hdmi *hdmi;
 	int ret;
 
-	hdmi = dev_get_drvdata(dev);
-	memset(&hdmi->encoder, 0, sizeof(hdmi->encoder));
+	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);
 
-	encoder = &hdmi->encoder;
+	hdmi_encoder->hdmi = dev_get_drvdata(dev);
+	encoder = &hdmi_encoder->encoder;
 
 	ret = imx_drm_encoder_parse_of(drm, encoder, dev->of_node);
 	if (ret)
 		return ret;
 
 	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
 
-	return drm_bridge_attach(encoder, hdmi->bridge, NULL, 0);
+	return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
 }
 
 static const struct component_ops dw_hdmi_imx_ops = {
-- 
2.20.1

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

[PATCH v4 15/19] drm/imx: imx-ldb: use drm managed resources

[Philipp Zabel]

Use drmm_simple_encoder_alloc() to align encoder memory lifetime with
the drm device. drm_encoder_cleanup() is called automatically before
the memory is freed.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_simple_encoder_alloc()
---
 drivers/gpu/drm/imx/imx-ldb.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index c3639cc32ddf..dbfe39e2f7f6 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -22,6 +22,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
@@ -47,12 +48,18 @@
 #define LDB_DI1_VS_POL_ACT_LOW		(1 << 10)
 #define LDB_BGREF_RMODE_INT		(1 << 15)
 
+struct imx_ldb_channel;
+
+struct imx_ldb_encoder {
+	struct drm_connector connector;
+	struct drm_encoder encoder;
+	struct imx_ldb_channel *channel;
+};
+
 struct imx_ldb;
 
 struct imx_ldb_channel {
 	struct imx_ldb *ldb;
-	struct drm_connector connector;
-	struct drm_encoder encoder;
 
 	/* Defines what is connected to the ldb, only one at a time */
 	struct drm_panel *panel;
@@ -70,12 +77,12 @@ struct imx_ldb_channel {
 
 static inline struct imx_ldb_channel *con_to_imx_ldb_ch(struct drm_connector *c)
 {
-	return container_of(c, struct imx_ldb_channel, connector);
+	return container_of(c, struct imx_ldb_encoder, connector)->channel;
 }
 
 static inline struct imx_ldb_channel *enc_to_imx_ldb_ch(struct drm_encoder *e)
 {
-	return container_of(e, struct imx_ldb_channel, encoder);
+	return container_of(e, struct imx_ldb_encoder, encoder)->channel;
 }
 
 struct bus_mux {
@@ -411,12 +418,19 @@ static int imx_ldb_register(struct drm_device *drm,
 	struct imx_ldb_channel *imx_ldb_ch)
 {
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	struct drm_connector *connector = &imx_ldb_ch->connector;
-	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
+	struct imx_ldb_encoder *ldb_encoder;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
 	int ret;
 
-	memset(connector, 0, sizeof(*connector));
-	memset(encoder, 0, sizeof(*encoder));
+	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;
 
 	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
 	if (ret)
@@ -433,7 +447,6 @@ static int imx_ldb_register(struct drm_device *drm,
 	}
 
 	drm_encoder_helper_add(encoder, &imx_ldb_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
 
 	if (imx_ldb_ch->bridge) {
 		ret = drm_bridge_attach(encoder, imx_ldb_ch->bridge, NULL, 0);
-- 
2.20.1

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

[PATCH v4 16/19] drm/imx: imx-tve: use drm managed resources

[Philipp Zabel]

Use drmm_simple_encoder_alloc() to align encoder memory lifetime with
the drm device. drm_encoder_cleanup() is called automatically before
the memory is freed.
Also fold imx_tve_register() into imx_tve_bind().

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_simple_encoder_alloc()
 - fold imx_tve_register() into imx_tve_bind()
---
 drivers/gpu/drm/imx/imx-tve.c | 74 ++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 3ef71f688f79..bc8c3f802a15 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -19,6 +19,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -99,9 +100,13 @@ enum {
 	TVE_MODE_VGA,
 };
 
-struct imx_tve {
+struct imx_tve_encoder {
 	struct drm_connector connector;
 	struct drm_encoder encoder;
+	struct imx_tve *tve;
+};
+
+struct imx_tve {
 	struct device *dev;
 	int mode;
 	int di_hsync_pin;
@@ -118,12 +123,12 @@ struct imx_tve {
 
 static inline struct imx_tve *con_to_tve(struct drm_connector *c)
 {
-	return container_of(c, struct imx_tve, connector);
+	return container_of(c, struct imx_tve_encoder, connector)->tve;
 }
 
 static inline struct imx_tve *enc_to_tve(struct drm_encoder *e)
 {
-	return container_of(e, struct imx_tve, encoder);
+	return container_of(e, struct imx_tve_encoder, encoder)->tve;
 }
 
 static void tve_enable(struct imx_tve *tve)
@@ -428,37 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
 	return 0;
 }
 
-static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
-{
-	struct drm_encoder *encoder = &tve->encoder;
-	struct drm_connector *connector = &tve->connector;
-	int encoder_type;
-	int ret;
-
-	encoder_type = tve->mode == TVE_MODE_VGA ?
-				DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
-
-	memset(connector, 0, sizeof(*connector));
-	memset(encoder, 0, sizeof(*encoder));
-
-	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
-	if (ret)
-		return ret;
-
-	drm_encoder_helper_add(encoder, &imx_tve_encoder_helper_funcs);
-	drm_simple_encoder_init(drm, encoder, encoder_type);
-
-	drm_connector_helper_add(connector, &imx_tve_connector_helper_funcs);
-	drm_connector_init_with_ddc(drm, connector,
-				    &imx_tve_connector_funcs,
-				    DRM_MODE_CONNECTOR_VGA,
-				    tve->ddc);
-
-	drm_connector_attach_encoder(connector, encoder);
-
-	return 0;
-}
-
 static void imx_tve_disable_regulator(void *data)
 {
 	struct imx_tve *tve = data;
@@ -508,8 +482,38 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 	struct imx_tve *tve = dev_get_drvdata(dev);
+	struct imx_tve_encoder *tvee;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	int encoder_type;
+	int ret;
+
+	encoder_type = tve->mode == TVE_MODE_VGA ?
+		       DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
+
+	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 = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
+	if (ret)
+		return ret;
+
+	drm_encoder_helper_add(encoder, &imx_tve_encoder_helper_funcs);
+
+	drm_connector_helper_add(connector, &imx_tve_connector_helper_funcs);
+	ret = drm_connector_init_with_ddc(drm, connector,
+					  &imx_tve_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA, tve->ddc);
+	if (ret)
+		return ret;
 
-	return imx_tve_register(drm, tve);
+	return drm_connector_attach_encoder(connector, encoder);
 }
 
 static const struct component_ops imx_tve_ops = {
-- 
2.20.1

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

[PATCH v4 17/19] drm/imx: parallel-display: use drm managed resources

[Philipp Zabel]

Use drmm_simple_encoder_alloc() to align encoder memory lifetime with
the drm device. drm_encoder_cleanup() is called automatically before
the memory is freed.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_simple_encoder_alloc()
---
 drivers/gpu/drm/imx/parallel-display.c | 57 +++++++++++++-------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 42b44dbf45f5..9b1ec7e73c30 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -15,6 +15,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
@@ -22,10 +23,14 @@
 
 #include "imx-drm.h"
 
-struct imx_parallel_display {
+struct imx_parallel_display_encoder {
 	struct drm_connector connector;
 	struct drm_encoder encoder;
 	struct drm_bridge bridge;
+	struct imx_parallel_display *pd;
+};
+
+struct imx_parallel_display {
 	struct device *dev;
 	void *edid;
 	u32 bus_format;
@@ -37,12 +42,12 @@ struct imx_parallel_display {
 
 static inline struct imx_parallel_display *con_to_imxpd(struct drm_connector *c)
 {
-	return container_of(c, struct imx_parallel_display, connector);
+	return container_of(c, struct imx_parallel_display_encoder, connector)->pd;
 }
 
 static inline struct imx_parallel_display *bridge_to_imxpd(struct drm_bridge *b)
 {
-	return container_of(b, struct imx_parallel_display, bridge);
+	return container_of(b, struct imx_parallel_display_encoder, bridge)->pd;
 }
 
 static int imx_pd_connector_get_modes(struct drm_connector *connector)
@@ -253,17 +258,25 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
 	.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
 };
 
-static int imx_pd_register(struct drm_device *drm,
-	struct imx_parallel_display *imxpd)
+static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 {
-	struct drm_connector *connector = &imxpd->connector;
-	struct drm_encoder *encoder = &imxpd->encoder;
-	struct drm_bridge *bridge = &imxpd->bridge;
+	struct drm_device *drm = data;
+	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
+	struct imx_parallel_display_encoder *imxpd_encoder;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	struct drm_bridge *bridge;
 	int ret;
 
-	memset(connector, 0, sizeof(*connector));
-	memset(encoder, 0, sizeof(*encoder));
-	memset(bridge, 0, sizeof(*bridge));
+	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 = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
 	if (ret)
@@ -276,18 +289,9 @@ static int imx_pd_register(struct drm_device *drm,
 	 */
 	connector->dpms = DRM_MODE_DPMS_OFF;
 
-	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
-
 	bridge->funcs = &imx_pd_bridge_funcs;
 	drm_bridge_attach(encoder, bridge, NULL, 0);
 
-	if (!imxpd->next_bridge) {
-		drm_connector_helper_add(connector,
-					 &imx_pd_connector_helper_funcs);
-		drm_connector_init(drm, connector, &imx_pd_connector_funcs,
-				   DRM_MODE_CONNECTOR_DPI);
-	}
-
 	if (imxpd->next_bridge) {
 		ret = drm_bridge_attach(encoder, imxpd->next_bridge, bridge, 0);
 		if (ret < 0) {
@@ -296,20 +300,17 @@ static int imx_pd_register(struct drm_device *drm,
 			return ret;
 		}
 	} else {
+		drm_connector_helper_add(connector,
+					 &imx_pd_connector_helper_funcs);
+		drm_connector_init(drm, connector, &imx_pd_connector_funcs,
+				   DRM_MODE_CONNECTOR_DPI);
+
 		drm_connector_attach_encoder(connector, encoder);
 	}
 
 	return 0;
 }
 
-static int imx_pd_bind(struct device *dev, struct device *master, void *data)
-{
-	struct drm_device *drm = data;
-	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
-
-	return imx_pd_register(drm, imxpd);
-}
-
 static const struct component_ops imx_pd_ops = {
 	.bind	= imx_pd_bind,
 };
-- 
2.20.1

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

[PATCH v4 18/19] drm/imx: ipuv3-plane: use drm managed resources

[Philipp Zabel]

Use drmm_universal_plane_alloc() to align plane memory lifetime with
the drm device. drm_plane_cleanup() is called automatically before the
memory is freed.
Also move the call to ipu_plane_get_resources() into ipu_plane_init()
and use drm managed resources to put IPU resources automatically when
required. Handle error return values of the plane property creation
functions.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_universal_plane_alloc()
 - squash with patch "drm/imx: move call to ipu_plane_get_resources()
   into ipu_plane_init()"
---
 drivers/gpu/drm/imx/ipuv3-crtc.c  | 27 +-----------
 drivers/gpu/drm/imx/ipuv3-plane.c | 69 +++++++++++++++----------------
 drivers/gpu/drm/imx/ipuv3-plane.h |  3 --
 3 files changed, 36 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 7ebd99ee3240..6ce8fa4348c9 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -384,29 +384,14 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 	drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, NULL,
 				  &ipu_crtc_funcs, NULL);
 
-	ret = ipu_plane_get_resources(ipu_crtc->plane[0]);
-	if (ret) {
-		dev_err(ipu_crtc->dev, "getting plane 0 resources failed with %d.\n",
-			ret);
-		goto err_put_resources;
-	}
-
 	/* If this crtc is using the DP, add an overlay plane */
 	if (pdata->dp >= 0 && pdata->dma[1] > 0) {
 		ipu_crtc->plane[1] = ipu_plane_init(drm, ipu, pdata->dma[1],
 						IPU_DP_FLOW_SYNC_FG,
 						drm_crtc_mask(&ipu_crtc->base),
 						DRM_PLANE_TYPE_OVERLAY);
-		if (IS_ERR(ipu_crtc->plane[1])) {
+		if (IS_ERR(ipu_crtc->plane[1]))
 			ipu_crtc->plane[1] = NULL;
-		} else {
-			ret = ipu_plane_get_resources(ipu_crtc->plane[1]);
-			if (ret) {
-				dev_err(ipu_crtc->dev, "getting plane 1 "
-					"resources failed with %d.\n", ret);
-				goto err_put_plane0_res;
-			}
-		}
 	}
 
 	ipu_crtc->irq = ipu_plane_irq(ipu_crtc->plane[0]);
@@ -414,18 +399,13 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 			"imx_drm", ipu_crtc);
 	if (ret < 0) {
 		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
-		goto err_put_plane1_res;
+		goto err_put_resources;
 	}
 	/* Only enable IRQ when we actually need it to trigger work. */
 	disable_irq(ipu_crtc->irq);
 
 	return 0;
 
-err_put_plane1_res:
-	if (ipu_crtc->plane[1])
-		ipu_plane_put_resources(ipu_crtc->plane[1]);
-err_put_plane0_res:
-	ipu_plane_put_resources(ipu_crtc->plane[0]);
 err_put_resources:
 	ipu_put_resources(ipu_crtc);
 
@@ -452,9 +432,6 @@ static void ipu_drm_unbind(struct device *dev, struct device *master,
 	struct ipu_crtc *ipu_crtc = dev_get_drvdata(dev);
 
 	ipu_put_resources(ipu_crtc);
-	if (ipu_crtc->plane[1])
-		ipu_plane_put_resources(ipu_crtc->plane[1]);
-	ipu_plane_put_resources(ipu_crtc->plane[0]);
 }
 
 static const struct component_ops ipu_crtc_ops = {
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 8a4235d9d9f1..075508051b5f 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -11,6 +11,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_plane_helper.h>
 
 #include <video/imx-ipu-v3.h>
@@ -142,8 +143,10 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
 	       fb->format->cpp[2] * x - eba;
 }
 
-void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
+static void ipu_plane_put_resources(struct drm_device *dev, void *ptr)
 {
+	struct ipu_plane *ipu_plane = ptr;
+
 	if (!IS_ERR_OR_NULL(ipu_plane->dp))
 		ipu_dp_put(ipu_plane->dp);
 	if (!IS_ERR_OR_NULL(ipu_plane->dmfc))
@@ -154,7 +157,8 @@ void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
 		ipu_idmac_put(ipu_plane->alpha_ch);
 }
 
-int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
+static int ipu_plane_get_resources(struct drm_device *dev,
+				   struct ipu_plane *ipu_plane)
 {
 	int ret;
 	int alpha_ch;
@@ -166,6 +170,10 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
 		return ret;
 	}
 
+	ret = drmm_add_action_or_reset(dev, ipu_plane_put_resources, ipu_plane);
+	if (ret)
+		return ret;
+
 	alpha_ch = ipu_channel_alpha_channel(ipu_plane->dma);
 	if (alpha_ch >= 0) {
 		ipu_plane->alpha_ch = ipu_idmac_get(ipu_plane->ipu, alpha_ch);
@@ -181,7 +189,7 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
 	if (IS_ERR(ipu_plane->dmfc)) {
 		ret = PTR_ERR(ipu_plane->dmfc);
 		DRM_ERROR("failed to get dmfc: ret %d\n", ret);
-		goto err_out;
+		return ret;
 	}
 
 	if (ipu_plane->dp_flow >= 0) {
@@ -189,15 +197,11 @@ int ipu_plane_get_resources(struct ipu_plane *ipu_plane)
 		if (IS_ERR(ipu_plane->dp)) {
 			ret = PTR_ERR(ipu_plane->dp);
 			DRM_ERROR("failed to get dp flow: %d\n", ret);
-			goto err_out;
+			return ret;
 		}
 	}
 
 	return 0;
-err_out:
-	ipu_plane_put_resources(ipu_plane);
-
-	return ret;
 }
 
 static bool ipu_plane_separate_alpha(struct ipu_plane *ipu_plane)
@@ -262,16 +266,6 @@ void ipu_plane_disable_deferred(struct drm_plane *plane)
 }
 EXPORT_SYMBOL_GPL(ipu_plane_disable_deferred);
 
-static void ipu_plane_destroy(struct drm_plane *plane)
-{
-	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
-
-	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
-
-	drm_plane_cleanup(plane);
-	kfree(ipu_plane);
-}
-
 static void ipu_plane_state_reset(struct drm_plane *plane)
 {
 	unsigned int zpos = (plane->type == DRM_PLANE_TYPE_PRIMARY) ? 0 : 1;
@@ -336,7 +330,6 @@ static bool ipu_plane_format_mod_supported(struct drm_plane *plane,
 static const struct drm_plane_funcs ipu_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
-	.destroy	= ipu_plane_destroy,
 	.reset		= ipu_plane_state_reset,
 	.atomic_duplicate_state	= ipu_plane_duplicate_state,
 	.atomic_destroy_state	= ipu_plane_destroy_state,
@@ -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 = kzalloc(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,22 +845,23 @@ 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 plane\n");
-		kfree(ipu_plane);
-		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)
-		drm_plane_create_zpos_property(&ipu_plane->base, zpos, 0, 1);
+		ret = drm_plane_create_zpos_property(&ipu_plane->base, zpos, 0,
+						     1);
 	else
-		drm_plane_create_zpos_immutable_property(&ipu_plane->base, 0);
+		ret = drm_plane_create_zpos_immutable_property(&ipu_plane->base,
+							       0);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = ipu_plane_get_resources(dev, ipu_plane);
+	if (ret) {
+		DRM_ERROR("failed to get %s plane resources: %pe\n",
+			  zpos ? "overlay" : "primary", &ret);
+		return ERR_PTR(ret);
+	}
 
 	return ipu_plane;
 }
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
index ffacbcdd2f98..6d544e6ce63f 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.h
+++ b/drivers/gpu/drm/imx/ipuv3-plane.h
@@ -41,9 +41,6 @@ int ipu_plane_mode_set(struct ipu_plane *plane, struct drm_crtc *crtc,
 		       uint32_t src_x, uint32_t src_y, uint32_t src_w,
 		       uint32_t src_h, bool interlaced);
 
-int ipu_plane_get_resources(struct ipu_plane *plane);
-void ipu_plane_put_resources(struct ipu_plane *plane);
-
 int ipu_plane_irq(struct ipu_plane *plane);
 
 void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel);
-- 
2.20.1

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

[PATCH v4 19/19] drm/imx: ipuv3-crtc: use drm managed resources

[Philipp Zabel]

Use use drmm_crtc_alloc_with_planes() to align crtc memory lifetime
with the drm device. drm_crtc_cleanup() is called automatically before
the memory is freed.
Also use drmm_add_action_or_reset() to make sure IPU resources are
released automatically.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - use drmm_crtc_alloc_with_planes()
 - merge ipu_crtc_init into ipu_drm_bind
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 114 ++++++++++++-------------------
 1 file changed, 43 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 6ce8fa4348c9..e6431a227feb 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -20,6 +20,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
@@ -163,7 +164,6 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
 
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
-	.destroy = drm_crtc_cleanup,
 	.page_flip = drm_atomic_helper_page_flip,
 	.reset = imx_drm_crtc_reset,
 	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
@@ -322,67 +322,74 @@ static const struct drm_crtc_helper_funcs ipu_helper_funcs = {
 	.atomic_enable = ipu_crtc_atomic_enable,
 };
 
-static void ipu_put_resources(struct ipu_crtc *ipu_crtc)
+static void ipu_put_resources(struct drm_device *dev, void *ptr)
 {
+	struct ipu_crtc *ipu_crtc = ptr;
+
 	if (!IS_ERR_OR_NULL(ipu_crtc->dc))
 		ipu_dc_put(ipu_crtc->dc);
 	if (!IS_ERR_OR_NULL(ipu_crtc->di))
 		ipu_di_put(ipu_crtc->di);
 }
 
-static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
-		struct ipu_client_platformdata *pdata)
+static int ipu_get_resources(struct drm_device *dev, struct ipu_crtc *ipu_crtc,
+			     struct ipu_client_platformdata *pdata)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 	int ret;
 
 	ipu_crtc->dc = ipu_dc_get(ipu, pdata->dc);
-	if (IS_ERR(ipu_crtc->dc)) {
-		ret = PTR_ERR(ipu_crtc->dc);
-		goto err_out;
-	}
+	if (IS_ERR(ipu_crtc->dc))
+		return PTR_ERR(ipu_crtc->dc);
+
+	ret = drmm_add_action_or_reset(dev, ipu_put_resources, ipu_crtc);
+	if (ret)
+		return ret;
 
 	ipu_crtc->di = ipu_di_get(ipu, pdata->di);
-	if (IS_ERR(ipu_crtc->di)) {
-		ret = PTR_ERR(ipu_crtc->di);
-		goto err_out;
-	}
+	if (IS_ERR(ipu_crtc->di))
+		return PTR_ERR(ipu_crtc->di);
 
 	return 0;
-err_out:
-	ipu_put_resources(ipu_crtc);
-
-	return ret;
 }
 
-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(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]);
-		goto err_put_resources;
-	}
+	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);
-	drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base, NULL,
-				  &ipu_crtc_funcs, NULL);
+
+	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) {
@@ -399,50 +406,21 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 			"imx_drm", ipu_crtc);
 	if (ret < 0) {
 		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
-		goto err_put_resources;
+		return ret;
 	}
 	/* Only enable IRQ when we actually need it to trigger work. */
 	disable_irq(ipu_crtc->irq);
 
 	return 0;
-
-err_put_resources:
-	ipu_put_resources(ipu_crtc);
-
-	return ret;
-}
-
-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 = dev_get_drvdata(dev);
-	memset(ipu_crtc, 0, sizeof(*ipu_crtc));
-
-	ipu_crtc->dev = dev;
-
-	return ipu_crtc_init(ipu_crtc, pdata, drm);
-}
-
-static void ipu_drm_unbind(struct device *dev, struct device *master,
-	void *data)
-{
-	struct ipu_crtc *ipu_crtc = dev_get_drvdata(dev);
-
-	ipu_put_resources(ipu_crtc);
 }
 
 static const struct component_ops ipu_crtc_ops = {
 	.bind = ipu_drm_bind,
-	.unbind = ipu_drm_unbind,
 };
 
 static int ipu_drm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ipu_crtc *ipu_crtc;
 	int ret;
 
 	if (!dev->platform_data)
@@ -452,12 +430,6 @@ static int ipu_drm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
-	if (!ipu_crtc)
-		return -ENOMEM;
-
-	dev_set_drvdata(dev, ipu_crtc);
-
 	return component_add(dev, &ipu_crtc_ops);
 }
 
-- 
2.20.1

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

Re: [PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Philipp Zabel]

On Tue, 2020-12-08 at 16:54 +0100, Philipp Zabel wrote:
> Hi,
> 
> this is an update of the drmm_(simple_)encoder_alloc(),
> drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
> functions in v3 [1] together with the imx-drm managed allocation
> conversion from [2] as an example usage.
> a bit.
  ^^^^^^
this is a left-over of ", reordered an squashed a bit." before I decided
to move it into the list of changes below.

> 
> Changes since v3:
[...]
>  - Reorder and squash the imx-drm managed allocation conversion patches
>    to use the new functions directly.

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

Re: [PATCH v4 01/19] drm/encoder: make encoder control functions optional

[Sam Ravnborg]

Hi Philipp,
On Tue, Dec 08, 2020 at 04:54:33PM +0100, Philipp Zabel wrote:
> Simple managed encoders do not require the .destroy callback,
> make the whole funcs structure optional.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> New in v4.
> ---
>  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);

So late_register and early_unregister are both optional.
But if encoder->funcs is set then the destroy callback is mandatory.

I am just wondering if this is intended.
Reding the documnetation of drm_encoder_funcs thist matches the
documentation but anyway..

With this comment considered,
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 01/19] drm/encoder: make encoder control functions optional

[Philipp Zabel]

Hi Sam,

On Tue, 2020-12-08 at 19:48 +0100, Sam Ravnborg wrote:
> Hi Philipp,
> On Tue, Dec 08, 2020 at 04:54:33PM +0100, Philipp Zabel wrote:
> > Simple managed encoders do not require the .destroy callback,
> > make the whole funcs structure optional.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > New in v4.
> > ---
> >  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_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
[...]
> > @@ -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);
> 
> So late_register and early_unregister are both optional.
> But if encoder->funcs is set then the destroy callback is mandatory.

For encoders that are kept on the mode_config.encoder_list until
drm_mode_config_cleanup() is called, the destroy callback is still
mandatory.

Encoders allocated with drmm_encoder_alloc() on the other hand should
have the destroy callback set to NULL, if encoder->funcs is set.
These encoders are removed from the mode_config.encoder_list by the drmm
cleanup code, before drm_mode_config_cleanup is called.

> I am just wondering if this is intended.
> Reding the documnetation of drm_encoder_funcs thist matches the
> documentation but anyway..
>
> With this comment considered,
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Thank you for bringing this up, should we just leave
drm_mode_config_cleanup() as-is?

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

Re: [PATCH v4 01/19] drm/encoder: make encoder control functions optional

[Daniel Vetter]

On Wed, Dec 09, 2020 at 11:58:44AM +0100, Philipp Zabel wrote:
> Hi Sam,
> 
> On Tue, 2020-12-08 at 19:48 +0100, Sam Ravnborg wrote:
> > Hi Philipp,
> > On Tue, Dec 08, 2020 at 04:54:33PM +0100, Philipp Zabel wrote:
> > > Simple managed encoders do not require the .destroy callback,
> > > make the whole funcs structure optional.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > New in v4.
> > > ---
> > >  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_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
> [...]
> > > @@ -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);
> > 
> > So late_register and early_unregister are both optional.
> > But if encoder->funcs is set then the destroy callback is mandatory.
> 
> For encoders that are kept on the mode_config.encoder_list until
> drm_mode_config_cleanup() is called, the destroy callback is still
> mandatory.
> 
> Encoders allocated with drmm_encoder_alloc() on the other hand should
> have the destroy callback set to NULL, if encoder->funcs is set.
> These encoders are removed from the mode_config.encoder_list by the drmm
> cleanup code, before drm_mode_config_cleanup is called.
> 
> > I am just wondering if this is intended.
> > Reding the documnetation of drm_encoder_funcs thist matches the
> > documentation but anyway..
> >
> > With this comment considered,
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Thank you for bringing this up, should we just leave
> drm_mode_config_cleanup() as-is?

Yup, not having a funcs->destroy there is a bug. Maybe should even warn
about the lack of that in the traditional non-drmm_ encoder init function.
-Daniel

> 
> regards
> Philipp
> _______________________________________________
> 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

Re: [PATCH v4 01/19] drm/encoder: make encoder control functions optional

[Daniel Vetter]

On Wed, Dec 09, 2020 at 04:58:05PM +0100, Daniel Vetter wrote:
> On Wed, Dec 09, 2020 at 11:58:44AM +0100, Philipp Zabel wrote:
> > Hi Sam,
> > 
> > On Tue, 2020-12-08 at 19:48 +0100, Sam Ravnborg wrote:
> > > Hi Philipp,
> > > On Tue, Dec 08, 2020 at 04:54:33PM +0100, Philipp Zabel wrote:
> > > > Simple managed encoders do not require the .destroy callback,
> > > > make the whole funcs structure optional.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > New in v4.
> > > > ---
> > > >  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_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
> > [...]
> > > > @@ -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);
> > > 
> > > So late_register and early_unregister are both optional.
> > > But if encoder->funcs is set then the destroy callback is mandatory.
> > 
> > For encoders that are kept on the mode_config.encoder_list until
> > drm_mode_config_cleanup() is called, the destroy callback is still
> > mandatory.
> > 
> > Encoders allocated with drmm_encoder_alloc() on the other hand should
> > have the destroy callback set to NULL, if encoder->funcs is set.
> > These encoders are removed from the mode_config.encoder_list by the drmm
> > cleanup code, before drm_mode_config_cleanup is called.
> > 
> > > I am just wondering if this is intended.
> > > Reding the documnetation of drm_encoder_funcs thist matches the
> > > documentation but anyway..
> > >
> > > With this comment considered,
> > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> > Thank you for bringing this up, should we just leave
> > drm_mode_config_cleanup() as-is?
> 
> Yup, not having a funcs->destroy there is a bug. Maybe should even warn
> about the lack of that in the traditional non-drmm_ encoder init function.

Forgot to add: With that Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -Daniel
> 
> > 
> > regards
> > Philipp
> > _______________________________________________
> > 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

-- 
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

Re: [PATCH v4 02/19] drm: add drmm_encoder_alloc()

[Daniel Vetter]

On Tue, Dec 08, 2020 at 04:54:34PM +0100, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
>  - allow the funcs parameter to __drmm_encoder_alloc() to be NULL
> ---
>  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 b0b86a3c08f5..cc0edb8361e8 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.

I think this should also mention that ->destroy should kfree() the encoder
structure, and it should not be allocated with devm_kzalloc.

Also a hint to use drmm_encoder_alloc instead would be good to point
people in the right direction.

> + *
> + * 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;

WARN_ON(!funcs->destroy);

WARN_ON(!funcs->destroy);


> +
> +	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 833123637fbf..fb2f56c006db 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -194,6 +194,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

@funcsw are optional in this variant.

> + * @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), \

Need to upcast with container_of or this breaks if the base class is in
the wrong spot.

> +				      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

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.20.1
> 

-- 
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

Re: [PATCH v4 03/19] drm/simple_kms_helper: add drmm_simple_encoder_alloc()

[Daniel Vetter]

On Tue, Dec 08, 2020 at 04:54:35PM +0100, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
>  - drop drmm_simple_encoder_funcs_empty, now that encoder control
>    functions are optional
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c |  9 +++++++++
>  include/drm/drm_simple_kms_helper.h     | 24 ++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 743e57c1b44f..ccf0908eb82e 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,14 @@ int drm_simple_encoder_init(struct drm_device *dev,

Please also address the FIXME in the kerneldoc for drm_simple_encoder_init
and replace it with a recommendation to use this new function you're
adding here.

>  }
>  EXPORT_SYMBOL(drm_simple_encoder_init);
>  
> +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, NULL, 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), \

Same comment about container_of. Also, since this is a dummy encoder, do
we really need the subclassing support here? I guess it makes sense for
consistency at least, which is always good.

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +					     offsetof(type, member), \
> +					     encoder_type))
> +
>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> -- 
> 2.20.1
> 

-- 
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

Re: [PATCH v4 04/19] drm/plane: add drmm_universal_plane_alloc()

[Daniel Vetter]

On Tue, Dec 08, 2020 at 04:54:36PM +0100, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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 e6231947f987..6b1261fe09fb 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.
> + *

Same as with the others, please include a note that funcs->destroy must
clean stuff up and call kfree(), and that drivers should consider the new
drmm_ version instead.

> + * 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;

I think WARN_ON(!funcs.destroy); maybe.

> +
> +	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 1d82b264e5e4..8ef06ee1c8eb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -764,6 +764,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.

I wonder whether we should warn about this somewhere ...

> + *
> + * 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), \

Type upcast with container_of

With the nits addressed somehow:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +					      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
> 

-- 
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

Re: [PATCH v4 05/19] drm/crtc: add drmm_crtc_alloc_with_planes()

[Daniel Vetter]

On Tue, Dec 08, 2020 at 04:54:37PM +0100, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Same bikesheds as with the others, imo important one is to point at the
new drmm_ version from the old kerneldoc. The others just make sure you
roll them out consistently (or not, if you don't like my suggestions).

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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 f927976eca50..668c31332230 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>
> @@ -240,30 +241,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *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;
> @@ -291,11 +274,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));
> @@ -339,8 +318,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 ba839e5e357d..9053f80b613f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1223,6 +1223,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
> 

-- 
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

Re: [PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Daniel Vetter]

On Tue, Dec 08, 2020 at 04:59:16PM +0100, Philipp Zabel wrote:
> On Tue, 2020-12-08 at 16:54 +0100, Philipp Zabel wrote:
> > Hi,
> > 
> > this is an update of the drmm_(simple_)encoder_alloc(),
> > drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
> > functions in v3 [1] together with the imx-drm managed allocation
> > conversion from [2] as an example usage.
> > a bit.
>   ^^^^^^
> this is a left-over of ", reordered an squashed a bit." before I decided
> to move it into the list of changes below.
> 
> > 
> > Changes since v3:
> [...]
> >  - Reorder and squash the imx-drm managed allocation conversion patches
> >    to use the new functions directly.

imx parts all look good. One thing I spotted is that you could use
devm_drm_dev_alloc instead of drm_dev_alloc, at least my tree here doesn't
have that one yet. Feel free to add a-b: me on top of the imx patches too,
but probably not worth much :-)
-Daniel
-- 
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

Re: [PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Daniel Vetter]

On Wed, Dec 09, 2020 at 10:10:47PM +0100, Daniel Vetter wrote:
> On Tue, Dec 08, 2020 at 04:59:16PM +0100, Philipp Zabel wrote:
> > On Tue, 2020-12-08 at 16:54 +0100, Philipp Zabel wrote:
> > > Hi,
> > > 
> > > this is an update of the drmm_(simple_)encoder_alloc(),
> > > drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
> > > functions in v3 [1] together with the imx-drm managed allocation
> > > conversion from [2] as an example usage.
> > > a bit.
> >   ^^^^^^
> > this is a left-over of ", reordered an squashed a bit." before I decided
> > to move it into the list of changes below.
> > 
> > > 
> > > Changes since v3:
> > [...]
> > >  - Reorder and squash the imx-drm managed allocation conversion patches
> > >    to use the new functions directly.
> 
> imx parts all look good. One thing I spotted is that you could use
> devm_drm_dev_alloc instead of drm_dev_alloc, at least my tree here doesn't
> have that one yet. Feel free to add a-b: me on top of the imx patches too,
> but probably not worth much :-)

Oh also for merging, ack for merging through whatever tree you feel like.
Since 5.11 merge window is done already if you go through imx please make
sure to send a feature pull soon after -rc1 so it's not holding up
Laurent. Or coordinate with Laurent (and maybe others).
-Daniel
-- 
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

Re: [PATCH v4 02/19] drm: add drmm_encoder_alloc()

[Philipp Zabel]

Hi Daniel,

thank you for the review. I'll work in all your other comments, there's
just one I'm not sure what to do about:

On Wed, 2020-12-09 at 17:05 +0100, Daniel Vetter wrote:
[...]
> > +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);
> > +
[...]
> > + * @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), \
> 
> Need to upcast with container_of or this breaks if the base class is in
> the wrong spot.

This is modeled after devm_drm_dev_alloc(). Like __devm_drm_dev_alloc(),
__drmm_encoder_alloc() returns a pointer to the allocated container
structure, not a pointer to the embedded struct drm_encoder. I think
this direct cast is correct, unless you suggest that
__drmm_encoder_alloc should return encoder instead of container?

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

Re: [PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Laurent Pinchart]

Hi Daniel,

On Wed, Dec 09, 2020 at 10:13:54PM +0100, Daniel Vetter wrote:
> On Wed, Dec 09, 2020 at 10:10:47PM +0100, Daniel Vetter wrote:
> > On Tue, Dec 08, 2020 at 04:59:16PM +0100, Philipp Zabel wrote:
> > > On Tue, 2020-12-08 at 16:54 +0100, Philipp Zabel wrote:
> > > > Hi,
> > > > 
> > > > this is an update of the drmm_(simple_)encoder_alloc(),
> > > > drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
> > > > functions in v3 [1] together with the imx-drm managed allocation
> > > > conversion from [2] as an example usage.
> > > > a bit.
> > >   ^^^^^^
> > > this is a left-over of ", reordered an squashed a bit." before I decided
> > > to move it into the list of changes below.
> > > 
> > > > 
> > > > Changes since v3:
> > > [...]
> > > >  - Reorder and squash the imx-drm managed allocation conversion patches
> > > >    to use the new functions directly.
> > 
> > imx parts all look good. One thing I spotted is that you could use
> > devm_drm_dev_alloc instead of drm_dev_alloc, at least my tree here doesn't
> > have that one yet. Feel free to add a-b: me on top of the imx patches too,
> > but probably not worth much :-)
> 
> Oh also for merging, ack for merging through whatever tree you feel like.
> Since 5.11 merge window is done already if you go through imx please make
> sure to send a feature pull soon after -rc1 so it's not holding up
> Laurent. Or coordinate with Laurent (and maybe others).

I've sent my rcar-du fixes without depending on this series, to ease
backporting. I'll then submit additional cleanups using the new managed
allocators when these patches will hit the drm tree. There's thus no
need for a feature branch on my side.

-- 
Regards,

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

Re: [PATCH v4 02/19] drm: add drmm_encoder_alloc()

[Daniel Vetter]

On Thu, Dec 10, 2020 at 12:59 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Daniel,
>
> thank you for the review. I'll work in all your other comments, there's
> just one I'm not sure what to do about:
>
> On Wed, 2020-12-09 at 17:05 +0100, Daniel Vetter wrote:
> [...]
> > > +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);
> > > +
> [...]
> > > + * @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), \
> >
> > Need to upcast with container_of or this breaks if the base class is in
> > the wrong spot.
>
> This is modeled after devm_drm_dev_alloc(). Like __devm_drm_dev_alloc(),
> __drmm_encoder_alloc() returns a pointer to the allocated container
> structure, not a pointer to the embedded struct drm_encoder. I think
> this direct cast is correct, unless you suggest that
> __drmm_encoder_alloc should return encoder instead of container?

Uh sorry, I misread and forgot how __devm_drm_dev_alloc works too.
Looks all correct.
-Daniel
-- 
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

Re: [PATCH v4 00/19] drm: managed encoder/plane/crtc allocation

[Philipp Zabel]

Hi Laurent, Daniel,

On Thu, 2020-12-10 at 14:19 +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wed, Dec 09, 2020 at 10:13:54PM +0100, Daniel Vetter wrote:
> > On Wed, Dec 09, 2020 at 10:10:47PM +0100, Daniel Vetter wrote:
> > > On Tue, Dec 08, 2020 at 04:59:16PM +0100, Philipp Zabel wrote:
> > > > On Tue, 2020-12-08 at 16:54 +0100, Philipp Zabel wrote:
> > > > > Hi,
> > > > > 
> > > > > this is an update of the drmm_(simple_)encoder_alloc(),
> > > > > drmm_universal_plane_alloc(), and drmm_crtc_alloc_with_plane()
> > > > > functions in v3 [1] together with the imx-drm managed allocation
> > > > > conversion from [2] as an example usage.
> > > > > a bit.
> > > >   ^^^^^^
> > > > this is a left-over of ", reordered an squashed a bit." before I decided
> > > > to move it into the list of changes below.
> > > > 
> > > > > Changes since v3:
> > > > [...]
> > > > >  - Reorder and squash the imx-drm managed allocation conversion patches
> > > > >    to use the new functions directly.
> > > 
> > > imx parts all look good. One thing I spotted is that you could use
> > > devm_drm_dev_alloc instead of drm_dev_alloc, at least my tree here doesn't
> > > have that one yet. Feel free to add a-b: me on top of the imx patches too,
> > > but probably not worth much :-)
> > 
> > Oh also for merging, ack for merging through whatever tree you feel like.
> > Since 5.11 merge window is done already if you go through imx please make
> > sure to send a feature pull soon after -rc1 so it's not holding up
> > Laurent.

If nobody minds, I'll do just that.

> >  Or coordinate with Laurent (and maybe others).
> 
> I've sent my rcar-du fixes without depending on this series, to ease
> backporting. I'll then submit additional cleanups using the new managed
> allocators when these patches will hit the drm tree. There's thus no
> need for a feature branch on my side.

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