[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[daniel at ffwll.ch]

On Wed, Jul 22, 2020 at 05:08:03PM +0200, Philipp Zabel wrote:
> Hi Thomas,
> 
> thank you for your comment.
> 
> On Wed, 2020-07-22 at 16:43 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > 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)
> > > +{
> > > +	struct drm_encoder *encoder = ptr;
> > > +
> > > +	drm_encoder_cleanup(encoder);
> > > +}
> > 
> > This doesn't work. DRM cleans up the encoder by invoking the destroy
> > callback from the encoder functions. This additional helper would
> > cleanup the encoder a second time.
> 
> Indeed this would require the encoder destroy callback to be NULL.

Yeah the drmm_ versions of these need to check that the ->cleanup hook is
NULL.

Also there's not actually a double-free, since drm_foo_cleanup removes it
from the lists, which means drm_mode_config_cleanup won't even see it. But
if the driver has some additional code in ->cleanup that won't ever run,
so probably still a bug.

I also think that the drmm_foo_ wrappers should also do the allocation
(and upcasting) kinda like drmm_dev_alloc(). Otherwise we're still stuck
with tons of boilerplate.

For now I think it's ok if drivers that switch to drmm_ just copypaste,
until we're sure this is the right thing to do. And then maybe also roll
these out for all objects that stay for the entire lifetime of drm_device
(plane, crtc, encoder, plus variants). Just to make sure we're consistent
across all of them.
-Daniel

> > You can already embed the encoder in another structure and things should
> > work as expected.
> 
> If the embedding structure is a component allocated with drmm_kmalloc()
> after the call to drmm_mode_config_init(), the structure will already be
> freed before the destroy callback is run from
> drmm_mode_config_init_release().
> 
> regards
> Philipp
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch