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

On Tue, Dec 08, 2020 at 04:54:34PM +0100, Philipp Zabel wrote:

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

Signed-off-by: Philipp Zabel p.zabel@pengutronix.de Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

---

Changes since v3:

• allow the funcs parameter to __drmm_encoder_alloc() to be NULL

---

drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- include/drm/drm_encoder.h | 30 ++++++++++ 2 files changed, 108 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index b0b86a3c08f5..cc0edb8361e8 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -26,6 +26,7 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_encoder.h> +#include <drm/drm_managed.h>

#include "drm_crtc_internal.h"

@@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev) } }

-/**

• • drm_encoder_init - Init a preallocated encoder
• • @dev: drm device
• • @encoder: the encoder to init
• • @funcs: callbacks for this encoder
• • @encoder_type: user visible type of the encoder
• • @name: printf style format string for the encoder name, or NULL for default name
• • Initialises a preallocated encoder. Encoder should be subclassed as part of
• • driver encoder objects. At driver unload time drm_encoder_cleanup() should be
• • called from the driver's &drm_encoder_funcs.destroy hook.
• • Returns:
• • Zero on success, error code on failure.
• */

-int drm_encoder_init(struct drm_device *dev,

•      struct drm_encoder *encoder,
  

•      const struct drm_encoder_funcs *funcs,
  

•      int encoder_type, const char *name, ...)
  

+__printf(5, 0) +static int __drm_encoder_init(struct drm_device *dev,

• 	      struct drm_encoder *encoder,
  

• 	      const struct drm_encoder_funcs *funcs,
  

• 	      int encoder_type, const char *name, va_list ap)
  

{ int ret;

@@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev, encoder->encoder_type = encoder_type; encoder->funcs = funcs; if (name) {

• va_list ap;
  

• va_start(ap, name);
  

  encoder->name = kvasprintf(GFP_KERNEL, name, ap);

• va_end(ap);
  

  } else { encoder->name = kasprintf(GFP_KERNEL, "%s-%d", drm_encoder_enum_list[encoder_type].name,

@@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,

return ret; }

+/**

• • drm_encoder_init - Init a preallocated encoder
• • @dev: drm device
• • @encoder: the encoder to init
• • @funcs: callbacks for this encoder
• • @encoder_type: user visible type of the encoder
• • @name: printf style format string for the encoder name, or NULL for default name
• • Initializes a preallocated encoder. Encoder should be subclassed as part of
• • driver encoder objects. At driver unload time drm_encoder_cleanup() should be
• • called from the driver's &drm_encoder_funcs.destroy hook.

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

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

• • Returns:
• • Zero on success, error code on failure.
• */

+int drm_encoder_init(struct drm_device *dev,

•      struct drm_encoder *encoder,
  

•      const struct drm_encoder_funcs *funcs,
  

•      int encoder_type, const char *name, ...)
  

+{

• va_list ap;
• int ret;

WARN_ON(!funcs->destroy);

WARN_ON(!funcs->destroy);

• va_start(ap, name);
• ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
• va_end(ap);
• return ret;

+} EXPORT_SYMBOL(drm_encoder_init);

/** @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup);

+static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr) +{

• struct drm_encoder *encoder = ptr;
• if (WARN_ON(!encoder->dev))

• return;
  

• drm_encoder_cleanup(encoder);

+}

+void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,

• 	   const struct drm_encoder_funcs *funcs,
  

• 	   int encoder_type, const char *name, ...)
  

+{

• void *container;
• struct drm_encoder *encoder;
• va_list ap;
• int ret;
• if (WARN_ON(funcs && funcs->destroy))

• return ERR_PTR(-EINVAL);
  

• container = drmm_kzalloc(dev, size, GFP_KERNEL);
• if (!container)

• return ERR_PTR(-EINVAL);
  

• encoder = container + offset;
• va_start(ap, name);
• ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
• va_end(ap);
• if (ret)

• return ERR_PTR(ret);
  

• ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
• if (ret)

• return ERR_PTR(ret);
  

• return container;

+} +EXPORT_SYMBOL(__drmm_encoder_alloc);

static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) { struct drm_connector *connector; diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 833123637fbf..fb2f56c006db 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -194,6 +194,36 @@ int drm_encoder_init(struct drm_device *dev, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...);

+__printf(6, 7) +void *__drmm_encoder_alloc(struct drm_device *dev,

• 	   size_t size, size_t offset,
  

• 	   const struct drm_encoder_funcs *funcs,
  

• 	   int encoder_type,
  

• 	   const char *name, ...);
  

+/**

• • drmm_encoder_alloc - Allocate and initialize an encoder
• • @dev: drm device
• • @type: the type of the struct which contains struct &drm_encoder
• • @member: the name of the &drm_encoder within @type.
• • @funcs: callbacks for this encoder

@funcsw are optional in this variant.

• • @encoder_type: user visible type of the encoder
• • @name: printf style format string for the encoder name, or NULL for default name
• • Allocates and initializes an encoder. Encoder should be subclassed as part of
• • driver encoder objects. Cleanup is automatically handled through registering
• • drm_encoder_cleanup() with drmm_add_action().
• • The @drm_encoder_funcs.destroy hook must be NULL.
• • Returns:
• • Pointer to new encoder, or ERR_PTR on failure.
• */

+#define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \

• ((type *)__drmm_encoder_alloc(dev, sizeof(type), \

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

• 		      offsetof(type, member), funcs, \
  

• 		      encoder_type, name, ##__VA_ARGS__))
  

/**

• drm_encoder_index - find the index of a registered encoder
• @encoder: encoder to find index for

With the nits addressed:

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

-- 2.20.1