On Mon, Jun 20, 2022 at 09:04:11PM +0200, Thomas Zimmermann wrote:
Hi
Am 20.06.22 um 16:45 schrieb Thomas Zimmermann:
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 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.
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.
I guess that if you want to change something here, you could add drmm_encoder_init() instead and convert vc4. That function might be more useful for other drivers in the long run.
I already had that patch in the series. I've dropped this one and converted everyone to a !simple encoder.
Thanks! maxime