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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241024-slim-onyx-emu-3e4869@houat>
Date: Thu, 24 Oct 2024 10:19:56 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>, 
	Melissa Wen <melissa.srw@...il.com>, Maíra Canal <mairacanal@...eup.net>, 
	Haneen Mohammed <hamohammed.sa@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Simona Vetter <simona.vetter@...ll.ch>, 
	dri-devel@...ts.freedesktop.org, arthurgrillo@...eup.net, linux-kernel@...r.kernel.org, 
	jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com, thomas.petazzoni@...tlin.com, 
	seanpaul@...gle.com, nicolejadeyee@...gle.com, 
	20241010-vkms-remove-index-v2-1-6b8d6cfd5a15@...tlin.com
Subject: Re: [PATCH v4 4/5] drm: writeback: Introduce drm managed helpers

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@...tlin.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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)