| Message ID | 20200911135724.25833-2-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/7] drm: add drmm_encoder_alloc() | expand |
Hi Philipp, Thank you for the patch. On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote: > Add an alternative to drm_simple_encoder_init() that allocates and > initializes a simple encoder and registers drm_encoder_cleanup() with > drmm_add_action_or_reset(). > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++ > include/drm/drm_simple_kms_helper.h | 24 ++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > index 74946690aba4..3cbbfb0f9b51 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -9,6 +9,7 @@ > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > +#include <drm/drm_managed.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_simple_encoder_init); > > +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { }; > + > +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, > + size_t offset, int encoder_type) > +{ > + return __drmm_encoder_alloc(dev, size, offset, > + &drmm_simple_encoder_funcs_empty, > + encoder_type, NULL); > +} > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc); Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs in the core (if we don't already) and use drmm_encoder_alloc() directly in drivers ? > + > static enum drm_mode_status > drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, > const struct drm_display_mode *mode) > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h > index a026375464ff..e6dbf3161c2f 100644 > --- a/include/drm/drm_simple_kms_helper.h > +++ b/include/drm/drm_simple_kms_helper.h > @@ -185,4 +185,28 @@ int drm_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); > + > +/** > + * drmm_simple_encoder_alloc - Allocate and initialize an encoder with basic > + * functionality. > + * @dev: drm device > + * @type: the type of the struct which contains struct &drm_encoder > + * @member: the name of the &drm_encoder within @type. > + * @encoder_type: user visible type of the encoder > + * > + * Allocates and initializes an encoder that has no further functionality. > + * Settings for possible CRTC and clones are left to their initial values. > + * Cleanup is automatically handled through registering drm_encoder_cleanup() > + * with drmm_add_action(). > + * > + * Returns: > + * Pointer to new encoder, or ERR_PTR on failure. > + */ > +#define drmm_simple_encoder_alloc(dev, type, member, encoder_type) \ > + ((type *)__drmm_simple_encoder_alloc(dev, sizeof(type), \ > + offsetof(type, member), \ > + encoder_type)) > + > #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
Hi Laurent, On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote: > Hi Philipp, > > Thank you for the patch. > > On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote: > > Add an alternative to drm_simple_encoder_init() that allocates and > > initializes a simple encoder and registers drm_encoder_cleanup() with > > drmm_add_action_or_reset(). > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++ > > include/drm/drm_simple_kms_helper.h | 24 ++++++++++++++++++++++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > > index 74946690aba4..3cbbfb0f9b51 100644 > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > > @@ -9,6 +9,7 @@ > > #include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_plane_helper.h> > > #include <drm/drm_probe_helper.h> > > #include <drm/drm_simple_kms_helper.h> > > @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_simple_encoder_init); > > > > +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { }; > > + > > +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, > > + size_t offset, int encoder_type) > > +{ > > + return __drmm_encoder_alloc(dev, size, offset, > > + &drmm_simple_encoder_funcs_empty, > > + encoder_type, NULL); > > +} > > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc); > > Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs > in the core (if we don't already) and use drmm_encoder_alloc() directly > in drivers ? I could change it to remove the empty drm_encoder_funcs, and prepend something like this: ----------8<---------- From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001 From: Philipp Zabel <p.zabel@pengutronix.de> Date: Fri, 4 Dec 2020 10:49:41 +0100 Subject: [PATCH] drm/encoder: make encoder control functions optional Simple managed encoders do not require the .destroy callback, make the whole funcs structure optional. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/drm_encoder.c | 4 ++-- drivers/gpu/drm/drm_mode_config.c | 5 +++-- include/drm/drm_encoder.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index e555281f43d4..b0b86a3c08f5 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev) int ret = 0; drm_for_each_encoder(encoder, dev) { - if (encoder->funcs->late_register) + if (encoder->funcs && encoder->funcs->late_register) ret = encoder->funcs->late_register(encoder); if (ret) return ret; @@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev) struct drm_encoder *encoder; drm_for_each_encoder(encoder, dev) { - if (encoder->funcs->early_unregister) + if (encoder->funcs && encoder->funcs->early_unregister) encoder->funcs->early_unregister(encoder); } } diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..87e144155456 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev) crtc->funcs->reset(crtc); drm_for_each_encoder(encoder, dev) - if (encoder->funcs->reset) + if (encoder->funcs && encoder->funcs->reset) encoder->funcs->reset(encoder); drm_connector_list_iter_begin(dev, &conn_iter); @@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev) list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list, head) { - encoder->funcs->destroy(encoder); + if (encoder->funcs) + encoder->funcs->destroy(encoder); } drm_connector_list_iter_begin(dev, &conn_iter); diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 5dfa5f7a80a7..833123637fbf 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -89,7 +89,7 @@ struct drm_encoder_funcs { * @head: list management * @base: base KMS object * @name: human readable name, can be overwritten by the driver - * @funcs: control functions + * @funcs: control functions, can be NULL for simple managed encoders * @helper_private: mid-layer private data * * CRTCs drive pixels to encoders, which convert them into signals
Hi Philipp, On Fri, Dec 04, 2020 at 11:13:33AM +0100, Philipp Zabel wrote: > On Fri, 2020-12-04 at 11:19 +0200, Laurent Pinchart wrote: > > On Fri, Sep 11, 2020 at 03:57:19PM +0200, Philipp Zabel wrote: > > > Add an alternative to drm_simple_encoder_init() that allocates and > > > initializes a simple encoder and registers drm_encoder_cleanup() with > > > drmm_add_action_or_reset(). > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++ > > > include/drm/drm_simple_kms_helper.h | 24 ++++++++++++++++++++++++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > > > index 74946690aba4..3cbbfb0f9b51 100644 > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > > > @@ -9,6 +9,7 @@ > > > #include <drm/drm_atomic.h> > > > #include <drm/drm_atomic_helper.h> > > > #include <drm/drm_bridge.h> > > > +#include <drm/drm_managed.h> > > > #include <drm/drm_plane_helper.h> > > > #include <drm/drm_probe_helper.h> > > > #include <drm/drm_simple_kms_helper.h> > > > @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev, > > > } > > > EXPORT_SYMBOL(drm_simple_encoder_init); > > > > > > +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { }; > > > + > > > +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, > > > + size_t offset, int encoder_type) > > > +{ > > > + return __drmm_encoder_alloc(dev, size, offset, > > > + &drmm_simple_encoder_funcs_empty, > > > + encoder_type, NULL); > > > +} > > > +EXPORT_SYMBOL(__drmm_simple_encoder_alloc); > > > > Do we need this ? Wouldn't it be better support a NULL drm_encoder_funcs > > in the core (if we don't already) and use drmm_encoder_alloc() directly > > in drivers ? > > I could change it to remove the empty drm_encoder_funcs, and prepend > something like this: > > ----------8<---------- > From 12147d90a8ae48dabc16ca8750fa0f629cc46570 Mon Sep 17 00:00:00 2001 > From: Philipp Zabel <p.zabel@pengutronix.de> > Date: Fri, 4 Dec 2020 10:49:41 +0100 > Subject: [PATCH] drm/encoder: make encoder control functions optional > > Simple managed encoders do not require the .destroy callback, > make the whole funcs structure optional. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> That's perfect. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/drm_encoder.c | 4 ++-- > drivers/gpu/drm/drm_mode_config.c | 5 +++-- > include/drm/drm_encoder.h | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > index e555281f43d4..b0b86a3c08f5 100644 > --- a/drivers/gpu/drm/drm_encoder.c > +++ b/drivers/gpu/drm/drm_encoder.c > @@ -72,7 +72,7 @@ int drm_encoder_register_all(struct drm_device *dev) > int ret = 0; > > drm_for_each_encoder(encoder, dev) { > - if (encoder->funcs->late_register) > + if (encoder->funcs && encoder->funcs->late_register) > ret = encoder->funcs->late_register(encoder); > if (ret) > return ret; > @@ -86,7 +86,7 @@ void drm_encoder_unregister_all(struct drm_device *dev) > struct drm_encoder *encoder; > > drm_for_each_encoder(encoder, dev) { > - if (encoder->funcs->early_unregister) > + if (encoder->funcs && encoder->funcs->early_unregister) > encoder->funcs->early_unregister(encoder); > } > } > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index f1affc1bb679..87e144155456 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -195,7 +195,7 @@ void drm_mode_config_reset(struct drm_device *dev) > crtc->funcs->reset(crtc); > > drm_for_each_encoder(encoder, dev) > - if (encoder->funcs->reset) > + if (encoder->funcs && encoder->funcs->reset) > encoder->funcs->reset(encoder); > > drm_connector_list_iter_begin(dev, &conn_iter); > @@ -487,7 +487,8 @@ void drm_mode_config_cleanup(struct drm_device *dev) > > list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list, > head) { > - encoder->funcs->destroy(encoder); > + if (encoder->funcs) > + encoder->funcs->destroy(encoder); > } > > drm_connector_list_iter_begin(dev, &conn_iter); > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > index 5dfa5f7a80a7..833123637fbf 100644 > --- a/include/drm/drm_encoder.h > +++ b/include/drm/drm_encoder.h > @@ -89,7 +89,7 @@ struct drm_encoder_funcs { > * @head: list management > * @base: base KMS object > * @name: human readable name, can be overwritten by the driver > - * @funcs: control functions > + * @funcs: control functions, can be NULL for simple managed encoders > * @helper_private: mid-layer private data > * > * CRTCs drive pixels to encoders, which convert them into signals
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 74946690aba4..3cbbfb0f9b51 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -9,6 +9,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -71,6 +72,17 @@ int drm_simple_encoder_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_simple_encoder_init); +static const struct drm_encoder_funcs drmm_simple_encoder_funcs_empty = { }; + +void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size, + size_t offset, int encoder_type) +{ + return __drmm_encoder_alloc(dev, size, offset, + &drmm_simple_encoder_funcs_empty, + encoder_type, NULL); +} +EXPORT_SYMBOL(__drmm_simple_encoder_alloc); + static enum drm_mode_status drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index a026375464ff..e6dbf3161c2f 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -185,4 +185,28 @@ int drm_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); + +/** + * drmm_simple_encoder_alloc - Allocate and initialize an encoder with basic + * functionality. + * @dev: drm device + * @type: the type of the struct which contains struct &drm_encoder + * @member: the name of the &drm_encoder within @type. + * @encoder_type: user visible type of the encoder + * + * Allocates and initializes an encoder that has no further functionality. + * Settings for possible CRTC and clones are left to their initial values. + * Cleanup is automatically handled through registering drm_encoder_cleanup() + * with drmm_add_action(). + * + * Returns: + * Pointer to new encoder, or ERR_PTR on failure. + */ +#define drmm_simple_encoder_alloc(dev, type, member, encoder_type) \ + ((type *)__drmm_simple_encoder_alloc(dev, sizeof(type), \ + offsetof(type, member), \ + encoder_type)) + #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
Add an alternative to drm_simple_encoder_init() that allocates and initializes a simple encoder and registers drm_encoder_cleanup() with drmm_add_action_or_reset(). Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++++ include/drm/drm_simple_kms_helper.h | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+)