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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
