Hi Am 20.06.22 um 16:39 schrieb Maxime Ripard:
On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote:Hi Am 20.06.22 um 15:48 schrieb Maxime Ripard:Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote:Am 10.06.22 um 11:28 schrieb Maxime Ripard:The DRM-managed function to register an encoder is drmm_simple_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder. However, we might want to separate the structure creation and the encoder initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector. Let's create an helper to only initialise an encoder that would be passed as an argument.There's nothing wrong with this patch, but I have to admit that adding drm_simple_encoder_init() et al was a mistake.Why do you think it was a mistake?The simple-encoder stuff is an interface that no one really needs. Compared to open-coding the function, it's barely an improvement in LOCs, but nothing else. Back when I added drm_simple_encoder_init(), I wanted to simplify the many drivers that initialized the encoder with a cleanup callback and nothing else. IIRC it was an improvement back then. But now we already have a related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro back then, we could use the regular encoder helpers.It would have been better to add an initializer macro like #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ .destroy = drm_encoder_cleanup It's way more flexible and similarly easy to use. Anyway...We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's neededNot necessary. It's not super important.The corollary is though :) If I understand you right, it means that you'd rather have a destroy callback everywhere instead of calling the _cleanup function through a drm-managed callback, and let drm_dev_unregister do its job? If so, it means that we shouldn't be following the drmm_.*_alloc functions and should drop all the new ones from this series.
No, no. What I'm saying is that simple-encoder is a pointless mid-layer. There's nothing that couldn't easily be achieved with the regular encoder functions.
Best regards Thomas
Maxime
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
