* [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-12-04 9:19 ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
Add an alternative to drm_simple_encoder_init() that allocates and
initializes a simple encoder and registers drm_encoder_cleanup() with
drmm_add_action_or_reset().
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
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..3cbbfb0f9b51 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_alloc(struct drm_device *dev, size_t size,
+ size_t offset, int encoder_type)
+{
+ return __drmm_encoder_alloc(dev, size, offset,
+ &drmm_simple_encoder_funcs_empty,
+ encoder_type, NULL);
+}
+EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
+
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..e6dbf3161c2f 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_alloc(struct drm_device *dev, size_t size,
+ size_t offset, int encoder_type);
+
+/**
+ * drmm_simple_encoder_alloc - 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_alloc(dev, type, member, encoder_type) \
+ ((type *)__drmm_simple_encoder_alloc(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] 18+ messages in thread
* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-12-04 9:19 ` Laurent Pinchart
2020-12-04 10:13 ` Philipp Zabel
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04 9:19 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_simple_encoder_init() that allocates and
> initializes a simple encoder and registers drm_encoder_cleanup() with
> drmm_add_action_or_reset().
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> 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..3cbbfb0f9b51 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_alloc(struct drm_device *dev, size_t size,
> + size_t offset, int encoder_type)
> +{
> + return __drmm_encoder_alloc(dev, size, offset,
> + &drmm_simple_encoder_funcs_empty,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
in the core (if we don't already) and use drmm_encoder_alloc() directly
in drivers ?
> +
> 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..e6dbf3161c2f 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_alloc(struct drm_device *dev, size_t size,
> + size_t offset, int encoder_type);
> +
> +/**
> + * drmm_simple_encoder_alloc - 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_alloc(dev, type, member, encoder_type) \
> + ((type *)__drmm_simple_encoder_alloc(dev, sizeof(type), \
> + offsetof(type, member), \
> + encoder_type))
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
2020-12-04 9:19 ` Laurent Pinchart
@ 2020-12-04 10:13 ` Philipp Zabel
2020-12-05 18:58 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-12-04 10:13 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: kernel, dri-devel
Hi Laurent,
On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote:
> Hi Philipp,
>
> Thank you for the patch.
>
> On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> > Add an alternative to drm_simple_encoder_init() that allocates and
> > initializes a simple encoder and registers drm_encoder_cleanup() with
> > drmm_add_action_or_reset().
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > 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..3cbbfb0f9b51 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_alloc(struct drm_device *dev, size_t size,
> > + size_t offset, int encoder_type)
> > +{
> > + return __drmm_encoder_alloc(dev, size, offset,
> > + &drmm_simple_encoder_funcs_empty,
> > + encoder_type, NULL);
> > +}
> > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
>
> Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
> in the core (if we don't already) and use drmm_encoder_alloc() directly
> in drivers ?
I could change it to remove the empty drm_encoder_funcs, and prepend
something like this:
----------8<----------
From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Fri, 4 Dec 2020 10:49:41 +0100
Subject: [PATCH] drm/encoder: make encoder control functions optional
Simple managed encoders do not require the .destroy callback,
make the whole funcs structure optional.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/gpu/drm/drm_encoder.c | 4 ++--
drivers/gpu/drm/drm_mode_config.c | 5 +++--
include/drm/drm_encoder.h | 2 +-
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index e555281f43d4..b0b86a3c08f5 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev)
int ret = 0;
drm_for_each_encoder(encoder, dev) {
- if (encoder->funcs->late_register)
+ if (encoder->funcs && encoder->funcs->late_register)
ret = encoder->funcs->late_register(encoder);
if (ret)
return ret;
@@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev)
struct drm_encoder *encoder;
drm_for_each_encoder(encoder, dev) {
- if (encoder->funcs->early_unregister)
+ if (encoder->funcs && encoder->funcs->early_unregister)
encoder->funcs->early_unregister(encoder);
}
}
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index f1affc1bb679..87e144155456 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev)
crtc->funcs->reset(crtc);
drm_for_each_encoder(encoder, dev)
- if (encoder->funcs->reset)
+ if (encoder->funcs && encoder->funcs->reset)
encoder->funcs->reset(encoder);
drm_connector_list_iter_begin(dev, &conn_iter);
@@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
head) {
- encoder->funcs->destroy(encoder);
+ if (encoder->funcs)
+ encoder->funcs->destroy(encoder);
}
drm_connector_list_iter_begin(dev, &conn_iter);
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5dfa5f7a80a7..833123637fbf 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -89,7 +89,7 @@ struct drm_encoder_funcs {
* @head: list management
* @base: base KMS object
* @name: human readable name, can be overwritten by the driver
- * @funcs: control functions
+ * @funcs: control functions, can be NULL for simple managed encoders
* @helper_private: mid-layer private data
*
* CRTCs drive pixels to encoders, which convert them into signals
--
2.20.1
---------->8----------
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc()
2020-12-04 10:13 ` Philipp Zabel
@ 2020-12-05 18:58 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-05 18:58 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
On Fri, Dec 04, 2020 at 11:13:33AM +0100, Philipp Zabel wrote:
> On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote:
> > On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
> > > Add an alternative to drm_simple_encoder_init() that allocates and
> > > initializes a simple encoder and registers drm_encoder_cleanup() with
> > > drmm_add_action_or_reset().
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > 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..3cbbfb0f9b51 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_alloc(struct drm_device *dev, size_t size,
> > > + size_t offset, int encoder_type)
> > > +{
> > > + return __drmm_encoder_alloc(dev, size, offset,
> > > + &drmm_simple_encoder_funcs_empty,
> > > + encoder_type, NULL);
> > > +}
> > > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc);
> >
> > Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs
> > in the core (if we don't already) and use drmm_encoder_alloc() directly
> > in drivers ?
>
> I could change it to remove the empty drm_encoder_funcs, and prepend
> something like this:
>
> ----------8<----------
> From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Fri, 4 Dec 2020 10:49:41 +0100
> Subject: [PATCH] drm/encoder: make encoder control functions optional
>
> Simple managed encoders do not require the .destroy callback,
> make the whole funcs structure optional.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
That's perfect.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/drm_encoder.c | 4 ++--
> drivers/gpu/drm/drm_mode_config.c | 5 +++--
> include/drm/drm_encoder.h | 2 +-
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..b0b86a3c08f5 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev)
> int ret = 0;
>
> drm_for_each_encoder(encoder, dev) {
> - if (encoder->funcs->late_register)
> + if (encoder->funcs && encoder->funcs->late_register)
> ret = encoder->funcs->late_register(encoder);
> if (ret)
> return ret;
> @@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> struct drm_encoder *encoder;
>
> drm_for_each_encoder(encoder, dev) {
> - if (encoder->funcs->early_unregister)
> + if (encoder->funcs && encoder->funcs->early_unregister)
> encoder->funcs->early_unregister(encoder);
> }
> }
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index f1affc1bb679..87e144155456 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev)
> crtc->funcs->reset(crtc);
>
> drm_for_each_encoder(encoder, dev)
> - if (encoder->funcs->reset)
> + if (encoder->funcs && encoder->funcs->reset)
> encoder->funcs->reset(encoder);
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> @@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>
> list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
> head) {
> - encoder->funcs->destroy(encoder);
> + if (encoder->funcs)
> + encoder->funcs->destroy(encoder);
> }
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 5dfa5f7a80a7..833123637fbf 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -89,7 +89,7 @@ struct drm_encoder_funcs {
> * @head: list management
> * @base: base KMS object
> * @name: human readable name, can be overwritten by the driver
> - * @funcs: control functions
> + * @funcs: control functions, can be NULL for simple managed encoders
> * @helper_private: mid-layer private data
> *
> * CRTCs drive pixels to encoders, which convert them into signals
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-12-04 9:22 ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
Add an alternative to drm_universal_plane_init() that allocates
and initializes a plane and registers drm_plane_cleanup() with
drmm_add_action_or_reset().
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
- call va_start() / va_end() unconditionally
---
drivers/gpu/drm/drm_plane.c | 126 +++++++++++++++++++++++++++---------
include/drm/drm_plane.h | 42 ++++++++++++
2 files changed, 139 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index affe1cfed009..0081f6bb76b2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -30,6 +30,7 @@
#include <drm/drm_file.h>
#include <drm/drm_crtc.h>
#include <drm/drm_fourcc.h>
+#include <drm/drm_managed.h>
#include <drm/drm_vblank.h>
#include "drm_crtc_internal.h"
@@ -152,31 +153,16 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
return 0;
}
-/**
- * drm_universal_plane_init - Initialize a new universal plane object
- * @dev: DRM device
- * @plane: plane object to init
- * @possible_crtcs: bitmask of possible CRTCs
- * @funcs: callbacks for the new plane
- * @formats: array of supported formats (DRM_FORMAT\_\*)
- * @format_count: number of elements in @formats
- * @format_modifiers: array of struct drm_format modifiers terminated by
- * DRM_FORMAT_MOD_INVALID
- * @type: type of plane (overlay, primary, cursor)
- * @name: printf style format string for the plane name, or NULL for default name
- *
- * Initializes a plane object of type @type.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
- uint32_t possible_crtcs,
- const struct drm_plane_funcs *funcs,
- const uint32_t *formats, unsigned int format_count,
- const uint64_t *format_modifiers,
- enum drm_plane_type type,
- const char *name, ...)
+__printf(9, 0)
+static int __drm_universal_plane_init(struct drm_device *dev,
+ struct drm_plane *plane,
+ uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats,
+ unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type type,
+ const char *name, va_list ap)
{
struct drm_mode_config *config = &dev->mode_config;
unsigned int format_modifier_count = 0;
@@ -237,11 +223,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
}
if (name) {
- va_list ap;
-
- va_start(ap, name);
plane->name = kvasprintf(GFP_KERNEL, name, ap);
- va_end(ap);
} else {
plane->name = kasprintf(GFP_KERNEL, "plane-%d",
drm_num_planes(dev));
@@ -286,8 +268,94 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
return 0;
}
+
+/**
+ * drm_universal_plane_init - Initialize a new universal plane object
+ * @dev: DRM device
+ * @plane: plane object to init
+ * @possible_crtcs: bitmask of possible CRTCs
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ * DRM_FORMAT_MOD_INVALID
+ * @type: type of plane (overlay, primary, cursor)
+ * @name: printf style format string for the plane name, or NULL for default name
+ *
+ * Initializes a plane object of type @type.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
+ uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type type,
+ const char *name, ...)
+{
+ va_list ap;
+ int ret;
+
+ va_start(ap, name);
+ ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+ formats, format_count, format_modifiers,
+ type, name, ap);
+ va_end(ap);
+ return ret;
+}
EXPORT_SYMBOL(drm_universal_plane_init);
+static void drmm_universal_plane_alloc_release(struct drm_device *dev, void *ptr)
+{
+ struct drm_plane *plane = ptr;
+
+ if (WARN_ON(!plane->dev))
+ return;
+
+ drm_plane_cleanup(plane);
+}
+
+void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
+ size_t offset, uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type type,
+ const char *name, ...)
+{
+ void *container;
+ struct drm_plane *plane;
+ va_list ap;
+ int ret;
+
+ if (!funcs || funcs->destroy)
+ return ERR_PTR(-EINVAL);
+
+ container = drmm_kzalloc(dev, size, GFP_KERNEL);
+ if (!container)
+ return ERR_PTR(-ENOMEM);
+
+ plane = container + offset;
+
+ va_start(ap, name);
+ ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+ formats, format_count, format_modifiers,
+ type, name, ap);
+ va_end(ap);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = drmm_add_action_or_reset(dev, drmm_universal_plane_alloc_release,
+ plane);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return container;
+}
+EXPORT_SYMBOL(__drmm_universal_plane_alloc);
+
int drm_plane_register_all(struct drm_device *dev)
{
unsigned int num_planes = 0;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3f396d94afe4..82bd63710a39 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -746,6 +746,48 @@ int drm_plane_init(struct drm_device *dev,
bool is_primary);
void drm_plane_cleanup(struct drm_plane *plane);
+__printf(10, 11)
+void *__drmm_universal_plane_alloc(struct drm_device *dev,
+ size_t size, size_t offset,
+ uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats,
+ unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type plane_type,
+ const char *name, ...);
+
+/**
+ * drmm_universal_plane_alloc - Allocate and initialize an universal plane object
+ * @dev: DRM device
+ * @type: the type of the struct which contains struct &drm_plane
+ * @member: the name of the &drm_plane within @type
+ * @possible_crtcs: bitmask of possible CRTCs
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ * DRM_FORMAT_MOD_INVALID
+ * @plane_type: type of plane (overlay, primary, cursor)
+ * @name: printf style format string for the plane name, or NULL for default name
+ *
+ * Allocates and initializes a plane object of type @type. Cleanup is
+ * automatically handled through registering drm_plane_cleanup() with
+ * drmm_add_action().
+ *
+ * The @drm_plane_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Pointer to new plane, or ERR_PTR on failure.
+ */
+#define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
+ format_count, format_modifiers, plane_type, name, ...) \
+ ((type *)__drmm_universal_plane_alloc(dev, sizeof(type), \
+ offsetof(type, member), \
+ possible_crtcs, funcs, formats, \
+ format_count, format_modifiers, \
+ plane_type, name, ##__VA_ARGS__))
+
/**
* drm_plane_index - find the index of a registered plane
* @plane: plane 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] 18+ messages in thread
* Re: [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc()
2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
@ 2020-12-04 9:22 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04 9:22 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:20PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_universal_plane_init() that allocates
> and initializes a plane and registers drm_plane_cleanup() with
> drmm_add_action_or_reset().
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
> - call va_start() / va_end() unconditionally
> ---
> drivers/gpu/drm/drm_plane.c | 126 +++++++++++++++++++++++++++---------
> include/drm/drm_plane.h | 42 ++++++++++++
> 2 files changed, 139 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index affe1cfed009..0081f6bb76b2 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -30,6 +30,7 @@
> #include <drm/drm_file.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_fourcc.h>
> +#include <drm/drm_managed.h>
> #include <drm/drm_vblank.h>
>
> #include "drm_crtc_internal.h"
> @@ -152,31 +153,16 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
> return 0;
> }
>
> -/**
> - * drm_universal_plane_init - Initialize a new universal plane object
> - * @dev: DRM device
> - * @plane: plane object to init
> - * @possible_crtcs: bitmask of possible CRTCs
> - * @funcs: callbacks for the new plane
> - * @formats: array of supported formats (DRM_FORMAT\_\*)
> - * @format_count: number of elements in @formats
> - * @format_modifiers: array of struct drm_format modifiers terminated by
> - * DRM_FORMAT_MOD_INVALID
> - * @type: type of plane (overlay, primary, cursor)
> - * @name: printf style format string for the plane name, or NULL for default name
> - *
> - * Initializes a plane object of type @type.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> - uint32_t possible_crtcs,
> - const struct drm_plane_funcs *funcs,
> - const uint32_t *formats, unsigned int format_count,
> - const uint64_t *format_modifiers,
> - enum drm_plane_type type,
> - const char *name, ...)
> +__printf(9, 0)
> +static int __drm_universal_plane_init(struct drm_device *dev,
> + struct drm_plane *plane,
> + uint32_t possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + const uint32_t *formats,
> + unsigned int format_count,
> + const uint64_t *format_modifiers,
> + enum drm_plane_type type,
> + const char *name, va_list ap)
> {
> struct drm_mode_config *config = &dev->mode_config;
> unsigned int format_modifier_count = 0;
> @@ -237,11 +223,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> }
>
> if (name) {
> - va_list ap;
> -
> - va_start(ap, name);
> plane->name = kvasprintf(GFP_KERNEL, name, ap);
> - va_end(ap);
> } else {
> plane->name = kasprintf(GFP_KERNEL, "plane-%d",
> drm_num_planes(dev));
> @@ -286,8 +268,94 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>
> return 0;
> }
> +
> +/**
> + * drm_universal_plane_init - Initialize a new universal plane object
> + * @dev: DRM device
> + * @plane: plane object to init
> + * @possible_crtcs: bitmask of possible CRTCs
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (DRM_FORMAT\_\*)
> + * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + * DRM_FORMAT_MOD_INVALID
> + * @type: type of plane (overlay, primary, cursor)
> + * @name: printf style format string for the plane name, or NULL for default name
> + *
> + * Initializes a plane object of type @type.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> + uint32_t possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers,
> + enum drm_plane_type type,
> + const char *name, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, name);
> + ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> + formats, format_count, format_modifiers,
> + type, name, ap);
> + va_end(ap);
> + return ret;
> +}
> EXPORT_SYMBOL(drm_universal_plane_init);
>
> +static void drmm_universal_plane_alloc_release(struct drm_device *dev, void *ptr)
> +{
> + struct drm_plane *plane = ptr;
> +
> + if (WARN_ON(!plane->dev))
> + return;
> +
> + drm_plane_cleanup(plane);
> +}
> +
> +void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
> + size_t offset, uint32_t possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + const uint32_t *formats, unsigned int format_count,
> + const uint64_t *format_modifiers,
> + enum drm_plane_type type,
> + const char *name, ...)
> +{
> + void *container;
> + struct drm_plane *plane;
> + va_list ap;
> + int ret;
> +
> + if (!funcs || funcs->destroy)
> + return ERR_PTR(-EINVAL);
> +
> + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> + if (!container)
> + return ERR_PTR(-ENOMEM);
> +
> + plane = container + offset;
> +
> + va_start(ap, name);
> + ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> + formats, format_count, format_modifiers,
> + type, name, ap);
> + va_end(ap);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + ret = drmm_add_action_or_reset(dev, drmm_universal_plane_alloc_release,
> + plane);
Similarly as in 1/9, how about using kzalloc() with a kfree() in
drmm_universal_plane_alloc_release() ? With this,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return container;
> +}
> +EXPORT_SYMBOL(__drmm_universal_plane_alloc);
> +
> int drm_plane_register_all(struct drm_device *dev)
> {
> unsigned int num_planes = 0;
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 3f396d94afe4..82bd63710a39 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -746,6 +746,48 @@ int drm_plane_init(struct drm_device *dev,
> bool is_primary);
> void drm_plane_cleanup(struct drm_plane *plane);
>
> +__printf(10, 11)
> +void *__drmm_universal_plane_alloc(struct drm_device *dev,
> + size_t size, size_t offset,
> + uint32_t possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + const uint32_t *formats,
> + unsigned int format_count,
> + const uint64_t *format_modifiers,
> + enum drm_plane_type plane_type,
> + const char *name, ...);
> +
> +/**
> + * drmm_universal_plane_alloc - Allocate and initialize an universal plane object
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct &drm_plane
> + * @member: the name of the &drm_plane within @type
> + * @possible_crtcs: bitmask of possible CRTCs
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (DRM_FORMAT\_\*)
> + * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + * DRM_FORMAT_MOD_INVALID
> + * @plane_type: type of plane (overlay, primary, cursor)
> + * @name: printf style format string for the plane name, or NULL for default name
> + *
> + * Allocates and initializes a plane object of type @type. Cleanup is
> + * automatically handled through registering drm_plane_cleanup() with
> + * drmm_add_action().
> + *
> + * The @drm_plane_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new plane, or ERR_PTR on failure.
> + */
> +#define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
> + format_count, format_modifiers, plane_type, name, ...) \
> + ((type *)__drmm_universal_plane_alloc(dev, sizeof(type), \
> + offsetof(type, member), \
> + possible_crtcs, funcs, formats, \
> + format_count, format_modifiers, \
> + plane_type, name, ##__VA_ARGS__))
> +
> /**
> * drm_plane_index - find the index of a registered plane
> * @plane: plane to find index for
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 2/7] drm/simple_kms_helper: add drmm_simple_encoder_alloc() Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 3/7] drm/plane: add drmm_universal_plane_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-12-04 9:23 ` Laurent Pinchart
2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
Add an alternative to drm_crtc_init_with_planes() that allocates
and initializes a crtc and registers drm_crtc_cleanup() with
drmm_add_action_or_reset().
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
- call va_start() / va_end() unconditionally
---
drivers/gpu/drm/drm_crtc.c | 116 ++++++++++++++++++++++++++++---------
include/drm/drm_crtc.h | 33 +++++++++++
2 files changed, 121 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index aecdd7ea26dc..b9e7c11a76b7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -38,6 +38,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
#include <drm/drm_fourcc.h>
+#include <drm/drm_managed.h>
#include <drm/drm_modeset_lock.h>
#include <drm/drm_atomic.h>
#include <drm/drm_auth.h>
@@ -231,30 +232,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
* Setting MODE_ID to 0 will release reserved resources for the CRTC.
*/
-/**
- * drm_crtc_init_with_planes - Initialise a new CRTC object with
- * specified primary and cursor planes.
- * @dev: DRM device
- * @crtc: CRTC object to init
- * @primary: Primary plane for CRTC
- * @cursor: Cursor plane for CRTC
- * @funcs: callbacks for the new CRTC
- * @name: printf style format string for the CRTC name, or NULL for default name
- *
- * Inits a new object created as base part of a driver crtc object. Drivers
- * should use this function instead of drm_crtc_init(), which is only provided
- * for backwards compatibility with drivers which do not yet support universal
- * planes). For really simple hardware which has only 1 plane look at
- * drm_simple_display_pipe_init() instead.
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
- struct drm_plane *primary,
- struct drm_plane *cursor,
- const struct drm_crtc_funcs *funcs,
- const char *name, ...)
+__printf(6, 0)
+static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
+ struct drm_plane *primary,
+ struct drm_plane *cursor,
+ const struct drm_crtc_funcs *funcs,
+ const char *name, va_list ap)
{
struct drm_mode_config *config = &dev->mode_config;
int ret;
@@ -282,11 +265,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
return ret;
if (name) {
- va_list ap;
-
- va_start(ap, name);
crtc->name = kvasprintf(GFP_KERNEL, name, ap);
- va_end(ap);
} else {
crtc->name = kasprintf(GFP_KERNEL, "crtc-%d",
drm_num_crtcs(dev));
@@ -330,8 +309,89 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
return 0;
}
+
+/**
+ * drm_crtc_init_with_planes - Initialise a new CRTC object with
+ * specified primary and cursor planes.
+ * @dev: DRM device
+ * @crtc: CRTC object to init
+ * @primary: Primary plane for CRTC
+ * @cursor: Cursor plane for CRTC
+ * @funcs: callbacks for the new CRTC
+ * @name: printf style format string for the CRTC name, or NULL for default name
+ *
+ * Inits a new object created as base part of a driver crtc object. Drivers
+ * should use this function instead of drm_crtc_init(), which is only provided
+ * for backwards compatibility with drivers which do not yet support universal
+ * planes). For really simple hardware which has only 1 plane look at
+ * drm_simple_display_pipe_init() instead.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
+ struct drm_plane *primary,
+ struct drm_plane *cursor,
+ const struct drm_crtc_funcs *funcs,
+ const char *name, ...)
+{
+ va_list ap;
+ int ret;
+
+ va_start(ap, name);
+ ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
+ name, ap);
+ va_end(ap);
+
+ return ret;
+}
EXPORT_SYMBOL(drm_crtc_init_with_planes);
+static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev,
+ void *ptr)
+{
+ struct drm_crtc *crtc = ptr;
+
+ drm_crtc_cleanup(crtc);
+}
+
+void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
+ size_t size, size_t offset,
+ struct drm_plane *primary,
+ struct drm_plane *cursor,
+ const struct drm_crtc_funcs *funcs,
+ const char *name, ...)
+{
+ void *container;
+ struct drm_crtc *crtc;
+ va_list ap;
+ int ret;
+
+ if (!funcs || funcs->destroy)
+ return ERR_PTR(-EINVAL);
+
+ container = drmm_kzalloc(dev, size, GFP_KERNEL);
+ if (!container)
+ return ERR_PTR(-ENOMEM);
+
+ crtc = container + offset;
+
+ va_start(ap, name);
+ ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
+ name, ap);
+ va_end(ap);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = drmm_add_action_or_reset(dev, drmm_crtc_alloc_with_planes_cleanup,
+ crtc);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return container;
+}
+EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes);
+
/**
* drm_crtc_cleanup - Clean up the core crtc usage
* @crtc: CRTC to cleanup
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 59b51a09cae6..b71c0a3d4126 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1210,6 +1210,39 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
const char *name, ...);
void drm_crtc_cleanup(struct drm_crtc *crtc);
+__printf(7, 8)
+void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
+ size_t size, size_t offset,
+ struct drm_plane *primary,
+ struct drm_plane *cursor,
+ const struct drm_crtc_funcs *funcs,
+ const char *name, ...);
+
+/**
+ * drm_crtc_alloc_with_planes - Allocate and initialize a new CRTC object with
+ * specified primary and cursor planes.
+ * @dev: DRM device
+ * @type: the type of the struct which contains struct &drm_crtc
+ * @member: the name of the &drm_crtc within @type.
+ * @primary: Primary plane for CRTC
+ * @cursor: Cursor plane for CRTC
+ * @funcs: callbacks for the new CRTC
+ * @name: printf style format string for the CRTC name, or NULL for default name
+ *
+ * Allocates and initializes a new crtc object. Cleanup is automatically
+ * handled through registering drmm_crtc_cleanup() with drmm_add_action().
+ *
+ * The @drm_crtc_funcs.destroy hook must be NULL.
+ *
+ * Returns:
+ * Pointer to new crtc, or ERR_PTR on failure.
+ */
+#define drmm_crtc_alloc_with_planes(dev, type, member, primary, cursor, funcs, name, ...) \
+ ((type *)__drmm_crtc_alloc_with_planes(dev, sizeof(type), \
+ offsetof(type, member), \
+ primary, cursor, funcs, \
+ name, ##__VA_ARGS__))
+
/**
* drm_crtc_index - find the index of a registered CRTC
* @crtc: CRTC 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] 18+ messages in thread
* Re: [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes()
2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-12-04 9:23 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04 9:23 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:21PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_crtc_init_with_planes() that allocates
> and initializes a crtc and registers drm_crtc_cleanup() with
> drmm_add_action_or_reset().
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
> - call va_start() / va_end() unconditionally
> ---
> drivers/gpu/drm/drm_crtc.c | 116 ++++++++++++++++++++++++++++---------
> include/drm/drm_crtc.h | 33 +++++++++++
> 2 files changed, 121 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index aecdd7ea26dc..b9e7c11a76b7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -38,6 +38,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> #include <drm/drm_fourcc.h>
> +#include <drm/drm_managed.h>
> #include <drm/drm_modeset_lock.h>
> #include <drm/drm_atomic.h>
> #include <drm/drm_auth.h>
> @@ -231,30 +232,12 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc)
> * Setting MODE_ID to 0 will release reserved resources for the CRTC.
> */
>
> -/**
> - * drm_crtc_init_with_planes - Initialise a new CRTC object with
> - * specified primary and cursor planes.
> - * @dev: DRM device
> - * @crtc: CRTC object to init
> - * @primary: Primary plane for CRTC
> - * @cursor: Cursor plane for CRTC
> - * @funcs: callbacks for the new CRTC
> - * @name: printf style format string for the CRTC name, or NULL for default name
> - *
> - * Inits a new object created as base part of a driver crtc object. Drivers
> - * should use this function instead of drm_crtc_init(), which is only provided
> - * for backwards compatibility with drivers which do not yet support universal
> - * planes). For really simple hardware which has only 1 plane look at
> - * drm_simple_display_pipe_init() instead.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> - struct drm_plane *primary,
> - struct drm_plane *cursor,
> - const struct drm_crtc_funcs *funcs,
> - const char *name, ...)
> +__printf(6, 0)
> +static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> + struct drm_plane *primary,
> + struct drm_plane *cursor,
> + const struct drm_crtc_funcs *funcs,
> + const char *name, va_list ap)
> {
> struct drm_mode_config *config = &dev->mode_config;
> int ret;
> @@ -282,11 +265,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> return ret;
>
> if (name) {
> - va_list ap;
> -
> - va_start(ap, name);
> crtc->name = kvasprintf(GFP_KERNEL, name, ap);
> - va_end(ap);
> } else {
> crtc->name = kasprintf(GFP_KERNEL, "crtc-%d",
> drm_num_crtcs(dev));
> @@ -330,8 +309,89 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>
> return 0;
> }
> +
> +/**
> + * drm_crtc_init_with_planes - Initialise a new CRTC object with
> + * specified primary and cursor planes.
> + * @dev: DRM device
> + * @crtc: CRTC object to init
> + * @primary: Primary plane for CRTC
> + * @cursor: Cursor plane for CRTC
> + * @funcs: callbacks for the new CRTC
> + * @name: printf style format string for the CRTC name, or NULL for default name
> + *
> + * Inits a new object created as base part of a driver crtc object. Drivers
> + * should use this function instead of drm_crtc_init(), which is only provided
> + * for backwards compatibility with drivers which do not yet support universal
> + * planes). For really simple hardware which has only 1 plane look at
> + * drm_simple_display_pipe_init() instead.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> + struct drm_plane *primary,
> + struct drm_plane *cursor,
> + const struct drm_crtc_funcs *funcs,
> + const char *name, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + va_start(ap, name);
> + ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
> + name, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> EXPORT_SYMBOL(drm_crtc_init_with_planes);
>
> +static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev,
> + void *ptr)
> +{
> + struct drm_crtc *crtc = ptr;
> +
> + drm_crtc_cleanup(crtc);
> +}
> +
> +void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
> + size_t size, size_t offset,
> + struct drm_plane *primary,
> + struct drm_plane *cursor,
> + const struct drm_crtc_funcs *funcs,
> + const char *name, ...)
> +{
> + void *container;
> + struct drm_crtc *crtc;
> + va_list ap;
> + int ret;
> +
> + if (!funcs || funcs->destroy)
> + return ERR_PTR(-EINVAL);
> +
> + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> + if (!container)
> + return ERR_PTR(-ENOMEM);
> +
> + crtc = container + offset;
> +
> + va_start(ap, name);
> + ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
> + name, ap);
> + va_end(ap);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + ret = drmm_add_action_or_reset(dev, drmm_crtc_alloc_with_planes_cleanup,
> + crtc);
Similarly as in 1/9, how about using kzalloc() with a kfree() in
drmm_crtc_alloc_with_planes_cleanup() ? With this,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return container;
> +}
> +EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes);
> +
> /**
> * drm_crtc_cleanup - Clean up the core crtc usage
> * @crtc: CRTC to cleanup
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 59b51a09cae6..b71c0a3d4126 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1210,6 +1210,39 @@ int drm_crtc_init_with_planes(struct drm_device *dev,
> const char *name, ...);
> void drm_crtc_cleanup(struct drm_crtc *crtc);
>
> +__printf(7, 8)
> +void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
> + size_t size, size_t offset,
> + struct drm_plane *primary,
> + struct drm_plane *cursor,
> + const struct drm_crtc_funcs *funcs,
> + const char *name, ...);
> +
> +/**
> + * drm_crtc_alloc_with_planes - Allocate and initialize a new CRTC object with
> + * specified primary and cursor planes.
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct &drm_crtc
> + * @member: the name of the &drm_crtc within @type.
> + * @primary: Primary plane for CRTC
> + * @cursor: Cursor plane for CRTC
> + * @funcs: callbacks for the new CRTC
> + * @name: printf style format string for the CRTC name, or NULL for default name
> + *
> + * Allocates and initializes a new crtc object. Cleanup is automatically
> + * handled through registering drmm_crtc_cleanup() with drmm_add_action().
> + *
> + * The @drm_crtc_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new crtc, or ERR_PTR on failure.
> + */
> +#define drmm_crtc_alloc_with_planes(dev, type, member, primary, cursor, funcs, name, ...) \
> + ((type *)__drmm_crtc_alloc_with_planes(dev, sizeof(type), \
> + offsetof(type, member), \
> + primary, cursor, funcs, \
> + name, ##__VA_ARGS__))
> +
> /**
> * drm_crtc_index - find the index of a registered CRTC
> * @crtc: CRTC to find index for
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
` (2 preceding siblings ...)
2020-09-11 13:57 ` [PATCH v3 4/7] drm/crtc: add drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-09-16 9:08 ` Daniel Vetter
2020-09-11 13:57 ` [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc() Philipp Zabel
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
This allows to drop the custom drm_encoder_cleanup() actions.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_simple_encoder_init() users.
This and the following patches depend on the drm/imx conversion to use managed
resources [1].
[1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
---
drivers/gpu/drm/imx/dw_hdmi-imx.c | 19 ++++---------------
drivers/gpu/drm/imx/imx-ldb.c | 20 ++++----------------
drivers/gpu/drm/imx/imx-tve.c | 22 ++++------------------
drivers/gpu/drm/imx/parallel-display.c | 22 ++++------------------
4 files changed, 16 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index 16be8bd92653..87428fb23d9f 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -188,13 +188,6 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
-static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data)
-{
- struct drm_encoder *encoder = data;
-
- drm_encoder_cleanup(encoder);
-}
-
static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
void *data)
{
@@ -203,9 +196,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
struct drm_encoder *encoder;
int ret;
- hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL);
- if (!hdmi_encoder)
- return -ENOMEM;
+ hdmi_encoder = drmm_simple_encoder_alloc(drm, struct imx_hdmi_encoder,
+ encoder, DRM_MODE_ENCODER_TMDS);
+ if (IS_ERR(hdmi_encoder))
+ return PTR_ERR(hdmi_encoder);
hdmi_encoder->hdmi = dev_get_drvdata(dev);
encoder = &hdmi_encoder->encoder;
@@ -215,11 +209,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
return ret;
drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
-
- ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder);
- if (ret)
- return ret;
return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
}
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index d4beb58f509d..dbfe39e2f7f6 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -414,13 +414,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
}
-static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data)
-{
- struct drm_encoder *encoder = data;
-
- drm_encoder_cleanup(encoder);
-}
-
static int imx_ldb_register(struct drm_device *drm,
struct imx_ldb_channel *imx_ldb_ch)
{
@@ -430,20 +423,15 @@ static int imx_ldb_register(struct drm_device *drm,
struct drm_encoder *encoder;
int ret;
- ldb_encoder = drmm_kzalloc(drm, sizeof(*ldb_encoder), GFP_KERNEL);
- if (!ldb_encoder)
- return -ENOMEM;
+ ldb_encoder = drmm_simple_encoder_alloc(drm, struct imx_ldb_encoder,
+ encoder, DRM_MODE_ENCODER_LVDS);
+ if (IS_ERR(ldb_encoder))
+ return PTR_ERR(ldb_encoder);
ldb_encoder->channel = imx_ldb_ch;
connector = &ldb_encoder->connector;
encoder = &ldb_encoder->encoder;
- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
-
- ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder);
- if (ret)
- return ret;
-
ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index bac025eafa1f..0746f0b425df 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -433,13 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
return 0;
}
-static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr)
-{
- struct drm_encoder *encoder = ptr;
-
- drm_encoder_cleanup(encoder);
-}
-
static void imx_tve_disable_regulator(void *data)
{
struct imx_tve *tve = data;
@@ -498,22 +491,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
encoder_type = tve->mode == TVE_MODE_VGA ?
DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
- tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL);
- if (!tvee)
- return -ENOMEM;
+ tvee = drmm_simple_encoder_alloc(drm, struct imx_tve_encoder, encoder,
+ encoder_type);
+ if (IS_ERR(tvee))
+ return PTR_ERR(tvee);
tvee->tve = tve;
encoder = &tvee->encoder;
connector = &tvee->connector;
- ret = drm_simple_encoder_init(drm, encoder, encoder_type);
- if (ret)
- return ret;
-
- ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder);
- if (ret)
- return ret;
-
ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 50b5b89c2db2..9b1ec7e73c30 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -258,13 +258,6 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
.atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
};
-static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr)
-{
- struct drm_encoder *encoder = ptr;
-
- drm_encoder_cleanup(encoder);
-}
-
static int imx_pd_bind(struct device *dev, struct device *master, void *data)
{
struct drm_device *drm = data;
@@ -275,23 +268,16 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
struct drm_bridge *bridge;
int ret;
- imxpd_encoder = drmm_kzalloc(drm, sizeof(*imxpd_encoder), GFP_KERNEL);
- if (!imxpd_encoder)
- return -ENOMEM;
+ imxpd_encoder = drmm_simple_encoder_alloc(drm, struct imx_parallel_display_encoder,
+ encoder, DRM_MODE_ENCODER_NONE);
+ if (IS_ERR(imxpd_encoder))
+ return PTR_ERR(imxpd_encoder);
imxpd_encoder->pd = imxpd;
connector = &imxpd_encoder->connector;
encoder = &imxpd_encoder->encoder;
bridge = &imxpd_encoder->bridge;
- ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
- if (ret)
- return ret;
-
- ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
- if (ret)
- return ret;
-
ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
if (ret)
return ret;
--
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] 18+ messages in thread
* Re: [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-16 9:08 ` Daniel Vetter
2020-09-16 10:22 ` Philipp Zabel
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2020-09-16 9:08 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
> This allows to drop the custom drm_encoder_cleanup() actions.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> New in v3, example conversion of drm_simple_encoder_init() users.
>
> This and the following patches depend on the drm/imx conversion to use managed
> resources [1].
>
> [1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
> ---
> drivers/gpu/drm/imx/dw_hdmi-imx.c | 19 ++++---------------
> drivers/gpu/drm/imx/imx-ldb.c | 20 ++++----------------
> drivers/gpu/drm/imx/imx-tve.c | 22 ++++------------------
> drivers/gpu/drm/imx/parallel-display.c | 22 ++++------------------
> 4 files changed, 16 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index 16be8bd92653..87428fb23d9f 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -188,13 +188,6 @@ static const struct of_device_id dw_hdmi_imx_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, dw_hdmi_imx_dt_ids);
>
> -static void dw_hdmi_imx_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> - struct drm_encoder *encoder = data;
> -
> - drm_encoder_cleanup(encoder);
> -}
> -
> static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> void *data)
> {
> @@ -203,9 +196,10 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> struct drm_encoder *encoder;
> int ret;
>
> - hdmi_encoder = drmm_kzalloc(drm, sizeof(*hdmi_encoder), GFP_KERNEL);
Where does this apply to? Neither upstream nor linux-next seems to have
the drmm_ conversion for imx already applied, and that's kinda the juicy
part I'd like to look at a bit. The patches here are just mechanical
conversion. Can you pls include the drmm_ conversion too (maybe even
squash these patches here in, I think that would be more readable)?
Or am I looking at the wrong tree?
Anyway I think it looks all neat.
-Daniel
> - if (!hdmi_encoder)
> - return -ENOMEM;
> + hdmi_encoder = drmm_simple_encoder_alloc(drm, struct imx_hdmi_encoder,
> + encoder, DRM_MODE_ENCODER_TMDS);
> + if (IS_ERR(hdmi_encoder))
> + return PTR_ERR(hdmi_encoder);
>
> hdmi_encoder->hdmi = dev_get_drvdata(dev);
> encoder = &hdmi_encoder->encoder;
> @@ -215,11 +209,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> return ret;
>
> drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> -
> - ret = drmm_add_action_or_reset(drm, dw_hdmi_imx_encoder_cleanup, encoder);
> - if (ret)
> - return ret;
>
> return drm_bridge_attach(encoder, hdmi_encoder->hdmi->bridge, NULL, 0);
> }
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index d4beb58f509d..dbfe39e2f7f6 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -414,13 +414,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
> return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
> }
>
> -static void imx_ldb_encoder_cleanup(struct drm_device *drm, void *data)
> -{
> - struct drm_encoder *encoder = data;
> -
> - drm_encoder_cleanup(encoder);
> -}
> -
> static int imx_ldb_register(struct drm_device *drm,
> struct imx_ldb_channel *imx_ldb_ch)
> {
> @@ -430,20 +423,15 @@ static int imx_ldb_register(struct drm_device *drm,
> struct drm_encoder *encoder;
> int ret;
>
> - ldb_encoder = drmm_kzalloc(drm, sizeof(*ldb_encoder), GFP_KERNEL);
> - if (!ldb_encoder)
> - return -ENOMEM;
> + ldb_encoder = drmm_simple_encoder_alloc(drm, struct imx_ldb_encoder,
> + encoder, DRM_MODE_ENCODER_LVDS);
> + if (IS_ERR(ldb_encoder))
> + return PTR_ERR(ldb_encoder);
>
> ldb_encoder->channel = imx_ldb_ch;
> connector = &ldb_encoder->connector;
> encoder = &ldb_encoder->encoder;
>
> - drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_LVDS);
> -
> - ret = drmm_add_action_or_reset(drm, imx_ldb_encoder_cleanup, encoder);
> - if (ret)
> - return ret;
> -
> ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index bac025eafa1f..0746f0b425df 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -433,13 +433,6 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
> return 0;
> }
>
> -static void imx_tve_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> - struct drm_encoder *encoder = ptr;
> -
> - drm_encoder_cleanup(encoder);
> -}
> -
> static void imx_tve_disable_regulator(void *data)
> {
> struct imx_tve *tve = data;
> @@ -498,22 +491,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> encoder_type = tve->mode == TVE_MODE_VGA ?
> DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
>
> - tvee = drmm_kzalloc(drm, sizeof(*tvee), GFP_KERNEL);
> - if (!tvee)
> - return -ENOMEM;
> + tvee = drmm_simple_encoder_alloc(drm, struct imx_tve_encoder, encoder,
> + encoder_type);
> + if (IS_ERR(tvee))
> + return PTR_ERR(tvee);
>
> tvee->tve = tve;
> encoder = &tvee->encoder;
> connector = &tvee->connector;
>
> - ret = drm_simple_encoder_init(drm, encoder, encoder_type);
> - if (ret)
> - return ret;
> -
> - ret = drmm_add_action_or_reset(drm, imx_tve_encoder_cleanup, encoder);
> - if (ret)
> - return ret;
> -
> ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 50b5b89c2db2..9b1ec7e73c30 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,13 +258,6 @@ static const struct drm_bridge_funcs imx_pd_bridge_funcs = {
> .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts,
> };
>
> -static void imx_pd_encoder_cleanup(struct drm_device *drm, void *ptr)
> -{
> - struct drm_encoder *encoder = ptr;
> -
> - drm_encoder_cleanup(encoder);
> -}
> -
> static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> {
> struct drm_device *drm = data;
> @@ -275,23 +268,16 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> struct drm_bridge *bridge;
> int ret;
>
> - imxpd_encoder = drmm_kzalloc(drm, sizeof(*imxpd_encoder), GFP_KERNEL);
> - if (!imxpd_encoder)
> - return -ENOMEM;
> + imxpd_encoder = drmm_simple_encoder_alloc(drm, struct imx_parallel_display_encoder,
> + encoder, DRM_MODE_ENCODER_NONE);
> + if (IS_ERR(imxpd_encoder))
> + return PTR_ERR(imxpd_encoder);
>
> imxpd_encoder->pd = imxpd;
> connector = &imxpd_encoder->connector;
> encoder = &imxpd_encoder->encoder;
> bridge = &imxpd_encoder->bridge;
>
> - ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE);
> - if (ret)
> - return ret;
> -
> - ret = drmm_add_action_or_reset(drm, imx_pd_encoder_cleanup, encoder);
> - if (ret)
> - return ret;
> -
> ret = imx_drm_encoder_parse_of(drm, encoder, imxpd->dev->of_node);
> if (ret)
> return ret;
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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] 18+ messages in thread
* Re: [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc()
2020-09-16 9:08 ` Daniel Vetter
@ 2020-09-16 10:22 ` Philipp Zabel
0 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-16 10:22 UTC (permalink / raw)
To: Daniel Vetter; +Cc: kernel, dri-devel
Hi Daniel,
On Wed, 2020-09-16 at 11:08 +0200, Daniel Vetter wrote:
> On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
> > This allows to drop the custom drm_encoder_cleanup() actions.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > New in v3, example conversion of drm_simple_encoder_init() users.
> >
> > This and the following patches depend on the drm/imx conversion to use managed
> > resources [1].
> >
> > [1] https://lore.kernel.org/dri-devel/20200911133855.29801-3-p.zabel@pengutronix.de/T/#m335a28c5c26ab14bccc998d4dc0aed6850e9bc36
[...]
> Where does this apply to? Neither upstream nor linux-next seems to have
> the drmm_ conversion for imx already applied, and that's kinda the juicy
> part I'd like to look at a bit.
Oh, ok. I did (re)send the basic drmm_ conversion separately at [1],
mentioned above.
> The patches here are just mechanical conversion. Can you pls include
> the drmm_ conversion too (maybe even squash these patches here in, I
> think that would be more readable)?
I'll squash and resend as a single series.
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
` (3 preceding siblings ...)
2020-09-11 13:57 ` [PATCH v3 5/7] drm/imx: use drmm_simple_encoder_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-09-11 13:57 ` [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes() Philipp Zabel
2020-12-04 9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
6 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
This allows to drop the custom drm_plane_cleanup action.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_universal_plane_init() user.
---
drivers/gpu/drm/imx/ipuv3-plane.c | 34 ++++++++-----------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 38b959aa3564..075508051b5f 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -815,13 +815,6 @@ int ipu_planes_assign_pre(struct drm_device *dev,
}
EXPORT_SYMBOL_GPL(ipu_planes_assign_pre);
-static void ipu_plane_cleanup(struct drm_device *dev, void *data)
-{
- struct ipu_plane *ipu_plane = data;
-
- drm_plane_cleanup(&ipu_plane->base);
-}
-
struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
int dma, int dp, unsigned int possible_crtcs,
enum drm_plane_type type)
@@ -834,10 +827,15 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
DRM_DEBUG_KMS("channel %d, dp flow %d, possible_crtcs=0x%x\n",
dma, dp, possible_crtcs);
- ipu_plane = drmm_kzalloc(dev, sizeof(*ipu_plane), GFP_KERNEL);
- if (!ipu_plane) {
- DRM_ERROR("failed to allocate plane\n");
- return ERR_PTR(-ENOMEM);
+ ipu_plane = drmm_universal_plane_alloc(dev, struct ipu_plane, base,
+ possible_crtcs, &ipu_plane_funcs,
+ ipu_plane_formats,
+ ARRAY_SIZE(ipu_plane_formats),
+ modifiers, type, NULL);
+ if (IS_ERR(ipu_plane)) {
+ DRM_ERROR("failed to allocate and initialize %s plane\n",
+ zpos ? "overlay" : "primary");
+ return ipu_plane;
}
ipu_plane->ipu = ipu;
@@ -847,20 +845,6 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
if (ipu_prg_present(ipu))
modifiers = pre_format_modifiers;
- ret = drm_universal_plane_init(dev, &ipu_plane->base, possible_crtcs,
- &ipu_plane_funcs, ipu_plane_formats,
- ARRAY_SIZE(ipu_plane_formats),
- modifiers, type, NULL);
- if (ret) {
- DRM_ERROR("failed to initialize %s plane\n",
- zpos ? "overlay" : "primary");
- return ERR_PTR(ret);
- }
-
- ret = drmm_add_action_or_reset(dev, ipu_plane_cleanup, ipu_plane);
- if (ret)
- return ERR_PTR(ret);
-
drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG)
--
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] 18+ messages in thread
* [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
` (4 preceding siblings ...)
2020-09-11 13:57 ` [PATCH v3 6/7] drm/imx: use drmm_universal_plane_alloc() Philipp Zabel
@ 2020-09-11 13:57 ` Philipp Zabel
2020-12-04 9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
6 siblings, 0 replies; 18+ messages in thread
From: Philipp Zabel @ 2020-09-11 13:57 UTC (permalink / raw)
To: dri-devel; +Cc: kernel
Merge ipu_crtc_init into ipu_drm_bind and use
drmm_crtc_alloc_with_planes().
This allows to drop the custom drm_crtc_cleanup action.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
New in v3, example conversion of drm_crtc_init_with_planes() user.
---
drivers/gpu/drm/imx/ipuv3-crtc.c | 71 ++++++++++++--------------------
1 file changed, 26 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index f284e4ea6d5f..982ef79653ae 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -349,47 +349,43 @@ static int ipu_get_resources(struct drm_device *dev, struct ipu_crtc *ipu_crtc,
return 0;
}
-static void ipu_crtc_cleanup(struct drm_device *drm, void *ptr)
-{
- struct drm_crtc *crtc = ptr;
-
- drm_crtc_cleanup(crtc);
-}
-
-static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
- struct ipu_client_platformdata *pdata, struct drm_device *drm)
+static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
{
- struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
- struct drm_crtc *crtc = &ipu_crtc->base;
+ struct ipu_client_platformdata *pdata = dev->platform_data;
+ struct ipu_soc *ipu = dev_get_drvdata(dev->parent);
+ struct drm_device *drm = data;
+ struct ipu_plane *primary_plane;
+ struct ipu_crtc *ipu_crtc;
+ struct drm_crtc *crtc;
int dp = -EINVAL;
int ret;
- ret = ipu_get_resources(drm, ipu_crtc, pdata);
- if (ret) {
- dev_err(ipu_crtc->dev, "getting resources failed with %d.\n",
- ret);
- return ret;
- }
-
if (pdata->dp >= 0)
dp = IPU_DP_FLOW_SYNC_BG;
- ipu_crtc->plane[0] = ipu_plane_init(drm, ipu, pdata->dma[0], dp, 0,
- DRM_PLANE_TYPE_PRIMARY);
- if (IS_ERR(ipu_crtc->plane[0])) {
- ret = PTR_ERR(ipu_crtc->plane[0]);
- return ret;
- }
+ primary_plane = ipu_plane_init(drm, ipu, pdata->dma[0], dp, 0,
+ DRM_PLANE_TYPE_PRIMARY);
+ if (IS_ERR(primary_plane))
+ return PTR_ERR(primary_plane);
+ ipu_crtc = drmm_crtc_alloc_with_planes(drm, struct ipu_crtc, base,
+ &primary_plane->base, NULL,
+ &ipu_crtc_funcs, NULL);
+ if (IS_ERR(ipu_crtc))
+ return PTR_ERR(ipu_crtc);
+
+ ipu_crtc->dev = dev;
+ ipu_crtc->plane[0] = primary_plane;
+
+ crtc = &ipu_crtc->base;
crtc->port = pdata->of_node;
drm_crtc_helper_add(crtc, &ipu_helper_funcs);
- ret = drm_crtc_init_with_planes(drm, crtc, &ipu_crtc->plane[0]->base,
- NULL, &ipu_crtc_funcs, NULL);
- if (ret)
- return ret;
- ret = drmm_add_action_or_reset(drm, ipu_crtc_cleanup, crtc);
- if (ret)
+ ret = ipu_get_resources(drm, ipu_crtc, pdata);
+ if (ret) {
+ dev_err(ipu_crtc->dev, "getting resources failed with %d.\n",
+ ret);
return ret;
+ }
/* If this crtc is using the DP, add an overlay plane */
if (pdata->dp >= 0 && pdata->dma[1] > 0) {
@@ -414,21 +410,6 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
return 0;
}
-static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
-{
- struct ipu_client_platformdata *pdata = dev->platform_data;
- struct drm_device *drm = data;
- struct ipu_crtc *ipu_crtc;
-
- ipu_crtc = drmm_kzalloc(drm, sizeof(*ipu_crtc), GFP_KERNEL);
- if (!ipu_crtc)
- return -ENOMEM;
-
- ipu_crtc->dev = dev;
-
- return ipu_crtc_init(ipu_crtc, pdata, drm);
-}
-
static const struct component_ops ipu_crtc_ops = {
.bind = ipu_drm_bind,
};
--
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] 18+ messages in thread
* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
2020-09-11 13:57 [PATCH v3 1/7] drm: add drmm_encoder_alloc() Philipp Zabel
` (5 preceding siblings ...)
2020-09-11 13:57 ` [PATCH v3 7/7] drm/imx: ipuv3-crtc: use drmm_crtc_alloc_with_planes() Philipp Zabel
@ 2020-12-04 9:17 ` Laurent Pinchart
2020-12-04 10:12 ` Philipp Zabel
6 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-04 9:17 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> Add an alternative to drm_encoder_init() that allocates and initializes
> an encoder and registers drm_encoder_cleanup() with
> drmm_add_action_or_reset().
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v2:
> - call va_start() / va_end() unconditionally
> ---
> 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..f5414705f9ad 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -26,6 +26,7 @@
> #include <drm/drm_device.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_managed.h>
>
> #include "drm_crtc_internal.h"
>
> @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> }
> }
>
> -/**
> - * drm_encoder_init - Init a preallocated encoder
> - * @dev: drm device
> - * @encoder: the encoder to init
> - * @funcs: callbacks for this encoder
> - * @encoder_type: user visible type of the encoder
> - * @name: printf style format string for the encoder name, or NULL for default name
> - *
> - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> - * called from the driver's &drm_encoder_funcs.destroy hook.
> - *
> - * Returns:
> - * Zero on success, error code on failure.
> - */
> -int drm_encoder_init(struct drm_device *dev,
> - struct drm_encoder *encoder,
> - const struct drm_encoder_funcs *funcs,
> - int encoder_type, const char *name, ...)
> +__printf(5, 0)
> +static int __drm_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + const struct drm_encoder_funcs *funcs,
> + int encoder_type, const char *name, va_list ap)
> {
> int ret;
>
> @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> encoder->encoder_type = encoder_type;
> encoder->funcs = funcs;
> if (name) {
> - va_list ap;
> -
> - va_start(ap, name);
> encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> - va_end(ap);
> } else {
> encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> drm_encoder_enum_list[encoder_type].name,
> @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
>
> return ret;
> }
> +
> +/**
> + * drm_encoder_init - Init a preallocated encoder
> + * @dev: drm device
> + * @encoder: the encoder to init
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for default name
> + *
> + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> + * called from the driver's &drm_encoder_funcs.destroy hook.
> + *
> + * 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;
> +
> + va_start(ap, name);
> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> EXPORT_SYMBOL(drm_encoder_init);
>
> /**
> @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> }
> EXPORT_SYMBOL(drm_encoder_cleanup);
>
> +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> +{
> + struct drm_encoder *encoder = ptr;
> +
> + if (WARN_ON(!encoder->dev))
> + return;
> +
> + drm_encoder_cleanup(encoder);
> +}
> +
> +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> + const struct drm_encoder_funcs *funcs,
> + int encoder_type, const char *name, ...)
> +{
> + void *container;
> + struct drm_encoder *encoder;
> + va_list ap;
> + int ret;
> +
> + if (WARN_ON(!funcs || funcs->destroy))
> + return ERR_PTR(-EINVAL);
> +
> + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> + if (!container)
> + return ERR_PTR(-EINVAL);
> +
> + encoder = container + offset;
> +
> + va_start(ap, name);
> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> + va_end(ap);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> + if (ret)
> + return ERR_PTR(ret);
Given that drmm_encoder_alloc_release() will be called right before the
kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
to replace drmm_kzalloc() with kzalloc() and add a kfree() in
drmm_encoder_alloc_release() ? That will save one context allocation.
With this addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> + return container;
> +}
> +EXPORT_SYMBOL(__drmm_encoder_alloc);
> +
> static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
> {
> struct drm_connector *connector;
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index a60f5f1555ac..4ecad1260ff7 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_alloc(struct drm_device *dev,
> + size_t size, size_t offset,
> + const struct drm_encoder_funcs *funcs,
> + int encoder_type,
> + const char *name, ...);
> +
> +/**
> + * drmm_encoder_alloc - Allocate and initialize an encoder
> + * @dev: drm device
> + * @type: the type of the struct which contains struct &drm_encoder
> + * @member: the name of the &drm_encoder within @type.
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for default name
> + *
> + * Allocates and initializes an encoder. Encoder should be subclassed as part of
> + * driver encoder objects. Cleanup is automatically handled through registering
> + * drm_encoder_cleanup() with drmm_add_action().
> + *
> + * The @drm_encoder_funcs.destroy hook must be NULL.
> + *
> + * Returns:
> + * Pointer to new encoder, or ERR_PTR on failure.
> + */
> +#define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \
> + ((type *)__drmm_encoder_alloc(dev, sizeof(type), \
> + 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
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
2020-12-04 9:17 ` [PATCH v3 1/7] drm: add drmm_encoder_alloc() Laurent Pinchart
@ 2020-12-04 10:12 ` Philipp Zabel
2020-12-05 18:57 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Philipp Zabel @ 2020-12-04 10:12 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: kernel, dri-devel
Hi Laurent,
On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> Hi Philipp,
>
> Thank you for the patch.
Thank you for the review.
> On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > Add an alternative to drm_encoder_init() that allocates and initializes
> > an encoder and registers drm_encoder_cleanup() with
> > drmm_add_action_or_reset().
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v2:
> > - call va_start() / va_end() unconditionally
> > ---
> > 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..f5414705f9ad 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -26,6 +26,7 @@
> > #include <drm/drm_device.h>
> > #include <drm/drm_drv.h>
> > #include <drm/drm_encoder.h>
> > +#include <drm/drm_managed.h>
> >
> > #include "drm_crtc_internal.h"
> >
> > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> > }
> > }
> >
> > -/**
> > - * drm_encoder_init - Init a preallocated encoder
> > - * @dev: drm device
> > - * @encoder: the encoder to init
> > - * @funcs: callbacks for this encoder
> > - * @encoder_type: user visible type of the encoder
> > - * @name: printf style format string for the encoder name, or NULL for default name
> > - *
> > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > - *
> > - * Returns:
> > - * Zero on success, error code on failure.
> > - */
> > -int drm_encoder_init(struct drm_device *dev,
> > - struct drm_encoder *encoder,
> > - const struct drm_encoder_funcs *funcs,
> > - int encoder_type, const char *name, ...)
> > +__printf(5, 0)
> > +static int __drm_encoder_init(struct drm_device *dev,
> > + struct drm_encoder *encoder,
> > + const struct drm_encoder_funcs *funcs,
> > + int encoder_type, const char *name, va_list ap)
> > {
> > int ret;
> >
> > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> > encoder->encoder_type = encoder_type;
> > encoder->funcs = funcs;
> > if (name) {
> > - va_list ap;
> > -
> > - va_start(ap, name);
> > encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > - va_end(ap);
> > } else {
> > encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > drm_encoder_enum_list[encoder_type].name,
> > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> >
> > return ret;
> > }
> > +
> > +/**
> > + * drm_encoder_init - Init a preallocated encoder
> > + * @dev: drm device
> > + * @encoder: the encoder to init
> > + * @funcs: callbacks for this encoder
> > + * @encoder_type: user visible type of the encoder
> > + * @name: printf style format string for the encoder name, or NULL for default name
> > + *
> > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > + *
> > + * 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;
> > +
> > + va_start(ap, name);
> > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > + va_end(ap);
> > +
> > + return ret;
> > +}
> > EXPORT_SYMBOL(drm_encoder_init);
> >
> > /**
> > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > }
> > EXPORT_SYMBOL(drm_encoder_cleanup);
> >
> > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > +{
> > + struct drm_encoder *encoder = ptr;
> > +
> > + if (WARN_ON(!encoder->dev))
> > + return;
> > +
> > + drm_encoder_cleanup(encoder);
> > +}
> > +
> > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > + const struct drm_encoder_funcs *funcs,
> > + int encoder_type, const char *name, ...)
> > +{
> > + void *container;
> > + struct drm_encoder *encoder;
> > + va_list ap;
> > + int ret;
> > +
> > + if (WARN_ON(!funcs || funcs->destroy))
> > + return ERR_PTR(-EINVAL);
> > +
> > + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > + if (!container)
> > + return ERR_PTR(-EINVAL);
> > +
> > + encoder = container + offset;
> > +
> > + va_start(ap, name);
> > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > + va_end(ap);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > + if (ret)
> > + return ERR_PTR(ret);
>
> Given that drmm_encoder_alloc_release() will be called right before the
> kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> drmm_encoder_alloc_release() ? That will save one context allocation.
That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
the container pointer that must be passed to kfree(), nor the offset
between container and encoder.
> With this addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
2020-12-04 10:12 ` Philipp Zabel
@ 2020-12-05 18:57 ` Laurent Pinchart
2020-12-09 0:22 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-12-05 18:57 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kernel, dri-devel
Hi Philipp,
On Fri, Dec 04, 2020 at 11:12:20AM +0100, Philipp Zabel wrote:
> On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> > On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > > Add an alternative to drm_encoder_init() that allocates and initializes
> > > an encoder and registers drm_encoder_cleanup() with
> > > drmm_add_action_or_reset().
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > Changes since v2:
> > > - call va_start() / va_end() unconditionally
> > > ---
> > > 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..f5414705f9ad 100644
> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -26,6 +26,7 @@
> > > #include <drm/drm_device.h>
> > > #include <drm/drm_drv.h>
> > > #include <drm/drm_encoder.h>
> > > +#include <drm/drm_managed.h>
> > >
> > > #include "drm_crtc_internal.h"
> > >
> > > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> > > }
> > > }
> > >
> > > -/**
> > > - * drm_encoder_init - Init a preallocated encoder
> > > - * @dev: drm device
> > > - * @encoder: the encoder to init
> > > - * @funcs: callbacks for this encoder
> > > - * @encoder_type: user visible type of the encoder
> > > - * @name: printf style format string for the encoder name, or NULL for default name
> > > - *
> > > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > > - *
> > > - * Returns:
> > > - * Zero on success, error code on failure.
> > > - */
> > > -int drm_encoder_init(struct drm_device *dev,
> > > - struct drm_encoder *encoder,
> > > - const struct drm_encoder_funcs *funcs,
> > > - int encoder_type, const char *name, ...)
> > > +__printf(5, 0)
> > > +static int __drm_encoder_init(struct drm_device *dev,
> > > + struct drm_encoder *encoder,
> > > + const struct drm_encoder_funcs *funcs,
> > > + int encoder_type, const char *name, va_list ap)
> > > {
> > > int ret;
> > >
> > > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> > > encoder->encoder_type = encoder_type;
> > > encoder->funcs = funcs;
> > > if (name) {
> > > - va_list ap;
> > > -
> > > - va_start(ap, name);
> > > encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > > - va_end(ap);
> > > } else {
> > > encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > > drm_encoder_enum_list[encoder_type].name,
> > > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> > >
> > > return ret;
> > > }
> > > +
> > > +/**
> > > + * drm_encoder_init - Init a preallocated encoder
> > > + * @dev: drm device
> > > + * @encoder: the encoder to init
> > > + * @funcs: callbacks for this encoder
> > > + * @encoder_type: user visible type of the encoder
> > > + * @name: printf style format string for the encoder name, or NULL for default name
> > > + *
> > > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > > + *
> > > + * 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;
> > > +
> > > + va_start(ap, name);
> > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > + va_end(ap);
> > > +
> > > + return ret;
> > > +}
> > > EXPORT_SYMBOL(drm_encoder_init);
> > >
> > > /**
> > > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > > }
> > > EXPORT_SYMBOL(drm_encoder_cleanup);
> > >
> > > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > > +{
> > > + struct drm_encoder *encoder = ptr;
> > > +
> > > + if (WARN_ON(!encoder->dev))
> > > + return;
> > > +
> > > + drm_encoder_cleanup(encoder);
> > > +}
> > > +
> > > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > > + const struct drm_encoder_funcs *funcs,
> > > + int encoder_type, const char *name, ...)
> > > +{
> > > + void *container;
> > > + struct drm_encoder *encoder;
> > > + va_list ap;
> > > + int ret;
> > > +
> > > + if (WARN_ON(!funcs || funcs->destroy))
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > > + if (!container)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + encoder = container + offset;
> > > +
> > > + va_start(ap, name);
> > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > + va_end(ap);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> >
> > Given that drmm_encoder_alloc_release() will be called right before the
> > kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> > to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> > drmm_encoder_alloc_release() ? That will save one context allocation.
>
> That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
> the container pointer that must be passed to kfree(), nor the offset
> between container and encoder.
Indeed :-S Adding more context to the drmres when creating it with
drmm_add_action_or_reset() would solve this, but it's probably overkill
(although I think this would definitely be useful if/when we turn the
DRM resource manager to a more generic component usable by other
subsystems).
> > With this addressed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
You can add my tag to this patch and the other ones in the series where
I've made the same comment.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/7] drm: add drmm_encoder_alloc()
2020-12-05 18:57 ` Laurent Pinchart
@ 2020-12-09 0:22 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2020-12-09 0:22 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel, kernel
On Sat, Dec 05, 2020 at 08:57:38PM +0200, Laurent Pinchart wrote:
> Hi Philipp,
>
> On Fri, Dec 04, 2020 at 11:12:20AM +0100, Philipp Zabel wrote:
> > On Fri, 2020-12-04 at 11:17 +0200, Laurent Pinchart wrote:
> > > On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
> > > > Add an alternative to drm_encoder_init() that allocates and initializes
> > > > an encoder and registers drm_encoder_cleanup() with
> > > > drmm_add_action_or_reset().
> > > >
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > > Changes since v2:
> > > > - call va_start() / va_end() unconditionally
> > > > ---
> > > > 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..f5414705f9ad 100644
> > > > --- a/drivers/gpu/drm/drm_encoder.c
> > > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > > @@ -26,6 +26,7 @@
> > > > #include <drm/drm_device.h>
> > > > #include <drm/drm_drv.h>
> > > > #include <drm/drm_encoder.h>
> > > > +#include <drm/drm_managed.h>
> > > >
> > > > #include "drm_crtc_internal.h"
> > > >
> > > > @@ -91,25 +92,11 @@ void drm_encoder_unregister_all(struct drm_device *dev)
> > > > }
> > > > }
> > > >
> > > > -/**
> > > > - * drm_encoder_init - Init a preallocated encoder
> > > > - * @dev: drm device
> > > > - * @encoder: the encoder to init
> > > > - * @funcs: callbacks for this encoder
> > > > - * @encoder_type: user visible type of the encoder
> > > > - * @name: printf style format string for the encoder name, or NULL for default name
> > > > - *
> > > > - * Initialises a preallocated encoder. Encoder should be subclassed as part of
> > > > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > > - * called from the driver's &drm_encoder_funcs.destroy hook.
> > > > - *
> > > > - * Returns:
> > > > - * Zero on success, error code on failure.
> > > > - */
> > > > -int drm_encoder_init(struct drm_device *dev,
> > > > - struct drm_encoder *encoder,
> > > > - const struct drm_encoder_funcs *funcs,
> > > > - int encoder_type, const char *name, ...)
> > > > +__printf(5, 0)
> > > > +static int __drm_encoder_init(struct drm_device *dev,
> > > > + struct drm_encoder *encoder,
> > > > + const struct drm_encoder_funcs *funcs,
> > > > + int encoder_type, const char *name, va_list ap)
> > > > {
> > > > int ret;
> > > >
> > > > @@ -125,11 +112,7 @@ int drm_encoder_init(struct drm_device *dev,
> > > > encoder->encoder_type = encoder_type;
> > > > encoder->funcs = funcs;
> > > > if (name) {
> > > > - va_list ap;
> > > > -
> > > > - va_start(ap, name);
> > > > encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> > > > - va_end(ap);
> > > > } else {
> > > > encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> > > > drm_encoder_enum_list[encoder_type].name,
> > > > @@ -150,6 +133,36 @@ int drm_encoder_init(struct drm_device *dev,
> > > >
> > > > return ret;
> > > > }
> > > > +
> > > > +/**
> > > > + * drm_encoder_init - Init a preallocated encoder
> > > > + * @dev: drm device
> > > > + * @encoder: the encoder to init
> > > > + * @funcs: callbacks for this encoder
> > > > + * @encoder_type: user visible type of the encoder
> > > > + * @name: printf style format string for the encoder name, or NULL for default name
> > > > + *
> > > > + * Initializes a preallocated encoder. Encoder should be subclassed as part of
> > > > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
> > > > + * called from the driver's &drm_encoder_funcs.destroy hook.
> > > > + *
> > > > + * 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;
> > > > +
> > > > + va_start(ap, name);
> > > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > > + va_end(ap);
> > > > +
> > > > + return ret;
> > > > +}
> > > > EXPORT_SYMBOL(drm_encoder_init);
> > > >
> > > > /**
> > > > @@ -181,6 +194,48 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> > > > }
> > > > EXPORT_SYMBOL(drm_encoder_cleanup);
> > > >
> > > > +static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
> > > > +{
> > > > + struct drm_encoder *encoder = ptr;
> > > > +
> > > > + if (WARN_ON(!encoder->dev))
> > > > + return;
> > > > +
> > > > + drm_encoder_cleanup(encoder);
> > > > +}
> > > > +
> > > > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
> > > > + const struct drm_encoder_funcs *funcs,
> > > > + int encoder_type, const char *name, ...)
> > > > +{
> > > > + void *container;
> > > > + struct drm_encoder *encoder;
> > > > + va_list ap;
> > > > + int ret;
> > > > +
> > > > + if (WARN_ON(!funcs || funcs->destroy))
> > > > + return ERR_PTR(-EINVAL);
> > > > +
> > > > + container = drmm_kzalloc(dev, size, GFP_KERNEL);
> > > > + if (!container)
> > > > + return ERR_PTR(-EINVAL);
> > > > +
> > > > + encoder = container + offset;
> > > > +
> > > > + va_start(ap, name);
> > > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
> > > > + va_end(ap);
> > > > + if (ret)
> > > > + return ERR_PTR(ret);
> > > > +
> > > > + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder);
> > > > + if (ret)
> > > > + return ERR_PTR(ret);
> > >
> > > Given that drmm_encoder_alloc_release() will be called right before the
> > > kfree related to the above drmm_kzalloc(), wouldn't it be more efficient
> > > to replace drmm_kzalloc() with kzalloc() and add a kfree() in
> > > drmm_encoder_alloc_release() ? That will save one context allocation.
> >
> > That is not quite as trivial: drmm_encoder_alloc_release() doesn't know
> > the container pointer that must be passed to kfree(), nor the offset
> > between container and encoder.
>
> Indeed :-S Adding more context to the drmres when creating it with
> drmm_add_action_or_reset() would solve this, but it's probably overkill
> (although I think this would definitely be useful if/when we turn the
> DRM resource manager to a more generic component usable by other
> subsystems).
Internally it's even funnier, because the drmm_kzalloc doesn't even have a
callback - the allocation is freed as part of the drmm tracking structure.
Which means that drmm_kzalloc has a "free" release callback already.
This irked me to no end, but I couldn't come up with an interface to stuff
the release callback into the allocation which I deemed safe enough for
drmm, which is supposed to make life easy and bugs disappear, not greate
new surprises.
What I just realized is that drmm_add_action_or_reset could look at the
drmm cleanup action stack, and if the topmost one doesn't have a hook set
already, use that one instead of allocating a new tracker structure. This
way ordering is always guaranteed (e.g. if you mix up drmm_kzalloc calls
with add_action for different objects in strange orders) and there's no
problem ever, we just avoid a few small allocations.
Still needs a few special cases, so not sure it's actually a win.
-Daniel
>
> > > With this addressed,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> You can add my tag to this patch and the other ones in the series where
> I've made the same comment.
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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] 18+ messages in thread