On Tue, Jun 14, 2022 at 01:47:28PM +0200, Thomas Zimmermann wrote: > Am 14.06.22 um 11:04 schrieb Maxime Ripard: > > On Tue, Jun 14, 2022 at 10:29:20AM +0200, Thomas Zimmermann wrote: > > > Am 14.06.22 um 09:37 schrieb Maxime Ripard: > > > > Hi Thomas, > > > > > > > > On Mon, Jun 13, 2022 at 01:23:54PM +0200, Thomas Zimmermann wrote: > > > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > > > > > The DRM-managed function to register a CRTC is > > > > > > drmm_crtc_alloc_with_planes(), which will allocate the underlying > > > > > > structure and initialisation the CRTC. > > > > > > > > > > > > However, we might want to separate the structure creation and the CRTC > > > > > > 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 a CRTC that would be passed as > > > > > > an argument. > > > > > > > > > > Before I review all of thes patches, one question. it's yet not clear to me > > > > > why drm_crtc_init_with_planes() wouldn't work. (I know we discussed this on > > > > > IRC.) > > > > > > > > > > If you're calling drmm_mode_config_init(), it will clean up all the CRTC, > > > > > encoder connector, etc. data structures for you. For CRTC instances in > > > > > kmalloced memory, wouldn't it be simpler to put the corresponding kfree into > > > > > vc4_crtc_destroy()? > > > > > > > > My intent was to remove as much of the lifetime handling and > > > > memory-management from drivers as possible. > > > > > > > > My feeling is that while the solution you suggest would definitely work, > > > > it relies on drivers authors to know about this, and make the effort to > > > > convert the drivers themselves. And then we would have to review that, > > > > which we will probably miss (collectively), because it's a bit obscure. > > > > > > > > While with either the drmm_alloc_* functions, or the new functions I > > > > introduce there, we can just deprecate the old ones, create a TODO entry > > > > and a coccinelle script and trust that it works properly. > > > > > > Thanks for explaining the motivation. > > > > > > I would not want to deprecate any of the existing functions, as they work > > > for many drivers. The drmm_ functions add additional overhead that not > > > everyone is willing to pay. > > > > I'm a bit confused. drm_crtc_init_with_planes() at the moment has: > > > > * Note: consider using drmm_crtc_alloc_with_planes() instead of > > * drm_crtc_init_with_planes() to let the DRM managed resource infrastructure > > * take care of cleanup and deallocation. > > > > Just like drm_encoder_init(), drm_simple_encoder_init() and > > drm_universal_plane_init(), so all the functions we have a drmm_* helper > > for. > > > > And we have a TODO-list item that heavily hints at using them: > > https://dri.freedesktop.org/docs/drm/gpu/todo.html#object-lifetime-fixes > > > > So it looks like we're already well on the deprecation path? > > AFAIU that TODO item is about replacing devm_kzalloc() with drmm_kzalloc(). > It's not about the plain init functions, such as drm_crtc_init_with_planes() > or drm_universal_plane_init(). Many simple drivers allocate their > modesetting pipeline's components first and then build the pipeline with the > drm_ functions. I don't think we can take that away from them. Sure, that's exactly what those first patches are about? It allows to use a DRM managed initialization without disrupting the drivers structure too much? > The concern I have is that we're adding lots of helpers for all kind of > scenarios and end up with a lot of duplication (and fragmentation among > drivers). I can see two: whether you want to allocate / init, or just init? We're not going to have more than that. > For example, drmm_crtc_alloc_with_planes() really isn't much used > by anything. [1] Not that I disagree here, but it might be that it isn't the most helpful helper? In vc4 case, we just can't use it easily. Our CRTC driver is shared between the "regular" CRTCs in the display path, and another instance dedicated to the writeback connector. The shared stuff is initialized through vc4_crtc_init(): https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_crtc.c#L1120 It initializes the structure, set up the planes, etc. Basically everything that our CRTC controller will be doing, and would be shared by both cases. However, since the writeback and regular CRTC structures are different, we can't really make that function allocate the backing structure either. We could do some compiler magic to pass the total size and the offset to that function, just like what drmm_crtc_alloc_with_planes is doing, but then we would have the same issue with the writeback stuff that needs to initialize the encoder and connector. So we would need a drmm_encoder_init anyway. > Same for drmm_universal_plane_alloc(). [2] > > Instead of adding new helpers, it would be better to build upon > drmm_crtc_alloc_with_planes(), drmm_univeral_plane_alloc(), etc. > > For example, a good starting point would be vc4_plane_init(). It could alloc > with drmm_univeral_plane_alloc(), which would replace devm_kzalloc() [3] and > drm_univeral_plane_alloc() [4] in one step. From what I understand, that's > what your patchset wants to do. But it looks like you're effectively > open-coding drmm_universl_plane_alloc(). Where I could use the alloc helper, I did. See the following patch that does exactly what you described: https://lore.kernel.org/dri-devel/20220610092924.754942-17-maxime@xxxxxxxxxx/ > With vc4_plane_init() correctly managed, the next candidate could be > vc4_crtc_init(). You probably want to pull vc4_plane_init() [5] into > callers. to get it out of the way. If you move calls to devm_kzalloc() [6] > and drm_crtc_init_with_planes() [7] closer together, you can replace them > with drmm_crtc_alloc_with_planes(). See above Maxime
Attachment:
signature.asc
Description: PGP signature
