Re: [PATCH v6 7/8] drm: writeback: Create drmm variants for drm_writeback_connector initialization

[Maxime Ripard <mripard@kernel.org>]

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

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@bootlin.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?

Maxime

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