Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> 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 <louis.chauvet@bootlin.com>
>> > ---
>> >  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.

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.

BR,
Jani.


>
> Louis Chauvet
>  
>> Maxime
>
>

-- 
Jani Nikula, Intel