[PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init

[Maxime Ripard]

On Mon, Jun 20, 2022 at 09:04:11PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 20.06.22 um 16:45 schrieb Thomas Zimmermann:
> > Hi
> > 
> > Am 20.06.22 um 16:39 schrieb Maxime Ripard:
> > > On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote:
> > > > Hi
> > > > 
> > > > Am 20.06.22 um 15:48 schrieb Maxime Ripard:
> > > > > Hi,
> > > > > 
> > > > > On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote:
> > > > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard:
> > > > > > > The DRM-managed function to register an encoder is
> > > > > > > drmm_simple_encoder_alloc() and its variants, which will allocate the
> > > > > > > underlying structure and initialisation the encoder.
> > > > > > > 
> > > > > > > However, we might want to separate the structure
> > > > > > > creation and the encoder
> > > > > > > initialisation, for example if the structure is
> > > > > > > shared across multiple DRM
> > > > > > > entities, for example an encoder and a connector.
> > > > > > > 
> > > > > > > Let's create an helper to only initialise an encoder
> > > > > > > that would be passed
> > > > > > > as an argument.
> > > > > > > 
> > > > > > 
> > > > > > There's nothing wrong with this patch, but I have to admit that adding
> > > > > > drm_simple_encoder_init() et al was a mistake.
> > > > > 
> > > > > Why do you think it was a mistake?
> > > > 
> > > > The simple-encoder stuff is an interface that no one really
> > > > needs. Compared
> > > > to open-coding the function, it's barely an improvement in LOCs,
> > > > but nothing
> > > > else.
> > > > 
> > > > Back when I added drm_simple_encoder_init(), I wanted to
> > > > simplify the many
> > > > drivers that initialized the encoder with a cleanup callback and nothing
> > > > else.  IIRC it was an improvement back then.  But now we already have a
> > > > related drmm_ helper and a drmm_alloc_ helper. If I had only
> > > > added the macro
> > > > back then, we could use the regular encoder helpers.
> > > > 
> > > > > 
> > > > > > It would have been better to add an initializer macro like
> > > > > > 
> > > > > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \
> > > > > >     .destroy = drm_encoder_cleanup
> > > > > > 
> > > > > > It's way more flexible and similarly easy to use. Anyway...
> > > > > 
> > > > > We can still have this. It would simplify this series so I could
> > > > > definitely squeeze that patch in and add a TODO item / deprecation
> > > > > notice for simple encoders if you think it's needed
> > > > 
> > > > Not necessary. It's not super important.
> > > 
> > > The corollary is though :)
> > > 
> > > If I understand you right, it means that you'd rather have a destroy
> > > callback everywhere instead of calling the _cleanup function through a
> > > drm-managed callback, and let drm_dev_unregister do its job?
> > > 
> > > If so, it means that we shouldn't be following the drmm_.*_alloc
> > > functions and should drop all the new ones from this series.
> > 
> > No, no. What I'm saying is that simple-encoder is a pointless mid-layer.
> > There's nothing that couldn't easily be achieved with the regular
> > encoder functions.
> 
> I guess that if you want to change something here, you could add
> drmm_encoder_init() instead and convert vc4. That function might be more
> useful for other drivers in the long run.

I already had that patch in the series. I've dropped this one and
converted everyone to a !simple encoder.

Thanks!
maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220622/82ba5126/attachment.sig>