| Message ID | 20220610092924.754942-10-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix hotplug for vc4 | expand |
Hi Am 10.06.22 um 11:28 schrieb Maxime Ripard: > The DRM-managed function to register an encoder is > drmm_simple_encoder_alloc() and its variants, which will allocate the > underlying structure and initialisation the encoder. > > However, we might want to separate the structure creation and the encoder > 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 an encoder that would be passed > as an argument. > There's nothing wrong with this patch, but I have to admit that adding drm_simple_encoder_init() et al was a mistake. It would have been better to add an initializer macro like #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ .destroy = drm_encoder_cleanup It's way more flexible and similarly easy to use. Anyway... > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Best regards Thomas > --- > drivers/gpu/drm/drm_simple_kms_helper.c | 46 +++++++++++++++++++++++-- > include/drm/drm_simple_kms_helper.h | 3 ++ > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > index 72989ed1baba..876870dd98e5 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -58,9 +58,10 @@ static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = { > * stored in the device structure. Free the encoder's memory as part of > * the device release function. > * > - * Note: consider using drmm_simple_encoder_alloc() instead of > - * drm_simple_encoder_init() to let the DRM managed resource infrastructure > - * take care of cleanup and deallocation. > + * Note: consider using drmm_simple_encoder_alloc() or > + * drmm_simple_encoder_init() instead of drm_simple_encoder_init() to > + * let the DRM managed resource infrastructure take care of cleanup and > + * deallocation. > * > * Returns: > * Zero on success, error code on failure. > @@ -75,6 +76,45 @@ int drm_simple_encoder_init(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_simple_encoder_init); > > +static void drmm_simple_encoder_cleanup(struct drm_device *dev, void *ptr) > +{ > + struct drm_encoder *encoder = ptr; > + > + drm_encoder_cleanup(encoder); > +} > + > +/** > + * drmm_simple_encoder_init - Initialize a preallocated encoder with > + * basic functionality. > + * @dev: drm device > + * @encoder: the encoder to initialize > + * @encoder_type: user visible type of the encoder > + * > + * Initialises a preallocated encoder that has no further functionality. > + * Settings for possible CRTC and clones are left to their initial > + * values. The encoder will be cleaned up automatically using a > + * DRM-managed action. > + * > + * The structure containing the encoder's memory should be allocated > + * using drmm_kzalloc(). > + * > + * Returns: > + * Zero on success, error code on failure. > + */ > +int drmm_simple_encoder_init(struct drm_device *dev, > + struct drm_encoder *encoder, > + int encoder_type) > +{ > + int ret; > + > + ret = drm_encoder_init(dev, encoder, NULL, encoder_type, NULL); > + if (ret) > + return ret; > + > + return drmm_add_action_or_reset(dev, drmm_simple_encoder_cleanup, encoder); > +} > +EXPORT_SYMBOL(drmm_simple_encoder_init); > + > void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, > size_t offset, int encoder_type) > { > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h > index 0b3647e614dd..20456f4712f0 100644 > --- a/include/drm/drm_simple_kms_helper.h > +++ b/include/drm/drm_simple_kms_helper.h > @@ -241,6 +241,9 @@ int drm_simple_display_pipe_init(struct drm_device *dev, > int drm_simple_encoder_init(struct drm_device *dev, > struct drm_encoder *encoder, > int encoder_type); > +int drmm_simple_encoder_init(struct drm_device *dev, > + struct drm_encoder *encoder, > + int encoder_type); > > void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, > size_t offset, int encoder_type);
Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > The DRM-managed function to register an encoder is > > drmm_simple_encoder_alloc() and its variants, which will allocate the > > underlying structure and initialisation the encoder. > > > > However, we might want to separate the structure creation and the encoder > > 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 an encoder that would be passed > > as an argument. > > > > There's nothing wrong with this patch, but I have to admit that adding > drm_simple_encoder_init() et al was a mistake. Why do you think it was a mistake? > It would have been better to add an initializer macro like > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ > .destroy = drm_encoder_cleanup > > It's way more flexible and similarly easy to use. Anyway... We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's needed Maxime
Hi Am 20.06.22 um 15:48 schrieb Maxime Ripard: > Hi, > > On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: >> Am 10.06.22 um 11:28 schrieb Maxime Ripard: >>> The DRM-managed function to register an encoder is >>> drmm_simple_encoder_alloc() and its variants, which will allocate the >>> underlying structure and initialisation the encoder. >>> >>> However, we might want to separate the structure creation and the encoder >>> 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 an encoder that would be passed >>> as an argument. >>> >> >> There's nothing wrong with this patch, but I have to admit that adding >> drm_simple_encoder_init() et al was a mistake. > > Why do you think it was a mistake? The simple-encoder stuff is an interface that no one really needs. Compared to open-coding the function, it's barely an improvement in LOCs, but nothing else. Back when I added drm_simple_encoder_init(), I wanted to simplify the many drivers that initialized the encoder with a cleanup callback and nothing else. IIRC it was an improvement back then. But now we already have a related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro back then, we could use the regular encoder helpers. > >> It would have been better to add an initializer macro like >> >> #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ >> .destroy = drm_encoder_cleanup >> >> It's way more flexible and similarly easy to use. Anyway... > > We can still have this. It would simplify this series so I could > definitely squeeze that patch in and add a TODO item / deprecation > notice for simple encoders if you think it's needed Not necessary. It's not super important. Best regards Thomas > > Maxime
On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: > Hi > > Am 20.06.22 um 15:48 schrieb Maxime Ripard: > > Hi, > > > > On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > > > The DRM-managed function to register an encoder is > > > > drmm_simple_encoder_alloc() and its variants, which will allocate the > > > > underlying structure and initialisation the encoder. > > > > > > > > However, we might want to separate the structure creation and the encoder > > > > 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 an encoder that would be passed > > > > as an argument. > > > > > > > > > > There's nothing wrong with this patch, but I have to admit that adding > > > drm_simple_encoder_init() et al was a mistake. > > > > Why do you think it was a mistake? > > The simple-encoder stuff is an interface that no one really needs. Compared > to open-coding the function, it's barely an improvement in LOCs, but nothing > else. > > Back when I added drm_simple_encoder_init(), I wanted to simplify the many > drivers that initialized the encoder with a cleanup callback and nothing > else. IIRC it was an improvement back then. But now we already have a > related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro > back then, we could use the regular encoder helpers. > > > > > > It would have been better to add an initializer macro like > > > > > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ > > > .destroy = drm_encoder_cleanup > > > > > > It's way more flexible and similarly easy to use. Anyway... > > > > We can still have this. It would simplify this series so I could > > definitely squeeze that patch in and add a TODO item / deprecation > > notice for simple encoders if you think it's needed > > Not necessary. It's not super important. The corollary is though :) If I understand you right, it means that you'd rather have a destroy callback everywhere instead of calling the _cleanup function through a drm-managed callback, and let drm_dev_unregister do its job? If so, it means that we shouldn't be following the drmm_.*_alloc functions and should drop all the new ones from this series. Maxime
Hi Am 20.06.22 um 16:39 schrieb Maxime Ripard: > On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 20.06.22 um 15:48 schrieb Maxime Ripard: >>> Hi, >>> >>> On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: >>>> Am 10.06.22 um 11:28 schrieb Maxime Ripard: >>>>> The DRM-managed function to register an encoder is >>>>> drmm_simple_encoder_alloc() and its variants, which will allocate the >>>>> underlying structure and initialisation the encoder. >>>>> >>>>> However, we might want to separate the structure creation and the encoder >>>>> 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 an encoder that would be passed >>>>> as an argument. >>>>> >>>> >>>> There's nothing wrong with this patch, but I have to admit that adding >>>> drm_simple_encoder_init() et al was a mistake. >>> >>> Why do you think it was a mistake? >> >> The simple-encoder stuff is an interface that no one really needs. Compared >> to open-coding the function, it's barely an improvement in LOCs, but nothing >> else. >> >> Back when I added drm_simple_encoder_init(), I wanted to simplify the many >> drivers that initialized the encoder with a cleanup callback and nothing >> else. IIRC it was an improvement back then. But now we already have a >> related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro >> back then, we could use the regular encoder helpers. >> >>> >>>> It would have been better to add an initializer macro like >>>> >>>> #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ >>>> .destroy = drm_encoder_cleanup >>>> >>>> It's way more flexible and similarly easy to use. Anyway... >>> >>> We can still have this. It would simplify this series so I could >>> definitely squeeze that patch in and add a TODO item / deprecation >>> notice for simple encoders if you think it's needed >> >> Not necessary. It's not super important. > > The corollary is though :) > > If I understand you right, it means that you'd rather have a destroy > callback everywhere instead of calling the _cleanup function through a > drm-managed callback, and let drm_dev_unregister do its job? > > If so, it means that we shouldn't be following the drmm_.*_alloc > functions and should drop all the new ones from this series. No, no. What I'm saying is that simple-encoder is a pointless mid-layer. There's nothing that couldn't easily be achieved with the regular encoder functions. Best regards Thomas > > Maxime
Hi Am 20.06.22 um 16:45 schrieb Thomas Zimmermann: > Hi > > Am 20.06.22 um 16:39 schrieb Maxime Ripard: >> On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 20.06.22 um 15:48 schrieb Maxime Ripard: >>>> Hi, >>>> >>>> On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: >>>>> Am 10.06.22 um 11:28 schrieb Maxime Ripard: >>>>>> The DRM-managed function to register an encoder is >>>>>> drmm_simple_encoder_alloc() and its variants, which will allocate the >>>>>> underlying structure and initialisation the encoder. >>>>>> >>>>>> However, we might want to separate the structure creation and the >>>>>> encoder >>>>>> 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 an encoder that would be >>>>>> passed >>>>>> as an argument. >>>>>> >>>>> >>>>> There's nothing wrong with this patch, but I have to admit that adding >>>>> drm_simple_encoder_init() et al was a mistake. >>>> >>>> Why do you think it was a mistake? >>> >>> The simple-encoder stuff is an interface that no one really needs. >>> Compared >>> to open-coding the function, it's barely an improvement in LOCs, but >>> nothing >>> else. >>> >>> Back when I added drm_simple_encoder_init(), I wanted to simplify the >>> many >>> drivers that initialized the encoder with a cleanup callback and nothing >>> else. IIRC it was an improvement back then. But now we already have a >>> related drmm_ helper and a drmm_alloc_ helper. If I had only added >>> the macro >>> back then, we could use the regular encoder helpers. >>> >>>> >>>>> It would have been better to add an initializer macro like >>>>> >>>>> #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ >>>>> .destroy = drm_encoder_cleanup >>>>> >>>>> It's way more flexible and similarly easy to use. Anyway... >>>> >>>> We can still have this. It would simplify this series so I could >>>> definitely squeeze that patch in and add a TODO item / deprecation >>>> notice for simple encoders if you think it's needed >>> >>> Not necessary. It's not super important. >> >> The corollary is though :) >> >> If I understand you right, it means that you'd rather have a destroy >> callback everywhere instead of calling the _cleanup function through a >> drm-managed callback, and let drm_dev_unregister do its job? >> >> If so, it means that we shouldn't be following the drmm_.*_alloc >> functions and should drop all the new ones from this series. > > No, no. What I'm saying is that simple-encoder is a pointless mid-layer. > There's nothing that couldn't easily be achieved with the regular > encoder functions. I guess that if you want to change something here, you could add drmm_encoder_init() instead and convert vc4. That function might be more useful for other drivers in the long run. Best regards Thomas > > Best regards > Thomas > >> >> Maxime >
On Mon, Jun 20, 2022 at 09:04:11PM +0200, Thomas Zimmermann wrote: > Hi > > Am 20.06.22 um 16:45 schrieb Thomas Zimmermann: > > Hi > > > > Am 20.06.22 um 16:39 schrieb Maxime Ripard: > > > On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: > > > > Hi > > > > > > > > Am 20.06.22 um 15:48 schrieb Maxime Ripard: > > > > > Hi, > > > > > > > > > > On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: > > > > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > > > > > > The DRM-managed function to register an encoder is > > > > > > > drmm_simple_encoder_alloc() and its variants, which will allocate the > > > > > > > underlying structure and initialisation the encoder. > > > > > > > > > > > > > > However, we might want to separate the structure > > > > > > > creation and the encoder > > > > > > > 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 an encoder > > > > > > > that would be passed > > > > > > > as an argument. > > > > > > > > > > > > > > > > > > > There's nothing wrong with this patch, but I have to admit that adding > > > > > > drm_simple_encoder_init() et al was a mistake. > > > > > > > > > > Why do you think it was a mistake? > > > > > > > > The simple-encoder stuff is an interface that no one really > > > > needs. Compared > > > > to open-coding the function, it's barely an improvement in LOCs, > > > > but nothing > > > > else. > > > > > > > > Back when I added drm_simple_encoder_init(), I wanted to > > > > simplify the many > > > > drivers that initialized the encoder with a cleanup callback and nothing > > > > else. IIRC it was an improvement back then. But now we already have a > > > > related drmm_ helper and a drmm_alloc_ helper. If I had only > > > > added the macro > > > > back then, we could use the regular encoder helpers. > > > > > > > > > > > > > > > It would have been better to add an initializer macro like > > > > > > > > > > > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ > > > > > > .destroy = drm_encoder_cleanup > > > > > > > > > > > > It's way more flexible and similarly easy to use. Anyway... > > > > > > > > > > We can still have this. It would simplify this series so I could > > > > > definitely squeeze that patch in and add a TODO item / deprecation > > > > > notice for simple encoders if you think it's needed > > > > > > > > Not necessary. It's not super important. > > > > > > The corollary is though :) > > > > > > If I understand you right, it means that you'd rather have a destroy > > > callback everywhere instead of calling the _cleanup function through a > > > drm-managed callback, and let drm_dev_unregister do its job? > > > > > > If so, it means that we shouldn't be following the drmm_.*_alloc > > > functions and should drop all the new ones from this series. > > > > No, no. What I'm saying is that simple-encoder is a pointless mid-layer. > > There's nothing that couldn't easily be achieved with the regular > > encoder functions. > > I guess that if you want to change something here, you could add > drmm_encoder_init() instead and convert vc4. That function might be more > useful for other drivers in the long run. I already had that patch in the series. I've dropped this one and converted everyone to a !simple encoder. Thanks! maxime
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 72989ed1baba..876870dd98e5 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -58,9 +58,10 @@ static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = { * stored in the device structure. Free the encoder's memory as part of * the device release function. * - * Note: consider using drmm_simple_encoder_alloc() instead of - * drm_simple_encoder_init() to let the DRM managed resource infrastructure - * take care of cleanup and deallocation. + * Note: consider using drmm_simple_encoder_alloc() or + * drmm_simple_encoder_init() instead of drm_simple_encoder_init() to + * let the DRM managed resource infrastructure take care of cleanup and + * deallocation. * * Returns: * Zero on success, error code on failure. @@ -75,6 +76,45 @@ int drm_simple_encoder_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_simple_encoder_init); +static void drmm_simple_encoder_cleanup(struct drm_device *dev, void *ptr) +{ + struct drm_encoder *encoder = ptr; + + drm_encoder_cleanup(encoder); +} + +/** + * drmm_simple_encoder_init - Initialize a preallocated encoder with + * basic functionality. + * @dev: drm device + * @encoder: the encoder to initialize + * @encoder_type: user visible type of the encoder + * + * Initialises a preallocated encoder that has no further functionality. + * Settings for possible CRTC and clones are left to their initial + * values. The encoder will be cleaned up automatically using a + * DRM-managed action. + * + * The structure containing the encoder's memory should be allocated + * using drmm_kzalloc(). + * + * Returns: + * Zero on success, error code on failure. + */ +int drmm_simple_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + int encoder_type) +{ + int ret; + + ret = drm_encoder_init(dev, encoder, NULL, encoder_type, NULL); + if (ret) + return ret; + + return drmm_add_action_or_reset(dev, drmm_simple_encoder_cleanup, encoder); +} +EXPORT_SYMBOL(drmm_simple_encoder_init); + void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, size_t offset, int encoder_type) { diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index 0b3647e614dd..20456f4712f0 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -241,6 +241,9 @@ int drm_simple_display_pipe_init(struct drm_device *dev, int drm_simple_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, int encoder_type); +int drmm_simple_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + int encoder_type); void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, size_t offset, int encoder_type);
The DRM-managed function to register an encoder is drmm_simple_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder. However, we might want to separate the structure creation and the encoder 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 an encoder that would be passed as an argument. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/drm_simple_kms_helper.c | 46 +++++++++++++++++++++++-- include/drm/drm_simple_kms_helper.h | 3 ++ 2 files changed, 46 insertions(+), 3 deletions(-)