Hi Thomas,
On Fri, 2020-07-24 at 09:17 +0200, Thomas Zimmermann wrote: [...]
@@ -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.
I don't think this can happen, a previously called drm_encoder_cleanup() would have removed the encoder from the drm_mode_config::encoder list.
It cannot legally happen, but AFAICS a careless driver could run drm_mode_config_cleanup() and release the encoder. This release callback would still run afterwards and it should warn about the error.
Alright, I'll add a
if (WARN_ON(!encoder->dev)) return;
then.
[...]
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.
I was hoping that we would eventually switch to drmres cleanup as the preferred method, thus getting rid of the need for per-driver cleanup in the error paths and destroy callbacks in most cases.
True. I do support that. But we should also consider how to do this efficiently. Using drmm_mode_config_init() sets up a release function that handles all initialized encoders. For the majority of drivers, this is sufficient. Using drmm_encoder_init() in those drivers just adds overhead without additional benefits. That's also why I consider imx' requirements a corner case.
They certainly are. How about "drivers that can embed encoders in a preallocated structure should use drm_simple_encoder_init() instead"? The difference between the two will be clearer when the actual allocation is folded into drmm_simple_encoder_init() as well.
regards Philipp