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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 24.07.20 um 11:07 schrieb Daniel Vetter:
> On Fri, Jul 24, 2020 at 10:49 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> 
> Thanks for doing this!
> 
>>> ---
>>> New in v2
>>> ---
>>>  drivers/gpu/drm/drm_encoder.c | 101 ++++++++++++++++++++++++++--------
>>>  include/drm/drm_encoder.h     |  30 ++++++++++
>>>  2 files changed, 108 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
>>> index e555281f43d4..91184f67333c 100644
>>> --- a/drivers/gpu/drm/drm_encoder.c
>>> +++ b/drivers/gpu/drm/drm_encoder.c
>>> @@ -26,6 +26,7 @@
>>>  #include <drm/drm_device.h>
>>>  #include <drm/drm_drv.h>
>>>  #include <drm/drm_encoder.h>
>>> +#include <drm/drm_managed.h>
>>>
>>>  #include "drm_crtc_internal.h"
>>>
>>> @@ -91,25 +92,10 @@ void drm_encoder_unregister_all(struct drm_device *dev)
>>>       }
>>>  }
>>>
>>> -/**
>>> - * drm_encoder_init - Init a preallocated encoder
>>> - * @dev: drm device
>>> - * @encoder: the encoder to init
>>> - * @funcs: callbacks for this encoder
>>> - * @encoder_type: user visible type of the encoder
>>> - * @name: printf style format string for the encoder name, or NULL for default name
>>> - *
>>> - * Initialises a preallocated encoder. Encoder should be subclassed as part of
>>> - * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
>>> - * called from the driver's &drm_encoder_funcs.destroy hook.
>>> - *
>>> - * Returns:
>>> - * Zero on success, error code on failure.
>>> - */
>>> -int drm_encoder_init(struct drm_device *dev,
>>> -                  struct drm_encoder *encoder,
>>> -                  const struct drm_encoder_funcs *funcs,
>>> -                  int encoder_type, const char *name, ...)
>>> +static int __drm_encoder_init(struct drm_device *dev,
>>> +                           struct drm_encoder *encoder,
>>> +                           const struct drm_encoder_funcs *funcs,
>>> +                           int encoder_type, const char *name, va_list ap)
>>>  {
>>>       int ret;
>>>
>>> @@ -125,11 +111,7 @@ int drm_encoder_init(struct drm_device *dev,
>>>       encoder->encoder_type = encoder_type;
>>>       encoder->funcs = funcs;
>>>       if (name) {
>>> -             va_list ap;
>>> -
>>> -             va_start(ap, name);
>>>               encoder->name = kvasprintf(GFP_KERNEL, name, ap);
>>> -             va_end(ap);
>>>       } else {
>>>               encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
>>>                                         drm_encoder_enum_list[encoder_type].name,
>>> @@ -150,6 +132,38 @@ int drm_encoder_init(struct drm_device *dev,
>>>
>>>       return ret;
>>>  }
>>> +
>>> +/**
>>> + * drm_encoder_init - Init a preallocated encoder
>>> + * @dev: drm device
>>> + * @encoder: the encoder to init
>>> + * @funcs: callbacks for this encoder
>>> + * @encoder_type: user visible type of the encoder
>>> + * @name: printf style format string for the encoder name, or NULL for default name
>>> + *
>>> + * Initialises a preallocated encoder. Encoder should be subclassed as part of
>>> + * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
>>> + * called from the driver's &drm_encoder_funcs.destroy hook.
>>> + *
>>> + * Returns:
>>> + * Zero on success, error code on failure.
>>> + */
>>> +int drm_encoder_init(struct drm_device *dev,
>>> +                  struct drm_encoder *encoder,
>>> +                  const struct drm_encoder_funcs *funcs,
>>> +                  int encoder_type, const char *name, ...)
>>> +{
>>> +     va_list ap;
>>> +     int ret;
>>> +
>>> +     if (name)
>>> +             va_start(ap, name);
>>> +     ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
>>> +     if (name)
>>> +             va_end(ap);
>>> +
>>> +     return ret;
>>> +}
>>>  EXPORT_SYMBOL(drm_encoder_init);
>>>
>>>  /**
>>> @@ -181,6 +195,47 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>>>  }
>>>  EXPORT_SYMBOL(drm_encoder_cleanup);
>>>
>>> +void drmm_encoder_init_release(struct drm_device *dev, void *ptr)
>>> +{
>>> +     struct drm_encoder *encoder = ptr;
>>
>> I'll add
>>
>>         if (WARN_ON(!encoder->dev))
>>                 return;
>>
>> here.
>>
>>> +     drm_encoder_cleanup(encoder);
>>> +}
>>> +
>>> +void *__drmm_encoder_init(struct drm_device *dev, size_t size, size_t offset,
>>> +                       const struct drm_encoder_funcs *funcs,
>>> +                       int encoder_type, const char *name, ...)
>>> +{
>>> +     void *container;
>>> +     struct drm_encoder *encoder;
>>> +     va_list ap;
>>> +     int ret;
>>> +
>>> +     if (WARN_ON(!funcs || funcs->destroy))
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     container = drmm_kzalloc(dev, size, GFP_KERNEL);
>>> +     if (!container)
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     encoder = container + offset;
>>> +
>>> +     if (name)
>>> +             va_start(ap, name);
>>> +     ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
>>> +     if (name)
>>> +             va_end(ap);
>>> +     if (ret)
>>> +             return ERR_PTR(ret);
>>> +
>>> +     ret = drmm_add_action_or_reset(dev, drmm_encoder_init_release, encoder);
>>> +     if (ret)
>>> +             return ERR_PTR(ret);
>>> +
>>> +     return container;
>>> +}
>>> +EXPORT_SYMBOL(__drmm_encoder_init);
>>> +
>>>  static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
>>>  {
>>>       struct drm_connector *connector;
>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>>> index a60f5f1555ac..54b82554ee88 100644
>>> --- a/include/drm/drm_encoder.h
>>> +++ b/include/drm/drm_encoder.h
>>> @@ -195,6 +195,36 @@ int drm_encoder_init(struct drm_device *dev,
>>>                    const struct drm_encoder_funcs *funcs,
>>>                    int encoder_type, const char *name, ...);
>>>
>>> +__printf(6, 7)
>>> +void *__drmm_encoder_init(struct drm_device *dev,
>>> +                       size_t size, size_t offset,
>>> +                       const struct drm_encoder_funcs *funcs,
>>> +                       int encoder_type,
>>> +                       const char *name, ...);
>>> +
>>> +/**
>>> + * drmm_encoder_init - Allocate and initialize an encoder
>>> + * @dev: drm device
>>> + * @type: the type of the struct which contains struct &drm_encoder
>>> + * @member: the name of the &drm_encoder within @type.
>>> + * @funcs: callbacks for this encoder
>>> + * @encoder_type: user visible type of the encoder
>>> + * @name: printf style format string for the encoder name, or NULL for default name
>>> + *
>>> + * Allocates and initializes an encoder. Encoder should be subclassed as part of
>>> + * driver encoder objects. Cleanup is automatically handled through registering
>>> + * drm_encoder_cleanup() with drmm_add_action().
>>> + *
>>> + * The @drm_encoder_funcs.destroy hook must be NULL.
>>> + *
>>> + * Returns:
>>> + * Pointer to new encoder, or ERR_PTR on failure.
>>> + */
>>> +#define drmm_encoder_init(dev, type, member, funcs, encoder_type, name, ...) \
>>> +     ((type *) __drmm_encoder_init(dev, sizeof(type), \
>>> +                                   offsetof(type, member), funcs, \
>>> +                                   encoder_type, name, ##__VA_ARGS__))
>>> +
>>
>> Should this be called drmm_encoder_alloc instead?
> 
> Yes. Same for the internal helper for consistency. Also see my other

I'd suggest calling it drmm_encoder_create() to express that it's _alloc
+ _init. Calling it just _alloc sounds as if it doesn't do the _init.

Best regards
Thomas

> reply, if the _alloc() variant is all that's needed that makes me
> happy, since then we don't need to code up the drmm_assert_managed
> check for the _init() variants to make sure drivers dont give it
> something stupid like a devm_kzalloc range :-)
> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux