Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux