| Message ID | 20200911135724.25833-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/7] drm: add drmm_encoder_alloc() | expand |
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
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
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.
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
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_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(-)