[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Philipp Zabel]

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