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); + + 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
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 */
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote:
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 ?
Hi Laurent,
On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote:
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
Hi Philipp,
On Fri, Dec 04, 2020 at 11:13:33AM +0100, Philipp Zabel wrote:
That's perfect.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:20PM +0200, Philipp Zabel wrote:
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
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
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:21PM +0200, Philipp Zabel wrote:
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
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... --- 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;
On Fri, Sep 11, 2020 at 03:57:22PM +0200, Philipp Zabel wrote:
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
Hi Daniel,
On Wed, 2020-09-16 at 11:08 +0200, Daniel Vetter wrote:
[...]
Oh, ok. I did (re)send the basic drmm_ conversion separately at [1], mentioned above.
I'll squash and resend as a single series.
regards Philipp
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)
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, };
Hi Philipp,
Thank you for the patch.
On Fri, Sep 11, 2020 at 03:57:18PM +0200, Philipp Zabel wrote:
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
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.
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
Hi Philipp,
On Fri, Dec 04, 2020 at 11:12:20AM +0100, Philipp Zabel wrote:
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.
On Sat, Dec 05, 2020 at 08:57:38PM +0200, Laurent Pinchart wrote:
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Philipp Zabel