On Tue, Jun 25, 2024 at 04:03:21PM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 25.06.24 um 15:55 schrieb Jocelyn Falempe:
> >
> >
> > On 25/06/2024 15:18, Thomas Zimmermann wrote:
> > > The function drm_simple_encoder_init() is a trivial helper and
> > > deprecated. Replace it with the regular call to drm_encoder_init().
> > > Resolves the dependency on drm_simple_kms_helper.h. No functional
> > > changes.
> >
> > Do you think it's possible to add a default to drm_encoder_init() to
> > avoid having to declare the same struct for each encoder ?
> >
> > something like:
> >
> > drm_encoder_init(...)
> > {
> >
> > if (!funcs)
> > funcs = &drm_encoder_default_funcs;
> >
> > So you can call it like this to get the default funcs:
> >
> > drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL);
> >
> >
> > I don't see this pattern in other drm functions, so it might not fit the
> > current coding style.
>
> Yeah, we don't do this in other places. And it's not guaranteed that
> drm_encoder_cleanup() is the correct helper in all cases; even the common
> ones. I would prefer to not add such tweaks. As for
> drm_simple_encoder_init(), it was an attempt to solve exactly this problem,
> but the function is so trivial that it's not actually worth the dependency.
For drmm_encoder_init/alloc this is doable, because it makes sure that
there really is only one correct encoder cleanup hook for simple encoders.
Without drmm_ there's the entire "who calls kfree() and how buggy is it"
mess :-/
So I'd very much welcome this kind of handling in drmm_encoder_alloc/init,
but in the unmanaged drm_encoder_init I agree it sounds like a mistake.
Cheers,
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
