[PATCH v2 1/2] drm: add drmm_encoder_init()

[PATCH v2 1/2] drm: add drmm_encoder_init()

[Philipp Zabel]

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

[PATCH v2 2/2] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Philipp Zabel]

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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[Philipp Zabel]

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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[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
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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[Thomas Zimmermann]

[-- 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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[Daniel Vetter]

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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[Daniel Vetter]

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

Re: [PATCH v2 1/2] drm: add drmm_encoder_init()

[kernel test robot]

[-- 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