| Message ID | 20220610092924.754942-9-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: > Let's create a DRM-managed variant of drmm_writeback_connector_init that > will take care of an action of the encoder and connector cleanup. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/drm_writeback.c | 136 ++++++++++++++++++++++++-------- > include/drm/drm_writeback.h | 5 ++ > 2 files changed, 108 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index dccf4504f1bb..0b00921f3379 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -149,32 +149,27 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { > .destroy = drm_encoder_cleanup, > }; > > -/** > - * drm_writeback_connector_init - Initialize a writeback connector and its properties > - * @dev: DRM device > - * @wb_connector: Writeback connector to initialize > - * @con_funcs: Connector funcs vtable > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder > - * @formats: Array of supported pixel formats for the writeback engine > - * @n_formats: Length of the formats array > - * > - * This function creates the writeback-connector-specific properties if they > - * have not been already created, initializes the connector as > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property > - * values. It will also create an internal encoder associated with the > - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for > - * the encoder helper. > - * > - * Drivers should always use this function instead of drm_connector_init() to > - * set up writeback connectors. > - * > - * Returns: 0 on success, or a negative error code > - */ > -int drm_writeback_connector_init(struct drm_device *dev, > - struct drm_writeback_connector *wb_connector, > - const struct drm_connector_funcs *con_funcs, > - const struct drm_encoder_helper_funcs *enc_helper_funcs, > - const u32 *formats, int n_formats) > +typedef int (*encoder_init_t)(struct drm_device *dev, > + struct drm_encoder *encoder, > + const struct drm_encoder_funcs *funcs, > + int encoder_type, > + const char *name, ...); > + > +typedef int (*connector_init_t)(struct drm_device *dev, > + struct drm_connector *connector, > + const struct drm_connector_funcs *funcs, > + int connector_type); This code has menawhile changed in drm-tip, which makes it harder to give a good review for such refactoring. But generally, I'd do the connector and encoder initialization in drmm_writeback_connector_init() and give the initialized encoders to an internal helper that does the common init steps. That avoids such indirections with functions pointers. Best regards Thomas > + > +static int writeback_init(struct drm_device *dev, > + struct drm_writeback_connector *wb_connector, > + connector_init_t conn_init, > + void (*conn_destroy)(struct drm_connector *connector), > + encoder_init_t enc_init, > + void (*enc_destroy)(struct drm_encoder *encoder), > + const struct drm_connector_funcs *con_funcs, > + const struct drm_encoder_funcs *enc_funcs, > + const struct drm_encoder_helper_funcs *enc_helper_funcs, > + const u32 *formats, int n_formats) > { > struct drm_property_blob *blob; > struct drm_connector *connector = &wb_connector->base; > @@ -190,16 +185,16 @@ int drm_writeback_connector_init(struct drm_device *dev, > return PTR_ERR(blob); > > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > - ret = drm_encoder_init(dev, &wb_connector->encoder, > - &drm_writeback_encoder_funcs, > - DRM_MODE_ENCODER_VIRTUAL, NULL); > + ret = enc_init(dev, &wb_connector->encoder, > + enc_funcs, > + DRM_MODE_ENCODER_VIRTUAL, NULL); > if (ret) > goto fail; > > connector->interlace_allowed = 0; > > - ret = drm_connector_init(dev, connector, con_funcs, > - DRM_MODE_CONNECTOR_WRITEBACK); > + ret = conn_init(dev, connector, con_funcs, > + DRM_MODE_CONNECTOR_WRITEBACK); > if (ret) > goto connector_fail; > > @@ -231,15 +226,90 @@ int drm_writeback_connector_init(struct drm_device *dev, > return 0; > > attach_fail: > - drm_connector_cleanup(connector); > + if (conn_destroy) > + conn_destroy(connector); > connector_fail: > - drm_encoder_cleanup(&wb_connector->encoder); > + if (enc_destroy) > + enc_destroy(&wb_connector->encoder); > fail: > drm_property_blob_put(blob); > return ret; > } > + > +/** > + * drm_writeback_connector_init - Initialize a writeback connector and its properties > + * @dev: DRM device > + * @wb_connector: Writeback connector to initialize > + * @con_funcs: Connector funcs vtable > + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder > + * @formats: Array of supported pixel formats for the writeback engine > + * @n_formats: Length of the formats array > + * > + * This function creates the writeback-connector-specific properties if they > + * have not been already created, initializes the connector as > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property > + * values. It will also create an internal encoder associated with the > + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for > + * the encoder helper. > + * > + * Drivers should always use this function instead of drm_connector_init() to > + * set up writeback connectors. > + * > + * Returns: 0 on success, or a negative error code > + */ > +int drm_writeback_connector_init(struct drm_device *dev, > + struct drm_writeback_connector *wb_connector, > + const struct drm_connector_funcs *con_funcs, > + const struct drm_encoder_helper_funcs *enc_helper_funcs, > + const u32 *formats, int n_formats) > +{ > + return writeback_init(dev, wb_connector, > + drm_connector_init, drm_connector_cleanup, > + drm_encoder_init, drm_encoder_cleanup, > + con_funcs, > + &drm_writeback_encoder_funcs, enc_helper_funcs, > + formats, n_formats); > +} > EXPORT_SYMBOL(drm_writeback_connector_init); > > +/** > + * drmm_writeback_connector_init - Initialize a writeback connector and its properties > + * @dev: DRM device > + * @wb_connector: Writeback connector to initialize > + * @con_funcs: Connector funcs vtable > + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder > + * @formats: Array of supported pixel formats for the writeback engine > + * @n_formats: Length of the formats array > + * > + * This function creates the writeback-connector-specific properties if they > + * have not been already created, initializes the connector as > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property > + * values. It will also create an internal encoder associated with the > + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for > + * the encoder helper. > + * > + * The writeback connector is DRM-managed and won't need any cleanup. > + * > + * Drivers should always use this function instead of drm_connector_init() to > + * set up writeback connectors. > + * > + * Returns: 0 on success, or a negative error code > + */ > +int drmm_writeback_connector_init(struct drm_device *dev, > + struct drm_writeback_connector *wb_connector, > + const struct drm_connector_funcs *con_funcs, > + const struct drm_encoder_helper_funcs *enc_helper_funcs, > + const u32 *formats, int n_formats) > +{ > + return writeback_init(dev, wb_connector, > + drmm_connector_init, NULL, > + drmm_encoder_init, NULL, > + con_funcs, > + NULL, enc_helper_funcs, > + formats, n_formats); > +} > +EXPORT_SYMBOL(drmm_writeback_connector_init); > + > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > struct drm_framebuffer *fb) > { > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > index 9697d2714d2a..cc259ae44734 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -151,6 +151,11 @@ int drm_writeback_connector_init(struct drm_device *dev, > const struct drm_connector_funcs *con_funcs, > const struct drm_encoder_helper_funcs *enc_helper_funcs, > const u32 *formats, int n_formats); > +int drmm_writeback_connector_init(struct drm_device *dev, > + struct drm_writeback_connector *wb_connector, > + const struct drm_connector_funcs *con_funcs, > + const struct drm_encoder_helper_funcs *enc_helper_funcs, > + const u32 *formats, int n_formats); > > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504f1bb..0b00921f3379 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -149,32 +149,27 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = { .destroy = drm_encoder_cleanup, }; -/** - * drm_writeback_connector_init - Initialize a writeback connector and its properties - * @dev: DRM device - * @wb_connector: Writeback connector to initialize - * @con_funcs: Connector funcs vtable - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder - * @formats: Array of supported pixel formats for the writeback engine - * @n_formats: Length of the formats array - * - * This function creates the writeback-connector-specific properties if they - * have not been already created, initializes the connector as - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property - * values. It will also create an internal encoder associated with the - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for - * the encoder helper. - * - * Drivers should always use this function instead of drm_connector_init() to - * set up writeback connectors. - * - * Returns: 0 on success, or a negative error code - */ -int drm_writeback_connector_init(struct drm_device *dev, - struct drm_writeback_connector *wb_connector, - const struct drm_connector_funcs *con_funcs, - const struct drm_encoder_helper_funcs *enc_helper_funcs, - const u32 *formats, int n_formats) +typedef int (*encoder_init_t)(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type, + const char *name, ...); + +typedef int (*connector_init_t)(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type); + +static int writeback_init(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + connector_init_t conn_init, + void (*conn_destroy)(struct drm_connector *connector), + encoder_init_t enc_init, + void (*enc_destroy)(struct drm_encoder *encoder), + const struct drm_connector_funcs *con_funcs, + const struct drm_encoder_funcs *enc_funcs, + const struct drm_encoder_helper_funcs *enc_helper_funcs, + const u32 *formats, int n_formats) { struct drm_property_blob *blob; struct drm_connector *connector = &wb_connector->base; @@ -190,16 +185,16 @@ int drm_writeback_connector_init(struct drm_device *dev, return PTR_ERR(blob); drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); - ret = drm_encoder_init(dev, &wb_connector->encoder, - &drm_writeback_encoder_funcs, - DRM_MODE_ENCODER_VIRTUAL, NULL); + ret = enc_init(dev, &wb_connector->encoder, + enc_funcs, + DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) goto fail; connector->interlace_allowed = 0; - ret = drm_connector_init(dev, connector, con_funcs, - DRM_MODE_CONNECTOR_WRITEBACK); + ret = conn_init(dev, connector, con_funcs, + DRM_MODE_CONNECTOR_WRITEBACK); if (ret) goto connector_fail; @@ -231,15 +226,90 @@ int drm_writeback_connector_init(struct drm_device *dev, return 0; attach_fail: - drm_connector_cleanup(connector); + if (conn_destroy) + conn_destroy(connector); connector_fail: - drm_encoder_cleanup(&wb_connector->encoder); + if (enc_destroy) + enc_destroy(&wb_connector->encoder); fail: drm_property_blob_put(blob); return ret; } + +/** + * drm_writeback_connector_init - Initialize a writeback connector and its properties + * @dev: DRM device + * @wb_connector: Writeback connector to initialize + * @con_funcs: Connector funcs vtable + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder + * @formats: Array of supported pixel formats for the writeback engine + * @n_formats: Length of the formats array + * + * This function creates the writeback-connector-specific properties if they + * have not been already created, initializes the connector as + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property + * values. It will also create an internal encoder associated with the + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for + * the encoder helper. + * + * Drivers should always use this function instead of drm_connector_init() to + * set up writeback connectors. + * + * Returns: 0 on success, or a negative error code + */ +int drm_writeback_connector_init(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, + const struct drm_encoder_helper_funcs *enc_helper_funcs, + const u32 *formats, int n_formats) +{ + return writeback_init(dev, wb_connector, + drm_connector_init, drm_connector_cleanup, + drm_encoder_init, drm_encoder_cleanup, + con_funcs, + &drm_writeback_encoder_funcs, enc_helper_funcs, + formats, n_formats); +} EXPORT_SYMBOL(drm_writeback_connector_init); +/** + * drmm_writeback_connector_init - Initialize a writeback connector and its properties + * @dev: DRM device + * @wb_connector: Writeback connector to initialize + * @con_funcs: Connector funcs vtable + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder + * @formats: Array of supported pixel formats for the writeback engine + * @n_formats: Length of the formats array + * + * This function creates the writeback-connector-specific properties if they + * have not been already created, initializes the connector as + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property + * values. It will also create an internal encoder associated with the + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for + * the encoder helper. + * + * The writeback connector is DRM-managed and won't need any cleanup. + * + * Drivers should always use this function instead of drm_connector_init() to + * set up writeback connectors. + * + * Returns: 0 on success, or a negative error code + */ +int drmm_writeback_connector_init(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, + const struct drm_encoder_helper_funcs *enc_helper_funcs, + const u32 *formats, int n_formats) +{ + return writeback_init(dev, wb_connector, + drmm_connector_init, NULL, + drmm_encoder_init, NULL, + con_funcs, + NULL, enc_helper_funcs, + formats, n_formats); +} +EXPORT_SYMBOL(drmm_writeback_connector_init); + int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) { diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d2714d2a..cc259ae44734 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -151,6 +151,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_connector_funcs *con_funcs, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats); +int drmm_writeback_connector_init(struct drm_device *dev, + struct drm_writeback_connector *wb_connector, + const struct drm_connector_funcs *con_funcs, + const struct drm_encoder_helper_funcs *enc_helper_funcs, + const u32 *formats, int n_formats); int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);
Let's create a DRM-managed variant of drmm_writeback_connector_init that will take care of an action of the encoder and connector cleanup. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/drm_writeback.c | 136 ++++++++++++++++++++++++-------- include/drm/drm_writeback.h | 5 ++ 2 files changed, 108 insertions(+), 33 deletions(-)