| Message ID | 20200723145614.18459-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm: add drmm_encoder_init() | expand |
On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote: > Add a drm_encoder_init() variant that allocates an encoder with > drmm_kzalloc() and registers drm_encoder_cleanup() with > drmm_add_action_or_reset(). > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > New in v2 > --- > drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- > include/drm/drm_encoder.h | 30 ++++++++++ > 2 files changed, 108 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > index e555281f43d4..91184f67333c 100644 > --- a/drivers/gpu/drm/drm_encoder.c > +++ b/drivers/gpu/drm/drm_encoder.c > @@ -26,6 +26,7 @@ > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > #include <drm/drm_encoder.h> > +#include <drm/drm_managed.h> > > #include "drm_crtc_internal.h" > > @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) > } > } > > -/** > - * drm_encoder_init - Init a preallocated encoder > - * @dev: drm device > - * @encoder: the encoder to init > - * @funcs: callbacks for this encoder > - * @encoder_type: user visible type of the encoder > - * @name: printf style format string for the encoder name, or NULL for default name > - * > - * Initialises a preallocated encoder. Encoder should be subclassed as part of > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > - * called from the driver's &drm_encoder_funcs.destroy hook. > - * > - * Returns: > - * Zero on success, error code on failure. > - */ > -int drm_encoder_init(struct drm_device *dev, > - struct drm_encoder *encoder, > - const struct drm_encoder_funcs *funcs, > - int encoder_type, const char *name, ...) > +static int __drm_encoder_init(struct drm_device *dev, > + struct drm_encoder *encoder, > + const struct drm_encoder_funcs *funcs, > + int encoder_type, const char *name, va_list ap) > { > int ret; > > @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, > encoder->encoder_type = encoder_type; > encoder->funcs = funcs; > if (name) { > - va_list ap; > - > - va_start(ap, name); > encoder->name = kvasprintf(GFP_KERNEL, name, ap); > - va_end(ap); > } else { > encoder->name = kasprintf(GFP_KERNEL, "%s-%d", > drm_encoder_enum_list[encoder_type].name, > @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, > > return ret; > } > + > +/** > + * drm_encoder_init - Init a preallocated encoder > + * @dev: drm device > + * @encoder: the encoder to init > + * @funcs: callbacks for this encoder > + * @encoder_type: user visible type of the encoder > + * @name: printf style format string for the encoder name, or NULL for default name > + * > + * Initialises a preallocated encoder. Encoder should be subclassed as part of > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > + * called from the driver's &drm_encoder_funcs.destroy hook. > + * > + * Returns: > + * Zero on success, error code on failure. > + */ > +int drm_encoder_init(struct drm_device *dev, > + struct drm_encoder *encoder, > + const struct drm_encoder_funcs *funcs, > + int encoder_type, const char *name, ...) > +{ > + va_list ap; > + int ret; > + > + if (name) > + va_start(ap, name); > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > + if (name) > + va_end(ap); > + > + return ret; > +} > EXPORT_SYMBOL(drm_encoder_init); > > /** > @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > } > EXPORT_SYMBOL(drm_encoder_cleanup); > > +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) > +{ > + struct drm_encoder *encoder = ptr; I'll add if (WARN_ON(!encoder->dev)) return; here. > + drm_encoder_cleanup(encoder); > +} > + > +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, > + const struct drm_encoder_funcs *funcs, > + int encoder_type, const char *name, ...) > +{ > + void *container; > + struct drm_encoder *encoder; > + va_list ap; > + int ret; > + > + if (WARN_ON(!funcs || funcs->destroy)) > + return ERR_PTR(-EINVAL); > + > + container = drmm_kzalloc(dev, size, GFP_KERNEL); > + if (!container) > + return ERR_PTR(-EINVAL); > + > + encoder = container + offset; > + > + if (name) > + va_start(ap, name); > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > + if (name) > + va_end(ap); > + if (ret) > + return ERR_PTR(ret); > + > + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); > + if (ret) > + return ERR_PTR(ret); > + > + return container; > +} > +EXPORT_SYMBOL(__drmm_encoder_init); > + > static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) > { > struct drm_connector *connector; > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > index a60f5f1555ac..54b82554ee88 100644 > --- a/include/drm/drm_encoder.h > +++ b/include/drm/drm_encoder.h > @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, > const struct drm_encoder_funcs *funcs, > int encoder_type, const char *name, ...); > > +__printf(6, 7) > +void *__drmm_encoder_init(struct drm_device *dev, > + size_t size, size_t offset, > + const struct drm_encoder_funcs *funcs, > + int encoder_type, > + const char *name, ...); > + > +/** > + * drmm_encoder_init - Allocate and initialize an encoder > + * @dev: drm device > + * @type: the type of the struct which contains struct &drm_encoder > + * @member: the name of the &drm_encoder within @type. > + * @funcs: callbacks for this encoder > + * @encoder_type: user visible type of the encoder > + * @name: printf style format string for the encoder name, or NULL for default name > + * > + * Allocates and initializes an encoder. Encoder should be subclassed as part of > + * driver encoder objects. Cleanup is automatically handled through registering > + * drm_encoder_cleanup() with drmm_add_action(). > + * > + * The @drm_encoder_funcs.destroy hook must be NULL. > + * > + * Returns: > + * Pointer to new encoder, or ERR_PTR on failure. > + */ > +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ > + ((type *) __drmm_encoder_init(dev, sizeof(type), \ > + offsetof(type, member), funcs, \ > + encoder_type, name, ##__VA_ARGS__)) > + Should this be called drmm_encoder_alloc instead? regards Philipp
On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote: > > Add a drm_encoder_init() variant that allocates an encoder with > > drmm_kzalloc() and registers drm_encoder_cleanup() with > > drmm_add_action_or_reset(). > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Thanks for doing this! > > --- > > New in v2 > > --- > > drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- > > include/drm/drm_encoder.h | 30 ++++++++++ > > 2 files changed, 108 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > > index e555281f43d4..91184f67333c 100644 > > --- a/drivers/gpu/drm/drm_encoder.c > > +++ b/drivers/gpu/drm/drm_encoder.c > > @@ -26,6 +26,7 @@ > > #include <drm/drm_device.h> > > #include <drm/drm_drv.h> > > #include <drm/drm_encoder.h> > > +#include <drm/drm_managed.h> > > > > #include "drm_crtc_internal.h" > > > > @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) > > } > > } > > > > -/** > > - * drm_encoder_init - Init a preallocated encoder > > - * @dev: drm device > > - * @encoder: the encoder to init > > - * @funcs: callbacks for this encoder > > - * @encoder_type: user visible type of the encoder > > - * @name: printf style format string for the encoder name, or NULL for default name > > - * > > - * Initialises a preallocated encoder. Encoder should be subclassed as part of > > - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > > - * called from the driver's &drm_encoder_funcs.destroy hook. > > - * > > - * Returns: > > - * Zero on success, error code on failure. > > - */ > > -int drm_encoder_init(struct drm_device *dev, > > - struct drm_encoder *encoder, > > - const struct drm_encoder_funcs *funcs, > > - int encoder_type, const char *name, ...) > > +static int __drm_encoder_init(struct drm_device *dev, > > + struct drm_encoder *encoder, > > + const struct drm_encoder_funcs *funcs, > > + int encoder_type, const char *name, va_list ap) > > { > > int ret; > > > > @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, > > encoder->encoder_type = encoder_type; > > encoder->funcs = funcs; > > if (name) { > > - va_list ap; > > - > > - va_start(ap, name); > > encoder->name = kvasprintf(GFP_KERNEL, name, ap); > > - va_end(ap); > > } else { > > encoder->name = kasprintf(GFP_KERNEL, "%s-%d", > > drm_encoder_enum_list[encoder_type].name, > > @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, > > > > return ret; > > } > > + > > +/** > > + * drm_encoder_init - Init a preallocated encoder > > + * @dev: drm device > > + * @encoder: the encoder to init > > + * @funcs: callbacks for this encoder > > + * @encoder_type: user visible type of the encoder > > + * @name: printf style format string for the encoder name, or NULL for default name > > + * > > + * Initialises a preallocated encoder. Encoder should be subclassed as part of > > + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > > + * called from the driver's &drm_encoder_funcs.destroy hook. > > + * > > + * Returns: > > + * Zero on success, error code on failure. > > + */ > > +int drm_encoder_init(struct drm_device *dev, > > + struct drm_encoder *encoder, > > + const struct drm_encoder_funcs *funcs, > > + int encoder_type, const char *name, ...) > > +{ > > + va_list ap; > > + int ret; > > + > > + if (name) > > + va_start(ap, name); > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > > + if (name) > > + va_end(ap); > > + > > + return ret; > > +} > > EXPORT_SYMBOL(drm_encoder_init); > > > > /** > > @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > > } > > EXPORT_SYMBOL(drm_encoder_cleanup); > > > > +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) > > +{ > > + struct drm_encoder *encoder = ptr; > > I'll add > > if (WARN_ON(!encoder->dev)) > return; > > here. > > > + drm_encoder_cleanup(encoder); > > +} > > + > > +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, > > + const struct drm_encoder_funcs *funcs, > > + int encoder_type, const char *name, ...) > > +{ > > + void *container; > > + struct drm_encoder *encoder; > > + va_list ap; > > + int ret; > > + > > + if (WARN_ON(!funcs || funcs->destroy)) > > + return ERR_PTR(-EINVAL); > > + > > + container = drmm_kzalloc(dev, size, GFP_KERNEL); > > + if (!container) > > + return ERR_PTR(-EINVAL); > > + > > + encoder = container + offset; > > + > > + if (name) > > + va_start(ap, name); > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > > + if (name) > > + va_end(ap); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return container; > > +} > > +EXPORT_SYMBOL(__drmm_encoder_init); > > + > > static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) > > { > > struct drm_connector *connector; > > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > > index a60f5f1555ac..54b82554ee88 100644 > > --- a/include/drm/drm_encoder.h > > +++ b/include/drm/drm_encoder.h > > @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, > > const struct drm_encoder_funcs *funcs, > > int encoder_type, const char *name, ...); > > > > +__printf(6, 7) > > +void *__drmm_encoder_init(struct drm_device *dev, > > + size_t size, size_t offset, > > + const struct drm_encoder_funcs *funcs, > > + int encoder_type, > > + const char *name, ...); > > + > > +/** > > + * drmm_encoder_init - Allocate and initialize an encoder > > + * @dev: drm device > > + * @type: the type of the struct which contains struct &drm_encoder > > + * @member: the name of the &drm_encoder within @type. > > + * @funcs: callbacks for this encoder > > + * @encoder_type: user visible type of the encoder > > + * @name: printf style format string for the encoder name, or NULL for default name > > + * > > + * Allocates and initializes an encoder. Encoder should be subclassed as part of > > + * driver encoder objects. Cleanup is automatically handled through registering > > + * drm_encoder_cleanup() with drmm_add_action(). > > + * > > + * The @drm_encoder_funcs.destroy hook must be NULL. > > + * > > + * Returns: > > + * Pointer to new encoder, or ERR_PTR on failure. > > + */ > > +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ > > + ((type *) __drmm_encoder_init(dev, sizeof(type), \ > > + offsetof(type, member), funcs, \ > > + encoder_type, name, ##__VA_ARGS__)) > > + > > Should this be called drmm_encoder_alloc instead? Yes. Same for the internal helper for consistency. Also see my other reply, if the _alloc() variant is all that's needed that makes me happy, since then we don't need to code up the drmm_assert_managed check for the _init() variants to make sure drivers dont give it something stupid like a devm_kzalloc range :-) -Daniel
Hi Am 24.07.20 um 11:07 schrieb Daniel Vetter: > On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: >> >> On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote: >>> Add a drm_encoder_init() variant that allocates an encoder with >>> drmm_kzalloc() and registers drm_encoder_cleanup() with >>> drmm_add_action_or_reset(). >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > Thanks for doing this! > >>> --- >>> New in v2 >>> --- >>> drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- >>> include/drm/drm_encoder.h | 30 ++++++++++ >>> 2 files changed, 108 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c >>> index e555281f43d4..91184f67333c 100644 >>> --- a/drivers/gpu/drm/drm_encoder.c >>> +++ b/drivers/gpu/drm/drm_encoder.c >>> @@ -26,6 +26,7 @@ >>> #include <drm/drm_device.h> >>> #include <drm/drm_drv.h> >>> #include <drm/drm_encoder.h> >>> +#include <drm/drm_managed.h> >>> >>> #include "drm_crtc_internal.h" >>> >>> @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) >>> } >>> } >>> >>> -/** >>> - * drm_encoder_init - Init a preallocated encoder >>> - * @dev: drm device >>> - * @encoder: the encoder to init >>> - * @funcs: callbacks for this encoder >>> - * @encoder_type: user visible type of the encoder >>> - * @name: printf style format string for the encoder name, or NULL for default name >>> - * >>> - * Initialises a preallocated encoder. Encoder should be subclassed as part of >>> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be >>> - * called from the driver's &drm_encoder_funcs.destroy hook. >>> - * >>> - * Returns: >>> - * Zero on success, error code on failure. >>> - */ >>> -int drm_encoder_init(struct drm_device *dev, >>> - struct drm_encoder *encoder, >>> - const struct drm_encoder_funcs *funcs, >>> - int encoder_type, const char *name, ...) >>> +static int __drm_encoder_init(struct drm_device *dev, >>> + struct drm_encoder *encoder, >>> + const struct drm_encoder_funcs *funcs, >>> + int encoder_type, const char *name, va_list ap) >>> { >>> int ret; >>> >>> @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, >>> encoder->encoder_type = encoder_type; >>> encoder->funcs = funcs; >>> if (name) { >>> - va_list ap; >>> - >>> - va_start(ap, name); >>> encoder->name = kvasprintf(GFP_KERNEL, name, ap); >>> - va_end(ap); >>> } else { >>> encoder->name = kasprintf(GFP_KERNEL, "%s-%d", >>> drm_encoder_enum_list[encoder_type].name, >>> @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, >>> >>> return ret; >>> } >>> + >>> +/** >>> + * drm_encoder_init - Init a preallocated encoder >>> + * @dev: drm device >>> + * @encoder: the encoder to init >>> + * @funcs: callbacks for this encoder >>> + * @encoder_type: user visible type of the encoder >>> + * @name: printf style format string for the encoder name, or NULL for default name >>> + * >>> + * Initialises a preallocated encoder. Encoder should be subclassed as part of >>> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be >>> + * called from the driver's &drm_encoder_funcs.destroy hook. >>> + * >>> + * Returns: >>> + * Zero on success, error code on failure. >>> + */ >>> +int drm_encoder_init(struct drm_device *dev, >>> + struct drm_encoder *encoder, >>> + const struct drm_encoder_funcs *funcs, >>> + int encoder_type, const char *name, ...) >>> +{ >>> + va_list ap; >>> + int ret; >>> + >>> + if (name) >>> + va_start(ap, name); >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); >>> + if (name) >>> + va_end(ap); >>> + >>> + return ret; >>> +} >>> EXPORT_SYMBOL(drm_encoder_init); >>> >>> /** >>> @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) >>> } >>> EXPORT_SYMBOL(drm_encoder_cleanup); >>> >>> +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) >>> +{ >>> + struct drm_encoder *encoder = ptr; >> >> I'll add >> >> if (WARN_ON(!encoder->dev)) >> return; >> >> here. >> >>> + drm_encoder_cleanup(encoder); >>> +} >>> + >>> +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, >>> + const struct drm_encoder_funcs *funcs, >>> + int encoder_type, const char *name, ...) >>> +{ >>> + void *container; >>> + struct drm_encoder *encoder; >>> + va_list ap; >>> + int ret; >>> + >>> + if (WARN_ON(!funcs || funcs->destroy)) >>> + return ERR_PTR(-EINVAL); >>> + >>> + container = drmm_kzalloc(dev, size, GFP_KERNEL); >>> + if (!container) >>> + return ERR_PTR(-EINVAL); >>> + >>> + encoder = container + offset; >>> + >>> + if (name) >>> + va_start(ap, name); >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); >>> + if (name) >>> + va_end(ap); >>> + if (ret) >>> + return ERR_PTR(ret); >>> + >>> + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); >>> + if (ret) >>> + return ERR_PTR(ret); >>> + >>> + return container; >>> +} >>> +EXPORT_SYMBOL(__drmm_encoder_init); >>> + >>> static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) >>> { >>> struct drm_connector *connector; >>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h >>> index a60f5f1555ac..54b82554ee88 100644 >>> --- a/include/drm/drm_encoder.h >>> +++ b/include/drm/drm_encoder.h >>> @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, >>> const struct drm_encoder_funcs *funcs, >>> int encoder_type, const char *name, ...); >>> >>> +__printf(6, 7) >>> +void *__drmm_encoder_init(struct drm_device *dev, >>> + size_t size, size_t offset, >>> + const struct drm_encoder_funcs *funcs, >>> + int encoder_type, >>> + const char *name, ...); >>> + >>> +/** >>> + * drmm_encoder_init - Allocate and initialize an encoder >>> + * @dev: drm device >>> + * @type: the type of the struct which contains struct &drm_encoder >>> + * @member: the name of the &drm_encoder within @type. >>> + * @funcs: callbacks for this encoder >>> + * @encoder_type: user visible type of the encoder >>> + * @name: printf style format string for the encoder name, or NULL for default name >>> + * >>> + * Allocates and initializes an encoder. Encoder should be subclassed as part of >>> + * driver encoder objects. Cleanup is automatically handled through registering >>> + * drm_encoder_cleanup() with drmm_add_action(). >>> + * >>> + * The @drm_encoder_funcs.destroy hook must be NULL. >>> + * >>> + * Returns: >>> + * Pointer to new encoder, or ERR_PTR on failure. >>> + */ >>> +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ >>> + ((type *) __drmm_encoder_init(dev, sizeof(type), \ >>> + offsetof(type, member), funcs, \ >>> + encoder_type, name, ##__VA_ARGS__)) >>> + >> >> Should this be called drmm_encoder_alloc instead? > > Yes. Same for the internal helper for consistency. Also see my other I'd suggest calling it drmm_encoder_create() to express that it's _alloc + _init. Calling it just _alloc sounds as if it doesn't do the _init. Best regards Thomas > reply, if the _alloc() variant is all that's needed that makes me > happy, since then we don't need to code up the drmm_assert_managed > check for the _init() variants to make sure drivers dont give it > something stupid like a devm_kzalloc range :-) > -Daniel >
On Fri, Jul 24, 2020 at 11:33 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 24.07.20 um 11:07 schrieb Daniel Vetter: > > On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> > >> On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote: > >>> Add a drm_encoder_init() variant that allocates an encoder with > >>> drmm_kzalloc() and registers drm_encoder_cleanup() with > >>> drmm_add_action_or_reset(). > >>> > >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > Thanks for doing this! > > > >>> --- > >>> New in v2 > >>> --- > >>> drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- > >>> include/drm/drm_encoder.h | 30 ++++++++++ > >>> 2 files changed, 108 insertions(+), 23 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > >>> index e555281f43d4..91184f67333c 100644 > >>> --- a/drivers/gpu/drm/drm_encoder.c > >>> +++ b/drivers/gpu/drm/drm_encoder.c > >>> @@ -26,6 +26,7 @@ > >>> #include <drm/drm_device.h> > >>> #include <drm/drm_drv.h> > >>> #include <drm/drm_encoder.h> > >>> +#include <drm/drm_managed.h> > >>> > >>> #include "drm_crtc_internal.h" > >>> > >>> @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) > >>> } > >>> } > >>> > >>> -/** > >>> - * drm_encoder_init - Init a preallocated encoder > >>> - * @dev: drm device > >>> - * @encoder: the encoder to init > >>> - * @funcs: callbacks for this encoder > >>> - * @encoder_type: user visible type of the encoder > >>> - * @name: printf style format string for the encoder name, or NULL for default name > >>> - * > >>> - * Initialises a preallocated encoder. Encoder should be subclassed as part of > >>> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > >>> - * called from the driver's &drm_encoder_funcs.destroy hook. > >>> - * > >>> - * Returns: > >>> - * Zero on success, error code on failure. > >>> - */ > >>> -int drm_encoder_init(struct drm_device *dev, > >>> - struct drm_encoder *encoder, > >>> - const struct drm_encoder_funcs *funcs, > >>> - int encoder_type, const char *name, ...) > >>> +static int __drm_encoder_init(struct drm_device *dev, > >>> + struct drm_encoder *encoder, > >>> + const struct drm_encoder_funcs *funcs, > >>> + int encoder_type, const char *name, va_list ap) > >>> { > >>> int ret; > >>> > >>> @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, > >>> encoder->encoder_type = encoder_type; > >>> encoder->funcs = funcs; > >>> if (name) { > >>> - va_list ap; > >>> - > >>> - va_start(ap, name); > >>> encoder->name = kvasprintf(GFP_KERNEL, name, ap); > >>> - va_end(ap); > >>> } else { > >>> encoder->name = kasprintf(GFP_KERNEL, "%s-%d", > >>> drm_encoder_enum_list[encoder_type].name, > >>> @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, > >>> > >>> return ret; > >>> } > >>> + > >>> +/** > >>> + * drm_encoder_init - Init a preallocated encoder > >>> + * @dev: drm device > >>> + * @encoder: the encoder to init > >>> + * @funcs: callbacks for this encoder > >>> + * @encoder_type: user visible type of the encoder > >>> + * @name: printf style format string for the encoder name, or NULL for default name > >>> + * > >>> + * Initialises a preallocated encoder. Encoder should be subclassed as part of > >>> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > >>> + * called from the driver's &drm_encoder_funcs.destroy hook. > >>> + * > >>> + * Returns: > >>> + * Zero on success, error code on failure. > >>> + */ > >>> +int drm_encoder_init(struct drm_device *dev, > >>> + struct drm_encoder *encoder, > >>> + const struct drm_encoder_funcs *funcs, > >>> + int encoder_type, const char *name, ...) > >>> +{ > >>> + va_list ap; > >>> + int ret; > >>> + > >>> + if (name) > >>> + va_start(ap, name); > >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > >>> + if (name) > >>> + va_end(ap); > >>> + > >>> + return ret; > >>> +} > >>> EXPORT_SYMBOL(drm_encoder_init); > >>> > >>> /** > >>> @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > >>> } > >>> EXPORT_SYMBOL(drm_encoder_cleanup); > >>> > >>> +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) > >>> +{ > >>> + struct drm_encoder *encoder = ptr; > >> > >> I'll add > >> > >> if (WARN_ON(!encoder->dev)) > >> return; > >> > >> here. > >> > >>> + drm_encoder_cleanup(encoder); > >>> +} > >>> + > >>> +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, > >>> + const struct drm_encoder_funcs *funcs, > >>> + int encoder_type, const char *name, ...) > >>> +{ > >>> + void *container; > >>> + struct drm_encoder *encoder; > >>> + va_list ap; > >>> + int ret; > >>> + > >>> + if (WARN_ON(!funcs || funcs->destroy)) > >>> + return ERR_PTR(-EINVAL); > >>> + > >>> + container = drmm_kzalloc(dev, size, GFP_KERNEL); > >>> + if (!container) > >>> + return ERR_PTR(-EINVAL); > >>> + > >>> + encoder = container + offset; > >>> + > >>> + if (name) > >>> + va_start(ap, name); > >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > >>> + if (name) > >>> + va_end(ap); > >>> + if (ret) > >>> + return ERR_PTR(ret); > >>> + > >>> + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); > >>> + if (ret) > >>> + return ERR_PTR(ret); > >>> + > >>> + return container; > >>> +} > >>> +EXPORT_SYMBOL(__drmm_encoder_init); > >>> + > >>> static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) > >>> { > >>> struct drm_connector *connector; > >>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > >>> index a60f5f1555ac..54b82554ee88 100644 > >>> --- a/include/drm/drm_encoder.h > >>> +++ b/include/drm/drm_encoder.h > >>> @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, > >>> const struct drm_encoder_funcs *funcs, > >>> int encoder_type, const char *name, ...); > >>> > >>> +__printf(6, 7) > >>> +void *__drmm_encoder_init(struct drm_device *dev, > >>> + size_t size, size_t offset, > >>> + const struct drm_encoder_funcs *funcs, > >>> + int encoder_type, > >>> + const char *name, ...); > >>> + > >>> +/** > >>> + * drmm_encoder_init - Allocate and initialize an encoder > >>> + * @dev: drm device > >>> + * @type: the type of the struct which contains struct &drm_encoder > >>> + * @member: the name of the &drm_encoder within @type. > >>> + * @funcs: callbacks for this encoder > >>> + * @encoder_type: user visible type of the encoder > >>> + * @name: printf style format string for the encoder name, or NULL for default name > >>> + * > >>> + * Allocates and initializes an encoder. Encoder should be subclassed as part of > >>> + * driver encoder objects. Cleanup is automatically handled through registering > >>> + * drm_encoder_cleanup() with drmm_add_action(). > >>> + * > >>> + * The @drm_encoder_funcs.destroy hook must be NULL. > >>> + * > >>> + * Returns: > >>> + * Pointer to new encoder, or ERR_PTR on failure. > >>> + */ > >>> +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ > >>> + ((type *) __drmm_encoder_init(dev, sizeof(type), \ > >>> + offsetof(type, member), funcs, \ > >>> + encoder_type, name, ##__VA_ARGS__)) > >>> + > >> > >> Should this be called drmm_encoder_alloc instead? > > > > Yes. Same for the internal helper for consistency. Also see my other > > I'd suggest calling it drmm_encoder_create() to express that it's _alloc > + _init. Calling it just _alloc sounds as if it doesn't do the _init. At least in drm the pattern we've had thus far is alloc = kzalloc + init. We do use create, but that's for properties and files and stuff, not allocating/initing driver objects. So maybe a bit a bikeshed, but imo alloc() is more consistent with e.g. devm_drm_dev_alloc. Otherwise I think we should rename that one (and all related ones) for consistency. -Daniel > > Best regards > Thomas > > > reply, if the _alloc() variant is all that's needed that makes me > > happy, since then we don't need to code up the drmm_assert_managed > > check for the _init() variants to make sure drivers dont give it > > something stupid like a devm_kzalloc range :-) > > -Daniel > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
On Fri, Jul 24, 2020 at 11:37 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jul 24, 2020 at 11:33 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > Hi > > > > Am 24.07.20 um 11:07 schrieb Daniel Vetter: > > > On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > >> > > >> On Thu, 2020-07-23 at 16:56 +0200, Philipp Zabel wrote: > > >>> Add a drm_encoder_init() variant that allocates an encoder with > > >>> drmm_kzalloc() and registers drm_encoder_cleanup() with > > >>> drmm_add_action_or_reset(). > > >>> > > >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > > Thanks for doing this! > > > > > >>> --- > > >>> New in v2 > > >>> --- > > >>> drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- > > >>> include/drm/drm_encoder.h | 30 ++++++++++ > > >>> 2 files changed, 108 insertions(+), 23 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > > >>> index e555281f43d4..91184f67333c 100644 > > >>> --- a/drivers/gpu/drm/drm_encoder.c > > >>> +++ b/drivers/gpu/drm/drm_encoder.c > > >>> @@ -26,6 +26,7 @@ > > >>> #include <drm/drm_device.h> > > >>> #include <drm/drm_drv.h> > > >>> #include <drm/drm_encoder.h> > > >>> +#include <drm/drm_managed.h> > > >>> > > >>> #include "drm_crtc_internal.h" > > >>> > > >>> @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) > > >>> } > > >>> } > > >>> > > >>> -/** > > >>> - * drm_encoder_init - Init a preallocated encoder > > >>> - * @dev: drm device > > >>> - * @encoder: the encoder to init > > >>> - * @funcs: callbacks for this encoder > > >>> - * @encoder_type: user visible type of the encoder > > >>> - * @name: printf style format string for the encoder name, or NULL for default name > > >>> - * > > >>> - * Initialises a preallocated encoder. Encoder should be subclassed as part of > > >>> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > > >>> - * called from the driver's &drm_encoder_funcs.destroy hook. > > >>> - * > > >>> - * Returns: > > >>> - * Zero on success, error code on failure. > > >>> - */ > > >>> -int drm_encoder_init(struct drm_device *dev, > > >>> - struct drm_encoder *encoder, > > >>> - const struct drm_encoder_funcs *funcs, > > >>> - int encoder_type, const char *name, ...) > > >>> +static int __drm_encoder_init(struct drm_device *dev, > > >>> + struct drm_encoder *encoder, > > >>> + const struct drm_encoder_funcs *funcs, > > >>> + int encoder_type, const char *name, va_list ap) > > >>> { > > >>> int ret; > > >>> > > >>> @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, > > >>> encoder->encoder_type = encoder_type; > > >>> encoder->funcs = funcs; > > >>> if (name) { > > >>> - va_list ap; > > >>> - > > >>> - va_start(ap, name); > > >>> encoder->name = kvasprintf(GFP_KERNEL, name, ap); > > >>> - va_end(ap); > > >>> } else { > > >>> encoder->name = kasprintf(GFP_KERNEL, "%s-%d", > > >>> drm_encoder_enum_list[encoder_type].name, > > >>> @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, > > >>> > > >>> return ret; > > >>> } > > >>> + > > >>> +/** > > >>> + * drm_encoder_init - Init a preallocated encoder > > >>> + * @dev: drm device > > >>> + * @encoder: the encoder to init > > >>> + * @funcs: callbacks for this encoder > > >>> + * @encoder_type: user visible type of the encoder > > >>> + * @name: printf style format string for the encoder name, or NULL for default name > > >>> + * > > >>> + * Initialises a preallocated encoder. Encoder should be subclassed as part of > > >>> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be > > >>> + * called from the driver's &drm_encoder_funcs.destroy hook. > > >>> + * > > >>> + * Returns: > > >>> + * Zero on success, error code on failure. > > >>> + */ > > >>> +int drm_encoder_init(struct drm_device *dev, > > >>> + struct drm_encoder *encoder, > > >>> + const struct drm_encoder_funcs *funcs, > > >>> + int encoder_type, const char *name, ...) > > >>> +{ > > >>> + va_list ap; > > >>> + int ret; > > >>> + > > >>> + if (name) > > >>> + va_start(ap, name); > > >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > > >>> + if (name) > > >>> + va_end(ap); > > >>> + > > >>> + return ret; > > >>> +} > > >>> EXPORT_SYMBOL(drm_encoder_init); > > >>> > > >>> /** > > >>> @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > > >>> } > > >>> EXPORT_SYMBOL(drm_encoder_cleanup); > > >>> > > >>> +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) > > >>> +{ > > >>> + struct drm_encoder *encoder = ptr; > > >> > > >> I'll add > > >> > > >> if (WARN_ON(!encoder->dev)) > > >> return; > > >> > > >> here. > > >> > > >>> + drm_encoder_cleanup(encoder); > > >>> +} > > >>> + > > >>> +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, > > >>> + const struct drm_encoder_funcs *funcs, > > >>> + int encoder_type, const char *name, ...) > > >>> +{ > > >>> + void *container; > > >>> + struct drm_encoder *encoder; > > >>> + va_list ap; > > >>> + int ret; > > >>> + > > >>> + if (WARN_ON(!funcs || funcs->destroy)) > > >>> + return ERR_PTR(-EINVAL); > > >>> + > > >>> + container = drmm_kzalloc(dev, size, GFP_KERNEL); > > >>> + if (!container) > > >>> + return ERR_PTR(-EINVAL); > > >>> + > > >>> + encoder = container + offset; > > >>> + > > >>> + if (name) > > >>> + va_start(ap, name); > > >>> + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > > >>> + if (name) > > >>> + va_end(ap); > > >>> + if (ret) > > >>> + return ERR_PTR(ret); > > >>> + > > >>> + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); > > >>> + if (ret) > > >>> + return ERR_PTR(ret); > > >>> + > > >>> + return container; > > >>> +} > > >>> +EXPORT_SYMBOL(__drmm_encoder_init); > > >>> + > > >>> static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) > > >>> { > > >>> struct drm_connector *connector; > > >>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > > >>> index a60f5f1555ac..54b82554ee88 100644 > > >>> --- a/include/drm/drm_encoder.h > > >>> +++ b/include/drm/drm_encoder.h > > >>> @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, > > >>> const struct drm_encoder_funcs *funcs, > > >>> int encoder_type, const char *name, ...); > > >>> > > >>> +__printf(6, 7) > > >>> +void *__drmm_encoder_init(struct drm_device *dev, > > >>> + size_t size, size_t offset, > > >>> + const struct drm_encoder_funcs *funcs, > > >>> + int encoder_type, > > >>> + const char *name, ...); > > >>> + > > >>> +/** > > >>> + * drmm_encoder_init - Allocate and initialize an encoder > > >>> + * @dev: drm device > > >>> + * @type: the type of the struct which contains struct &drm_encoder > > >>> + * @member: the name of the &drm_encoder within @type. > > >>> + * @funcs: callbacks for this encoder > > >>> + * @encoder_type: user visible type of the encoder > > >>> + * @name: printf style format string for the encoder name, or NULL for default name > > >>> + * > > >>> + * Allocates and initializes an encoder. Encoder should be subclassed as part of > > >>> + * driver encoder objects. Cleanup is automatically handled through registering > > >>> + * drm_encoder_cleanup() with drmm_add_action(). > > >>> + * > > >>> + * The @drm_encoder_funcs.destroy hook must be NULL. > > >>> + * > > >>> + * Returns: > > >>> + * Pointer to new encoder, or ERR_PTR on failure. > > >>> + */ > > >>> +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ > > >>> + ((type *) __drmm_encoder_init(dev, sizeof(type), \ > > >>> + offsetof(type, member), funcs, \ > > >>> + encoder_type, name, ##__VA_ARGS__)) > > >>> + > > >> > > >> Should this be called drmm_encoder_alloc instead? > > > > > > Yes. Same for the internal helper for consistency. Also see my other > > > > I'd suggest calling it drmm_encoder_create() to express that it's _alloc > > + _init. Calling it just _alloc sounds as if it doesn't do the _init. > > At least in drm the pattern we've had thus far is alloc = kzalloc + > init. We do use create, but that's for properties and files and stuff, > not allocating/initing driver objects. > > So maybe a bit a bikeshed, but imo alloc() is more consistent with > e.g. devm_drm_dev_alloc. Otherwise I think we should rename that one > (and all related ones) for consistency. _create also sounds a bit too much like "completely create" to me, this _alloc here is very much just a small part of what it takes to create a complete encoder. The create functions we do have are a lot more the kind of "it's all done, including all the links and registering and everything". -Daniel > -Daniel > > > > > Best regards > > Thomas > > > > > reply, if the _alloc() variant is all that's needed that makes me > > > happy, since then we don't need to code up the drmm_assert_managed > > > check for the _init() variants to make sure drivers dont give it > > > something stupid like a devm_kzalloc range :-) > > > -Daniel > > > > > > > -- > > Thomas Zimmermann > > Graphics Driver Developer > > SUSE Software Solutions Germany GmbH > > Maxfeldstr. 5, 90409 Nürnberg, Germany > > (HRB 36809, AG Nürnberg) > > Geschäftsführer: Felix Imendörffer > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Philipp,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.8-rc6 next-20200724]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Philipp-Zabel/drm-add-drmm_encoder_init/20200723-225705
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project e0ee2288424952e0445f096ae7800472eac11249)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_encoder.c:198:6: warning: no previous prototype for function 'drmm_encoder_init_release' [-Wmissing-prototypes]
void drmm_encoder_init_release(struct drm_device *dev, void *ptr)
^
drivers/gpu/drm/drm_encoder.c:198:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void drmm_encoder_init_release(struct drm_device *dev, void *ptr)
^
static
1 warning generated.
vim +/drmm_encoder_init_release +198 drivers/gpu/drm/drm_encoder.c
197
> 198 void drmm_encoder_init_release(struct drm_device *dev, void *ptr)
199 {
200 struct drm_encoder *encoder = ptr;
201
202 drm_encoder_cleanup(encoder);
203 }
204
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..91184f67333c 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -26,6 +26,7 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_encoder.h> +#include <drm/drm_managed.h> #include "drm_crtc_internal.h" @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev) } } -/** - * drm_encoder_init - Init a preallocated encoder - * @dev: drm device - * @encoder: the encoder to init - * @funcs: callbacks for this encoder - * @encoder_type: user visible type of the encoder - * @name: printf style format string for the encoder name, or NULL for default name - * - * Initialises a preallocated encoder. Encoder should be subclassed as part of - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be - * called from the driver's &drm_encoder_funcs.destroy hook. - * - * Returns: - * Zero on success, error code on failure. - */ -int drm_encoder_init(struct drm_device *dev, - struct drm_encoder *encoder, - const struct drm_encoder_funcs *funcs, - int encoder_type, const char *name, ...) +static int __drm_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type, const char *name, va_list ap) { int ret; @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev, encoder->encoder_type = encoder_type; encoder->funcs = funcs; if (name) { - va_list ap; - - va_start(ap, name); encoder->name = kvasprintf(GFP_KERNEL, name, ap); - va_end(ap); } else { encoder->name = kasprintf(GFP_KERNEL, "%s-%d", drm_encoder_enum_list[encoder_type].name, @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev, return ret; } + +/** + * drm_encoder_init - Init a preallocated encoder + * @dev: drm device + * @encoder: the encoder to init + * @funcs: callbacks for this encoder + * @encoder_type: user visible type of the encoder + * @name: printf style format string for the encoder name, or NULL for default name + * + * Initialises a preallocated encoder. Encoder should be subclassed as part of + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be + * called from the driver's &drm_encoder_funcs.destroy hook. + * + * Returns: + * Zero on success, error code on failure. + */ +int drm_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type, const char *name, ...) +{ + va_list ap; + int ret; + + if (name) + va_start(ap, name); + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); + if (name) + va_end(ap); + + return ret; +} EXPORT_SYMBOL(drm_encoder_init); /** @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) } EXPORT_SYMBOL(drm_encoder_cleanup); +void drmm_encoder_init_release(struct drm_device *dev, void *ptr) +{ + struct drm_encoder *encoder = ptr; + + drm_encoder_cleanup(encoder); +} + +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset, + const struct drm_encoder_funcs *funcs, + int encoder_type, const char *name, ...) +{ + void *container; + struct drm_encoder *encoder; + va_list ap; + int ret; + + if (WARN_ON(!funcs || funcs->destroy)) + return ERR_PTR(-EINVAL); + + container = drmm_kzalloc(dev, size, GFP_KERNEL); + if (!container) + return ERR_PTR(-EINVAL); + + encoder = container + offset; + + if (name) + va_start(ap, name); + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); + if (name) + va_end(ap); + if (ret) + return ERR_PTR(ret); + + ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder); + if (ret) + return ERR_PTR(ret); + + return container; +} +EXPORT_SYMBOL(__drmm_encoder_init); + static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) { struct drm_connector *connector; diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index a60f5f1555ac..54b82554ee88 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...); +__printf(6, 7) +void *__drmm_encoder_init(struct drm_device *dev, + size_t size, size_t offset, + const struct drm_encoder_funcs *funcs, + int encoder_type, + const char *name, ...); + +/** + * drmm_encoder_init - Allocate and initialize an encoder + * @dev: drm device + * @type: the type of the struct which contains struct &drm_encoder + * @member: the name of the &drm_encoder within @type. + * @funcs: callbacks for this encoder + * @encoder_type: user visible type of the encoder + * @name: printf style format string for the encoder name, or NULL for default name + * + * Allocates and initializes an encoder. Encoder should be subclassed as part of + * driver encoder objects. Cleanup is automatically handled through registering + * drm_encoder_cleanup() with drmm_add_action(). + * + * The @drm_encoder_funcs.destroy hook must be NULL. + * + * Returns: + * Pointer to new encoder, or ERR_PTR on failure. + */ +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \ + ((type *) __drmm_encoder_init(dev, sizeof(type), \ + offsetof(type, member), funcs, \ + encoder_type, name, ##__VA_ARGS__)) + /** * drm_encoder_index - find the index of a registered encoder * @encoder: encoder to find index for
Add a drm_encoder_init() variant that allocates an encoder with drmm_kzalloc() and registers drm_encoder_cleanup() with drmm_add_action_or_reset(). Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- New in v2 --- drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++-------- include/drm/drm_encoder.h | 30 ++++++++++ 2 files changed, 108 insertions(+), 23 deletions(-)