On Thu, Jul 23, 2020 at 9:36 AM Thomas Zimmermann tzimmermann@suse.de wrote:
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.
Uh preallocation is horribly, because you need to first preallocate all encoders, then call drmm_mode_config_init, and only then can you call drm_encoder_init. That's terrible flow, and I'm aware of the problem. That's why we need new set of drmm_ helpers to do all the steps for kms static object setup, if we actually want to improve things. -Daniel
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@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
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel