[PATCH] drm/simple_kms_helper: add drmm_simple_encoder_init()

[Daniel Vetter]

On Thu, Jul 23, 2020 at 4:46 PM Philipp Zabel <p.zabel at pengutronix.de> wrote:
>
> Hi Daniel,
>
> On Thu, 2020-07-23 at 00:22 +0200, daniel at ffwll.ch wrote:
> [...]
> > 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.
>
> Ok, I'll try this:
>
> drmm_encoder_init() variant can verify that the passed
> drm_encoder_funcs::destroy hook is NULL.
>
> drmm_simple_encoder_init() can just provide empty drm_encoder_funcs
> internally.
>
> > 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.
>
> Thank you for clarifying, I wasn't sure this was the goal. I've started
> with this function mostly because this is the most used one in imx-drm
> and the only one where I didn't have to deal with va_args boilerplate.

Hm if we go with also exposing the drmm_foo_init() variants then I
think these should check that the passed-in memory is indeed allocated
by drmres on the right device. That's kinda the bug I'm afraid of when
we exposed drmm_foo_init() to drivers and not just drmm_foo_alloc()
which does everything and correctly for you. For the drmm_is_manged or
so I think we can just reuse the same loop I've typed up already for
drmm_kfree. There shouldn't be too many allocations on that list that
the list walk at driver load will matter. Oh also better name than
drmm_is_managed would be good, devres doesn't seem to have that. Hm
maybe drmm_assert_managed(dev, void, size) and it checks that it's
fully contained within a drmm_kmalloc region.

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