[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Thomas Zimmermann]

Hi

I have meanwhile seen your imx patchset where this would be useful.

I still think you should try to pre-allocated all encoders up to a
limit, so that an extra drmm_kzalloc() is not required. But see my
comments below.

Am 22.07.20 um 15:25 schrieb Philipp Zabel:
> Add a drm_simple_encoder_init() variant that registers
> drm_encoder_cleanup() with drmm_add_action().
> 
> Now drivers can store encoders in memory allocated with drmm_kmalloc()
> after the call to drmm_mode_config_init(), without having to manually
> make sure that drm_encoder_cleanup() is called before the memory is
> freed.
> 
> Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 42 +++++++++++++++++++++++++
>  include/drm/drm_simple_kms_helper.h     |  4 +++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 74946690aba4..a243f00cf63d 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,47 @@ int drm_simple_encoder_init(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_simple_encoder_init);
>  
> +static void drmm_encoder_cleanup(struct drm_device *dev, void *ptr)

It's the reset helper, so drmm_simple_encoder_reset() would be appropriate.

> +{
> +	struct drm_encoder *encoder = ptr;
> +
> +	drm_encoder_cleanup(encoder);

This should first check for (encoder->dev) being true. If drivers
somehow manage to clean-up the mode config first, we should detect it. I
know it's a bug, but I wouldn't trust drivers with that.

> +}
> +
> +/**
> + * drmm_simple_encoder_init - Initialize a preallocated encoder with
> + *                            basic functionality.
> + * @dev: drm device
> + * @encoder: the encoder to initialize
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality.

'Initializes'

> + * 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().
> + *
> + * The caller of drmm_simple_encoder_init() is responsible for allocating
> + * the encoder's memory with drmm_kzalloc() to ensure it is automatically
> + * freed after the encoder has been cleaned up.
> + *

The idiomatic way of cleaning up an encoder is via mode-config cleanup.
This interface is an exception for a corner case. So there needs to be a
paragraph that clearly explains the corner case. Please also discourage
from using drmm_simple_encoder_init() if drm_simple_encoder_init() would
work.

Best regards
Thomas

> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drmm_simple_encoder_init(struct drm_device *dev,
> +			     struct drm_encoder *encoder,
> +			     int encoder_type)
> +{
> +	int ret;
> +
> +	ret = drm_encoder_init(dev, encoder, &drm_simple_encoder_funcs_cleanup,
> +			       encoder_type, NULL);
> +	if (ret)
> +		return ret;
> +
> +	return drmm_add_action_or_reset(dev, drmm_encoder_cleanup, encoder);
> +}
> +EXPORT_SYMBOL(drmm_simple_encoder_init);
> +
>  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..27f0915599c8 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -185,4 +185,8 @@ int drm_simple_encoder_init(struct drm_device *dev,
>  			    struct drm_encoder *encoder,
>  			    int encoder_type);
>  
> +int drmm_simple_encoder_init(struct drm_device *dev,
> +			     struct drm_encoder *encoder,
> +			     int encoder_type);
> +
>  #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> 

-- 
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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200723/f325a729/attachment.sig>