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 needed > > Not 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. Maxime
Attachment:
signature.asc
Description: PGP signature
