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 --- 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; + + 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__)) + /** * drm_encoder_index - find the index of a registered encoder * @encoder: encoder to find index for
Add a drm_simple_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 --- Changes since v1: - based on drmm_encoder_init - use AE initialize spelling - fold allocation into drmm_(simple_)encoder_init, to reduce boilerplate --- drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++ include/drm/drm_simple_kms_helper.h | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 74946690aba4..eb1ea7b6f05d 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -9,6 +9,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_simple_encoder_init);
+static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { }; + +void *__drmm_simple_encoder_init(struct drm_device *dev, size_t size, + size_t offset, int encoder_type) +{ + return __drmm_encoder_init(dev, size, offset, + &drmm_simple_encoder_funcs_empty, + encoder_type, NULL); +} +EXPORT_SYMBOL(__drmm_simple_encoder_init); + static enum drm_mode_status drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index a026375464ff..58f5d4d5e78a 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -185,4 +185,28 @@ int drm_simple_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, int encoder_type);
+void *__drmm_simple_encoder_init(struct drm_device *dev, size_t size, + size_t offset, int encoder_type); + +/** + * drmm_simple_encoder_init - Allocate and initialize an encoder with basic + * functionality. + * @dev: drm device + * @type: the type of the struct which contains struct &drm_encoder + * @member: the name of the &drm_encoder within @type. + * @encoder_type: user visible type of the encoder + * + * Allocates and initializes an encoder that has no further functionality. + * Settings for possible CRTC and clones are left to their initial values. + * Cleanup is automatically handled through registering drm_encoder_cleanup() + * with drmm_add_action(). + * + * Returns: + * Pointer to new encoder, or ERR_PTR on failure. + */ +#define drmm_simple_encoder_init(dev, type, member, encoder_type) \ + ((type *) __drmm_simple_encoder_init(dev, sizeof(type), \ + offsetof(type, member), \ + encoder_type)) + #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
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
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);va_end(ap);@@ -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?
regards Philipp
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 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
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
On Fri, Jul 24, 2020 at 11:33 AM Thomas Zimmermann tzimmermann@suse.de wrote:
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.
At least in drm the pattern we've had thus far is alloc = kzalloc + init. We do use create, but that's for properties and files and stuff, not allocating/initing driver objects.
So maybe a bit a bikeshed, but imo alloc() is more consistent with e.g. devm_drm_dev_alloc. Otherwise I think we should rename that one (and all related ones) for consistency. -Daniel
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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
On Fri, Jul 24, 2020 at 11:37 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jul 24, 2020 at 11:33 AM Thomas Zimmermann tzimmermann@suse.de wrote:
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.
At least in drm the pattern we've had thus far is alloc = kzalloc + init. We do use create, but that's for properties and files and stuff, not allocating/initing driver objects.
So maybe a bit a bikeshed, but imo alloc() is more consistent with e.g. devm_drm_dev_alloc. Otherwise I think we should rename that one (and all related ones) for consistency.
_create also sounds a bit too much like "completely create" to me, this _alloc here is very much just a small part of what it takes to create a complete encoder. The create functions we do have are a lot more the kind of "it's all done, including all the links and registering and everything". -Daniel
-Daniel
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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi Philipp,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.8-rc6 next-20200724] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Philipp-Zabel/drm-add-drmm_encoder_... base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-allyesconfig (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project e0ee2288424952e0445f096ae7800472eac11249) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/gpu/drm/drm_encoder.c:198:6: warning: no previous prototype for function 'drmm_encoder_init_release' [-Wmissing-prototypes]
void drmm_encoder_init_release(struct drm_device *dev, void *ptr) ^ drivers/gpu/drm/drm_encoder.c:198:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void drmm_encoder_init_release(struct drm_device *dev, void *ptr) ^ static 1 warning generated.
vim +/drmm_encoder_init_release +198 drivers/gpu/drm/drm_encoder.c
197
198 void drmm_encoder_init_release(struct drm_device *dev, void *ptr)
199 { 200 struct drm_encoder *encoder = ptr; 201 202 drm_encoder_cleanup(encoder); 203 } 204
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
kernel test robot -
Philipp Zabel -
Thomas Zimmermann