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@xxxxxxxxxxx> > > --- > > 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@xxxxxxxxx/ > > Maxime
