[PATCH 0/4] drm/plane: Remove drm_plane_init(), plus other cleanups

[PATCH 0/4] drm/plane: Remove drm_plane_init(), plus other cleanups

[Thomas Zimmermann]

This patchset does cleanups to the plane code, most noteably it removes
drm_plane_init(). The function is a small wrapper, which can easily be
inlined into the few callers. Patch #1 fixes this.

The other clean-up patches #2 to #4 affect plane creation. Modesetting
helpers and nouveau share some plane-allocation code that can be shared as
helper function. While the function is already outdated, it's now at least
well documented. As suggested by Daniel, patch #3 adds a warning to
non-atomic plane helpers when they are being called from atomic drivers.
Patch #4 adds an initializer macro for non-atomic plane functions. It
should not be used in new drivers, but at least documents the current
practice.

Tested with nouveau on Nvidia G72 hardware.

A possible next step would be the inlining of drm_crtc_init() and the
removal of drm_plane.format_default.

Thomas Zimmermann (4):
  drm/plane: Remove drm_plane_init()
  drm/plane: Allocate planes with drm_universal_plane_alloc()
  drm/plane-helper: Warn if atomic drivers call non-atomic helpers
  drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro

 drivers/gpu/drm/drm_modeset_helper.c       | 68 +++++++++------------
 drivers/gpu/drm/drm_plane.c                | 70 ++++++++++++----------
 drivers/gpu/drm/drm_plane_helper.c         | 10 ++++
 drivers/gpu/drm/nouveau/dispnv04/crtc.c    | 45 +++++---------
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 ++--
 drivers/gpu/drm/shmobile/shmob_drm_plane.c |  7 ++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  9 ++-
 include/drm/drm_plane.h                    | 52 +++++++++++++---
 include/drm/drm_plane_helper.h             | 12 ++++
 9 files changed, 162 insertions(+), 124 deletions(-)


base-commit: f2c3a05d33693ad51996fa7d12d3b2d4b0f372eb
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
-- 
2.37.2

[PATCH 1/4] drm/plane: Remove drm_plane_init()

[Thomas Zimmermann]

Open-code drm_plane_init() and remove the function from DRM. The
implementation of drm_plane_init() is a simple wrapper around a call
to drm_universal_plane_init(), so drivers can just use that instead.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_modeset_helper.c       |  3 +-
 drivers/gpu/drm/drm_plane.c                | 32 ----------------------
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 +++++----
 drivers/gpu/drm/shmobile/shmob_drm_plane.c |  7 +++--
 drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  9 +++---
 include/drm/drm_plane.h                    |  8 +-----
 6 files changed, 17 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index bd609a978848..611dd01fb604 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -100,8 +100,7 @@ EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
  * This is the minimal list of formats that seem to be safe for modeset use
  * with all current DRM drivers.  Most hardware can actually support more
  * formats than this and drivers may specify a more accurate list when
- * creating the primary plane.  However drivers that still call
- * drm_plane_init() will use this minimal format list as the default.
+ * creating the primary plane.
  */
 static const uint32_t safe_modeset_formats[] = {
 	DRM_FORMAT_XRGB8888,
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 726f2f163c26..0f14b4d3bb10 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -482,38 +482,6 @@ void drm_plane_unregister_all(struct drm_device *dev)
 	}
 }
 
-/**
- * drm_plane_init - Initialize a legacy plane
- * @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
- * @is_primary: plane type (primary vs overlay)
- *
- * Legacy API to initialize a DRM plane.
- *
- * New drivers should call drm_universal_plane_init() instead.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_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,
-		   bool is_primary)
-{
-	enum drm_plane_type type;
-
-	type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
-	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
-					formats, format_count,
-					NULL, type, NULL);
-}
-EXPORT_SYMBOL(drm_plane_init);
-
 /**
  * drm_plane_cleanup - Clean up the core plane usage
  * @plane: plane to cleanup
diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
index 37e63e98cd08..33f29736024a 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
@@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device)
 		break;
 	}
 
-	ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */,
-			     &nv10_plane_funcs,
-			     formats, num_formats, false);
+	ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's */,
+				       &nv10_plane_funcs,
+				       formats, num_formats, NULL,
+				       DRM_PLANE_TYPE_OVERLAY, NULL);
 	if (ret)
 		goto err;
 
@@ -475,9 +476,9 @@ nv04_overlay_init(struct drm_device *device)
 	if (!plane)
 		return;
 
-	ret = drm_plane_init(device, &plane->base, 1 /* single crtc */,
-			     &nv04_plane_funcs,
-			     formats, 2, false);
+	ret = drm_universal_plane_init(device, &plane->base, 1 /* single crtc */,
+				       &nv04_plane_funcs, formats, 2, NULL,
+				       DRM_PLANE_TYPE_OVERLAY, NULL);
 	if (ret)
 		goto err;
 
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
index 54228424793a..6c5f0cbe7d95 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
@@ -252,9 +252,10 @@ int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index)
 	splane->index = index;
 	splane->alpha = 255;
 
-	ret = drm_plane_init(sdev->ddev, &splane->plane, 1,
-			     &shmob_drm_plane_funcs, formats,
-			     ARRAY_SIZE(formats), false);
+	ret = drm_universal_plane_init(sdev->ddev, &splane->plane, 1,
+				       &shmob_drm_plane_funcs,
+				       formats, ARRAY_SIZE(formats), NULL,
+				       DRM_PLANE_TYPE_OVERLAY, NULL);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
index 0ccf791301cb..cf77a8ce7398 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -105,11 +105,10 @@ int tilcdc_plane_init(struct drm_device *dev,
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	int ret;
 
-	ret = drm_plane_init(dev, plane, 1,
-			     &tilcdc_plane_funcs,
-			     priv->pixelformats,
-			     priv->num_pixelformats,
-			     true);
+	ret = drm_universal_plane_init(dev, plane, 1, &tilcdc_plane_funcs,
+				       priv->pixelformats,
+				       priv->num_pixelformats,
+				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
 		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
 		return ret;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 89ea54652e87..910cb941f3d5 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -631,7 +631,7 @@ struct drm_plane {
 	unsigned int format_count;
 	/**
 	 * @format_default: driver hasn't supplied supported formats for the
-	 * plane. Used by the drm_plane_init compatibility wrapper only.
+	 * plane. Used by the non-atomic driver compatibility wrapper only.
 	 */
 	bool format_default;
 
@@ -762,12 +762,6 @@ int drm_universal_plane_init(struct drm_device *dev,
 			     const uint64_t *format_modifiers,
 			     enum drm_plane_type type,
 			     const char *name, ...);
-int drm_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,
-		   bool is_primary);
 void drm_plane_cleanup(struct drm_plane *plane);
 
 __printf(10, 11)
-- 
2.37.2

[PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Thomas Zimmermann]

Provide drm_univeral_plane_alloc(), which allocated an initializes a
plane. Code for non-atomic drivers uses this pattern. Convert it to
the new function. The modeset helpers contain a quirk for handling their
color formats differently. Set the flag outside plane allocation.

The new function is already deprecated to some extend. Drivers should
rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_modeset_helper.c    | 61 +++++++++++--------------
 drivers/gpu/drm/drm_plane.c             | 38 +++++++++++++++
 drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++-----------
 include/drm/drm_plane.h                 | 44 ++++++++++++++++++
 4 files changed, 121 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 611dd01fb604..38040eebfa16 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = {
 	.destroy = drm_plane_helper_destroy,
 };
 
-static struct drm_plane *create_primary_plane(struct drm_device *dev)
-{
-	struct drm_plane *primary;
-	int ret;
-
-	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-	if (primary == NULL) {
-		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
-		return NULL;
-	}
-
-	/*
-	 * Remove the format_default field from drm_plane when dropping
-	 * this helper.
-	 */
-	primary->format_default = true;
-
-	/* possible_crtc's will be filled in later by crtc_init */
-	ret = drm_universal_plane_init(dev, primary, 0,
-				       &primary_plane_funcs,
-				       safe_modeset_formats,
-				       ARRAY_SIZE(safe_modeset_formats),
-				       NULL,
-				       DRM_PLANE_TYPE_PRIMARY, NULL);
-	if (ret) {
-		kfree(primary);
-		primary = NULL;
-	}
-
-	return primary;
-}
-
 /**
  * drm_crtc_init - Legacy CRTC initialization function
  * @dev: DRM device
@@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		  const struct drm_crtc_funcs *funcs)
 {
 	struct drm_plane *primary;
+	int ret;
+
+	/* possible_crtc's will be filled in later by crtc_init */
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &primary_plane_funcs,
+					      safe_modeset_formats,
+					      ARRAY_SIZE(safe_modeset_formats),
+					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
 
-	primary = create_primary_plane(dev);
-	return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs,
-					 NULL);
+	/*
+	 * Remove the format_default field from drm_plane when dropping
+	 * this helper.
+	 */
+	primary->format_default = true;
+
+	ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
+	if (ret)
+		goto err_drm_plane_cleanup;
+
+	return 0;
+
+err_drm_plane_cleanup:
+	drm_plane_cleanup(primary);
+	kfree(primary);
+	return ret;
 }
 EXPORT_SYMBOL(drm_crtc_init);
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0f14b4d3bb10..33357629a7f5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
 }
 EXPORT_SYMBOL(__drmm_universal_plane_alloc);
 
+void *__drm_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 (drm_WARN_ON(dev, !funcs))
+		return ERR_PTR(-EINVAL);
+
+	container = kzalloc(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)
+		goto err_kfree;
+
+	return container;
+
+err_kfree:
+	kfree(container);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(__drm_universal_plane_alloc);
+
 int drm_plane_register_all(struct drm_device *dev)
 {
 	unsigned int num_planes = 0;
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 660c4cbc0b3d..6b8a014b5e97 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs nv04_primary_plane_funcs = {
 	.destroy = drm_plane_helper_destroy,
 };
 
-static struct drm_plane *
-create_primary_plane(struct drm_device *dev)
-{
-        struct drm_plane *primary;
-        int ret;
-
-        primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-        if (primary == NULL) {
-                DRM_DEBUG_KMS("Failed to allocate primary plane\n");
-                return NULL;
-        }
-
-        /* possible_crtc's will be filled in later by crtc_init */
-        ret = drm_universal_plane_init(dev, primary, 0,
-				       &nv04_primary_plane_funcs,
-                                       modeset_formats,
-                                       ARRAY_SIZE(modeset_formats), NULL,
-                                       DRM_PLANE_TYPE_PRIMARY, NULL);
-        if (ret) {
-                kfree(primary);
-                primary = NULL;
-        }
-
-        return primary;
-}
-
 static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
 {
 	struct nouveau_crtc *nv_crtc =
@@ -1321,6 +1295,7 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
 {
 	struct nouveau_display *disp = nouveau_display(dev);
 	struct nouveau_crtc *nv_crtc;
+	struct drm_plane *primary;
 	int ret;
 
 	nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL);
@@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
 	nv_crtc->save = nv_crtc_save;
 	nv_crtc->restore = nv_crtc_restore;
 
-	drm_crtc_init_with_planes(dev, &nv_crtc->base,
-                                  create_primary_plane(dev), NULL,
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &nv04_primary_plane_funcs,
+					      modeset_formats,
+					      ARRAY_SIZE(modeset_formats), NULL,
+					      DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary)) {
+		ret = PTR_ERR(primary);
+		kfree(nv_crtc);
+		return ret;
+	}
+
+	drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL,
                                   &nv04_crtc_funcs, NULL);
 	drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
 	drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 910cb941f3d5..21dfa7f97948 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
 					      format_count, format_modifiers, \
 					      plane_type, name, ##__VA_ARGS__))
 
+__printf(10, 11)
+void *__drm_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, ...);
+
+/**
+ * drm_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. The caller
+ * is responsible for releasing the allocated memory with kfree().
+ *
+ * Drivers are encouraged to use drmm_universal_plane_alloc() instead.
+ *
+ * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set
+ * @format_modifiers to NULL. The plane will advertise the linear modifier.
+ *
+ * Returns:
+ * Pointer to new plane, or ERR_PTR on failure.
+ */
+#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
+				   format_count, format_modifiers, plane_type, name, ...) \
+	((type *)__drm_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.37.2

[PATCH 3/4] drm/plane-helper: Warn if atomic drivers call non-atomic helpers

[Thomas Zimmermann]

The plane update and disable helpers are only useful for non-atomic
drivers. Print a warning if an atomic driver calls them.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_plane_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index c7785967f5bf..1c904fc26a58 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -30,8 +30,10 @@
 #include <drm/drm_atomic_uapi.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_device.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_print.h>
 #include <drm/drm_rect.h>
 
 #define SUBPIXEL_MASK 0xffff
@@ -195,10 +197,14 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
 		.x2 = crtc_x + crtc_w,
 		.y2 = crtc_y + crtc_h,
 	};
+	struct drm_device *dev = plane->dev;
 	struct drm_connector **connector_list;
 	int num_connectors, ret;
 	bool visible;
 
+	if (drm_WARN_ON_ONCE(dev, drm_drv_uses_atomic_modeset(dev)))
+		return -EINVAL;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    &src, &dest,
 					    DRM_MODE_ROTATE_0,
@@ -260,6 +266,10 @@ EXPORT_SYMBOL(drm_plane_helper_update_primary);
 int drm_plane_helper_disable_primary(struct drm_plane *plane,
 				     struct drm_modeset_acquire_ctx *ctx)
 {
+	struct drm_device *dev = plane->dev;
+
+	drm_WARN_ON_ONCE(dev, drm_drv_uses_atomic_modeset(dev));
+
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_plane_helper_disable_primary);
-- 
2.37.2

[PATCH 4/4] drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro

[Thomas Zimmermann]

Provide DRM_PLANE_NON_ATOMIC_FUNCS, which initializes plane functions
of non-atomic drivers to default values. The macro is not supposed to
be used in new code, but helps with documenting and finding existing
users.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_modeset_helper.c    |  4 +---
 drivers/gpu/drm/nouveau/dispnv04/crtc.c |  4 +---
 include/drm/drm_plane_helper.h          | 12 ++++++++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 38040eebfa16..f858dfedf2cf 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -108,9 +108,7 @@ static const uint32_t safe_modeset_formats[] = {
 };
 
 static const struct drm_plane_funcs primary_plane_funcs = {
-	.update_plane = drm_plane_helper_update_primary,
-	.disable_plane = drm_plane_helper_disable_primary,
-	.destroy = drm_plane_helper_destroy,
+	DRM_PLANE_NON_ATOMIC_FUNCS,
 };
 
 /**
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 6b8a014b5e97..ee92d576d277 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -1276,9 +1276,7 @@ static const uint32_t modeset_formats[] = {
 };
 
 static const struct drm_plane_funcs nv04_primary_plane_funcs = {
-	.update_plane = drm_plane_helper_update_primary,
-	.disable_plane = drm_plane_helper_disable_primary,
-	.destroy = drm_plane_helper_destroy,
+	DRM_PLANE_NON_ATOMIC_FUNCS,
 };
 
 static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index 1781fab24dd6..75f9c4830564 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -42,4 +42,16 @@ int drm_plane_helper_disable_primary(struct drm_plane *plane,
 				     struct drm_modeset_acquire_ctx *ctx);
 void drm_plane_helper_destroy(struct drm_plane *plane);
 
+/**
+ * DRM_PLANE_NON_ATOMIC_FUNCS - Default plane functions for non-atomic drivers
+ *
+ * This macro initializes plane functions for non-atomic drivers to default
+ * values. Non-atomic interfaces are deprecated and should not be used in new
+ * drivers.
+ */
+#define DRM_PLANE_NON_ATOMIC_FUNCS \
+	.update_plane = drm_plane_helper_update_primary, \
+	.disable_plane = drm_plane_helper_disable_primary, \
+	.destroy = drm_plane_helper_destroy
+
 #endif
-- 
2.37.2

Re: [PATCH 1/4] drm/plane: Remove drm_plane_init()

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>
for common and nouveau bits

On Fri, 2022-09-09 at 12:59 +0200, Thomas Zimmermann wrote:
> Open-code drm_plane_init() and remove the function from DRM. The
> implementation of drm_plane_init() is a simple wrapper around a call
> to drm_universal_plane_init(), so drivers can just use that instead.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_modeset_helper.c       |  3 +-
>  drivers/gpu/drm/drm_plane.c                | 32 ----------------------
>  drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 +++++----
>  drivers/gpu/drm/shmobile/shmob_drm_plane.c |  7 +++--
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  9 +++---
>  include/drm/drm_plane.h                    |  8 +-----
>  6 files changed, 17 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index bd609a978848..611dd01fb604 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -100,8 +100,7 @@ EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
>   * This is the minimal list of formats that seem to be safe for modeset use
>   * with all current DRM drivers.  Most hardware can actually support more
>   * formats than this and drivers may specify a more accurate list when
> - * creating the primary plane.  However drivers that still call
> - * drm_plane_init() will use this minimal format list as the default.
> + * creating the primary plane.
>   */
>  static const uint32_t safe_modeset_formats[] = {
>  	DRM_FORMAT_XRGB8888,
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 726f2f163c26..0f14b4d3bb10 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -482,38 +482,6 @@ void drm_plane_unregister_all(struct drm_device *dev)
>  	}
>  }
>  
> -/**
> - * drm_plane_init - Initialize a legacy plane
> - * @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
> - * @is_primary: plane type (primary vs overlay)
> - *
> - * Legacy API to initialize a DRM plane.
> - *
> - * New drivers should call drm_universal_plane_init() instead.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_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,
> -		   bool is_primary)
> -{
> -	enum drm_plane_type type;
> -
> -	type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> -	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> -					formats, format_count,
> -					NULL, type, NULL);
> -}
> -EXPORT_SYMBOL(drm_plane_init);
> -
>  /**
>   * drm_plane_cleanup - Clean up the core plane usage
>   * @plane: plane to cleanup
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> index 37e63e98cd08..33f29736024a 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> @@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device)
>  		break;
>  	}
>  
> -	ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */,
> -			     &nv10_plane_funcs,
> -			     formats, num_formats, false);
> +	ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's */,
> +				       &nv10_plane_funcs,
> +				       formats, num_formats, NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  	if (ret)
>  		goto err;
>  
> @@ -475,9 +476,9 @@ nv04_overlay_init(struct drm_device *device)
>  	if (!plane)
>  		return;
>  
> -	ret = drm_plane_init(device, &plane->base, 1 /* single crtc */,
> -			     &nv04_plane_funcs,
> -			     formats, 2, false);
> +	ret = drm_universal_plane_init(device, &plane->base, 1 /* single crtc */,
> +				       &nv04_plane_funcs, formats, 2, NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  	if (ret)
>  		goto err;
>  
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> index 54228424793a..6c5f0cbe7d95 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> @@ -252,9 +252,10 @@ int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index)
>  	splane->index = index;
>  	splane->alpha = 255;
>  
> -	ret = drm_plane_init(sdev->ddev, &splane->plane, 1,
> -			     &shmob_drm_plane_funcs, formats,
> -			     ARRAY_SIZE(formats), false);
> +	ret = drm_universal_plane_init(sdev->ddev, &splane->plane, 1,
> +				       &shmob_drm_plane_funcs,
> +				       formats, ARRAY_SIZE(formats), NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index 0ccf791301cb..cf77a8ce7398 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -105,11 +105,10 @@ int tilcdc_plane_init(struct drm_device *dev,
>  	struct tilcdc_drm_private *priv = dev->dev_private;
>  	int ret;
>  
> -	ret = drm_plane_init(dev, plane, 1,
> -			     &tilcdc_plane_funcs,
> -			     priv->pixelformats,
> -			     priv->num_pixelformats,
> -			     true);
> +	ret = drm_universal_plane_init(dev, plane, 1, &tilcdc_plane_funcs,
> +				       priv->pixelformats,
> +				       priv->num_pixelformats,
> +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
>  		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
>  		return ret;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..910cb941f3d5 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -631,7 +631,7 @@ struct drm_plane {
>  	unsigned int format_count;
>  	/**
>  	 * @format_default: driver hasn't supplied supported formats for the
> -	 * plane. Used by the drm_plane_init compatibility wrapper only.
> +	 * plane. Used by the non-atomic driver compatibility wrapper only.
>  	 */
>  	bool format_default;
>  
> @@ -762,12 +762,6 @@ int drm_universal_plane_init(struct drm_device *dev,
>  			     const uint64_t *format_modifiers,
>  			     enum drm_plane_type type,
>  			     const char *name, ...);
> -int drm_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,
> -		   bool is_primary);
>  void drm_plane_cleanup(struct drm_plane *plane);
>  
>  __printf(10, 11)

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 0/4] drm/plane: Remove drm_plane_init(), plus other cleanups

[Lyude Paul]

For the nouveau bits on 1, 2 and 4:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Fri, 2022-09-09 at 12:59 +0200, Thomas Zimmermann wrote:
> This patchset does cleanups to the plane code, most noteably it removes
> drm_plane_init(). The function is a small wrapper, which can easily be
> inlined into the few callers. Patch #1 fixes this.
> 
> The other clean-up patches #2 to #4 affect plane creation. Modesetting
> helpers and nouveau share some plane-allocation code that can be shared as
> helper function. While the function is already outdated, it's now at least
> well documented. As suggested by Daniel, patch #3 adds a warning to
> non-atomic plane helpers when they are being called from atomic drivers.
> Patch #4 adds an initializer macro for non-atomic plane functions. It
> should not be used in new drivers, but at least documents the current
> practice.
> 
> Tested with nouveau on Nvidia G72 hardware.
> 
> A possible next step would be the inlining of drm_crtc_init() and the
> removal of drm_plane.format_default.
> 
> Thomas Zimmermann (4):
>   drm/plane: Remove drm_plane_init()
>   drm/plane: Allocate planes with drm_universal_plane_alloc()
>   drm/plane-helper: Warn if atomic drivers call non-atomic helpers
>   drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro
> 
>  drivers/gpu/drm/drm_modeset_helper.c       | 68 +++++++++------------
>  drivers/gpu/drm/drm_plane.c                | 70 ++++++++++++----------
>  drivers/gpu/drm/drm_plane_helper.c         | 10 ++++
>  drivers/gpu/drm/nouveau/dispnv04/crtc.c    | 45 +++++---------
>  drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 ++--
>  drivers/gpu/drm/shmobile/shmob_drm_plane.c |  7 ++-
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  9 ++-
>  include/drm/drm_plane.h                    | 52 +++++++++++++---
>  include/drm/drm_plane_helper.h             | 12 ++++
>  9 files changed, 162 insertions(+), 124 deletions(-)
> 
> 
> base-commit: f2c3a05d33693ad51996fa7d12d3b2d4b0f372eb
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 1/4] drm/plane: Remove drm_plane_init()

[jyri.sarha]

September 9, 2022 at 1:59 PM, "Thomas Zimmermann" <tzimmermann@suse.de mailto:tzimmermann@suse.de?to=%22Thomas%20Zimmermann%22%20%3Ctzimmermann%40suse.de%3E > wrote:

> 
> Open-code drm_plane_init() and remove the function from DRM. The
> implementation of drm_plane_init() is a simple wrapper around a call
> to drm_universal_plane_init(), so drivers can just use that instead.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Jyri Sarha <jyri.sarha@iki.fi>

> ---
> drivers/gpu/drm/drm_modeset_helper.c | 3 +-
> drivers/gpu/drm/drm_plane.c | 32 ----------------------
> drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 +++++----
> drivers/gpu/drm/shmobile/shmob_drm_plane.c | 7 +++--
> drivers/gpu/drm/tilcdc/tilcdc_plane.c | 9 +++---
> include/drm/drm_plane.h | 8 +-----
> 6 files changed, 17 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index bd609a978848..611dd01fb604 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -100,8 +100,7 @@ EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
> * This is the minimal list of formats that seem to be safe for modeset use
> * with all current DRM drivers. Most hardware can actually support more
> * formats than this and drivers may specify a more accurate list when
> - * creating the primary plane. However drivers that still call
> - * drm_plane_init() will use this minimal format list as the default.
> + * creating the primary plane.
> */
> static const uint32_t safe_modeset_formats[] = {
>  DRM_FORMAT_XRGB8888,
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 726f2f163c26..0f14b4d3bb10 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -482,38 +482,6 @@ void drm_plane_unregister_all(struct drm_device *dev)
>  }
> }
> 
> -/**
> - * drm_plane_init - Initialize a legacy plane
> - * @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
> - * @is_primary: plane type (primary vs overlay)
> - *
> - * Legacy API to initialize a DRM plane.
> - *
> - * New drivers should call drm_universal_plane_init() instead.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_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,
> - bool is_primary)
> -{
> - enum drm_plane_type type;
> -
> - type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> - return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> - formats, format_count,
> - NULL, type, NULL);
> -}
> -EXPORT_SYMBOL(drm_plane_init);
> -
> /**
> * drm_plane_cleanup - Clean up the core plane usage
> * @plane: plane to cleanup
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> index 37e63e98cd08..33f29736024a 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> @@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device)
>  break;
>  }
> 
> - ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */,
> - &nv10_plane_funcs,
> - formats, num_formats, false);
> + ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's */,
> + &nv10_plane_funcs,
> + formats, num_formats, NULL,
> + DRM_PLANE_TYPE_OVERLAY, NULL);
>  if (ret)
>  goto err;
> 
> @@ -475,9 +476,9 @@ nv04_overlay_init(struct drm_device *device)
>  if (!plane)
>  return;
> 
> - ret = drm_plane_init(device, &plane->base, 1 /* single crtc */,
> - &nv04_plane_funcs,
> - formats, 2, false);
> + ret = drm_universal_plane_init(device, &plane->base, 1 /* single crtc */,
> + &nv04_plane_funcs, formats, 2, NULL,
> + DRM_PLANE_TYPE_OVERLAY, NULL);
>  if (ret)
>  goto err;
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> index 54228424793a..6c5f0cbe7d95 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> @@ -252,9 +252,10 @@ int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index)
>  splane->index = index;
>  splane->alpha = 255;
> 
> - ret = drm_plane_init(sdev->ddev, &splane->plane, 1,
> - &shmob_drm_plane_funcs, formats,
> - ARRAY_SIZE(formats), false);
> + ret = drm_universal_plane_init(sdev->ddev, &splane->plane, 1,
> + &shmob_drm_plane_funcs,
> + formats, ARRAY_SIZE(formats), NULL,
> + DRM_PLANE_TYPE_OVERLAY, NULL);
> 
>  return ret;
> }
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index 0ccf791301cb..cf77a8ce7398 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -105,11 +105,10 @@ int tilcdc_plane_init(struct drm_device *dev,
>  struct tilcdc_drm_private *priv = dev->dev_private;
>  int ret;
> 
> - ret = drm_plane_init(dev, plane, 1,
> - &tilcdc_plane_funcs,
> - priv->pixelformats,
> - priv->num_pixelformats,
> - true);
> + ret = drm_universal_plane_init(dev, plane, 1, &tilcdc_plane_funcs,
> + priv->pixelformats,
> + priv->num_pixelformats,
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>  if (ret) {
>  dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
>  return ret;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..910cb941f3d5 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -631,7 +631,7 @@ struct drm_plane {
>  unsigned int format_count;
>  /**
>  * @format_default: driver hasn't supplied supported formats for the
> - * plane. Used by the drm_plane_init compatibility wrapper only.
> + * plane. Used by the non-atomic driver compatibility wrapper only.
>  */
>  bool format_default;
> 
> @@ -762,12 +762,6 @@ int drm_universal_plane_init(struct drm_device *dev,
>  const uint64_t *format_modifiers,
>  enum drm_plane_type type,
>  const char *name, ...);
> -int drm_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,
> - bool is_primary);
> void drm_plane_cleanup(struct drm_plane *plane);
> 
> __printf(10, 11)
> -- 
> 2.37.2
>

Re: [PATCH 1/4] drm/plane: Remove drm_plane_init()

[Javier Martinez Canillas]

Hello Thomas,

On 9/9/22 12:59, Thomas Zimmermann wrote:
> Open-code drm_plane_init() and remove the function from DRM. The
> implementation of drm_plane_init() is a simple wrapper around a call
> to drm_universal_plane_init(), so drivers can just use that instead.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> index 37e63e98cd08..33f29736024a 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> @@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device)
>  		break;
>  	}
>  
> -	ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */,
> -			     &nv10_plane_funcs,
> -			     formats, num_formats, false);
> +	ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's */,
> +				       &nv10_plane_funcs,
> +				       formats, num_formats, NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);

Not only drm_plane_init() doesn't add much value but makes the code
harder to read. Since by calling drm_universal_plane_init() instead,
it's explicit whether the initialized plane is primary or an overlay.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/4] drm/plane: Remove drm_plane_init()

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Fri, Sep 09, 2022 at 12:59:44PM +0200, Thomas Zimmermann wrote:
> Open-code drm_plane_init() and remove the function from DRM. The
> implementation of drm_plane_init() is a simple wrapper around a call
> to drm_universal_plane_init(), so drivers can just use that instead.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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

> ---
>  drivers/gpu/drm/drm_modeset_helper.c       |  3 +-
>  drivers/gpu/drm/drm_plane.c                | 32 ----------------------
>  drivers/gpu/drm/nouveau/dispnv04/overlay.c | 13 +++++----
>  drivers/gpu/drm/shmobile/shmob_drm_plane.c |  7 +++--
>  drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  9 +++---
>  include/drm/drm_plane.h                    |  8 +-----
>  6 files changed, 17 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index bd609a978848..611dd01fb604 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -100,8 +100,7 @@ EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
>   * This is the minimal list of formats that seem to be safe for modeset use
>   * with all current DRM drivers.  Most hardware can actually support more
>   * formats than this and drivers may specify a more accurate list when
> - * creating the primary plane.  However drivers that still call
> - * drm_plane_init() will use this minimal format list as the default.
> + * creating the primary plane.
>   */
>  static const uint32_t safe_modeset_formats[] = {
>  	DRM_FORMAT_XRGB8888,
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 726f2f163c26..0f14b4d3bb10 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -482,38 +482,6 @@ void drm_plane_unregister_all(struct drm_device *dev)
>  	}
>  }
>  
> -/**
> - * drm_plane_init - Initialize a legacy plane
> - * @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
> - * @is_primary: plane type (primary vs overlay)
> - *
> - * Legacy API to initialize a DRM plane.
> - *
> - * New drivers should call drm_universal_plane_init() instead.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_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,
> -		   bool is_primary)
> -{
> -	enum drm_plane_type type;
> -
> -	type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> -	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> -					formats, format_count,
> -					NULL, type, NULL);
> -}
> -EXPORT_SYMBOL(drm_plane_init);
> -
>  /**
>   * drm_plane_cleanup - Clean up the core plane usage
>   * @plane: plane to cleanup
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> index 37e63e98cd08..33f29736024a 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> @@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device)
>  		break;
>  	}
>  
> -	ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */,
> -			     &nv10_plane_funcs,
> -			     formats, num_formats, false);
> +	ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's */,
> +				       &nv10_plane_funcs,
> +				       formats, num_formats, NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  	if (ret)
>  		goto err;
>  
> @@ -475,9 +476,9 @@ nv04_overlay_init(struct drm_device *device)
>  	if (!plane)
>  		return;
>  
> -	ret = drm_plane_init(device, &plane->base, 1 /* single crtc */,
> -			     &nv04_plane_funcs,
> -			     formats, 2, false);
> +	ret = drm_universal_plane_init(device, &plane->base, 1 /* single crtc */,
> +				       &nv04_plane_funcs, formats, 2, NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  	if (ret)
>  		goto err;
>  
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> index 54228424793a..6c5f0cbe7d95 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> @@ -252,9 +252,10 @@ int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index)
>  	splane->index = index;
>  	splane->alpha = 255;
>  
> -	ret = drm_plane_init(sdev->ddev, &splane->plane, 1,
> -			     &shmob_drm_plane_funcs, formats,
> -			     ARRAY_SIZE(formats), false);
> +	ret = drm_universal_plane_init(sdev->ddev, &splane->plane, 1,
> +				       &shmob_drm_plane_funcs,
> +				       formats, ARRAY_SIZE(formats), NULL,
> +				       DRM_PLANE_TYPE_OVERLAY, NULL);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index 0ccf791301cb..cf77a8ce7398 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -105,11 +105,10 @@ int tilcdc_plane_init(struct drm_device *dev,
>  	struct tilcdc_drm_private *priv = dev->dev_private;
>  	int ret;
>  
> -	ret = drm_plane_init(dev, plane, 1,
> -			     &tilcdc_plane_funcs,
> -			     priv->pixelformats,
> -			     priv->num_pixelformats,
> -			     true);
> +	ret = drm_universal_plane_init(dev, plane, 1, &tilcdc_plane_funcs,
> +				       priv->pixelformats,
> +				       priv->num_pixelformats,
> +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
>  		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
>  		return ret;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..910cb941f3d5 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -631,7 +631,7 @@ struct drm_plane {
>  	unsigned int format_count;
>  	/**
>  	 * @format_default: driver hasn't supplied supported formats for the
> -	 * plane. Used by the drm_plane_init compatibility wrapper only.
> +	 * plane. Used by the non-atomic driver compatibility wrapper only.
>  	 */
>  	bool format_default;
>  
> @@ -762,12 +762,6 @@ int drm_universal_plane_init(struct drm_device *dev,
>  			     const uint64_t *format_modifiers,
>  			     enum drm_plane_type type,
>  			     const char *name, ...);
> -int drm_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,
> -		   bool is_primary);
>  void drm_plane_cleanup(struct drm_plane *plane);
>  
>  __printf(10, 11)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Fri, Sep 09, 2022 at 12:59:45PM +0200, Thomas Zimmermann wrote:
> Provide drm_univeral_plane_alloc(), which allocated an initializes a
> plane. Code for non-atomic drivers uses this pattern. Convert it to
> the new function. The modeset helpers contain a quirk for handling their
> color formats differently. Set the flag outside plane allocation.
> 
> The new function is already deprecated to some extend. Drivers should
> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().

If this is already deprecated and used by a single driver, what is the
point ?

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_modeset_helper.c    | 61 +++++++++++--------------
>  drivers/gpu/drm/drm_plane.c             | 38 +++++++++++++++
>  drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++-----------
>  include/drm/drm_plane.h                 | 44 ++++++++++++++++++
>  4 files changed, 121 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 611dd01fb604..38040eebfa16 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = {
>  	.destroy = drm_plane_helper_destroy,
>  };
>  
> -static struct drm_plane *create_primary_plane(struct drm_device *dev)
> -{
> -	struct drm_plane *primary;
> -	int ret;
> -
> -	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> -	if (primary == NULL) {
> -		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> -		return NULL;
> -	}
> -
> -	/*
> -	 * Remove the format_default field from drm_plane when dropping
> -	 * this helper.
> -	 */
> -	primary->format_default = true;
> -
> -	/* possible_crtc's will be filled in later by crtc_init */
> -	ret = drm_universal_plane_init(dev, primary, 0,
> -				       &primary_plane_funcs,
> -				       safe_modeset_formats,
> -				       ARRAY_SIZE(safe_modeset_formats),
> -				       NULL,
> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> -	if (ret) {
> -		kfree(primary);
> -		primary = NULL;
> -	}
> -
> -	return primary;
> -}
> -
>  /**
>   * drm_crtc_init - Legacy CRTC initialization function
>   * @dev: DRM device
> @@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  		  const struct drm_crtc_funcs *funcs)
>  {
>  	struct drm_plane *primary;
> +	int ret;
> +
> +	/* possible_crtc's will be filled in later by crtc_init */
> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> +					      &primary_plane_funcs,
> +					      safe_modeset_formats,
> +					      ARRAY_SIZE(safe_modeset_formats),
> +					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (IS_ERR(primary))
> +		return PTR_ERR(primary);
>  
> -	primary = create_primary_plane(dev);
> -	return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs,
> -					 NULL);
> +	/*
> +	 * Remove the format_default field from drm_plane when dropping
> +	 * this helper.
> +	 */
> +	primary->format_default = true;
> +
> +	ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
> +	if (ret)
> +		goto err_drm_plane_cleanup;
> +
> +	return 0;
> +
> +err_drm_plane_cleanup:
> +	drm_plane_cleanup(primary);
> +	kfree(primary);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_crtc_init);
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0f14b4d3bb10..33357629a7f5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
>  }
>  EXPORT_SYMBOL(__drmm_universal_plane_alloc);
>  
> +void *__drm_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 (drm_WARN_ON(dev, !funcs))
> +		return ERR_PTR(-EINVAL);
> +
> +	container = kzalloc(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)
> +		goto err_kfree;
> +
> +	return container;
> +
> +err_kfree:
> +	kfree(container);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(__drm_universal_plane_alloc);
> +
>  int drm_plane_register_all(struct drm_device *dev)
>  {
>  	unsigned int num_planes = 0;
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> index 660c4cbc0b3d..6b8a014b5e97 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> @@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs nv04_primary_plane_funcs = {
>  	.destroy = drm_plane_helper_destroy,
>  };
>  
> -static struct drm_plane *
> -create_primary_plane(struct drm_device *dev)
> -{
> -        struct drm_plane *primary;
> -        int ret;
> -
> -        primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> -        if (primary == NULL) {
> -                DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> -                return NULL;
> -        }
> -
> -        /* possible_crtc's will be filled in later by crtc_init */
> -        ret = drm_universal_plane_init(dev, primary, 0,
> -				       &nv04_primary_plane_funcs,
> -                                       modeset_formats,
> -                                       ARRAY_SIZE(modeset_formats), NULL,
> -                                       DRM_PLANE_TYPE_PRIMARY, NULL);
> -        if (ret) {
> -                kfree(primary);
> -                primary = NULL;
> -        }
> -
> -        return primary;
> -}
> -
>  static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
>  {
>  	struct nouveau_crtc *nv_crtc =
> @@ -1321,6 +1295,7 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
>  {
>  	struct nouveau_display *disp = nouveau_display(dev);
>  	struct nouveau_crtc *nv_crtc;
> +	struct drm_plane *primary;
>  	int ret;
>  
>  	nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL);
> @@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
>  	nv_crtc->save = nv_crtc_save;
>  	nv_crtc->restore = nv_crtc_restore;
>  
> -	drm_crtc_init_with_planes(dev, &nv_crtc->base,
> -                                  create_primary_plane(dev), NULL,
> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> +					      &nv04_primary_plane_funcs,
> +					      modeset_formats,
> +					      ARRAY_SIZE(modeset_formats), NULL,
> +					      DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (IS_ERR(primary)) {
> +		ret = PTR_ERR(primary);
> +		kfree(nv_crtc);
> +		return ret;
> +	}
> +
> +	drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL,
>                                    &nv04_crtc_funcs, NULL);
>  	drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
>  	drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 910cb941f3d5..21dfa7f97948 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
>  					      format_count, format_modifiers, \
>  					      plane_type, name, ##__VA_ARGS__))
>  
> +__printf(10, 11)
> +void *__drm_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, ...);
> +
> +/**
> + * drm_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. The caller
> + * is responsible for releasing the allocated memory with kfree().
> + *
> + * Drivers are encouraged to use drmm_universal_plane_alloc() instead.
> + *
> + * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set
> + * @format_modifiers to NULL. The plane will advertise the linear modifier.
> + *
> + * Returns:
> + * Pointer to new plane, or ERR_PTR on failure.
> + */
> +#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
> +				   format_count, format_modifiers, plane_type, name, ...) \
> +	((type *)__drm_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.37.2
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/plane-helper: Warn if atomic drivers call non-atomic helpers

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Fri, Sep 09, 2022 at 12:59:46PM +0200, Thomas Zimmermann wrote:
> The plane update and disable helpers are only useful for non-atomic
> drivers. Print a warning if an atomic driver calls them.
> 
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index c7785967f5bf..1c904fc26a58 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -30,8 +30,10 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_print.h>
>  #include <drm/drm_rect.h>
>  
>  #define SUBPIXEL_MASK 0xffff
> @@ -195,10 +197,14 @@ int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *cr
>  		.x2 = crtc_x + crtc_w,
>  		.y2 = crtc_y + crtc_h,
>  	};
> +	struct drm_device *dev = plane->dev;
>  	struct drm_connector **connector_list;
>  	int num_connectors, ret;
>  	bool visible;
>  
> +	if (drm_WARN_ON_ONCE(dev, drm_drv_uses_atomic_modeset(dev)))
> +		return -EINVAL;
> +
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    &src, &dest,
>  					    DRM_MODE_ROTATE_0,
> @@ -260,6 +266,10 @@ EXPORT_SYMBOL(drm_plane_helper_update_primary);
>  int drm_plane_helper_disable_primary(struct drm_plane *plane,
>  				     struct drm_modeset_acquire_ctx *ctx)
>  {
> +	struct drm_device *dev = plane->dev;
> +
> +	drm_WARN_ON_ONCE(dev, drm_drv_uses_atomic_modeset(dev));
> +
>  	return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_plane_helper_disable_primary);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Javier Martinez Canillas]

On 9/9/22 12:59, Thomas Zimmermann wrote:
> Provide drm_univeral_plane_alloc(), which allocated an initializes a
> plane. Code for non-atomic drivers uses this pattern. Convert it to
> the new function. The modeset helpers contain a quirk for handling their
> color formats differently. Set the flag outside plane allocation.
> 
> The new function is already deprecated to some extend. Drivers should
> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

>  
> +__printf(10, 11)
> +void *__drm_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, ...);
> +
> +/**
> + * drm_universal_plane_alloc - Allocate and initialize an universal plane object

Should functions kernel-doc definitions use parenthesis or not? I see in
https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst#L59
that the example has it but notice that there is not consistency in DRM
about this.

> + * @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. The caller
> + * is responsible for releasing the allocated memory with kfree().
> + *

I thought that all the drmm_*_alloc() managed interfaces should use the
drmm_k{z,m}alloc() helpers, so that the memory is automatically freed
on the last drm_dev_put() call ?

Other than those two nits, the patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/4] drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Fri, Sep 09, 2022 at 12:59:47PM +0200, Thomas Zimmermann wrote:
> Provide DRM_PLANE_NON_ATOMIC_FUNCS, which initializes plane functions
> of non-atomic drivers to default values. The macro is not supposed to
> be used in new code, but helps with documenting and finding existing
> users.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

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

> ---
>  drivers/gpu/drm/drm_modeset_helper.c    |  4 +---
>  drivers/gpu/drm/nouveau/dispnv04/crtc.c |  4 +---
>  include/drm/drm_plane_helper.h          | 12 ++++++++++++
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 38040eebfa16..f858dfedf2cf 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -108,9 +108,7 @@ static const uint32_t safe_modeset_formats[] = {
>  };
>  
>  static const struct drm_plane_funcs primary_plane_funcs = {
> -	.update_plane = drm_plane_helper_update_primary,
> -	.disable_plane = drm_plane_helper_disable_primary,
> -	.destroy = drm_plane_helper_destroy,
> +	DRM_PLANE_NON_ATOMIC_FUNCS,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> index 6b8a014b5e97..ee92d576d277 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> @@ -1276,9 +1276,7 @@ static const uint32_t modeset_formats[] = {
>  };
>  
>  static const struct drm_plane_funcs nv04_primary_plane_funcs = {
> -	.update_plane = drm_plane_helper_update_primary,
> -	.disable_plane = drm_plane_helper_disable_primary,
> -	.destroy = drm_plane_helper_destroy,
> +	DRM_PLANE_NON_ATOMIC_FUNCS,
>  };
>  
>  static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 1781fab24dd6..75f9c4830564 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -42,4 +42,16 @@ int drm_plane_helper_disable_primary(struct drm_plane *plane,
>  				     struct drm_modeset_acquire_ctx *ctx);
>  void drm_plane_helper_destroy(struct drm_plane *plane);
>  
> +/**
> + * DRM_PLANE_NON_ATOMIC_FUNCS - Default plane functions for non-atomic drivers
> + *
> + * This macro initializes plane functions for non-atomic drivers to default
> + * values. Non-atomic interfaces are deprecated and should not be used in new
> + * drivers.

I wonder if we could teach checkpath.pl to catch new users.

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

> + */
> +#define DRM_PLANE_NON_ATOMIC_FUNCS \
> +	.update_plane = drm_plane_helper_update_primary, \
> +	.disable_plane = drm_plane_helper_disable_primary, \
> +	.destroy = drm_plane_helper_destroy
> +
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/plane-helper: Warn if atomic drivers call non-atomic helpers

[Javier Martinez Canillas]

On 9/9/22 12:59, Thomas Zimmermann wrote:
> The plane update and disable helpers are only useful for non-atomic
> drivers. Print a warning if an atomic driver calls them.
> 
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 4/4] drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro

[Javier Martinez Canillas]

On 9/9/22 12:59, Thomas Zimmermann wrote:
> Provide DRM_PLANE_NON_ATOMIC_FUNCS, which initializes plane functions
> of non-atomic drivers to default values. The macro is not supposed to
> be used in new code, but helps with documenting and finding existing
> users.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 11303 bytes --]

Hi

Am 16.09.22 um 13:06 schrieb Laurent Pinchart:
> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Fri, Sep 09, 2022 at 12:59:45PM +0200, Thomas Zimmermann wrote:
>> Provide drm_univeral_plane_alloc(), which allocated an initializes a
>> plane. Code for non-atomic drivers uses this pattern. Convert it to
>> the new function. The modeset helpers contain a quirk for handling their
>> color formats differently. Set the flag outside plane allocation.
>>
>> The new function is already deprecated to some extend. Drivers should
>> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
> 
> If this is already deprecated and used by a single driver, what is the
> point ?

It's used by nouveau and drm_modeset_helper.c. Since the code is 
duplicated, it seems generally better to have it located and documented 
in a central place.

Although it may look somewhat pointless now, the helper will get useful 
in the future. The affected code in drm_modeset_helper is in 
drm_crtc_init(), which is also a deprecated interface; only used by 
non-atomic drivers. The function is a good candidate to be inlined into 
calling drivers. Getting drm_crtc_init() removed will allow us to 
correct these drivers' color-format handling. Once that happened, 
several more drivers will call drm_univeral_plane_alloc().

Best regards
Thomas

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_modeset_helper.c    | 61 +++++++++++--------------
>>   drivers/gpu/drm/drm_plane.c             | 38 +++++++++++++++
>>   drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++-----------
>>   include/drm/drm_plane.h                 | 44 ++++++++++++++++++
>>   4 files changed, 121 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
>> index 611dd01fb604..38040eebfa16 100644
>> --- a/drivers/gpu/drm/drm_modeset_helper.c
>> +++ b/drivers/gpu/drm/drm_modeset_helper.c
>> @@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = {
>>   	.destroy = drm_plane_helper_destroy,
>>   };
>>   
>> -static struct drm_plane *create_primary_plane(struct drm_device *dev)
>> -{
>> -	struct drm_plane *primary;
>> -	int ret;
>> -
>> -	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
>> -	if (primary == NULL) {
>> -		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
>> -		return NULL;
>> -	}
>> -
>> -	/*
>> -	 * Remove the format_default field from drm_plane when dropping
>> -	 * this helper.
>> -	 */
>> -	primary->format_default = true;
>> -
>> -	/* possible_crtc's will be filled in later by crtc_init */
>> -	ret = drm_universal_plane_init(dev, primary, 0,
>> -				       &primary_plane_funcs,
>> -				       safe_modeset_formats,
>> -				       ARRAY_SIZE(safe_modeset_formats),
>> -				       NULL,
>> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
>> -	if (ret) {
>> -		kfree(primary);
>> -		primary = NULL;
>> -	}
>> -
>> -	return primary;
>> -}
>> -
>>   /**
>>    * drm_crtc_init - Legacy CRTC initialization function
>>    * @dev: DRM device
>> @@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>>   		  const struct drm_crtc_funcs *funcs)
>>   {
>>   	struct drm_plane *primary;
>> +	int ret;
>> +
>> +	/* possible_crtc's will be filled in later by crtc_init */
>> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
>> +					      &primary_plane_funcs,
>> +					      safe_modeset_formats,
>> +					      ARRAY_SIZE(safe_modeset_formats),
>> +					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> +	if (IS_ERR(primary))
>> +		return PTR_ERR(primary);
>>   
>> -	primary = create_primary_plane(dev);
>> -	return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs,
>> -					 NULL);
>> +	/*
>> +	 * Remove the format_default field from drm_plane when dropping
>> +	 * this helper.
>> +	 */
>> +	primary->format_default = true;
>> +
>> +	ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
>> +	if (ret)
>> +		goto err_drm_plane_cleanup;
>> +
>> +	return 0;
>> +
>> +err_drm_plane_cleanup:
>> +	drm_plane_cleanup(primary);
>> +	kfree(primary);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(drm_crtc_init);
>>   
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 0f14b4d3bb10..33357629a7f5 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
>>   }
>>   EXPORT_SYMBOL(__drmm_universal_plane_alloc);
>>   
>> +void *__drm_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 (drm_WARN_ON(dev, !funcs))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	container = kzalloc(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)
>> +		goto err_kfree;
>> +
>> +	return container;
>> +
>> +err_kfree:
>> +	kfree(container);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL(__drm_universal_plane_alloc);
>> +
>>   int drm_plane_register_all(struct drm_device *dev)
>>   {
>>   	unsigned int num_planes = 0;
>> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> index 660c4cbc0b3d..6b8a014b5e97 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
>> @@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs nv04_primary_plane_funcs = {
>>   	.destroy = drm_plane_helper_destroy,
>>   };
>>   
>> -static struct drm_plane *
>> -create_primary_plane(struct drm_device *dev)
>> -{
>> -        struct drm_plane *primary;
>> -        int ret;
>> -
>> -        primary = kzalloc(sizeof(*primary), GFP_KERNEL);
>> -        if (primary == NULL) {
>> -                DRM_DEBUG_KMS("Failed to allocate primary plane\n");
>> -                return NULL;
>> -        }
>> -
>> -        /* possible_crtc's will be filled in later by crtc_init */
>> -        ret = drm_universal_plane_init(dev, primary, 0,
>> -				       &nv04_primary_plane_funcs,
>> -                                       modeset_formats,
>> -                                       ARRAY_SIZE(modeset_formats), NULL,
>> -                                       DRM_PLANE_TYPE_PRIMARY, NULL);
>> -        if (ret) {
>> -                kfree(primary);
>> -                primary = NULL;
>> -        }
>> -
>> -        return primary;
>> -}
>> -
>>   static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
>>   {
>>   	struct nouveau_crtc *nv_crtc =
>> @@ -1321,6 +1295,7 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
>>   {
>>   	struct nouveau_display *disp = nouveau_display(dev);
>>   	struct nouveau_crtc *nv_crtc;
>> +	struct drm_plane *primary;
>>   	int ret;
>>   
>>   	nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL);
>> @@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
>>   	nv_crtc->save = nv_crtc_save;
>>   	nv_crtc->restore = nv_crtc_restore;
>>   
>> -	drm_crtc_init_with_planes(dev, &nv_crtc->base,
>> -                                  create_primary_plane(dev), NULL,
>> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
>> +					      &nv04_primary_plane_funcs,
>> +					      modeset_formats,
>> +					      ARRAY_SIZE(modeset_formats), NULL,
>> +					      DRM_PLANE_TYPE_PRIMARY, NULL);
>> +	if (IS_ERR(primary)) {
>> +		ret = PTR_ERR(primary);
>> +		kfree(nv_crtc);
>> +		return ret;
>> +	}
>> +
>> +	drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL,
>>                                     &nv04_crtc_funcs, NULL);
>>   	drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
>>   	drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 910cb941f3d5..21dfa7f97948 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
>>   					      format_count, format_modifiers, \
>>   					      plane_type, name, ##__VA_ARGS__))
>>   
>> +__printf(10, 11)
>> +void *__drm_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, ...);
>> +
>> +/**
>> + * drm_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. The caller
>> + * is responsible for releasing the allocated memory with kfree().
>> + *
>> + * Drivers are encouraged to use drmm_universal_plane_alloc() instead.
>> + *
>> + * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set
>> + * @format_modifiers to NULL. The plane will advertise the linear modifier.
>> + *
>> + * Returns:
>> + * Pointer to new plane, or ERR_PTR on failure.
>> + */
>> +#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
>> +				   format_count, format_modifiers, plane_type, name, ...) \
>> +	((type *)__drm_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.37.2
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3336 bytes --]

Hi

Am 16.09.22 um 13:22 schrieb Javier Martinez Canillas:
> On 9/9/22 12:59, Thomas Zimmermann wrote:
>> Provide drm_univeral_plane_alloc(), which allocated an initializes a
>> plane. Code for non-atomic drivers uses this pattern. Convert it to
>> the new function. The modeset helpers contain a quirk for handling their
>> color formats differently. Set the flag outside plane allocation.
>>
>> The new function is already deprecated to some extend. Drivers should
>> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>>   
>> +__printf(10, 11)
>> +void *__drm_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, ...);
>> +
>> +/**
>> + * drm_universal_plane_alloc - Allocate and initialize an universal plane object
> 
> Should functions kernel-doc definitions use parenthesis or not? I see in
> https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst#L59
> that the example has it but notice that there is not consistency in DRM
> about this.

A wasn't aware of this convention.

> 
>> + * @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. The caller
>> + * is responsible for releasing the allocated memory with kfree().
>> + *
> 
> I thought that all the drmm_*_alloc() managed interfaces should use the
> drmm_k{z,m}alloc() helpers, so that the memory is automatically freed
> on the last drm_dev_put() call ?

For new drivers, managed cleanup is preferred. But this is an existing 
unmanaged cleanup.

I don't know if drmm_ changes the semantics in unexpected ways (well, 
probably not). With managed memory releases, I was worried that plane 
memory might stay around longer than expected. And we'd have to fix the 
plane-destroy function in each user as well.

Adding the existing code as a new function is the trivial solution and 
allows to address each driver individually. Also see my reply to 
Laurent's question on that topic.

Best regards
Thomas

> 
> Other than those two nits, the patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Javier Martinez Canillas]

On 9/16/22 13:41, Thomas Zimmermann wrote:

[...]

>>
>>> + * @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. The caller
>>> + * is responsible for releasing the allocated memory with kfree().
>>> + *
>>
>> I thought that all the drmm_*_alloc() managed interfaces should use the
>> drmm_k{z,m}alloc() helpers, so that the memory is automatically freed
>> on the last drm_dev_put() call ?
> 
> For new drivers, managed cleanup is preferred. But this is an existing 
> unmanaged cleanup.
> 
> I don't know if drmm_ changes the semantics in unexpected ways (well, 
> probably not). With managed memory releases, I was worried that plane 
> memory might stay around longer than expected. And we'd have to fix the 
> plane-destroy function in each user as well.
> 
> Adding the existing code as a new function is the trivial solution and 
> allows to address each driver individually. Also see my reply to 
> Laurent's question on that topic.
>

Ah, never mind. I misread the function name definition and thought that
you had a drmm_ suffix but, now on second read I see that is only drm_
so just ignore my comment about using managed memory alloc / release.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Laurent Pinchart]

Hi Thomas,

On Fri, Sep 16, 2022 at 01:31:25PM +0200, Thomas Zimmermann wrote:
> Am 16.09.22 um 13:06 schrieb Laurent Pinchart:
> > On Fri, Sep 09, 2022 at 12:59:45PM +0200, Thomas Zimmermann wrote:
> >> Provide drm_univeral_plane_alloc(), which allocated an initializes a
> >> plane. Code for non-atomic drivers uses this pattern. Convert it to
> >> the new function. The modeset helpers contain a quirk for handling their
> >> color formats differently. Set the flag outside plane allocation.
> >>
> >> The new function is already deprecated to some extend. Drivers should
> >> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
> > 
> > If this is already deprecated and used by a single driver, what is the
> > point ?
> 
> It's used by nouveau and drm_modeset_helper.c. Since the code is 
> duplicated, it seems generally better to have it located and documented 
> in a central place.
> 
> Although it may look somewhat pointless now, the helper will get useful 
> in the future. The affected code in drm_modeset_helper is in 
> drm_crtc_init(), which is also a deprecated interface; only used by 
> non-atomic drivers. The function is a good candidate to be inlined into 
> calling drivers. Getting drm_crtc_init() removed will allow us to 
> correct these drivers' color-format handling. Once that happened, 
> several more drivers will call drm_univeral_plane_alloc().

OK, works for me.

> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>   drivers/gpu/drm/drm_modeset_helper.c    | 61 +++++++++++--------------
> >>   drivers/gpu/drm/drm_plane.c             | 38 +++++++++++++++
> >>   drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++-----------
> >>   include/drm/drm_plane.h                 | 44 ++++++++++++++++++
> >>   4 files changed, 121 insertions(+), 63 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> >> index 611dd01fb604..38040eebfa16 100644
> >> --- a/drivers/gpu/drm/drm_modeset_helper.c
> >> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> >> @@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = {
> >>   	.destroy = drm_plane_helper_destroy,
> >>   };
> >>   
> >> -static struct drm_plane *create_primary_plane(struct drm_device *dev)
> >> -{
> >> -	struct drm_plane *primary;
> >> -	int ret;
> >> -
> >> -	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> >> -	if (primary == NULL) {
> >> -		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> >> -		return NULL;
> >> -	}
> >> -
> >> -	/*
> >> -	 * Remove the format_default field from drm_plane when dropping
> >> -	 * this helper.
> >> -	 */
> >> -	primary->format_default = true;
> >> -
> >> -	/* possible_crtc's will be filled in later by crtc_init */
> >> -	ret = drm_universal_plane_init(dev, primary, 0,
> >> -				       &primary_plane_funcs,
> >> -				       safe_modeset_formats,
> >> -				       ARRAY_SIZE(safe_modeset_formats),
> >> -				       NULL,
> >> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> >> -	if (ret) {
> >> -		kfree(primary);
> >> -		primary = NULL;
> >> -	}
> >> -
> >> -	return primary;
> >> -}
> >> -
> >>   /**
> >>    * drm_crtc_init - Legacy CRTC initialization function
> >>    * @dev: DRM device
> >> @@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >>   		  const struct drm_crtc_funcs *funcs)
> >>   {
> >>   	struct drm_plane *primary;
> >> +	int ret;
> >> +
> >> +	/* possible_crtc's will be filled in later by crtc_init */
> >> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> >> +					      &primary_plane_funcs,
> >> +					      safe_modeset_formats,
> >> +					      ARRAY_SIZE(safe_modeset_formats),
> >> +					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> >> +	if (IS_ERR(primary))
> >> +		return PTR_ERR(primary);
> >>   
> >> -	primary = create_primary_plane(dev);
> >> -	return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs,
> >> -					 NULL);
> >> +	/*
> >> +	 * Remove the format_default field from drm_plane when dropping
> >> +	 * this helper.
> >> +	 */
> >> +	primary->format_default = true;
> >> +
> >> +	ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
> >> +	if (ret)
> >> +		goto err_drm_plane_cleanup;
> >> +
> >> +	return 0;
> >> +
> >> +err_drm_plane_cleanup:
> >> +	drm_plane_cleanup(primary);
> >> +	kfree(primary);
> >> +	return ret;
> >>   }
> >>   EXPORT_SYMBOL(drm_crtc_init);
> >>   
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index 0f14b4d3bb10..33357629a7f5 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
> >>   }
> >>   EXPORT_SYMBOL(__drmm_universal_plane_alloc);
> >>   
> >> +void *__drm_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 (drm_WARN_ON(dev, !funcs))
> >> +		return ERR_PTR(-EINVAL);
> >> +
> >> +	container = kzalloc(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)
> >> +		goto err_kfree;
> >> +
> >> +	return container;
> >> +
> >> +err_kfree:
> >> +	kfree(container);
> >> +	return ERR_PTR(ret);
> >> +}
> >> +EXPORT_SYMBOL(__drm_universal_plane_alloc);
> >> +
> >>   int drm_plane_register_all(struct drm_device *dev)
> >>   {
> >>   	unsigned int num_planes = 0;
> >> diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >> index 660c4cbc0b3d..6b8a014b5e97 100644
> >> --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >> +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
> >> @@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs nv04_primary_plane_funcs = {
> >>   	.destroy = drm_plane_helper_destroy,
> >>   };
> >>   
> >> -static struct drm_plane *
> >> -create_primary_plane(struct drm_device *dev)
> >> -{
> >> -        struct drm_plane *primary;
> >> -        int ret;
> >> -
> >> -        primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> >> -        if (primary == NULL) {
> >> -                DRM_DEBUG_KMS("Failed to allocate primary plane\n");
> >> -                return NULL;
> >> -        }
> >> -
> >> -        /* possible_crtc's will be filled in later by crtc_init */
> >> -        ret = drm_universal_plane_init(dev, primary, 0,
> >> -				       &nv04_primary_plane_funcs,
> >> -                                       modeset_formats,
> >> -                                       ARRAY_SIZE(modeset_formats), NULL,
> >> -                                       DRM_PLANE_TYPE_PRIMARY, NULL);
> >> -        if (ret) {
> >> -                kfree(primary);
> >> -                primary = NULL;
> >> -        }
> >> -
> >> -        return primary;
> >> -}
> >> -
> >>   static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
> >>   {
> >>   	struct nouveau_crtc *nv_crtc =
> >> @@ -1321,6 +1295,7 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
> >>   {
> >>   	struct nouveau_display *disp = nouveau_display(dev);
> >>   	struct nouveau_crtc *nv_crtc;
> >> +	struct drm_plane *primary;
> >>   	int ret;
> >>   
> >>   	nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL);
> >> @@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
> >>   	nv_crtc->save = nv_crtc_save;
> >>   	nv_crtc->restore = nv_crtc_restore;
> >>   
> >> -	drm_crtc_init_with_planes(dev, &nv_crtc->base,
> >> -                                  create_primary_plane(dev), NULL,
> >> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> >> +					      &nv04_primary_plane_funcs,
> >> +					      modeset_formats,
> >> +					      ARRAY_SIZE(modeset_formats), NULL,
> >> +					      DRM_PLANE_TYPE_PRIMARY, NULL);
> >> +	if (IS_ERR(primary)) {
> >> +		ret = PTR_ERR(primary);
> >> +		kfree(nv_crtc);
> >> +		return ret;
> >> +	}
> >> +
> >> +	drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL,
> >>                                     &nv04_crtc_funcs, NULL);
> >>   	drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
> >>   	drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 910cb941f3d5..21dfa7f97948 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
> >>   					      format_count, format_modifiers, \
> >>   					      plane_type, name, ##__VA_ARGS__))
> >>   
> >> +__printf(10, 11)
> >> +void *__drm_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, ...);
> >> +
> >> +/**
> >> + * drm_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. The caller
> >> + * is responsible for releasing the allocated memory with kfree().
> >> + *
> >> + * Drivers are encouraged to use drmm_universal_plane_alloc() instead.
> >> + *
> >> + * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set
> >> + * @format_modifiers to NULL. The plane will advertise the linear modifier.
> >> + *
> >> + * Returns:
> >> + * Pointer to new plane, or ERR_PTR on failure.
> >> + */
> >> +#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
> >> +				   format_count, format_modifiers, plane_type, name, ...) \
> >> +	((type *)__drm_universal_plane_alloc(dev, sizeof(type), \
> >> +					     offsetof(type, member), \
> >> +					     possible_crtcs, funcs, formats, \
> >> +					     format_count, format_modifiers, \
> >> +					     plane_type, name, ##__VA_ARGS__))
> >> +
> >>   /**
> >>    * drm_plane_index - find the index of a registered plane
> >>    * @plane: plane to find index for

-- 
Regards,

Laurent Pinchart