On Wed, Aug 28, 2024 at 11:07:37AM GMT, Louis Chauvet wrote: > Le 28/08/24 - 11:35, Jani Nikula a écrit : > > On Tue, 27 Aug 2024, Louis Chauvet <[email protected]> wrote: > > > Le 27/08/24 - 16:33, Maxime Ripard a écrit : > > >> Hi, > > >> > > >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: > > >> > Currently drm_writeback_connector are created by > > >> > drm_writeback_connector_init or > > >> > drm_writeback_connector_init_with_encoder. > > >> > Both of the function uses drm_connector_init and drm_encoder_init, but > > >> > there is no way to properly clean those structure from outside. > > >> > > > >> > This patch introduce the new function drm_writeback_connector_cleanup > > >> > to > > >> > allow a proper cleanup. > > >> > > > >> > Signed-off-by: Louis Chauvet <[email protected]> > > >> > --- > > >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ > > >> > include/drm/drm_writeback.h | 11 +++++++++++ > > >> > 2 files changed, 21 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/drm_writeback.c > > >> > b/drivers/gpu/drm/drm_writeback.c > > >> > index a031c335bdb9..505a4eb40f93 100644 > > >> > --- a/drivers/gpu/drm/drm_writeback.c > > >> > +++ b/drivers/gpu/drm/drm_writeback.c > > >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device > > >> > *dev, > > >> > drm_encoder_helper_add(&wb_connector->encoder, > > >> > enc_helper_funcs); > > >> > > > >> > wb_connector->encoder.possible_crtcs = possible_crtcs; > > >> > + wb_connector->managed_encoder = true; > > >> > > > >> > ret = drm_encoder_init(dev, &wb_connector->encoder, > > >> > &drm_writeback_encoder_funcs, > > >> > @@ -290,6 +291,15 @@ int > > >> > drm_writeback_connector_init_with_encoder(struct drm_device *dev, > > >> > } > > >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > > >> > > > >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector > > >> > *wb_connector) > > >> > +{ > > >> > + drm_connector_cleanup(&wb_connector->base); > > >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); > > >> > + if (wb_connector->managed_encoder) > > >> > + drm_encoder_cleanup(&wb_connector->encoder); > > >> > +} > > >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); > > >> > + > > >> > 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 17e576c80169..e651c0c0c84c 100644 > > >> > --- a/include/drm/drm_writeback.h > > >> > +++ b/include/drm/drm_writeback.h > > >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector { > > >> > */ > > >> > struct drm_encoder encoder; > > >> > > > >> > + /** > > >> > + * @managed_encoder: Sets to true if @encoder was created by > > >> > drm_writeback_connector_init() > > >> > + * > > >> > + * If the user used drm_writeback_connector_init_with_encoder() > > >> > to create the connector, > > >> > + * @encoder is not valid and not managed by > > >> > drm_writeback_connector. This fields allows > > >> > + * the drm_writeback_cleanup() function to properly destroy the > > >> > encoder if needed. > > >> > + */ > > >> > + bool managed_encoder; > > >> > + > > >> > > >> I think we should rather create drmm_writeback_connector variants, > > >> and make both deprecated in favor of these new functions. > > > > > > Hi, > > > > > > I can try to do it. If I understand correctly, you want to create two > > > functions like this? > > > > > > int drmm_writeback_connector_init([...]) { > > > /* drmm and alloc as we want to let drm core to manage this > > > encoder, no need to store it in drm_writeback_connector > > > */ > > > enc = drmm_plain_encoder_alloc(...); > > > > > > return drmm_writeback_connector_init_with_encoder([...], enc); > > > } > > > > > > int drmm_writeback_connector_init_with_encoder([...], enc) { > > > con = drmm_connector_init([...]); > > > > > > drm_connector_attach_encoder(enc, con); > > > > > > /* Needed for pixel_formats_blob_ptr, base is already > > > managed by drmm_connector_init. Maybe cleaning > > > job_queue is also needed? */ > > > drmm_add_action_or_reset([...], > > > &drm_writeback_connector_cleanup) > > > } > > > > Why add two variants, when you can have one and pass NULL for encoder? > > We have the _init_with_encoder variant only because nobody bothered to > > clean up existing call sites. > > I just followed the existing code, but yes, I can make only one function > and create the encoder if the pointer is NULL. > > > Side note, I'd still like to be able to pass driver's own allocated > > connector instead of having writeback midlayer force it on you. > > I just checked, it seems non-trivial to make this change and I don't feel > confident to change this much the drm core.
It's a great opportunity to add unit tests then :) Maxime
signature.asc
Description: PGP signature
