[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Philipp Zabel]

Hi Thomas,

On Thu, 2020-07-23 at 09:35 +0200, Thomas Zimmermann wrote:
> Hi
> 
> I have meanwhile seen your imx patchset where this would be useful.
> 
> I still think you should try to pre-allocated all encoders up to a
> limit, so that an extra drmm_kzalloc() is not required. But see my
> comments below.

Thank you for the review coments. The complication with imx-drm is that
the encoders are all in separate platform devices, using the component
framework. Preallocating encoders in the main driver would be
impractical.

The encoders are added in the component .bind() callback, so the main
driver must call drmm_mode_config_init() before binding all components.
The bind callback also is the first place where the component drivers
get to know the drm device, so it is not possible to use drmm_kzalloc()
any earlier.

> Am 22.07.20 um 15:25 schrieb Philipp Zabel:
> > Add a drm_simple_encoder_init() variant that registers
> > drm_encoder_cleanup() with drmm_add_action().
> > 
> > Now drivers can store encoders in memory allocated with drmm_kmalloc()
> > after the call to drmm_mode_config_init(), without having to manually
> > make sure that drm_encoder_cleanup() is called before the memory is
> > freed.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 42 +++++++++++++++++++++++++
> >  include/drm/drm_simple_kms_helper.h     |  4 +++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index 74946690aba4..a243f00cf63d 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -9,6 +9,7 @@
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_managed.h>
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/drm_simple_kms_helper.h>
> > @@ -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.

> > +}
> > +
> > +/**
> > + * drmm_simple_encoder_init - Initialize a preallocated encoder with
> > + *                            basic functionality.
> > + * @dev: drm device
> > + * @encoder: the encoder to initialize
> > + * @encoder_type: user visible type of the encoder
> > + *
> > + * Initialises a preallocated encoder that has no further functionality.
> 
> 'Initializes'

Copy & paste from the drm_simple_encoder_init, I'll fix this in the next
version.

> > + * Settings for possible CRTC and clones are left to their initial values.
> > + * Cleanup is automatically handled through registering drm_encoder_cleanup()
> > + * with drmm_add_action().
> > + *
> > + * The caller of drmm_simple_encoder_init() is responsible for allocating
> > + * the encoder's memory with drmm_kzalloc() to ensure it is automatically
> > + * freed after the encoder has been cleaned up.
> > + *
> 
> 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.

regards
Philipp