Re: [PATCH v4 4/5] drm: writeback: Introduce drm managed helpers

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

Hi,

On Thu, Oct 10, 2024 at 07:39:06PM +0200, 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. By using
> drm managed variants, we can ensure that the writeback connector is
> properly cleaned.
> 
> This patch introduce drmm_writeback_connector_init, an helper to initialize
> a writeback connector using drm managed helpers. This function allows the
> caller to use its own encoder.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  drivers/gpu/drm/drm_connector.c |   4 +
>  drivers/gpu/drm/drm_writeback.c | 224 ++++++++++++++++++++++++++++++++++------
>  include/drm/drm_writeback.h     |  10 ++
>  3 files changed, 208 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849..fe4c1967860a 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -613,6 +613,7 @@ static void drm_mode_remove(struct drm_connector *connector,
>  	drm_mode_destroy(connector->dev, mode);
>  }
>  
> +void drm_writeback_connector_cleanup(struct drm_device *dev, void *data);
>  /**
>   * drm_connector_cleanup - cleans up an initialised connector
>   * @connector: connector to cleanup
> @@ -631,6 +632,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		    DRM_CONNECTOR_REGISTERED))
>  		drm_connector_unregister(connector);
>  
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> +		drm_writeback_connector_cleanup(dev, connector);
> +

So I think it should live in its own patch.

You're doing multiple things here. There's a) the bug that writeback
connectors aren't built properly, b) the discussion about how it's best to
clean it up, and c) how to make every driver clean up properly.

AFAIU, you're trying to address a and c here.

I think putting that call in drm_connector_cleanup is backward compared
to the pattern we're using in the rest of DRM.

drm_connector_cleanup should just clean what was allocated by
drm_connector_init, and that's it.

So we should create a drm_writeback_connector_cleanup function to
address a). That should be your first patch.

Now, it would indeed be best if drm_writeback_connector_cleanup didn't
need to be called at all. That's the second part of your patch, and
should be in its own patch as well. It would address b).

And finally, addressing c will require some driver changes, to either a
call to drmm_writeback_connector_init_* or by using
drm_writeback_connector_cleanup, but we'll have to make that change in
every driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]