linux-kernel - Re: [PATCH v6 7/8] drm: writeback: Create drmm variants for drm_writeback

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: <Z3wCnk_WStJk9B5D@louis-chauvet-laptop>
Date: Mon, 6 Jan 2025 17:19:42 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
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
Subject: Re: [PATCH v6 7/8] drm: writeback: Create drmm variants for
 drm_writeback_connector initialization

On 06/01/25 - 14:04, Maxime Ripard wrote:
> On Mon, Dec 30, 2024 at 07:37:37PM +0100, Louis Chauvet wrote:
> > To allows driver to only use drmm objects, add helper to create
> > drm_writeback_connectors with automated lifetime management.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c | 69 +++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_writeback.h     |  8 +++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index 9c69f7181e02c23dabce488405608c40d4184af5..1251f65aae9e3b6fb5c5de9ab9280e5430342208 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -369,6 +369,75 @@ static void drm_writeback_connector_cleanup(struct drm_device *dev,
> >  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> >  }
> >  
> > +/**
> > + * drmm_writeback_connector_init - Initialize a writeback connector with
> > + * a custom encoder
> > + *
> > + * @dev: DRM device
> > + * @wb_connector: Writeback connector to initialize
> > + * @con_funcs: Connector funcs vtable
> > + * @enc: handle to the already initialized drm encoder, optional
> > + * @enc_funcs: Encoder funcs vtable, optional, only used when @enc is NULL
> > + * @formats: Array of supported pixel formats for the writeback engine
> > + * @n_formats: Length of the formats array
> > + * @possible_crtcs: if @enc is NULL, this will set the possible_crtc for the
> > + *		    newly created encoder
> > + *
> > + * This function initialize a writeback connector and register its cleanup.
> > + *
> > + * 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.
> > + *
> > + * If @enc is NULL, this function will create a drm-managed encoder and will
> > + * attach @enc_funcs on it. It will also attach the CRTC passed in
> > + * @possible_crtcs
> > + *
> > + * 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,
> > +				  struct drm_encoder *enc,
> > +				  const struct drm_encoder_helper_funcs *enc_funcs,
> > +				  const u32 *formats, int n_formats,
> > +				  u32 possible_crtcs)
> 
> The name isn't really consistent with the other functions. We already
> have a drm_writeback_connector_init that doesn't take the encoder point
> but will just read it from wb_connector->encoder, and we have
> drm_writeback_connector_with_encoder that assumes the encoder has
> already been created.
>
> We should the name or behavior on either one of them. Why do we need an
> optional encoder pointer? If enc is not NULL, then enc_funcs shouldn't
> be necessary, if it's NULL, then drm_writeback_connector_init will be
> sufficient?

This was requested by Jani in [1]. If you prefer I can create two variants 
for the next iteration.

[1]:https://lore.kernel.org/all/87a5gxyrhc.fsf@intel.com/

> 
> Maxime