RE: [v3,2/7] drm: writeback: Modify writeback init helpers

[Kandpal, Suraj]

> -----Original Message-----
> From: John Harrison <John.Harrison@xxxxxxxxxx>
> Sent: Monday, May 4, 2026 11:15 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; kernel-
> list@xxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Shankar, Uma
> <uma.shankar@xxxxxxxxx>; dmitry.baryshkov@xxxxxxxxxxxxxxxx; Murthy,
> Arun R <arun.r.murthy@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>;
> harry.wentland@xxxxxxx; siqueira@xxxxxxxxxx;
> alexander.deucher@xxxxxxx; christian.koenig@xxxxxxx;
> airlied@xxxxxxxxx; simona@xxxxxxxx; liviu.dudau@xxxxxxx;
> maarten.lankhorst@xxxxxxxxxxxxxxx; mripard@xxxxxxxxxx;
> robin.clark@xxxxxxxxxxxxxxxx; abhinav.kumar@xxxxxxxxx;
> tzimmermann@xxxxxxx; sean@xxxxxxxxx; marijn.suijten@xxxxxxxxxxxxxx;
> laurent.pinchart+renesas@xxxxxxxxxxxxxxxx;
> dave.stevenson@xxxxxxxxxxxxxxx;
> tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx;
> kieran.bingham+renesas@xxxxxxxxxxxxxxxx; kernel-dev@xxxxxxxxxx
> Subject: Re: [v3,2/7] drm: writeback: Modify writeback init helpers
> 
> On 3/16/26 01:30, Suraj Kandpal wrote:
> > Now with drm_writeback_connector moved to drm_connector it makes
> more
> > sense use drm_connector as an argument rather than
> > drm_writeback_connector. The writeback connector can easily be derived
> > from drm_connector.

Hi John
First of all thanks for helping to move this series forward.

> So this patch and all five subsequent patches are basically the same search
> and replace of base_conn->wb_conn to base_conn in the DRM level helper
> functions, yes? I would add a little more explanation of why "it makes more
> sense". Something like: "Some of the writeback helper functions require
> access to the parent drm_connector object as well as the
> drm_writeback_connector object itself. So, pass in the top level object and
> traverse down rather than passing in the lower level object and traversing
> back up. Even where such is not the case, update to use the top level object
> for consistency across the interface."

Sure will update the commit message.

> 
> Also, there could be better consistency across these 'modify' patches.
> First, the subject of patches 1-5 should be 'drm/writeback: ...' not
> 'drm: writeback: ...'. Then you have 'modify XXX helpers', 'modify XXX params'
> and 'modify params for XXX'. 

Sure will keep the subject consistent

> It would be cleaner to pick a single variant and
> use that for all the patches. Lastly, are the final two patches really
> 'drm/connector:'? The header file with the function declarations being
> updated is drm_modeset_helper_vtables.h. Which would make the prefix
> 'drm/modeset'? Although, given that the declarations are specific to
> writeback support, I would just stick with 'drm/writeback'
> for all seven patches.

Sure will update the prefix for last two patches as well.
Although in regard to drm/writeback after grepping the git log it seems that
drm: writeback: is the correct prefix actually the correct wording would be more prevalent prefix. So I would like to keep that
the consistently across all my patches as well.

Regards,
Suraj Kandpal

> 
> John.
> 
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c   |  2 +-
> >   .../drm/arm/display/komeda/komeda_wb_connector.c   |  5 +----
> >   drivers/gpu/drm/arm/malidp_mw.c                    |  2 +-
> >   drivers/gpu/drm/drm_writeback.c                    | 14 ++++++--------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c      |  2 +-
> >   .../gpu/drm/renesas/rcar-du/rcar_du_writeback.c    |  3 +--
> >   drivers/gpu/drm/vc4/vc4_txp.c                      |  2 +-
> >   drivers/gpu/drm/vkms/vkms_writeback.c              |  4 ++--
> >   include/drm/drm_writeback.h                        |  4 ++--
> >   9 files changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
> > index 8fea29720989..84a9c1d2bd8e 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c
> > @@ -204,7 +204,7 @@ int amdgpu_dm_wb_connector_init(struct
> > amdgpu_display_manager *dm,
> >
> >   	drm_connector_helper_add(&wbcon->base,
> > &amdgpu_dm_wb_conn_helper_funcs);
> >
> > -	res = drmm_writeback_connector_init(&dm->adev->ddev, &wbcon-
> >base.writeback,
> > +	res = drmm_writeback_connector_init(&dm->adev->ddev, &wbcon-
> >base,
> >
> &amdgpu_dm_wb_connector_funcs,
> >   					    encoder,
> >   					    amdgpu_dm_wb_formats,
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > index fa2f63c142cd..85b34375d275 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > @@ -135,7 +135,6 @@ static int komeda_wb_connector_add(struct
> komeda_kms_dev *kms,
> >   {
> >   	struct komeda_dev *mdev = kms->base.dev_private;
> >   	struct komeda_wb_connector *kwb_conn;
> > -	struct drm_writeback_connector *wb_conn;
> >   	struct drm_display_info *info;
> >   	struct drm_encoder *encoder;
> >
> > @@ -151,8 +150,6 @@ static int komeda_wb_connector_add(struct
> > komeda_kms_dev *kms,
> >
> >   	kwb_conn->wb_layer = kcrtc->master->wb_layer;
> >
> > -	wb_conn = &kwb_conn->base.writeback;
> > -
> >   	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
> >   					       kwb_conn->wb_layer-
> >layer_type,
> >   					       &n_formats);
> > @@ -170,7 +167,7 @@ static int komeda_wb_connector_add(struct
> > komeda_kms_dev *kms,
> >
> >   	encoder->possible_crtcs = drm_crtc_mask(&kcrtc->base);
> >
> > -	err = drmm_writeback_connector_init(&kms->base, wb_conn,
> > +	err = drmm_writeback_connector_init(&kms->base, &kwb_conn-
> >base,
> >   					    &komeda_wb_connector_funcs,
> >   					    encoder,
> >   					    formats, n_formats);
> > diff --git a/drivers/gpu/drm/arm/malidp_mw.c
> > b/drivers/gpu/drm/arm/malidp_mw.c index 472598b3e007..7d42b007ef19
> > 100644
> > --- a/drivers/gpu/drm/arm/malidp_mw.c
> > +++ b/drivers/gpu/drm/arm/malidp_mw.c
> > @@ -228,7 +228,7 @@ int malidp_mw_connector_init(struct drm_device
> > *drm)
> >
> >   	encoder->possible_crtcs = drm_crtc_mask(&malidp->crtc);
> >
> > -	ret = drmm_writeback_connector_init(drm, &malidp-
> >mw_connector.writeback,
> > +	ret = drmm_writeback_connector_init(drm, &malidp->mw_connector,
> >   					    &malidp_mw_connector_funcs,
> >   					    encoder,
> >   					    formats, n_formats);
> > diff --git a/drivers/gpu/drm/drm_writeback.c
> > b/drivers/gpu/drm/drm_writeback.c index 7bf9f6374712..9a3037d11009
> > 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -242,7 +242,7 @@ static int __drm_writeback_connector_init(struct
> drm_device *dev,
> >    * a custom encoder
> >    *
> >    * @dev: DRM device
> > - * @wb_connector: Writeback connector to initialize
> > + * @connector: Drm connector which contains the writeback connector
> > + to initialize
> >    * @enc: handle to the already initialized drm encoder
> >    * @con_funcs: Connector funcs vtable
> >    * @formats: Array of supported pixel formats for the writeback
> > engine @@ -267,13 +267,12 @@ static int
> __drm_writeback_connector_init(struct drm_device *dev,
> >    * Returns: 0 on success, or a negative error code
> >    */
> >   int drm_writeback_connector_init(struct drm_device *dev,
> > -				 struct drm_writeback_connector
> *wb_connector,
> > +				 struct drm_connector *connector,
> >   				 const struct drm_connector_funcs
> *con_funcs,
> >   				 struct drm_encoder *enc,
> >   				 const u32 *formats, int n_formats)
> >   {
> > -	struct drm_connector *connector =
> > -		drm_writeback_to_connector(wb_connector);
> > +	struct drm_writeback_connector *wb_connector =
> > +&connector->writeback;
> >   	int ret;
> >
> >   	ret = drm_connector_init(dev, connector, con_funcs, @@ -322,7
> > +321,7 @@ static void drm_writeback_connector_cleanup(struct
> drm_device *dev,
> >    * a custom encoder
> >    *
> >    * @dev: DRM device
> > - * @wb_connector: Writeback connector to initialize
> > + * @connector: Drm connector containing the writeback connector to
> > + initialize
> >    * @con_funcs: Connector funcs vtable
> >    * @enc: Encoder to connect this writeback connector
> >    * @formats: Array of supported pixel formats for the writeback
> > engine @@ -338,13 +337,12 @@ static void
> drm_writeback_connector_cleanup(struct drm_device *dev,
> >    * Returns: 0 on success, or a negative error code
> >    */
> >   int drmm_writeback_connector_init(struct drm_device *dev,
> > -				  struct drm_writeback_connector
> *wb_connector,
> > +				  struct drm_connector *connector,
> >   				  const struct drm_connector_funcs
> *con_funcs,
> >   				  struct drm_encoder *enc,
> >   				  const u32 *formats, int n_formats)
> >   {
> > -	struct drm_connector *connector =
> > -		drm_writeback_to_connector(wb_connector);
> > +	struct drm_writeback_connector *wb_connector =
> > +&connector->writeback;
> >   	int ret;
> >
> >   	ret = drmm_connector_init(dev, connector, con_funcs, diff --git
> > a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > index 930ba1ad777b..d4fc28951085 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > @@ -136,7 +136,7 @@ int dpu_writeback_init(struct drm_device *dev,
> > struct drm_encoder *enc,
> >
> >   	drm_connector_helper_add(&dpu_wb_conn->base,
> > &dpu_wb_conn_helper_funcs);
> >
> > -	rc = drmm_writeback_connector_init(dev, &dpu_wb_conn-
> >base.writeback,
> > +	rc = drmm_writeback_connector_init(dev, &dpu_wb_conn->base,
> >   					   &dpu_wb_conn_funcs, enc,
> >   					   format_list, num_formats);
> >
> > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > index cd09e0fbb030..1de8865fb751 100644
> > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > @@ -203,7 +203,6 @@ static const u32 writeback_formats[] = {
> >   int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> >   			   struct rcar_du_crtc *rcrtc)
> >   {
> > -	struct drm_writeback_connector *wb_conn = &rcrtc-
> >writeback.writeback;
> >   	struct drm_encoder *encoder;
> >
> >   	encoder = drmm_plain_encoder_alloc(&rcdu->ddev, NULL, @@ -
> 218,7
> > +217,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> >   	drm_connector_helper_add(&rcrtc->writeback,
> >   				 &rcar_du_wb_conn_helper_funcs);
> >
> > -	return drmm_writeback_connector_init(&rcdu->ddev, wb_conn,
> > +	return drmm_writeback_connector_init(&rcdu->ddev, &rcrtc-
> >writeback,
> >   					     &rcar_du_wb_conn_funcs,
> >   					     encoder,
> >   					     writeback_formats,
> > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c
> > b/drivers/gpu/drm/vc4/vc4_txp.c index de3db0834011..d08271142116
> > 100644
> > --- a/drivers/gpu/drm/vc4/vc4_txp.c
> > +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> > @@ -601,7 +601,7 @@ static int vc4_txp_bind(struct device *dev, struct
> > device *master, void *data)
> >
> >   	drm_connector_helper_add(&txp->connector,
> >   				 &vc4_txp_connector_helper_funcs);
> > -	ret = drmm_writeback_connector_init(drm, &txp-
> >connector.writeback,
> > +	ret = drmm_writeback_connector_init(drm, &txp->connector,
> >   					    &vc4_txp_connector_funcs,
> >   					    encoder,
> >   					    drm_fmts, ARRAY_SIZE(drm_fmts));
> diff --git
> > a/drivers/gpu/drm/vkms/vkms_writeback.c
> > b/drivers/gpu/drm/vkms/vkms_writeback.c
> > index cadb4cb372c5..b368c569cf0a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -170,7 +170,6 @@ static const struct drm_connector_helper_funcs
> vkms_wb_conn_helper_funcs = {
> >   int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
> >   				    struct vkms_output *vkms_output)
> >   {
> > -	struct drm_writeback_connector *wb = &vkms_output-
> >wb_connector.writeback;
> >   	int ret;
> >
> >   	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output-
> >wb_encoder, @@
> > -183,7 +182,8 @@ int vkms_enable_writeback_connector(struct
> > vkms_device *vkmsdev,
> >
> >   	drm_connector_helper_add(&vkms_output->wb_connector,
> > &vkms_wb_conn_helper_funcs);
> >
> > -	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
> > +	return drmm_writeback_connector_init(&vkmsdev->drm,
> > +					     &vkms_output->wb_connector,
> >   					     &vkms_wb_connector_funcs,
> >   					     &vkms_output->wb_encoder,
> >   					     vkms_wb_formats,
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index 702141099520..c6960c7e634e 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -78,13 +78,13 @@ drm_writeback_to_connector(struct
> drm_writeback_connector *wb_connector)
> >   }
> >
> >   int drm_writeback_connector_init(struct drm_device *dev,
> > -				 struct drm_writeback_connector
> *wb_connector,
> > +				 struct drm_connector *connector,
> >   				 const struct drm_connector_funcs
> *con_funcs,
> >   				 struct drm_encoder *enc,
> >   				 const u32 *formats, int n_formats);
> >
> >   int drmm_writeback_connector_init(struct drm_device *dev,
> > -				  struct drm_writeback_connector
> *wb_connector,
> > +				  struct drm_connector *connector,
> >   				  const struct drm_connector_funcs
> *con_funcs,
> >   				  struct drm_encoder *enc,
> >   				  const u32 *formats, int n_formats);