| Message ID | 20220610092924.754942-3-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix hotplug for vc4 | expand |
Hi Maxime 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()? It seems only useful if you need it strictly ordered with drmm_kzalloc()? Best regards Thomas > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/drm_crtc.c | 70 ++++++++++++++++++++++++++++++++++++-- > include/drm/drm_crtc.h | 6 ++++ > 2 files changed, 73 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 26a77a735905..fd986a7dd4ad 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -341,9 +341,10 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc * > * The @primary and @cursor planes are only relevant for legacy uAPI, see > * &drm_crtc.primary and &drm_crtc.cursor. > * > - * 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. > + * Note: consider using drmm_crtc_alloc_with_planes() or > + * drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes() > + * to let the DRM managed resource infrastructure take care of cleanup > + * and deallocation. > * > * Returns: > * Zero on success, error code on failure. > @@ -368,6 +369,69 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > } > EXPORT_SYMBOL(drm_crtc_init_with_planes); > > +static void drmm_crtc_init_with_planes_cleanup(struct drm_device *dev, > + void *ptr) > +{ > + struct drm_crtc *crtc = ptr; > + > + drm_crtc_cleanup(crtc); > +} > + > +/** > + * drmm_crtc_init_with_planes - Initialise a new CRTC object with > + * specified primary and cursor planes. > + * @dev: DRM device > + * @crtc: CRTC object to init > + * @primary: Primary plane for CRTC > + * @cursor: Cursor plane for CRTC > + * @funcs: callbacks for the new CRTC > + * @name: printf style format string for the CRTC name, or NULL for default name > + * > + * Inits a new object created as base part of a driver crtc object. Drivers > + * should use this function instead of drm_crtc_init(), which is only provided > + * for backwards compatibility with drivers which do not yet support universal > + * planes). For really simple hardware which has only 1 plane look at > + * drm_simple_display_pipe_init() instead. > + * > + * Cleanup is automatically handled through registering > + * drmm_crtc_cleanup() with drmm_add_action(). The crtc structure should > + * be allocated with drmm_kzalloc(). > + * > + * The @drm_crtc_funcs.destroy hook must be NULL. > + * > + * The @primary and @cursor planes are only relevant for legacy uAPI, see > + * &drm_crtc.primary and &drm_crtc.cursor. > + * > + * Returns: > + * Zero on success, error code on failure. > + */ > +int drmm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > + struct drm_plane *primary, > + struct drm_plane *cursor, > + const struct drm_crtc_funcs *funcs, > + const char *name, ...) > +{ > + va_list ap; > + int ret; > + > + WARN_ON(funcs && funcs->destroy); > + > + va_start(ap, name); > + ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs, > + name, ap); > + va_end(ap); > + if (ret) > + return ret; > + > + ret = drmm_add_action_or_reset(dev, drmm_crtc_init_with_planes_cleanup, > + crtc); > + if (ret) > + return ret; > + > + return 0; > +} > +EXPORT_SYMBOL(drmm_crtc_init_with_planes); > + > static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev, > void *ptr) > { > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index a70baea0636c..2babd5cffbf3 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1229,6 +1229,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > struct drm_plane *cursor, > const struct drm_crtc_funcs *funcs, > const char *name, ...); > +int drmm_crtc_init_with_planes(struct drm_device *dev, > + struct drm_crtc *crtc, > + struct drm_plane *primary, > + struct drm_plane *cursor, > + const struct drm_crtc_funcs *funcs, > + const char *name, ...); > void drm_crtc_cleanup(struct drm_crtc *crtc); > > __printf(7, 8)
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. Maxime
Hi 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. Best regards Thomas > > Maxime
On Tue, Jun 14, 2022 at 10:29:20AM +0200, Thomas Zimmermann wrote: > Hi > > 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? Maxime
Hi Maxime Am 14.06.22 um 11:04 schrieb Maxime Ripard: > On Tue, Jun 14, 2022 at 10:29:20AM +0200, Thomas Zimmermann wrote: >> Hi >> >> 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. 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). For example, drmm_crtc_alloc_with_planes() really isn't much used by anything. [1] 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(). 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(). Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/C/ident/drmm_crtc_alloc_with_planes [2] https://elixir.bootlin.com/linux/latest/A/ident/drmm_universal_plane_alloc [3] https://elixir.bootlin.com/linux/v5.18.3/source/drivers/gpu/drm/vc4/vc4_plane.c#L1478 [4] https://elixir.bootlin.com/linux/v5.18.3/source/drivers/gpu/drm/vc4/vc4_plane.c#L1491 [5] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_crtc.c#L1135 [6] https://elixir.bootlin.com/linux/v5.18.3/source/drivers/gpu/drm/vc4/vc4_crtc.c#L1176 [7] https://elixir.bootlin.com/linux/v5.18.3/source/drivers/gpu/drm/vc4/vc4_crtc.c#L1142 > > Maxime
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@cerno.tech/ > 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
Hi Am 14.06.22 um 14:09 schrieb Maxime Ripard: > 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. It appears to me that it's a problem with how vc4 organizes its pipeline. That's why I suggested to move some of vc4's init code around to make such allocations more flexible. > > 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. In vc4_crtc.c it should be possible to use drmm_crtc_alloc_with_planes(). In vc4_txp.c, the code apparently initializes struct vc4_txp, so it would be better to use a managed cleanup of struct vc4_txp. 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. Best regards Thomas > > 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@cerno.tech/ > >> 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
On Wed, Jun 15, 2022 at 09:22:55AM +0200, Thomas Zimmermann wrote: > Hi > > Am 14.06.22 um 14:09 schrieb Maxime Ripard: > > 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. > > It appears to me that it's a problem with how vc4 organizes its pipeline. > That's why I suggested to move some of vc4's init code around to make such > allocations more flexible. I mean, it's only a problem because the helpers aren't flexible enough. Reworking the code to allocate the CRTC in vc4_crtc_init() would create much more churn in the TXP driver. So sure, the core would be nice and tidy, but aren't helpers supposed to simplify drivers? > > 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. > > In vc4_crtc.c it should be possible to use drmm_crtc_alloc_with_planes(). In > vc4_txp.c, the code apparently initializes struct vc4_txp, so it would be > better to use a managed cleanup of struct vc4_txp. This only pushes the problem later on. We fixed the CRTC issue, but we have now the exact same situation with the encoder and connector. > 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. 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. It wouldn't have been the case if you kept it inside the drivers. Maxime
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
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
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 26a77a735905..fd986a7dd4ad 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -341,9 +341,10 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc * * The @primary and @cursor planes are only relevant for legacy uAPI, see * &drm_crtc.primary and &drm_crtc.cursor. * - * 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. + * Note: consider using drmm_crtc_alloc_with_planes() or + * drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes() + * to let the DRM managed resource infrastructure take care of cleanup + * and deallocation. * * Returns: * Zero on success, error code on failure. @@ -368,6 +369,69 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_crtc_init_with_planes); +static void drmm_crtc_init_with_planes_cleanup(struct drm_device *dev, + void *ptr) +{ + struct drm_crtc *crtc = ptr; + + drm_crtc_cleanup(crtc); +} + +/** + * drmm_crtc_init_with_planes - Initialise a new CRTC object with + * specified primary and cursor planes. + * @dev: DRM device + * @crtc: CRTC object to init + * @primary: Primary plane for CRTC + * @cursor: Cursor plane for CRTC + * @funcs: callbacks for the new CRTC + * @name: printf style format string for the CRTC name, or NULL for default name + * + * Inits a new object created as base part of a driver crtc object. Drivers + * should use this function instead of drm_crtc_init(), which is only provided + * for backwards compatibility with drivers which do not yet support universal + * planes). For really simple hardware which has only 1 plane look at + * drm_simple_display_pipe_init() instead. + * + * Cleanup is automatically handled through registering + * drmm_crtc_cleanup() with drmm_add_action(). The crtc structure should + * be allocated with drmm_kzalloc(). + * + * The @drm_crtc_funcs.destroy hook must be NULL. + * + * The @primary and @cursor planes are only relevant for legacy uAPI, see + * &drm_crtc.primary and &drm_crtc.cursor. + * + * Returns: + * Zero on success, error code on failure. + */ +int drmm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, + struct drm_plane *primary, + struct drm_plane *cursor, + const struct drm_crtc_funcs *funcs, + const char *name, ...) +{ + va_list ap; + int ret; + + WARN_ON(funcs && funcs->destroy); + + va_start(ap, name); + ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs, + name, ap); + va_end(ap); + if (ret) + return ret; + + ret = drmm_add_action_or_reset(dev, drmm_crtc_init_with_planes_cleanup, + crtc); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL(drmm_crtc_init_with_planes); + static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev, void *ptr) { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a70baea0636c..2babd5cffbf3 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1229,6 +1229,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_plane *cursor, const struct drm_crtc_funcs *funcs, const char *name, ...); +int drmm_crtc_init_with_planes(struct drm_device *dev, + struct drm_crtc *crtc, + struct drm_plane *primary, + struct drm_plane *cursor, + const struct drm_crtc_funcs *funcs, + const char *name, ...); void drm_crtc_cleanup(struct drm_crtc *crtc); __printf(7, 8)
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. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/drm_crtc.c | 70 ++++++++++++++++++++++++++++++++++++-- include/drm/drm_crtc.h | 6 ++++ 2 files changed, 73 insertions(+), 3 deletions(-)