[PATCH 02/64] drm/crtc: Introduce drmm_crtc_init_with_planes

[Maxime Ripard]

On Wed, Jun 15, 2022 at 12:34:46PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.06.22 um 10:32 schrieb Maxime Ripard:
> [...]
> > > See, helpers should be useful to many drivers. If we add them, we also add a
> > > resources and maintenance overhead to our libraries. And right now, these
> > > new functions appear to work around the design of the vc4 driver's data
> > > structures.  If you want to keep them, maybe let's first merge them into vc4
> > > (something like vc4_crtc_init_with_planes(), etc). If another driver with a
> > > use case comes along, we can still move them out easily.
> > 
> > Not that I disagree, but there's also the fact that people will start
> > using helpers because they are available.
> > 
> > You mentioned drmm_crtc_alloc_with_planes(). It was introduced in 5.12
> > with a single user (ipuv3-crtc.c). And then, because it was available,
> > in 5.17 was merged the Unisoc driver that was the second user of that
> > function.
> 
> OTOH, it actually took 5 releases to find another user. Maybe we need to
> look harder for possible reuse of helpers, but I wouldn't count 5 releases
> as a good track record.

Indeed, but I'm not sure it's due to the helper itself. I'm fairly sure
nobody really cared or knows about the lifetime issues solved by the
drm-managed functions, and so nobody feels an urge to convert.

And one can't ask during review to use it if they're not aware of the
helpers existence. Between 5.12 and 5.17, only hyperv and sprd were
merged. 50% of the new drivers using it is not too bad.

> > drmm_simple_encoder_alloc() and drmm_universal_plane_alloc() are in the
> > same situation and we wouldn't have had that discussion if it was kept
> > in the imx driver.
> > 
> > The helper being there allows driver authors to discover them easily,
> > pointing out an issue that possibly wasn't obvious to the author, and we
> > can also point during review that the helpers are there to be used.
> > 
> > None of that would be possible if we were to keep them in a driver,
> > because no one but the author would know about it.
> > 
> > My feeling is that the rule you mention works great when you know that
> > some deviation is going to happen. But we're replacing an init function
> > that has been proved good enough here, so it's not rocket science
> > really.
> > 
> > drmm_mutex_init() is a great example of that actually. You merged it
> > recently with two users. We could have used the exact same argument that
> > it belonged in those drivers because it wasn't generic enough or
> > something. But it's trivial, so it was a good decision to merge it as a
> > helper. And because you did so, I later found out that mutex_destroy()
> > was supposed to be called in the first place, I converted vc4 to
> > drmm_mutex_init(), and now that bug is fixed.
> 
> But when I added it, there actually were two users. I would not have added
> drmm_mutex_init() if it was only useful for a single driver.
> 
> In other cases, we tend to push single-user helpers into the drivers. That
> happened several times with TTM. Code was moved into vmwgfx, because there
> where no other users.

Yeah, and I introduced some in that series too. It makes sense to have
that restriction for stuff that we're not really sure about. But for
strict equivalents to functions that already exists with the same API
I'm not sure that restriction makes sense.

In fact, we also merged recently devm_drm_bridge_add with a single user
and that was fine too, because that function has been there for a while
and we know it's not going to change much.

> Anyway, as you insist on using this helper, go for it. But please, at least
> reimplement drm_crtc_alloc_with_planes() on top of a shared internal
> implementation. AFAICT drm_crtc_alloc_with_planes() is drmm_kzalloc +
> drmm_crtc_init_with_planes().

Ack

> Same for other related helpers in the other patches, if there are any.

drmm_encoder_alloc() and drmm_simple_encoder_alloc() are in the same
situation, I'll fix those too.

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/20220616/943bb52d/attachment-0001.sig>