[PATCH 01/28] drm/writeback: Add function that takes preallocated connector

[Kandpal, Suraj]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at oss.qualcomm.com>
> Sent: Sunday, July 27, 2025 9:03 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>
> Cc: dri-devel at lists.freedesktop.org; intel-xe at lists.freedesktop.org; intel-
> gfx at lists.freedesktop.org; Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>;
> Murthy, Arun R <arun.r.murthy at intel.com>; Shankar, Uma
> <uma.shankar at intel.com>
> Subject: Re: [PATCH 01/28] drm/writeback: Add function that takes preallocated
> connector
> 
> On Sat, Jul 26, 2025 at 04:41:29PM +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <dmitry.baryshkov at oss.qualcomm.com>
> > > Sent: Saturday, July 26, 2025 5:46 PM
> > > To: Kandpal, Suraj <suraj.kandpal at intel.com>
> > > Cc: dri-devel at lists.freedesktop.org; intel-xe at lists.freedesktop.org;
> > > intel- gfx at lists.freedesktop.org; Nautiyal, Ankit K
> > > <ankit.k.nautiyal at intel.com>; Murthy, Arun R
> > > <arun.r.murthy at intel.com>; Shankar, Uma <uma.shankar at 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.kandpal at 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.
> 
> All of this needs to go to the commit message.

Sure will get it there in the next revision.

> 
> >
> > >
> > > > + *
> > > > + * 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.
> 
> It surely is. This means that you need to extract common code rather than
> duplicate it.

Sure will find a more efficient way to do this in the next revision.

Regards,
Suraj Kandpal

> 
> 
> --
> With best wishes
> Dmitry