Hi
Am 24.07.20 um 11:07 schrieb Daniel Vetter:
On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel p.zabel@pengutronix.de wrote:
On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote:
Add a drm_encoder_init() variant that allocates an encoder with drmm_kzalloc() and registers drm_encoder_cleanup() with drmm_add_action_or_reset().
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
Thanks for doing this!
New in v2
drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- include/drm/drm_encoder.h | 30 ++++++++++ 2 files changed, 108 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..91184f67333c 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,10 @@ 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, ...)+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 +111,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 +132,38 @@ 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
- 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, ...)+{
va_list ap;int ret;if (name)va_start(ap, name);ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);if (name)va_end(ap);return ret;+} EXPORT_SYMBOL(drm_encoder_init);
/** @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup);
+void drmm_encoder_init_release(struct drm_device *dev, void *ptr) +{
struct drm_encoder *encoder = ptr;I'll add
if (WARN_ON(!encoder->dev)) return;here.
drm_encoder_cleanup(encoder);+}
+void *__drmm_encoder_init(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;if (name)va_start(ap, name);ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);if (name)va_end(ap);if (ret)return ERR_PTR(ret);ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder);if (ret)return ERR_PTR(ret);return container;+} +EXPORT_SYMBOL(__drmm_encoder_init);
static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) { struct drm_connector *connector; diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index a60f5f1555ac..54b82554ee88 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...);
+__printf(6, 7) +void *__drmm_encoder_init(struct drm_device *dev,
size_t size, size_t offset,const struct drm_encoder_funcs *funcs,int encoder_type,const char *name, ...);+/**
- drmm_encoder_init - Allocate and initialize an encoder
- @dev: drm device
- @type: the type of the struct which contains struct &drm_encoder
- @member: the name of the &drm_encoder within @type.
- @funcs: callbacks for this encoder
- @encoder_type: user visible type of the encoder
- @name: printf style format string for the encoder name, or NULL for default name
- Allocates and initializes an encoder. Encoder should be subclassed as part of
- driver encoder objects. Cleanup is automatically handled through registering
- drm_encoder_cleanup() with drmm_add_action().
- The @drm_encoder_funcs.destroy hook must be NULL.
- Returns:
- Pointer to new encoder, or ERR_PTR on failure.
- */
+#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \
((type *) __drmm_encoder_init(dev, sizeof(type), \offsetof(type, member), funcs, \encoder_type, name, ##__VA_ARGS__))Should this be called drmm_encoder_alloc instead?
Yes. Same for the internal helper for consistency. Also see my other
I'd suggest calling it drmm_encoder_create() to express that it's _alloc + _init. Calling it just _alloc sounds as if it doesn't do the _init.
Best regards Thomas
reply, if the _alloc() variant is all that's needed that makes me happy, since then we don't need to code up the drmm_assert_managed check for the _init() variants to make sure drivers dont give it something stupid like a devm_kzalloc range :-) -Daniel