> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.barysh...@oss.qualcomm.com>
> Sent: Saturday, July 26, 2025 5:46 PM
> To: Kandpal, Suraj <suraj.kand...@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel...@lists.freedesktop.org; intel-
> g...@lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nauti...@intel.com>;
> Murthy, Arun R <arun.r.mur...@intel.com>; Shankar, Uma
> <uma.shan...@intel.com>
> Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes
> preallocated connector
>
> On Fri, Jul 25, 2025 at 10:33:42AM +0530, Suraj Kandpal wrote:
> > Write a function that takes a preallocated drm_connector instead of
> > using the one allocated inside the drm writeback connector init
> > function.
>
> Please start your commit message with describing the problem.
>
> >
> > Signed-off-by: Suraj Kandpal <suraj.kand...@intel.com>
> > ---
> > drivers/gpu/drm/drm_writeback.c | 76
> +++++++++++++++++++++++++++++++++
> > include/drm/drm_writeback.h | 7 +++
> > 2 files changed, 83 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..fa58eb0dc7bf
> > 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -416,6 +416,82 @@ int drmm_writeback_connector_init(struct
> > drm_device *dev, } EXPORT_SYMBOL(drmm_writeback_connector_init);
> >
> > +/*
> > + * drm_writeback_connector_init_with_conn - Initialize a writeback
> > +connector with
> > + * custom encoder and connector
> > + *
> > + * @enc: handle to the already initialized drm encoder
> > + * @con_funcs: Connector funcs vtable
> > + * @formats: Array of supported pixel formats for the writeback
> > +engine
> > + * @n_formats: Length of the formats array
> > + *
> > + * This function assumes that the drm_writeback_connector's encoder
> > +has already been
> > + * created and initialized before invoking this function.
> > + *
> > + * In addition, this function also assumes that callers of this API
> > +will manage
> > + * assigning the encoder helper functions, possible_crtcs and any
> > +other encoder
> > + * specific operation.
>
> Why?
The problem would that not every want can have a drm_connector embedded inside
the drm_writeback_connector
We have a restraint where all connectors need to be a intel connector and since
the we are not allowed to make connector
Inside the drm_connector into a pointer this gives a good alternative.
>
> > + *
> > + * Drivers should always use this function instead of
> > +drm_connector_init() to
> > + * set up writeback connectors if they want to manage themselves the
> > +lifetime of the
> > + * associated encoder.
> > + *
> > + * Returns: 0 on success, or a negative error code */ int
> > +drm_writeback_connector_init_with_conn(struct drm_device *dev, struct
> drm_connector *connector,
> > + struct drm_writeback_connector
> *wb_connector,
> > + struct drm_encoder *enc,
> > + const struct drm_connector_funcs
> *con_funcs,
> > + const u32 *formats, int n_formats) {
> > + struct drm_property_blob *blob;
> > + struct drm_mode_config *config = &dev->mode_config;
> > + int ret = create_writeback_properties(dev);
> > +
> > + if (ret != 0)
> > + return ret;
> > +
> > + blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
> > + formats);
> > + if (IS_ERR(blob))
> > + return PTR_ERR(blob);
> > +
> > + connector->interlace_allowed = 0;
>
> This function contans a lot of copy-paste from
> __drm_writeback_connector_init(), which is obviously a no-go.
The whole point is the minore difference inbetween then and how it derives a
lot of things from the
drm_writeback_connector because of which this looks like a similar function but
is essentially different.
Regards,
Suraj Kandpal
>
> > +
> > + ret = drm_connector_init(dev, connector, con_funcs,
> > + DRM_MODE_CONNECTOR_WRITEBACK);
> > + if (ret)
> > + goto connector_fail;
> > +
> > + INIT_LIST_HEAD(&wb_connector->job_queue);
> > + spin_lock_init(&wb_connector->job_lock);
> > +
> > + wb_connector->fence_context = dma_fence_context_alloc(1);
> > + spin_lock_init(&wb_connector->fence_lock);
> > + snprintf(wb_connector->timeline_name,
> > + sizeof(wb_connector->timeline_name),
> > + "CONNECTOR:%d-%s", connector->base.id, connector-
> >name);
> > +
> > + drm_object_attach_property(&connector->base,
> > + config->writeback_out_fence_ptr_property,
> 0);
> > +
> > + drm_object_attach_property(&connector->base,
> > + config->writeback_fb_id_property, 0);
> > +
> > + drm_object_attach_property(&connector->base,
> > + config->writeback_pixel_formats_property,
> > + blob->base.id);
> > + wb_connector->pixel_formats_blob_ptr = blob;
> > +
> > + return 0;
> > +
> > +connector_fail:
> > + drm_property_blob_put(blob);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(drm_writeback_connector_init_with_conn);
> > +
> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > struct drm_framebuffer *fb)
> > {
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index c380a7b8f55a..149744dbeef0 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -167,6 +167,13 @@ int drmm_writeback_connector_init(struct
> drm_device *dev,
> > struct drm_encoder *enc,
> > const u32 *formats, int n_formats);
> >
> > +int
> > +drm_writeback_connector_init_with_conn(struct drm_device *dev, struct
> drm_connector *connector,
> > + struct drm_writeback_connector
> *wb_connector,
> > + struct drm_encoder *enc,
> > + const struct drm_connector_funcs
> *con_funcs,
> > + const u32 *formats, int n_formats);
> > +
> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > struct drm_framebuffer *fb);
> >
> > --
> > 2.34.1
> >
>
> --
> With best wishes
> Dmitry
