* [PATCH v2 1/2] drm: add drmm_encoder_init()
@ 2020-07-23 14:56 Philipp Zabel
2020-07-23 14:56 ` [PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init() Philipp Zabel
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Philipp Zabel @ 2020-07-23 14:56 UTC (permalink / raw)
To: dri-devel; +Cc: kernel, Thomas Zimmermann
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
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init()
2020-07-23 14:56 [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
@ 2020-07-23 14:56 ` Philipp Zabel
2020-07-24 8:49 ` [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
2020-07-25 16:38 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: Philipp Zabel @ 2020-07-23 14:56 UTC (permalink / raw)
To: dri-devel; +Cc: kernel, Thomas Zimmermann
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 */
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-23 14:56 [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
2020-07-23 14:56 ` [PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init() Philipp Zabel
@ 2020-07-24 8:49 ` Philipp Zabel
2020-07-24 9:07 ` Daniel Vetter
2020-07-25 16:38 ` kernel test robot
2 siblings, 1 reply; 8+ messages in thread
From: Philipp Zabel @ 2020-07-24 8:49 UTC (permalink / raw)
To: dri-devel; +Cc: Thomas Zimmermann, kernel
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);
> 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?
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-24 8:49 ` [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
@ 2020-07-24 9:07 ` Daniel Vetter
2020-07-24 9:33 ` Thomas Zimmermann
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2020-07-24 9:07 UTC (permalink / raw)
To: Philipp Zabel; +Cc: Thomas Zimmermann, Sascha Hauer, dri-devel
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-24 9:07 ` Daniel Vetter
@ 2020-07-24 9:33 ` Thomas Zimmermann
2020-07-24 9:37 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2020-07-24 9:33 UTC (permalink / raw)
To: Daniel Vetter, Philipp Zabel; +Cc: Sascha Hauer, dri-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 8575 bytes --]
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
>
--
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
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-24 9:33 ` Thomas Zimmermann
@ 2020-07-24 9:37 ` Daniel Vetter
2020-07-24 9:40 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2020-07-24 9:37 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: Sascha Hauer, dri-devel
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-24 9:37 ` Daniel Vetter
@ 2020-07-24 9:40 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2020-07-24 9:40 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: Sascha Hauer, dri-devel
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] drm: add drmm_encoder_init()
2020-07-23 14:56 [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
2020-07-23 14:56 ` [PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init() Philipp Zabel
2020-07-24 8:49 ` [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
@ 2020-07-25 16:38 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-07-25 16:38 UTC (permalink / raw)
To: Philipp Zabel, dri-devel
Cc: clang-built-linux, kbuild-all, Thomas Zimmermann, kernel
[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]
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_init/20200723-225705
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75338 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-25 16:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-23 14:56 [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
2020-07-23 14:56 ` [PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init() Philipp Zabel
2020-07-24 8:49 ` [PATCH v2 1/2] drm: add drmm_encoder_init() Philipp Zabel
2020-07-24 9:07 ` Daniel Vetter
2020-07-24 9:33 ` Thomas Zimmermann
2020-07-24 9:37 ` Daniel Vetter
2020-07-24 9:40 ` Daniel Vetter
2020-07-25 16:38 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox