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

[Thomas Zimmermann]

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.

> 
> 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.

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(). Same for other related 
helpers in the other patches, if there are any.

Best regards
Thomas

> 
> It wouldn't have been the case if you kept it inside the drivers.
> 
> Maxime

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220615/ca6425ac/attachment.sig>