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] [day] [month] [year] [list]

Message-ID: <20250107-polite-savvy-kiwi-8ada2b@houat>
Date: Tue, 7 Jan 2025 16:36:02 +0100
From: Maxime Ripard <mripard@...nel.org>
To: 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 Mon, Jan 06, 2025 at 05:19:42PM +0100, Louis Chauvet wrote:
> 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/

There was another suggestion in that review though ;)

I agree with Jani's second statement here: most of the weirdness of that
API stems from the fact that it deviates from the other APIs, and fixing
that should remove that weirdness.

Ultimately, allocating the encoder in the first place is weird. I don't
think we have any other example of an init function for one entity
allocating its own entity or another.

Why should we allocate that encoder in the helper?

Maxime

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