[RFC PATCH 0/8] Refactor drm_writeback_connector structure

[RFC PATCH 0/8] Refactor drm_writeback_connector structure

[Suraj Kandpal]

Some drivers cannot work with the current design where the connector
is embedded within the drm_writeback_connector such as intel and
some drivers that can get it working end up adding a lot of checks
all around the code to check if it's a writeback conenctor or not.
This series intends to solve it by moving the drm_writeback_connector
within the drm_connector and remove the drm_connector base which was in
drm_writeback_connector. A Proposal suggested by Dmitry.
We want to get an Ack from all drivers whom are affected by these
changes.
 We do all other required modifications that come with these changes
along with addition of new function which returns the drm_connector when
drm_writeback_connector is present.
All drivers will be expected to allocate the drm_connector.
This discussion was tiggered from [1] and sits on top of Dmitry's series
see [2].

[1] https://patchwork.freedesktop.org/series/152106/
[2] https://patchwork.freedesktop.org/series/152420/

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

Suraj Kandpal (8):
  drm: writeback: Refactor drm_writeback_connector structure
  drm/amd/display: Adapt amd writeback to new drm_writeback_connector
  drm/arm/komeda: Adapt komeda writeback to new drm_writeback_connector
  drm/arm/mali: Adapt mali writeback to new drm_writeback_connector
  drm/vc4: Adapt vc4 writeback to new drm_writeback_connector
  drm/vkms: Adapt vkms writeback to new drm_writeback_connector
  drm/rcar_du: Adapt vkms writeback to new drm_writeback_connector
  drm/msm/dpu: Adapt dpu writeback to new drm_writeback_connector

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +-
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c  |  8 +--
 .../gpu/drm/arm/display/komeda/komeda_crtc.c  |  6 +-
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  6 +-
 .../arm/display/komeda/komeda_wb_connector.c  |  8 +--
 drivers/gpu/drm/arm/malidp_drv.h              |  2 +-
 drivers/gpu/drm/arm/malidp_mw.c               |  6 +-
 drivers/gpu/drm/drm_writeback.c               | 33 ++++++---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 16 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h |  4 +-
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  4 +-
 .../drm/renesas/rcar-du/rcar_du_writeback.c   | 22 +++---
 drivers/gpu/drm/vc4/vc4_txp.c                 | 14 ++--
 drivers/gpu/drm/vkms/vkms_composer.c          |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h               |  2 +-
 drivers/gpu/drm/vkms/vkms_writeback.c         | 15 ++--
 include/drm/drm_connector.h                   | 60 ++++++++++++++++
 include/drm/drm_writeback.h                   | 68 ++-----------------
 20 files changed, 155 insertions(+), 130 deletions(-)

-- 
2.34.1

[RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Suraj Kandpal]

Some drivers cannot work with the current design where the connector
is embedded within the drm_writeback_connector such as intel and
some drivers that can get it working end up adding a lot of checks
all around the code to check if it's a writeback conenctor or not.
To solve this we move the drm_writeback_connector within the
drm_connector and remove the drm_connector base which was in
drm_writeback_connector. We do all other required
modifications that come with these changes along with addition
of new function which returns the drm_connector when
drm_writeback_connector is present.
All drivers will be expected to allocate the drm_connector.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
 include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
 include/drm/drm_writeback.h     | 68 ++++-----------------------------
 3 files changed, 89 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index ec2575c4c21b..198b8c488056 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -89,8 +89,10 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
 {
 	struct drm_writeback_connector *wb_connector =
 		fence_to_wb_connector(fence);
+	struct drm_connector *connector =
+		drm_writeback_to_connector(wb_connector);
 
-	return wb_connector->base.dev->driver->name;
+	return connector->dev->driver->name;
 }
 
 static const char *
@@ -187,7 +189,8 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
 					  struct drm_encoder *enc, const u32 *formats,
 					  int n_formats)
 {
-	struct drm_connector *connector = &wb_connector->base;
+	struct drm_connector *connector =
+		drm_writeback_to_connector(wb_connector);
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_property_blob *blob;
 	int ret = create_writeback_properties(dev);
@@ -269,7 +272,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
 				 struct drm_encoder *enc,
 				 const u32 *formats, int n_formats)
 {
-	struct drm_connector *connector = &wb_connector->base;
+	struct drm_connector *connector =
+		drm_writeback_to_connector(wb_connector);
 	int ret;
 
 	ret = drm_connector_init(dev, connector, con_funcs,
@@ -339,7 +343,8 @@ int drmm_writeback_connector_init(struct drm_device *dev,
 				  struct drm_encoder *enc,
 				  const u32 *formats, int n_formats)
 {
-	struct drm_connector *connector = &wb_connector->base;
+	struct drm_connector *connector =
+		drm_writeback_to_connector(wb_connector);
 	int ret;
 
 	ret = drmm_connector_init(dev, connector, con_funcs,
@@ -382,13 +387,15 @@ int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 
 int drm_writeback_prepare_job(struct drm_writeback_job *job)
 {
-	struct drm_writeback_connector *connector = job->connector;
+	struct drm_writeback_connector *wb_connector = job->connector;
+	struct drm_connector *connector
+		= drm_writeback_to_connector(wb_connector);
 	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->helper_private;
 	int ret;
 
 	if (funcs->prepare_writeback_job) {
-		ret = funcs->prepare_writeback_job(connector, job);
+		ret = funcs->prepare_writeback_job(wb_connector, job);
 		if (ret < 0)
 			return ret;
 	}
@@ -434,12 +441,14 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
 
 void drm_writeback_cleanup_job(struct drm_writeback_job *job)
 {
-	struct drm_writeback_connector *connector = job->connector;
+	struct drm_writeback_connector *wb_connector = job->connector;
+	struct drm_connector *connector
+		= drm_writeback_to_connector(wb_connector);
 	const struct drm_connector_helper_funcs *funcs =
-		connector->base.helper_private;
+		connector->helper_private;
 
 	if (job->prepared && funcs->cleanup_writeback_job)
-		funcs->cleanup_writeback_job(connector, job);
+		funcs->cleanup_writeback_job(wb_connector, job);
 
 	if (job->fb)
 		drm_framebuffer_put(job->fb);
@@ -521,8 +530,10 @@ struct dma_fence *
 drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
 {
 	struct dma_fence *fence;
+	struct drm_connector *connector =
+		drm_writeback_to_connector(wb_connector);
 
-	if (WARN_ON(wb_connector->base.connector_type !=
+	if (WARN_ON(connector->connector_type !=
 		    DRM_MODE_CONNECTOR_WRITEBACK))
 		return NULL;
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d..da63fdafd9f2 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1882,6 +1882,61 @@ struct drm_connector_cec {
 	void *data;
 };
 
+/**
+ * struct drm_writeback_connector - DRM writeback connector
+ */
+struct drm_writeback_connector {
+	/**
+	 * @pixel_formats_blob_ptr:
+	 *
+	 * DRM blob property data for the pixel formats list on writeback
+	 * connectors
+	 * See also drm_writeback_connector_init()
+	 */
+	struct drm_property_blob *pixel_formats_blob_ptr;
+
+	/** @job_lock: Protects job_queue */
+	spinlock_t job_lock;
+
+	/**
+	 * @job_queue:
+	 *
+	 * Holds a list of a connector's writeback jobs; the last item is the
+	 * most recent. The first item may be either waiting for the hardware
+	 * to begin writing, or currently being written.
+	 *
+	 * See also: drm_writeback_queue_job() and
+	 * drm_writeback_signal_completion()
+	 */
+	struct list_head job_queue;
+
+	/**
+	 * @fence_context:
+	 *
+	 * timeline context used for fence operations.
+	 */
+	unsigned int fence_context;
+	/**
+	 * @fence_lock:
+	 *
+	 * spinlock to protect the fences in the fence_context.
+	 */
+	spinlock_t fence_lock;
+	/**
+	 * @fence_seqno:
+	 *
+	 * Seqno variable used as monotonic counter for the fences
+	 * created on the connector's timeline.
+	 */
+	unsigned long fence_seqno;
+	/**
+	 * @timeline_name:
+	 *
+	 * The name of the connector's fence timeline.
+	 */
+	char timeline_name[32];
+};
+
 /**
  * struct drm_connector - central DRM connector control structure
  *
@@ -2305,6 +2360,11 @@ struct drm_connector {
 	 * @cec: CEC-related data.
 	 */
 	struct drm_connector_cec cec;
+
+	/**
+	 * @writeback: Writeback related valriables.
+	 */
+	struct drm_writeback_connector writeback;
 };
 
 #define obj_to_connector(x) container_of(x, struct drm_connector, base)
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 958466a05e60..2a52b6761797 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -15,66 +15,6 @@
 #include <drm/drm_encoder.h>
 #include <linux/workqueue.h>
 
-/**
- * struct drm_writeback_connector - DRM writeback connector
- */
-struct drm_writeback_connector {
-	/**
-	 * @base: base drm_connector object
-	 */
-	struct drm_connector base;
-
-	/**
-	 * @pixel_formats_blob_ptr:
-	 *
-	 * DRM blob property data for the pixel formats list on writeback
-	 * connectors
-	 * See also drm_writeback_connector_init()
-	 */
-	struct drm_property_blob *pixel_formats_blob_ptr;
-
-	/** @job_lock: Protects job_queue */
-	spinlock_t job_lock;
-
-	/**
-	 * @job_queue:
-	 *
-	 * Holds a list of a connector's writeback jobs; the last item is the
-	 * most recent. The first item may be either waiting for the hardware
-	 * to begin writing, or currently being written.
-	 *
-	 * See also: drm_writeback_queue_job() and
-	 * drm_writeback_signal_completion()
-	 */
-	struct list_head job_queue;
-
-	/**
-	 * @fence_context:
-	 *
-	 * timeline context used for fence operations.
-	 */
-	unsigned int fence_context;
-	/**
-	 * @fence_lock:
-	 *
-	 * spinlock to protect the fences in the fence_context.
-	 */
-	spinlock_t fence_lock;
-	/**
-	 * @fence_seqno:
-	 *
-	 * Seqno variable used as monotonic counter for the fences
-	 * created on the connector's timeline.
-	 */
-	unsigned long fence_seqno;
-	/**
-	 * @timeline_name:
-	 *
-	 * The name of the connector's fence timeline.
-	 */
-	char timeline_name[32];
-};
-
 /**
  * struct drm_writeback_job - DRM writeback job
  */
@@ -131,10 +71,16 @@ struct drm_writeback_job {
 	void *priv;
 };
 
+static inline struct drm_connector *
+drm_writeback_to_connector(struct drm_writeback_connector *wb_connector)
+{
+	return container_of(wb_connector, struct drm_connector, writeback);
+}
+
 static inline struct drm_writeback_connector *
 drm_connector_to_writeback(struct drm_connector *connector)
 {
-	return container_of(connector, struct drm_writeback_connector, base);
+	return &connector->writeback;
 }
 
 int drm_writeback_connector_init(struct drm_device *dev,
-- 
2.34.1

[RFC PATCH 2/8] drm/amd/display: Adapt amd writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the amd writeback functionality to this changes. This includes
changing the drm_writeback_connector to be changed to drm_connector
within the amdgpu_dm_wb_connector. Some other changes are done which
are a result of the all the above changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 4 +---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h    | 2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c | 8 ++++----
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 5e260b275082..d4628dadf55a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6915,11 +6915,9 @@ create_stream_for_sink(struct drm_connector *connector,
 		aconnector = to_amdgpu_dm_connector(connector);
 		link = aconnector->dc_link;
 	} else {
-		struct drm_writeback_connector *wbcon = NULL;
 		struct amdgpu_dm_wb_connector *dm_wbcon = NULL;
 
-		wbcon = drm_connector_to_writeback(connector);
-		dm_wbcon = to_amdgpu_dm_wb_connector(wbcon);
+		dm_wbcon = to_amdgpu_dm_wb_connector(connector);
 		link = dm_wbcon->link;
 	}
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index b937da0a4e4a..dbcdc8595e76 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -809,7 +809,7 @@ static inline void amdgpu_dm_set_mst_status(uint8_t *status,
 #define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
 
 struct amdgpu_dm_wb_connector {
-	struct drm_writeback_connector base;
+	struct drm_connector base;
 	struct dc_link *link;
 };
 
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 80c37487ca77..8fea29720989 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
@@ -202,9 +202,9 @@ int amdgpu_dm_wb_connector_init(struct amdgpu_display_manager *dm,
 
 	wbcon->link = link;
 
-	drm_connector_helper_add(&wbcon->base.base, &amdgpu_dm_wb_conn_helper_funcs);
+	drm_connector_helper_add(&wbcon->base, &amdgpu_dm_wb_conn_helper_funcs);
 
-	res = drmm_writeback_connector_init(&dm->adev->ddev, &wbcon->base,
+	res = drmm_writeback_connector_init(&dm->adev->ddev, &wbcon->base.writeback,
 					    &amdgpu_dm_wb_connector_funcs,
 					    encoder,
 					    amdgpu_dm_wb_formats,
@@ -216,8 +216,8 @@ int amdgpu_dm_wb_connector_init(struct amdgpu_display_manager *dm,
 	 * Some of the properties below require access to state, like bpc.
 	 * Allocate some default initial connector state with our reset helper.
 	 */
-	if (wbcon->base.base.funcs->reset)
-		wbcon->base.base.funcs->reset(&wbcon->base.base);
+	if (wbcon->base.funcs->reset)
+		wbcon->base.funcs->reset(&wbcon->base);
 
 	return 0;
 }
-- 
2.34.1

[RFC PATCH 3/8] drm/arm/komeda: Adapt komeda writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the komeda writeback functionality to this changes. This includes
changing the drm_writeback_connector to be changed to drm_connector
within the komeda_wb_connector.
Some other changes are done which are a result of the all the above
changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 6 +++---
 drivers/gpu/drm/arm/display/komeda/komeda_kms.h          | 6 +++---
 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 8 ++++----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 2ad33559a33a..1c2ba6eeb0e5 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -210,7 +210,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
 		struct komeda_wb_connector *wb_conn = kcrtc->wb_conn;
 
 		if (wb_conn)
-			drm_writeback_signal_completion(&wb_conn->base, 0);
+			drm_writeback_signal_completion(&wb_conn->base.writeback, 0);
 		else
 			DRM_WARN("CRTC[%d]: EOW happen but no wb_connector.\n",
 				 drm_crtc_index(&kcrtc->base));
@@ -266,9 +266,9 @@ komeda_crtc_do_flush(struct drm_crtc *crtc,
 	if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
 		komeda_pipeline_update(slave, old->state);
 
-	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
+	conn_st = wb_conn ? wb_conn->base.state : NULL;
 	if (conn_st && conn_st->writeback_job)
-		drm_writeback_queue_job(&wb_conn->base, conn_st);
+		drm_writeback_queue_job(&wb_conn->base.writeback, conn_st);
 
 	/* step 2: notify the HW to kickoff the update */
 	mdev->funcs->flush(mdev, master->id, kcrtc_st->active_pipes);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 83e61c4080c2..9c34302782c0 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -53,8 +53,8 @@ struct komeda_plane_state {
  * struct komeda_wb_connector
  */
 struct komeda_wb_connector {
-	/** @base: &drm_writeback_connector */
-	struct drm_writeback_connector base;
+	/** @base: &drm_connector */
+	struct drm_connector base;
 
 	/** @wb_layer: represents associated writeback pipeline of komeda */
 	struct komeda_layer *wb_layer;
@@ -139,7 +139,7 @@ struct komeda_kms_dev {
 static inline bool is_writeback_only(struct drm_crtc_state *st)
 {
 	struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn;
-	struct drm_connector *conn = wb_conn ? &wb_conn->base.base : NULL;
+	struct drm_connector *conn = wb_conn ? &wb_conn->base : NULL;
 
 	return conn && (st->connector_mask == BIT(drm_connector_index(conn)));
 }
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 210841b4453a..69822b9be574 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -53,7 +53,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder,
 		return -EINVAL;
 	}
 
-	wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer;
+	wb_layer = to_kconn(conn_st->connector)->wb_layer;
 
 	/*
 	 * No need for a full modested when the only connector changed is the
@@ -151,7 +151,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 
 	kwb_conn->wb_layer = kcrtc->master->wb_layer;
 
-	wb_conn = &kwb_conn->base;
+	wb_conn = &kwb_conn->base.writeback;
 
 	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
 					       kwb_conn->wb_layer->layer_type,
@@ -180,9 +180,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 		return err;
 	}
 
-	drm_connector_helper_add(&wb_conn->base, &komeda_wb_conn_helper_funcs);
+	drm_connector_helper_add(&kwb_conn->base, &komeda_wb_conn_helper_funcs);
 
-	info = &kwb_conn->base.base.display_info;
+	info = &kwb_conn->base.display_info;
 	info->bpc = __fls(kcrtc->master->improc->supported_color_depths);
 	info->color_formats = kcrtc->master->improc->supported_color_formats;
 
-- 
2.34.1

[RFC PATCH 4/8] drm/arm/mali: Adapt mali writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the malidp writeback functionality to this changes. This includes
changing the drm_writeback_connector to be changed to drm_connector
within the malidp_drm.
Some other changes are done which are a result of the all the above
changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/arm/malidp_drv.h | 2 +-
 drivers/gpu/drm/arm/malidp_mw.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index bc0387876dea..cc6d21aa5544 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -32,7 +32,7 @@ struct malidp_drm {
 	struct drm_device base;
 	struct malidp_hw_device *dev;
 	struct drm_crtc crtc;
-	struct drm_writeback_connector mw_connector;
+	struct drm_connector connector;
 	wait_queue_head_t wq;
 	struct drm_pending_vblank_event *event;
 	atomic_t config_valid;
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index 182275c0c29c..67fcdc7b3669 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -212,7 +212,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
 	if (!malidp->dev->hw->enable_memwrite)
 		return 0;
 
-	drm_connector_helper_add(&malidp->mw_connector.base,
+	drm_connector_helper_add(&malidp->connector,
 				 &malidp_mw_connector_helper_funcs);
 
 	formats = get_writeback_formats(malidp, &n_formats);
@@ -228,7 +228,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
 
 	encoder->possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
 
-	ret = drmm_writeback_connector_init(drm, &malidp->mw_connector,
+	ret = drmm_writeback_connector_init(drm, &malidp->connector.writeback,
 					    &malidp_mw_connector_funcs,
 					    encoder,
 					    formats, n_formats);
@@ -243,7 +243,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
 			     struct drm_atomic_state *old_state)
 {
 	struct malidp_drm *malidp = drm_to_malidp(drm);
-	struct drm_writeback_connector *mw_conn = &malidp->mw_connector;
+	struct drm_writeback_connector *mw_conn = &malidp->connector.writeback;
 	struct drm_connector_state *conn_state = mw_conn->base.state;
 	struct malidp_hw_device *hwdev = malidp->dev;
 	struct malidp_mw_connector_state *mw_state;
-- 
2.34.1

[RFC PATCH 5/8] drm/vc4: Adapt vc4 writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the vc4 writeback functionality to this changes. This
includes changing the drm_writeback_connector to be changed to drm_connector
within the vc4_txp.Some other changes are done which are a
result of the all the above changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/vc4/vc4_txp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index fef4e4ee47cf..2428807e9714 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -167,7 +167,7 @@ struct vc4_txp {
 	struct platform_device *pdev;
 
 	struct vc4_encoder encoder;
-	struct drm_writeback_connector connector;
+	struct drm_connector connector;
 
 	void __iomem *regs;
 };
@@ -176,7 +176,7 @@ struct vc4_txp {
 	container_of_const(_encoder, struct vc4_txp, encoder.base)
 
 #define connector_to_vc4_txp(_connector)				\
-	container_of_const(_connector, struct vc4_txp, connector.base)
+	container_of_const(_connector, struct vc4_txp, connector)
 
 static const struct debugfs_reg32 txp_regs[] = {
 	VC4_REG32(TXP_DST_PTR),
@@ -356,7 +356,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
 
 	TXP_WRITE(TXP_DST_CTRL, ctrl);
 
-	drm_writeback_queue_job(&txp->connector, conn_state);
+	drm_writeback_queue_job(&txp->connector.writeback, conn_state);
 
 	drm_dev_exit(idx);
 }
@@ -504,7 +504,7 @@ static irqreturn_t vc4_txp_interrupt(int irq, void *data)
 	 */
 	TXP_WRITE(TXP_DST_CTRL, TXP_READ(TXP_DST_CTRL) & ~TXP_EI);
 	vc4_crtc_handle_vblank(vc4_crtc);
-	drm_writeback_signal_completion(&txp->connector, 0);
+	drm_writeback_signal_completion(&txp->connector.writeback, 0);
 
 	return IRQ_HANDLED;
 }
@@ -598,9 +598,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	drm_connector_helper_add(&txp->connector.base,
+	drm_connector_helper_add(&txp->connector,
 				 &vc4_txp_connector_helper_funcs);
-	ret = drmm_writeback_connector_init(drm, &txp->connector,
+	ret = drmm_writeback_connector_init(drm, &txp->connector.writeback,
 					    &vc4_txp_connector_funcs,
 					    encoder,
 					    drm_fmts, ARRAY_SIZE(drm_fmts));
@@ -622,7 +622,7 @@ static void vc4_txp_unbind(struct device *dev, struct device *master,
 {
 	struct vc4_txp *txp = dev_get_drvdata(dev);
 
-	drm_connector_cleanup(&txp->connector.base);
+	drm_connector_cleanup(&txp->connector);
 }
 
 static const struct component_ops vc4_txp_ops = {
-- 
2.34.1

[RFC PATCH 6/8] drm/vkms: Adapt vkms writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the vkms writeback functionality to this changes. This
includes changing the drm_writeback_connector to be changed to
drm_connector within the vkms_output.
Some other changes are done which are a result of the all the above
changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c  |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h       |  2 +-
 drivers/gpu/drm/vkms/vkms_writeback.c | 15 +++++++++------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index fa269d279e25..b5f20637121c 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -543,7 +543,7 @@ void vkms_composer_worker(struct work_struct *work)
 		return;
 
 	if (wb_pending) {
-		drm_writeback_signal_completion(&out->wb_connector, 0);
+		drm_writeback_signal_completion(&out->connector.writeback, 0);
 		spin_lock_irq(&out->composer_lock);
 		crtc_state->wb_pending = false;
 		spin_unlock_irq(&out->composer_lock);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 8013c31efe3b..2e58a06c9ad8 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -213,7 +213,7 @@ struct vkms_crtc_state {
  */
 struct vkms_output {
 	struct drm_crtc crtc;
-	struct drm_writeback_connector wb_connector;
+	struct drm_connector connector;
 	struct drm_encoder wb_encoder;
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 45d69a3b85f6..13c2a5c8f57a 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -102,13 +102,16 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 	return ret;
 }
 
-static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
+static void vkms_wb_cleanup_job(struct drm_writeback_connector *wb_connector,
 				struct drm_writeback_job *job)
 {
 	struct vkms_writeback_job *vkmsjob = job->priv;
+	struct drm_connector *connector = container_of(wb_connector,
+						       struct drm_connector,
+						       writeback);
 	struct vkms_output *vkms_output = container_of(connector,
 						       struct vkms_output,
-						       wb_connector);
+						       connector);
 
 	if (!job->fb)
 		return;
@@ -127,8 +130,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
 											 conn);
 	struct vkms_output *output = drm_crtc_to_vkms_output(connector_state->crtc);
-	struct drm_writeback_connector *wb_conn = &output->wb_connector;
-	struct drm_connector_state *conn_state = wb_conn->base.state;
+	struct drm_writeback_connector *wb_conn = &output->connector.writeback;
+	struct drm_connector_state *conn_state = output->connector.state;
 	struct vkms_crtc_state *crtc_state = output->composer_state;
 	struct drm_framebuffer *fb = connector_state->writeback_job->fb;
 	u16 crtc_height = crtc_state->base.mode.vdisplay;
@@ -166,7 +169,7 @@ 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;
+	struct drm_writeback_connector *wb = &vkms_output->connector.writeback;
 	int ret;
 
 	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output->wb_encoder,
@@ -177,7 +180,7 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
 	vkms_output->wb_encoder.possible_clones |=
 		drm_encoder_mask(&vkms_output->wb_encoder);
 
-	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
+	drm_connector_helper_add(&vkms_output->connector, &vkms_wb_conn_helper_funcs);
 
 	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
 					     &vkms_wb_connector_funcs,
-- 
2.34.1

[RFC PATCH 7/8] drm/rcar_du: Adapt vkms writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the rcar-du writeback functionality to this changes. This
includes changing the drm_writeback_connector to be changed to drm_connector
within the rcar_du_crtc.
Some other changes are done which are a result of the all the above
changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  4 ++--
 .../drm/renesas/rcar-du/rcar_du_writeback.c   | 22 +++++++++++--------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
index d0f38a8b3561..457c803d75bc 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
@@ -72,11 +72,11 @@ struct rcar_du_crtc {
 	const char *const *sources;
 	unsigned int sources_count;
 
-	struct drm_writeback_connector writeback;
+	struct drm_connector connector;
 };
 
 #define to_rcar_crtc(c)		container_of(c, struct rcar_du_crtc, crtc)
-#define wb_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, writeback)
+#define connector_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, connector)
 
 /**
  * struct rcar_du_crtc_state - Driver-specific CRTC state
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 9986a10e8114..95e6810612c2 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
@@ -47,10 +47,12 @@ static int rcar_du_wb_conn_get_modes(struct drm_connector *connector)
 				    dev->mode_config.max_height);
 }
 
-static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
+static int rcar_du_wb_prepare_job(struct drm_writeback_connector *wb_connector,
 				  struct drm_writeback_job *job)
 {
-	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
+	struct drm_connector *connector =
+		container_of(wb_connector, struct drm_connector, writeback);
+	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
 	struct rcar_du_wb_job *rjob;
 	int ret;
 
@@ -72,10 +74,12 @@ static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
 	return 0;
 }
 
-static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *connector,
+static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *wb_connector,
 				   struct drm_writeback_job *job)
 {
-	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
+	struct drm_connector *connector =
+		container_of(wb_connector, struct drm_connector, writeback);
+	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
 	struct rcar_du_wb_job *rjob = job->priv;
 
 	if (!job->fb)
@@ -199,7 +203,7 @@ 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;
+	struct drm_writeback_connector *wb_conn = &rcrtc->connector.writeback;
 
 	struct drm_encoder *encoder;
 
@@ -212,7 +216,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
 
 	encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
 
-	drm_connector_helper_add(&wb_conn->base,
+	drm_connector_helper_add(&rcrtc->connector,
 				 &rcar_du_wb_conn_helper_funcs);
 
 	return drmm_writeback_connector_init(&rcdu->ddev, wb_conn,
@@ -231,7 +235,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
 	struct drm_framebuffer *fb;
 	unsigned int i;
 
-	state = rcrtc->writeback.base.state;
+	state = rcrtc->connector.state;
 	if (!state || !state->writeback_job)
 		return;
 
@@ -246,10 +250,10 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
 		cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
 			    + fb->offsets[i];
 
-	drm_writeback_queue_job(&rcrtc->writeback, state);
+	drm_writeback_queue_job(&rcrtc->connector.writeback, state);
 }
 
 void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
 {
-	drm_writeback_signal_completion(&rcrtc->writeback, 0);
+	drm_writeback_signal_completion(&rcrtc->connector.writeback, 0);
 }
-- 
2.34.1

[RFC PATCH 8/8] drm/msm/dpu: Adapt dpu writeback to new drm_writeback_connector

[Suraj Kandpal]

Now that drm_writeback_connector is embedded with the drm_connector
adapt the dpu writeback functionality to this changes. This
includes changing the drm_writeback_connector to be changed to
drm_connector within the dpu_wb_connector.
Some other changes are done which are a result of the all the above
changes mentioned.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  |  3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c    | 16 +++++++++-------
 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h    |  4 ++--
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 56a5b596554d..0e60c1ac07c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -484,7 +484,8 @@ static void dpu_encoder_phys_wb_prepare_for_kickoff(
 		return;
 	}
 
-	drm_conn = &wb_enc->wb_conn->base;
+	drm_conn =
+		container_of(wb_enc->wb_conn, struct drm_connector, writeback);
 	state = drm_conn->state;
 
 	if (wb_enc->wb_conn && wb_enc->wb_job)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
index cd73468e369a..49412d2ed9aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
@@ -28,8 +28,7 @@ static int dpu_wb_conn_get_modes(struct drm_connector *connector)
 static int dpu_wb_conn_atomic_check(struct drm_connector *connector,
 				    struct drm_atomic_state *state)
 {
-	struct drm_writeback_connector *wb_conn = drm_connector_to_writeback(connector);
-	struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(wb_conn);
+	struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(connector);
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
 	struct drm_crtc *crtc;
@@ -84,10 +83,11 @@ static const struct drm_connector_funcs dpu_wb_conn_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int dpu_wb_conn_prepare_job(struct drm_writeback_connector *connector,
+static int dpu_wb_conn_prepare_job(struct drm_writeback_connector *wb_conn,
 		struct drm_writeback_job *job)
 {
-
+	struct drm_connector *connector =
+		container_of(wb_conn, struct drm_connector, writeback);
 	struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(connector);
 
 	if (!job->fb)
@@ -98,9 +98,11 @@ static int dpu_wb_conn_prepare_job(struct drm_writeback_connector *connector,
 	return 0;
 }
 
-static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *connector,
+static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector *wb_connector,
 		struct drm_writeback_job *job)
 {
+	struct drm_connector *connector =
+		container_of(wb_connector, struct drm_connector, writeback);
 	struct dpu_wb_connector *dpu_wb_conn = to_dpu_wb_conn(connector);
 
 	if (!job->fb)
@@ -128,9 +130,9 @@ int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
 
 	dpu_wb_conn->maxlinewidth = maxlinewidth;
 
-	drm_connector_helper_add(&dpu_wb_conn->base.base, &dpu_wb_conn_helper_funcs);
+	drm_connector_helper_add(&dpu_wb_conn->base, &dpu_wb_conn_helper_funcs);
 
-	rc = drmm_writeback_connector_init(dev, &dpu_wb_conn->base,
+	rc = drmm_writeback_connector_init(dev, &dpu_wb_conn->base.writeback,
 					   &dpu_wb_conn_funcs, enc,
 					   format_list, num_formats);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
index 4b11cca8014c..9ebf15392b20 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
@@ -16,12 +16,12 @@
 #include "dpu_encoder_phys.h"
 
 struct dpu_wb_connector {
-	struct drm_writeback_connector base;
+	struct drm_connector base;
 	struct drm_encoder *wb_enc;
 	u32 maxlinewidth;
 };
 
-static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_writeback_connector *conn)
+static inline struct dpu_wb_connector *to_dpu_wb_conn(struct drm_connector *conn)
 {
 	return container_of(conn, struct dpu_wb_connector, base);
 }
-- 
2.34.1

Re: [RFC PATCH 7/8] drm/rcar_du: Adapt vkms writeback to new drm_writeback_connector

[Laurent Pinchart]

On Mon, Aug 11, 2025 at 02:57:06PM +0530, Suraj Kandpal wrote:
> Now that drm_writeback_connector is embedded with the drm_connector
> adapt the rcar-du writeback functionality to this changes. This
> includes changing the drm_writeback_connector to be changed to drm_connector
> within the rcar_du_crtc.
> Some other changes are done which are a result of the all the above
> changes mentioned.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  4 ++--
>  .../drm/renesas/rcar-du/rcar_du_writeback.c   | 22 +++++++++++--------
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> index d0f38a8b3561..457c803d75bc 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> @@ -72,11 +72,11 @@ struct rcar_du_crtc {
>  	const char *const *sources;
>  	unsigned int sources_count;
>  
> -	struct drm_writeback_connector writeback;
> +	struct drm_connector connector;

You forgot to update the documentation of the structure.

"connector" is a too generic name. I'd keep the existing field name.

>  };
>  
>  #define to_rcar_crtc(c)		container_of(c, struct rcar_du_crtc, crtc)
> -#define wb_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, writeback)
> +#define connector_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, connector)
>  
>  /**
>   * struct rcar_du_crtc_state - Driver-specific CRTC state
> 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 9986a10e8114..95e6810612c2 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> @@ -47,10 +47,12 @@ static int rcar_du_wb_conn_get_modes(struct drm_connector *connector)
>  				    dev->mode_config.max_height);
>  }
>  
> -static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
> +static int rcar_du_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>  				  struct drm_writeback_job *job)
>  {
> -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> +	struct drm_connector *connector =
> +		container_of(wb_connector, struct drm_connector, writeback);
> +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);

Modify wb_to_rcar_crtc() instead of changing the code here and below.

>  	struct rcar_du_wb_job *rjob;
>  	int ret;
>  
> @@ -72,10 +74,12 @@ static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
>  	return 0;
>  }
>  
> -static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *connector,
> +static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *wb_connector,
>  				   struct drm_writeback_job *job)
>  {
> -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> +	struct drm_connector *connector =
> +		container_of(wb_connector, struct drm_connector, writeback);
> +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
>  	struct rcar_du_wb_job *rjob = job->priv;
>  
>  	if (!job->fb)
> @@ -199,7 +203,7 @@ 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;
> +	struct drm_writeback_connector *wb_conn = &rcrtc->connector.writeback;
>  
>  	struct drm_encoder *encoder;
>  
> @@ -212,7 +216,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>  
>  	encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>  
> -	drm_connector_helper_add(&wb_conn->base,
> +	drm_connector_helper_add(&rcrtc->connector,
>  				 &rcar_du_wb_conn_helper_funcs);
>  
>  	return drmm_writeback_connector_init(&rcdu->ddev, wb_conn,
> @@ -231,7 +235,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>  	struct drm_framebuffer *fb;
>  	unsigned int i;
>  
> -	state = rcrtc->writeback.base.state;
> +	state = rcrtc->connector.state;
>  	if (!state || !state->writeback_job)
>  		return;
>  
> @@ -246,10 +250,10 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>  		cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
>  			    + fb->offsets[i];
>  
> -	drm_writeback_queue_job(&rcrtc->writeback, state);
> +	drm_writeback_queue_job(&rcrtc->connector.writeback, state);
>  }
>  
>  void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
>  {
> -	drm_writeback_signal_completion(&rcrtc->writeback, 0);
> +	drm_writeback_signal_completion(&rcrtc->connector.writeback, 0);
>  }

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Laurent Pinchart]

On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> Some drivers cannot work with the current design where the connector
> is embedded within the drm_writeback_connector such as intel and
> some drivers that can get it working end up adding a lot of checks
> all around the code to check if it's a writeback conenctor or not.
> To solve this we move the drm_writeback_connector within the
> drm_connector and remove the drm_connector base which was in
> drm_writeback_connector. We do all other required
> modifications that come with these changes along with addition
> of new function which returns the drm_connector when
> drm_writeback_connector is present.
> All drivers will be expected to allocate the drm_connector.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
>  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
>  include/drm/drm_writeback.h     | 68 ++++-----------------------------
>  3 files changed, 89 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index ec2575c4c21b..198b8c488056 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -89,8 +89,10 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
>  {
>  	struct drm_writeback_connector *wb_connector =
>  		fence_to_wb_connector(fence);
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  
> -	return wb_connector->base.dev->driver->name;
> +	return connector->dev->driver->name;
>  }
>  
>  static const char *
> @@ -187,7 +189,8 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>  					  struct drm_encoder *enc, const u32 *formats,
>  					  int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_property_blob *blob;
>  	int ret = create_writeback_properties(dev);
> @@ -269,7 +272,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  				 struct drm_encoder *enc,
>  				 const u32 *formats, int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  	int ret;
>  
>  	ret = drm_connector_init(dev, connector, con_funcs,
> @@ -339,7 +343,8 @@ int drmm_writeback_connector_init(struct drm_device *dev,
>  				  struct drm_encoder *enc,
>  				  const u32 *formats, int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  	int ret;
>  
>  	ret = drmm_connector_init(dev, connector, con_funcs,
> @@ -382,13 +387,15 @@ int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  
>  int drm_writeback_prepare_job(struct drm_writeback_job *job)
>  {
> -	struct drm_writeback_connector *connector = job->connector;
> +	struct drm_writeback_connector *wb_connector = job->connector;
> +	struct drm_connector *connector
> +		= drm_writeback_to_connector(wb_connector);
>  	const struct drm_connector_helper_funcs *funcs =
> -		connector->base.helper_private;
> +		connector->helper_private;
>  	int ret;
>  
>  	if (funcs->prepare_writeback_job) {
> -		ret = funcs->prepare_writeback_job(connector, job);
> +		ret = funcs->prepare_writeback_job(wb_connector, job);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -434,12 +441,14 @@ EXPORT_SYMBOL(drm_writeback_queue_job);
>  
>  void drm_writeback_cleanup_job(struct drm_writeback_job *job)
>  {
> -	struct drm_writeback_connector *connector = job->connector;
> +	struct drm_writeback_connector *wb_connector = job->connector;
> +	struct drm_connector *connector
> +		= drm_writeback_to_connector(wb_connector);
>  	const struct drm_connector_helper_funcs *funcs =
> -		connector->base.helper_private;
> +		connector->helper_private;
>  
>  	if (job->prepared && funcs->cleanup_writeback_job)
> -		funcs->cleanup_writeback_job(connector, job);
> +		funcs->cleanup_writeback_job(wb_connector, job);
>  
>  	if (job->fb)
>  		drm_framebuffer_put(job->fb);
> @@ -521,8 +530,10 @@ struct dma_fence *
>  drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector)
>  {
>  	struct dma_fence *fence;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  
> -	if (WARN_ON(wb_connector->base.connector_type !=
> +	if (WARN_ON(connector->connector_type !=
>  		    DRM_MODE_CONNECTOR_WRITEBACK))
>  		return NULL;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d..da63fdafd9f2 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1882,6 +1882,61 @@ struct drm_connector_cec {
>  	void *data;
>  };
>  
> +/**
> + * struct drm_writeback_connector - DRM writeback connector
> + */
> +struct drm_writeback_connector {
> +	/**
> +	 * @pixel_formats_blob_ptr:
> +	 *
> +	 * DRM blob property data for the pixel formats list on writeback
> +	 * connectors
> +	 * See also drm_writeback_connector_init()
> +	 */
> +	struct drm_property_blob *pixel_formats_blob_ptr;
> +
> +	/** @job_lock: Protects job_queue */
> +	spinlock_t job_lock;
> +
> +	/**
> +	 * @job_queue:
> +	 *
> +	 * Holds a list of a connector's writeback jobs; the last item is the
> +	 * most recent. The first item may be either waiting for the hardware
> +	 * to begin writing, or currently being written.
> +	 *
> +	 * See also: drm_writeback_queue_job() and
> +	 * drm_writeback_signal_completion()
> +	 */
> +	struct list_head job_queue;
> +
> +	/**
> +	 * @fence_context:
> +	 *
> +	 * timeline context used for fence operations.
> +	 */
> +	unsigned int fence_context;
> +	/**
> +	 * @fence_lock:
> +	 *
> +	 * spinlock to protect the fences in the fence_context.
> +	 */
> +	spinlock_t fence_lock;
> +	/**
> +	 * @fence_seqno:
> +	 *
> +	 * Seqno variable used as monotonic counter for the fences
> +	 * created on the connector's timeline.
> +	 */
> +	unsigned long fence_seqno;
> +	/**
> +	 * @timeline_name:
> +	 *
> +	 * The name of the connector's fence timeline.
> +	 */
> +	char timeline_name[32];
> +};
> +
>  /**
>   * struct drm_connector - central DRM connector control structure
>   *
> @@ -2305,6 +2360,11 @@ struct drm_connector {
>  	 * @cec: CEC-related data.
>  	 */
>  	struct drm_connector_cec cec;
> +
> +	/**
> +	 * @writeback: Writeback related valriables.
> +	 */
> +	struct drm_writeback_connector writeback;

No, sorry, that's a bad idea. Most connectors have nothing to do with
writeback, you shouldn't introduce writeback-specific fields here.
drm_writeback_connector happens to be a drm_connector because of
historical reasons (it was decided to reuse the connector API exposed to
userspace instead of exposing a completely separate API in order to
simplify the implementation), but that does not mean that every
connector is related to writeback.

I don't know what issues the Intel driver(s) have with
drm_writeback_connector, but you shouldn't make things worse for
everybody due to a driver problem.

>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 958466a05e60..2a52b6761797 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -15,66 +15,6 @@
>  #include <drm/drm_encoder.h>
>  #include <linux/workqueue.h>
>  
> -/**
> - * struct drm_writeback_connector - DRM writeback connector
> - */
> -struct drm_writeback_connector {
> -	/**
> -	 * @base: base drm_connector object
> -	 */
> -	struct drm_connector base;
> -
> -	/**
> -	 * @pixel_formats_blob_ptr:
> -	 *
> -	 * DRM blob property data for the pixel formats list on writeback
> -	 * connectors
> -	 * See also drm_writeback_connector_init()
> -	 */
> -	struct drm_property_blob *pixel_formats_blob_ptr;
> -
> -	/** @job_lock: Protects job_queue */
> -	spinlock_t job_lock;
> -
> -	/**
> -	 * @job_queue:
> -	 *
> -	 * Holds a list of a connector's writeback jobs; the last item is the
> -	 * most recent. The first item may be either waiting for the hardware
> -	 * to begin writing, or currently being written.
> -	 *
> -	 * See also: drm_writeback_queue_job() and
> -	 * drm_writeback_signal_completion()
> -	 */
> -	struct list_head job_queue;
> -
> -	/**
> -	 * @fence_context:
> -	 *
> -	 * timeline context used for fence operations.
> -	 */
> -	unsigned int fence_context;
> -	/**
> -	 * @fence_lock:
> -	 *
> -	 * spinlock to protect the fences in the fence_context.
> -	 */
> -	spinlock_t fence_lock;
> -	/**
> -	 * @fence_seqno:
> -	 *
> -	 * Seqno variable used as monotonic counter for the fences
> -	 * created on the connector's timeline.
> -	 */
> -	unsigned long fence_seqno;
> -	/**
> -	 * @timeline_name:
> -	 *
> -	 * The name of the connector's fence timeline.
> -	 */
> -	char timeline_name[32];
> -};
> -
>  /**
>   * struct drm_writeback_job - DRM writeback job
>   */
> @@ -131,10 +71,16 @@ struct drm_writeback_job {
>  	void *priv;
>  };
>  
> +static inline struct drm_connector *
> +drm_writeback_to_connector(struct drm_writeback_connector *wb_connector)
> +{
> +	return container_of(wb_connector, struct drm_connector, writeback);
> +}
> +
>  static inline struct drm_writeback_connector *
>  drm_connector_to_writeback(struct drm_connector *connector)
>  {
> -	return container_of(connector, struct drm_writeback_connector, base);
> +	return &connector->writeback;
>  }
>  
>  int drm_writeback_connector_init(struct drm_device *dev,

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 7/8] drm/rcar_du: Adapt vkms writeback to new drm_writeback_connector

[Louis Chauvet]

I think the commit title is wrong.

Le 11/08/2025 à 11:27, Suraj Kandpal a écrit :
> Now that drm_writeback_connector is embedded with the drm_connector
> adapt the rcar-du writeback functionality to this changes. This
> includes changing the drm_writeback_connector to be changed to drm_connector
> within the rcar_du_crtc.
> Some other changes are done which are a result of the all the above
> changes mentioned.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  4 ++--
>   .../drm/renesas/rcar-du/rcar_du_writeback.c   | 22 +++++++++++--------
>   2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> index d0f38a8b3561..457c803d75bc 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> @@ -72,11 +72,11 @@ struct rcar_du_crtc {
>   	const char *const *sources;
>   	unsigned int sources_count;
>   
> -	struct drm_writeback_connector writeback;
> +	struct drm_connector connector;
>   };
>   
>   #define to_rcar_crtc(c)		container_of(c, struct rcar_du_crtc, crtc)
> -#define wb_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, writeback)
> +#define connector_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, connector)
>   
>   /**
>    * struct rcar_du_crtc_state - Driver-specific CRTC state
> 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 9986a10e8114..95e6810612c2 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> @@ -47,10 +47,12 @@ static int rcar_du_wb_conn_get_modes(struct drm_connector *connector)
>   				    dev->mode_config.max_height);
>   }
>   
> -static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
> +static int rcar_du_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>   				  struct drm_writeback_job *job)
>   {
> -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> +	struct drm_connector *connector =
> +		container_of(wb_connector, struct drm_connector, writeback);
> +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
>   	struct rcar_du_wb_job *rjob;
>   	int ret;
>   
> @@ -72,10 +74,12 @@ static int rcar_du_wb_prepare_job(struct drm_writeback_connector *connector,
>   	return 0;
>   }
>   
> -static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *connector,
> +static void rcar_du_wb_cleanup_job(struct drm_writeback_connector *wb_connector,
>   				   struct drm_writeback_job *job)
>   {
> -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> +	struct drm_connector *connector =
> +		container_of(wb_connector, struct drm_connector, writeback);
> +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
>   	struct rcar_du_wb_job *rjob = job->priv;
>   
>   	if (!job->fb)
> @@ -199,7 +203,7 @@ 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;
> +	struct drm_writeback_connector *wb_conn = &rcrtc->connector.writeback;
>   
>   	struct drm_encoder *encoder;
>   
> @@ -212,7 +216,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>   
>   	encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>   
> -	drm_connector_helper_add(&wb_conn->base,
> +	drm_connector_helper_add(&rcrtc->connector,
>   				 &rcar_du_wb_conn_helper_funcs);
>   
>   	return drmm_writeback_connector_init(&rcdu->ddev, wb_conn,
> @@ -231,7 +235,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>   	struct drm_framebuffer *fb;
>   	unsigned int i;
>   
> -	state = rcrtc->writeback.base.state;
> +	state = rcrtc->connector.state;
>   	if (!state || !state->writeback_job)
>   		return;
>   
> @@ -246,10 +250,10 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>   		cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
>   			    + fb->offsets[i];
>   
> -	drm_writeback_queue_job(&rcrtc->writeback, state);
> +	drm_writeback_queue_job(&rcrtc->connector.writeback, state);
>   }
>   
>   void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)
>   {
> -	drm_writeback_signal_completion(&rcrtc->writeback, 0);
> +	drm_writeback_signal_completion(&rcrtc->connector.writeback, 0);
>   }

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [RFC PATCH 7/8] drm/rcar_du: Adapt vkms writeback to new drm_writeback_connector

[Kandpal, Suraj]

> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  4 ++--
> >  .../drm/renesas/rcar-du/rcar_du_writeback.c   | 22 +++++++++++--------
> >  2 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> > b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> > index d0f38a8b3561..457c803d75bc 100644
> > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
> > @@ -72,11 +72,11 @@ struct rcar_du_crtc {
> >  	const char *const *sources;
> >  	unsigned int sources_count;
> >
> > -	struct drm_writeback_connector writeback;
> > +	struct drm_connector connector;
> 
> You forgot to update the documentation of the structure.
> 
> "connector" is a too generic name. I'd keep the existing field name.
> 

Sure will leave the name as writeback 

> >  };
> >
> >  #define to_rcar_crtc(c)		container_of(c, struct rcar_du_crtc,
> crtc)
> > -#define wb_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc, writeback)
> > +#define connector_to_rcar_crtc(c)	container_of(c, struct rcar_du_crtc,
> connector)
> >
> >  /**
> >   * struct rcar_du_crtc_state - Driver-specific CRTC state 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 9986a10e8114..95e6810612c2 100644
> > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_writeback.c
> > @@ -47,10 +47,12 @@ static int rcar_du_wb_conn_get_modes(struct
> drm_connector *connector)
> >  				    dev->mode_config.max_height);  }
> >
> > -static int rcar_du_wb_prepare_job(struct drm_writeback_connector
> > *connector,
> > +static int rcar_du_wb_prepare_job(struct drm_writeback_connector
> > +*wb_connector,
> >  				  struct drm_writeback_job *job)
> >  {
> > -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> > +	struct drm_connector *connector =
> > +		container_of(wb_connector, struct drm_connector, writeback);
> > +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
> 
> Modify wb_to_rcar_crtc() instead of changing the code here and below.
> 

Sure will modify that when I send this as a real series once I get the Ack for this design.

Regards,
Suraj Kandpal

> >  	struct rcar_du_wb_job *rjob;
> >  	int ret;
> >
> > @@ -72,10 +74,12 @@ static int rcar_du_wb_prepare_job(struct
> drm_writeback_connector *connector,
> >  	return 0;
> >  }
> >
> > -static void rcar_du_wb_cleanup_job(struct drm_writeback_connector
> > *connector,
> > +static void rcar_du_wb_cleanup_job(struct drm_writeback_connector
> > +*wb_connector,
> >  				   struct drm_writeback_job *job)  {
> > -	struct rcar_du_crtc *rcrtc = wb_to_rcar_crtc(connector);
> > +	struct drm_connector *connector =
> > +		container_of(wb_connector, struct drm_connector, writeback);
> > +	struct rcar_du_crtc *rcrtc = connector_to_rcar_crtc(connector);
> >  	struct rcar_du_wb_job *rjob = job->priv;
> >
> >  	if (!job->fb)
> > @@ -199,7 +203,7 @@ 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;
> > +	struct drm_writeback_connector *wb_conn =
> > +&rcrtc->connector.writeback;
> >
> >  	struct drm_encoder *encoder;
> >
> > @@ -212,7 +216,7 @@ int rcar_du_writeback_init(struct rcar_du_device
> > *rcdu,
> >
> >  	encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> >
> > -	drm_connector_helper_add(&wb_conn->base,
> > +	drm_connector_helper_add(&rcrtc->connector,
> >  				 &rcar_du_wb_conn_helper_funcs);
> >
> >  	return drmm_writeback_connector_init(&rcdu->ddev, wb_conn, @@ -
> 231,7
> > +235,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> >  	struct drm_framebuffer *fb;
> >  	unsigned int i;
> >
> > -	state = rcrtc->writeback.base.state;
> > +	state = rcrtc->connector.state;
> >  	if (!state || !state->writeback_job)
> >  		return;
> >
> > @@ -246,10 +250,10 @@ void rcar_du_writeback_setup(struct rcar_du_crtc
> *rcrtc,
> >  		cfg->mem[i] = sg_dma_address(rjob->sg_tables[i].sgl)
> >  			    + fb->offsets[i];
> >
> > -	drm_writeback_queue_job(&rcrtc->writeback, state);
> > +	drm_writeback_queue_job(&rcrtc->connector.writeback, state);
> >  }
> >
> >  void rcar_du_writeback_complete(struct rcar_du_crtc *rcrtc)  {
> > -	drm_writeback_signal_completion(&rcrtc->writeback, 0);
> > +	drm_writeback_signal_completion(&rcrtc->connector.writeback, 0);
> >  }
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [RFC PATCH 6/8] drm/vkms: Adapt vkms writeback to new drm_writeback_connector

[Louis Chauvet]

Le 11/08/2025 à 11:27, Suraj Kandpal a écrit :
> Now that drm_writeback_connector is embedded with the drm_connector
> adapt the vkms writeback functionality to this changes. This
> includes changing the drm_writeback_connector to be changed to
> drm_connector within the vkms_output.
> Some other changes are done which are a result of the all the above
> changes mentioned.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   drivers/gpu/drm/vkms/vkms_composer.c  |  2 +-
>   drivers/gpu/drm/vkms/vkms_drv.h       |  2 +-
>   drivers/gpu/drm/vkms/vkms_writeback.c | 15 +++++++++------
>   3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index fa269d279e25..b5f20637121c 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -543,7 +543,7 @@ void vkms_composer_worker(struct work_struct *work)
>   		return;
>   
>   	if (wb_pending) {
> -		drm_writeback_signal_completion(&out->wb_connector, 0);
> +		drm_writeback_signal_completion(&out->connector.writeback, 0);
>   		spin_lock_irq(&out->composer_lock);
>   		crtc_state->wb_pending = false;
>   		spin_unlock_irq(&out->composer_lock);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8013c31efe3b..2e58a06c9ad8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -213,7 +213,7 @@ struct vkms_crtc_state {
>    */
>   struct vkms_output {
>   	struct drm_crtc crtc;
> -	struct drm_writeback_connector wb_connector;
> +	struct drm_connector connector;

Can you keep wb_connector here?

>   	struct drm_encoder wb_encoder;
>   	struct hrtimer vblank_hrtimer;
>   	ktime_t period_ns;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 45d69a3b85f6..13c2a5c8f57a 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -102,13 +102,16 @@ static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
>   	return ret;
>   }
>   
> -static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> +static void vkms_wb_cleanup_job(struct drm_writeback_connector *wb_connector,
>   				struct drm_writeback_job *job)
>   {
>   	struct vkms_writeback_job *vkmsjob = job->priv;
> +	struct drm_connector *connector = container_of(wb_connector,
> +						       struct drm_connector,
> +						       writeback);
>   	struct vkms_output *vkms_output = container_of(connector,
>   						       struct vkms_output,
> -						       wb_connector);
> +						       connector);
>   
>   	if (!job->fb)
>   		return;
> @@ -127,8 +130,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>   	struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
>   											 conn);
>   	struct vkms_output *output = drm_crtc_to_vkms_output(connector_state->crtc);
> -	struct drm_writeback_connector *wb_conn = &output->wb_connector;
> -	struct drm_connector_state *conn_state = wb_conn->base.state;
> +	struct drm_writeback_connector *wb_conn = &output->connector.writeback;
> +	struct drm_connector_state *conn_state = output->connector.state;
>   	struct vkms_crtc_state *crtc_state = output->composer_state;
>   	struct drm_framebuffer *fb = connector_state->writeback_job->fb;
>   	u16 crtc_height = crtc_state->base.mode.vdisplay;
> @@ -166,7 +169,7 @@ 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;
> +	struct drm_writeback_connector *wb = &vkms_output->connector.writeback;
>   	int ret;
>   
>   	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output->wb_encoder,
> @@ -177,7 +180,7 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
>   	vkms_output->wb_encoder.possible_clones |=
>   		drm_encoder_mask(&vkms_output->wb_encoder);
>   
> -	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> +	drm_connector_helper_add(&vkms_output->connector, &vkms_wb_conn_helper_funcs);
>   
>   	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
>   					     &vkms_wb_connector_funcs,

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> Some drivers cannot work with the current design where the connector
> is embedded within the drm_writeback_connector such as intel and
> some drivers that can get it working end up adding a lot of checks
> all around the code to check if it's a writeback conenctor or not.
> To solve this we move the drm_writeback_connector within the
> drm_connector and remove the drm_connector base which was in
> drm_writeback_connector. We do all other required
> modifications that come with these changes along with addition
> of new function which returns the drm_connector when
> drm_writeback_connector is present.
> All drivers will be expected to allocate the drm_connector.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
>  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
>  include/drm/drm_writeback.h     | 68 ++++-----------------------------
>  3 files changed, 89 insertions(+), 72 deletions(-)

This patch breaks building of drivers:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c: In function ‘dpu_encoder_phys_wb_prepare_for_kickoff’:
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c:487:36: error: ‘struct drm_writeback_connector’ has no member named ‘base’
  487 |         drm_conn = &wb_enc->wb_conn->base;
      |                                    ^~

Please perform step-by-step modifications, making sure that on each
step all the drivers can be built and function as expected.

> 
> @@ -2305,6 +2360,11 @@ struct drm_connector {
>  	 * @cec: CEC-related data.
>  	 */
>  	struct drm_connector_cec cec;
> +
> +	/**
> +	 * @writeback: Writeback related valriables.
> +	 */
> +	struct drm_writeback_connector writeback;

I will respond to this in another thread.

>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Mon, Aug 11, 2025 at 12:44:29PM +0300, Laurent Pinchart wrote:
> On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> > Some drivers cannot work with the current design where the connector
> > is embedded within the drm_writeback_connector such as intel and
> > some drivers that can get it working end up adding a lot of checks
> > all around the code to check if it's a writeback conenctor or not.
> > To solve this we move the drm_writeback_connector within the
> > drm_connector and remove the drm_connector base which was in
> > drm_writeback_connector. We do all other required
> > modifications that come with these changes along with addition
> > of new function which returns the drm_connector when
> > drm_writeback_connector is present.
> > All drivers will be expected to allocate the drm_connector.
> > 
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
> >  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
> >  include/drm/drm_writeback.h     | 68 ++++-----------------------------
> >  3 files changed, 89 insertions(+), 72 deletions(-)
> > 
> > @@ -2305,6 +2360,11 @@ struct drm_connector {
> >  	 * @cec: CEC-related data.
> >  	 */
> >  	struct drm_connector_cec cec;
> > +
> > +	/**
> > +	 * @writeback: Writeback related valriables.
> > +	 */
> > +	struct drm_writeback_connector writeback;
> 
> No, sorry, that's a bad idea. Most connectors have nothing to do with
> writeback, you shouldn't introduce writeback-specific fields here.
> drm_writeback_connector happens to be a drm_connector because of
> historical reasons (it was decided to reuse the connector API exposed to
> userspace instead of exposing a completely separate API in order to
> simplify the implementation), but that does not mean that every
> connector is related to writeback.
> 
> I don't know what issues the Intel driver(s) have with
> drm_writeback_connector, but you shouldn't make things worse for
> everybody due to a driver problem.

Suraj is trying to solve a problem that in Intel code every drm_connector
must be an intel_connector too. His previous attempt resulted in a loose
abstraction where drm_writeback_connector.base wasn't initialized in
some cases (which is a bad idea IMO).

I know the historical reasons for drm_writeback_connector, but I think
we can do better now.

So, I think, a proper approach would be:

struct drm_connector {
    // other fields

    union {
        struct drm_connector_hdmi hdmi; // we already have it
        struct drm_connector_wb wb;  // this is new
    };

    // rest of the fields.
};

I plan to add drm_connector_dp in a similar way, covering DP needs
(currently WIP).

-- 
With best wishes
Dmitry

Re: [RFC PATCH 8/8] drm/msm/dpu: Adapt dpu writeback to new drm_writeback_connector

[Dmitry Baryshkov]

On Mon, Aug 11, 2025 at 02:57:07PM +0530, Suraj Kandpal wrote:
> Now that drm_writeback_connector is embedded with the drm_connector
> adapt the dpu writeback functionality to this changes. This
> includes changing the drm_writeback_connector to be changed to
> drm_connector within the dpu_wb_connector.
> Some other changes are done which are a result of the all the above
> changes mentioned.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  |  3 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c    | 16 +++++++++-------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h    |  4 ++--
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index 56a5b596554d..0e60c1ac07c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -484,7 +484,8 @@ static void dpu_encoder_phys_wb_prepare_for_kickoff(
>  		return;
>  	}
>  
> -	drm_conn = &wb_enc->wb_conn->base;
> +	drm_conn =
> +		container_of(wb_enc->wb_conn, struct drm_connector, writeback);

Just store drm_connector in dpu_encoder_phys_wb instead of
drm_writeback_connector.

>  	state = drm_conn->state;
>  
>  	if (wb_enc->wb_conn && wb_enc->wb_job)

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> Some drivers cannot work with the current design where the connector
> is embedded within the drm_writeback_connector such as intel and
> some drivers that can get it working end up adding a lot of checks
> all around the code to check if it's a writeback conenctor or not.
> To solve this we move the drm_writeback_connector within the
> drm_connector and remove the drm_connector base which was in
> drm_writeback_connector. We do all other required
> modifications that come with these changes along with addition
> of new function which returns the drm_connector when
> drm_writeback_connector is present.
> All drivers will be expected to allocate the drm_connector.
> 
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
>  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
>  include/drm/drm_writeback.h     | 68 ++++-----------------------------
>  3 files changed, 89 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index ec2575c4c21b..198b8c488056 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -89,8 +89,10 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence)
>  {
>  	struct drm_writeback_connector *wb_connector =
>  		fence_to_wb_connector(fence);
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  
> -	return wb_connector->base.dev->driver->name;
> +	return connector->dev->driver->name;
>  }
>  
>  static const char *
> @@ -187,7 +189,8 @@ static int __drm_writeback_connector_init(struct drm_device *dev,
>  					  struct drm_encoder *enc, const u32 *formats,
>  					  int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_property_blob *blob;
>  	int ret = create_writeback_properties(dev);
> @@ -269,7 +272,8 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  				 struct drm_encoder *enc,
>  				 const u32 *formats, int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);

Please pass drm_connector instead (to all init functions). It would make
more sense.

>  	int ret;
>  
>  	ret = drm_connector_init(dev, connector, con_funcs,
> @@ -339,7 +343,8 @@ int drmm_writeback_connector_init(struct drm_device *dev,
>  				  struct drm_encoder *enc,
>  				  const u32 *formats, int n_formats)
>  {
> -	struct drm_connector *connector = &wb_connector->base;
> +	struct drm_connector *connector =
> +		drm_writeback_to_connector(wb_connector);
>  	int ret;
>  
>  	ret = drmm_connector_init(dev, connector, con_funcs,

-- 
With best wishes
Dmitry

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> 
> This patch breaks building of drivers:
> 
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c: In function
> ‘dpu_encoder_phys_wb_prepare_for_kickoff’:
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c:487:36: error:
> ‘struct drm_writeback_connector’ has no member named ‘base’
>   487 |         drm_conn = &wb_enc->wb_conn->base;
>       |                                    ^~
> 
> Please perform step-by-step modifications, making sure that on each step all
> the drivers can be built and function as expected.

Yes I am aware of that currently sent this series out like this for the reason that all drivers
can see all the changes that come in the same patch and comment if they want it done a particular
way.
Otherwise I end up having to make patches with all the driver patches in the same patches.
Anyways when I send a more final version of these patches I'll have it done that way.

Regards,
Suraj Kandpal

> 
> >
> > @@ -2305,6 +2360,11 @@ struct drm_connector {
> >  	 * @cec: CEC-related data.
> >  	 */
> >  	struct drm_connector_cec cec;
> > +
> > +	/**
> > +	 * @writeback: Writeback related valriables.
> > +	 */
> > +	struct drm_writeback_connector writeback;
> 
> I will respond to this in another thread.
> 
> >  };
> >
> >  #define obj_to_connector(x) container_of(x, struct drm_connector,
> > base)
> 
> --
> With best wishes
> Dmitry

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> >  static const char *
> > @@ -187,7 +189,8 @@ static int __drm_writeback_connector_init(struct
> drm_device *dev,
> >  					  struct drm_encoder *enc, const u32
> *formats,
> >  					  int n_formats)
> >  {
> > -	struct drm_connector *connector = &wb_connector->base;
> > +	struct drm_connector *connector =
> > +		drm_writeback_to_connector(wb_connector);
> >  	struct drm_mode_config *config = &dev->mode_config;
> >  	struct drm_property_blob *blob;
> >  	int ret = create_writeback_properties(dev); @@ -269,7 +272,8 @@ int
> > drm_writeback_connector_init(struct drm_device *dev,
> >  				 struct drm_encoder *enc,
> >  				 const u32 *formats, int n_formats)  {
> > -	struct drm_connector *connector = &wb_connector->base;
> > +	struct drm_connector *connector =
> > +		drm_writeback_to_connector(wb_connector);
> 
> Please pass drm_connector instead (to all init functions). It would make more
> sense.

Was thinking around the same lines too let's see how other react to this RFC series.

Regards,
Suraj Kandpal

> 
> >  	int ret;
> >
> >  	ret = drm_connector_init(dev, connector, con_funcs, @@ -339,7 +343,8
> > @@ int drmm_writeback_connector_init(struct drm_device *dev,
> >  				  struct drm_encoder *enc,
> >  				  const u32 *formats, int n_formats)  {
> > -	struct drm_connector *connector = &wb_connector->base;
> > +	struct drm_connector *connector =
> > +		drm_writeback_to_connector(wb_connector);
> >  	int ret;
> >
> >  	ret = drmm_connector_init(dev, connector, con_funcs,
> 
> --
> With best wishes
> Dmitry

RE: [RFC PATCH 8/8] drm/msm/dpu: Adapt dpu writeback to new drm_writeback_connector

[Kandpal, Suraj]

> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c  |  3 ++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c    | 16 +++++++++-------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h    |  4 ++--
> >  3 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > index 56a5b596554d..0e60c1ac07c5 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> > @@ -484,7 +484,8 @@ static void
> dpu_encoder_phys_wb_prepare_for_kickoff(
> >  		return;
> >  	}
> >
> > -	drm_conn = &wb_enc->wb_conn->base;
> > +	drm_conn =
> > +		container_of(wb_enc->wb_conn, struct drm_connector,
> writeback);
> 
> Just store drm_connector in dpu_encoder_phys_wb instead of
> drm_writeback_connector.
> 

Sure will keep that in mind in the next series

Regards,
Suraj Kandpal

> >  	state = drm_conn->state;
> >
> >  	if (wb_enc->wb_conn && wb_enc->wb_job)
> 
> --
> With best wishes
> Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Laurent Pinchart]

On Mon, Aug 11, 2025 at 01:22:30PM +0300, Dmitry Baryshkov wrote:
> On Mon, Aug 11, 2025 at 12:44:29PM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> > > Some drivers cannot work with the current design where the connector
> > > is embedded within the drm_writeback_connector such as intel and
> > > some drivers that can get it working end up adding a lot of checks
> > > all around the code to check if it's a writeback conenctor or not.
> > > To solve this we move the drm_writeback_connector within the
> > > drm_connector and remove the drm_connector base which was in
> > > drm_writeback_connector. We do all other required
> > > modifications that come with these changes along with addition
> > > of new function which returns the drm_connector when
> > > drm_writeback_connector is present.
> > > All drivers will be expected to allocate the drm_connector.
> > > 
> > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
> > >  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
> > >  include/drm/drm_writeback.h     | 68 ++++-----------------------------
> > >  3 files changed, 89 insertions(+), 72 deletions(-)
> > > 
> > > @@ -2305,6 +2360,11 @@ struct drm_connector {
> > >  	 * @cec: CEC-related data.
> > >  	 */
> > >  	struct drm_connector_cec cec;
> > > +
> > > +	/**
> > > +	 * @writeback: Writeback related valriables.
> > > +	 */
> > > +	struct drm_writeback_connector writeback;
> > 
> > No, sorry, that's a bad idea. Most connectors have nothing to do with
> > writeback, you shouldn't introduce writeback-specific fields here.
> > drm_writeback_connector happens to be a drm_connector because of
> > historical reasons (it was decided to reuse the connector API exposed to
> > userspace instead of exposing a completely separate API in order to
> > simplify the implementation), but that does not mean that every
> > connector is related to writeback.
> > 
> > I don't know what issues the Intel driver(s) have with
> > drm_writeback_connector, but you shouldn't make things worse for
> > everybody due to a driver problem.
> 
> Suraj is trying to solve a problem that in Intel code every drm_connector
> must be an intel_connector too. His previous attempt resulted in a loose
> abstraction where drm_writeback_connector.base wasn't initialized in
> some cases (which is a bad idea IMO).
> 
> I know the historical reasons for drm_writeback_connector, but I think
> we can do better now.
> 
> So, I think, a proper approach would be:
> 
> struct drm_connector {
>     // other fields
> 
>     union {
>         struct drm_connector_hdmi hdmi; // we already have it
>         struct drm_connector_wb wb;  // this is new
>     };
> 
>     // rest of the fields.
> };

I still don't like that. This really doesn't belong here. If anything,
the drm_connector for writeback belongs to drm_crtc.

If the issue is that some drivers need a custom drm_connector subclass,
then I'd rather turn the connector field of drm_writeback_connector into
a pointer.

> I plan to add drm_connector_dp in a similar way, covering DP needs
> (currently WIP).

-- 
Regards,

Laurent Pinchart

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> > > @@ -2305,6 +2360,11 @@ struct drm_connector {
> > >  	 * @cec: CEC-related data.
> > >  	 */
> > >  	struct drm_connector_cec cec;
> > > +
> > > +	/**
> > > +	 * @writeback: Writeback related valriables.
> > > +	 */
> > > +	struct drm_writeback_connector writeback;
> >
> > No, sorry, that's a bad idea. Most connectors have nothing to do with
> > writeback, you shouldn't introduce writeback-specific fields here.
> > drm_writeback_connector happens to be a drm_connector because of
> > historical reasons (it was decided to reuse the connector API exposed
> > to userspace instead of exposing a completely separate API in order to
> > simplify the implementation), but that does not mean that every
> > connector is related to writeback.
> >
> > I don't know what issues the Intel driver(s) have with
> > drm_writeback_connector, but you shouldn't make things worse for
> > everybody due to a driver problem.
> 
> Suraj is trying to solve a problem that in Intel code every drm_connector must
> be an intel_connector too. His previous attempt resulted in a loose abstraction
> where drm_writeback_connector.base wasn't initialized in some cases (which is
> a bad idea IMO).
> 
> I know the historical reasons for drm_writeback_connector, but I think we can
> do better now.
> 
> So, I think, a proper approach would be:
> 
> struct drm_connector {
>     // other fields
> 
>     union {
>         struct drm_connector_hdmi hdmi; // we already have it
>         struct drm_connector_wb wb;  // this is new
>     };
> 
>     // rest of the fields.
> };
> 
> I plan to add drm_connector_dp in a similar way, covering DP needs (currently
> WIP).
> 

Right we are seeking to get an ACK on this design.

Regards,
Suraj Kandpal

> --
> With best wishes
> Dmitry

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> > > > @@ -2305,6 +2360,11 @@ struct drm_connector {
> > > >  	 * @cec: CEC-related data.
> > > >  	 */
> > > >  	struct drm_connector_cec cec;
> > > > +
> > > > +	/**
> > > > +	 * @writeback: Writeback related valriables.
> > > > +	 */
> > > > +	struct drm_writeback_connector writeback;
> > >
> > > No, sorry, that's a bad idea. Most connectors have nothing to do
> > > with writeback, you shouldn't introduce writeback-specific fields here.
> > > drm_writeback_connector happens to be a drm_connector because of
> > > historical reasons (it was decided to reuse the connector API
> > > exposed to userspace instead of exposing a completely separate API
> > > in order to simplify the implementation), but that does not mean
> > > that every connector is related to writeback.
> > >
> > > I don't know what issues the Intel driver(s) have with
> > > drm_writeback_connector, but you shouldn't make things worse for
> > > everybody due to a driver problem.
> >
> > Suraj is trying to solve a problem that in Intel code every
> > drm_connector must be an intel_connector too. His previous attempt
> > resulted in a loose abstraction where drm_writeback_connector.base
> > wasn't initialized in some cases (which is a bad idea IMO).
> >
> > I know the historical reasons for drm_writeback_connector, but I think
> > we can do better now.
> >
> > So, I think, a proper approach would be:
> >
> > struct drm_connector {
> >     // other fields
> >
> >     union {
> >         struct drm_connector_hdmi hdmi; // we already have it
> >         struct drm_connector_wb wb;  // this is new
> >     };
> >
> >     // rest of the fields.
> > };
> 
> I still don't like that. This really doesn't belong here. If anything, the
> drm_connector for writeback belongs to drm_crtc.
> 
> If the issue is that some drivers need a custom drm_connector subclass, then
> I'd rather turn the connector field of drm_writeback_connector into a pointer.
> 

This design or turning drm_connector to inside drm_writeback_connector to a pointer
I am okay either way.

Regards,
Suraj Kandpal

> > I plan to add drm_connector_dp in a similar way, covering DP needs
> > (currently WIP).
> 
> --
> Regards,
> 
> Laurent Pinchart

RE: [RFC PATCH 6/8] drm/vkms: Adapt vkms writeback to new drm_writeback_connector

[Kandpal, Suraj]

> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -213,7 +213,7 @@ struct vkms_crtc_state {
> >    */
> >   struct vkms_output {
> >   	struct drm_crtc crtc;
> > -	struct drm_writeback_connector wb_connector;
> > +	struct drm_connector connector;
> 
> Can you keep wb_connector here?

Reason for keeping drm_connector here is that drm_writeback_connector now resides within
drm_connector so kzalloc for drm_writeback_connector wont allocate memory for drm_connector
hence the reason for drm_connector.
Unless you meant that I keep the variable name as wb_connector then yes can be done 😃

Regards,
Suraj Kandpal

> 
> >   	struct drm_encoder wb_encoder;
> >   	struct hrtimer vblank_hrtimer;
> >   	ktime_t period_ns;
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
> > b/drivers/gpu/drm/vkms/vkms_writeback.c
> > index 45d69a3b85f6..13c2a5c8f57a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -102,13 +102,16 @@ static int vkms_wb_prepare_job(struct
> drm_writeback_connector *wb_connector,
> >   	return ret;
> >   }
> >
> > -static void vkms_wb_cleanup_job(struct drm_writeback_connector
> > *connector,
> > +static void vkms_wb_cleanup_job(struct drm_writeback_connector
> > +*wb_connector,
> >   				struct drm_writeback_job *job)
> >   {
> >   	struct vkms_writeback_job *vkmsjob = job->priv;
> > +	struct drm_connector *connector = container_of(wb_connector,
> > +						       struct drm_connector,
> > +						       writeback);
> >   	struct vkms_output *vkms_output = container_of(connector,
> >   						       struct vkms_output,
> > -						       wb_connector);
> > +						       connector);
> >
> >   	if (!job->fb)
> >   		return;
> > @@ -127,8 +130,8 @@ static void vkms_wb_atomic_commit(struct
> drm_connector *conn,
> >   	struct drm_connector_state *connector_state =
> drm_atomic_get_new_connector_state(state,
> >
> 		 conn);
> >   	struct vkms_output *output =
> drm_crtc_to_vkms_output(connector_state->crtc);
> > -	struct drm_writeback_connector *wb_conn = &output-
> >wb_connector;
> > -	struct drm_connector_state *conn_state = wb_conn->base.state;
> > +	struct drm_writeback_connector *wb_conn = &output-
> >connector.writeback;
> > +	struct drm_connector_state *conn_state = output->connector.state;
> >   	struct vkms_crtc_state *crtc_state = output->composer_state;
> >   	struct drm_framebuffer *fb = connector_state->writeback_job->fb;
> >   	u16 crtc_height = crtc_state->base.mode.vdisplay; @@ -166,7 +169,7
> > @@ 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;
> > +	struct drm_writeback_connector *wb =
> > +&vkms_output->connector.writeback;
> >   	int ret;
> >
> >   	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output-
> >wb_encoder, @@
> > -177,7 +180,7 @@ int vkms_enable_writeback_connector(struct vkms_device
> *vkmsdev,
> >   	vkms_output->wb_encoder.possible_clones |=
> >   		drm_encoder_mask(&vkms_output->wb_encoder);
> >
> > -	drm_connector_helper_add(&wb->base,
> &vkms_wb_conn_helper_funcs);
> > +	drm_connector_helper_add(&vkms_output->connector,
> > +&vkms_wb_conn_helper_funcs);
> >
> >   	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
> >   					     &vkms_wb_connector_funcs,
> 
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Mon, Aug 11, 2025 at 02:15:46PM +0300, Laurent Pinchart wrote:
> On Mon, Aug 11, 2025 at 01:22:30PM +0300, Dmitry Baryshkov wrote:
> > On Mon, Aug 11, 2025 at 12:44:29PM +0300, Laurent Pinchart wrote:
> > > On Mon, Aug 11, 2025 at 02:57:00PM +0530, Suraj Kandpal wrote:
> > > > Some drivers cannot work with the current design where the connector
> > > > is embedded within the drm_writeback_connector such as intel and
> > > > some drivers that can get it working end up adding a lot of checks
> > > > all around the code to check if it's a writeback conenctor or not.
> > > > To solve this we move the drm_writeback_connector within the
> > > > drm_connector and remove the drm_connector base which was in
> > > > drm_writeback_connector. We do all other required
> > > > modifications that come with these changes along with addition
> > > > of new function which returns the drm_connector when
> > > > drm_writeback_connector is present.
> > > > All drivers will be expected to allocate the drm_connector.
> > > > 
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_writeback.c | 33 ++++++++++------
> > > >  include/drm/drm_connector.h     | 60 +++++++++++++++++++++++++++++
> > > >  include/drm/drm_writeback.h     | 68 ++++-----------------------------
> > > >  3 files changed, 89 insertions(+), 72 deletions(-)
> > > > 
> > > > @@ -2305,6 +2360,11 @@ struct drm_connector {
> > > >  	 * @cec: CEC-related data.
> > > >  	 */
> > > >  	struct drm_connector_cec cec;
> > > > +
> > > > +	/**
> > > > +	 * @writeback: Writeback related valriables.
> > > > +	 */
> > > > +	struct drm_writeback_connector writeback;
> > > 
> > > No, sorry, that's a bad idea. Most connectors have nothing to do with
> > > writeback, you shouldn't introduce writeback-specific fields here.
> > > drm_writeback_connector happens to be a drm_connector because of
> > > historical reasons (it was decided to reuse the connector API exposed to
> > > userspace instead of exposing a completely separate API in order to
> > > simplify the implementation), but that does not mean that every
> > > connector is related to writeback.
> > > 
> > > I don't know what issues the Intel driver(s) have with
> > > drm_writeback_connector, but you shouldn't make things worse for
> > > everybody due to a driver problem.
> > 
> > Suraj is trying to solve a problem that in Intel code every drm_connector
> > must be an intel_connector too. His previous attempt resulted in a loose
> > abstraction where drm_writeback_connector.base wasn't initialized in
> > some cases (which is a bad idea IMO).
> > 
> > I know the historical reasons for drm_writeback_connector, but I think
> > we can do better now.
> > 
> > So, I think, a proper approach would be:
> > 
> > struct drm_connector {
> >     // other fields
> > 
> >     union {
> >         struct drm_connector_hdmi hdmi; // we already have it
> >         struct drm_connector_wb wb;  // this is new
> >     };
> > 
> >     // rest of the fields.
> > };
> 
> I still don't like that. This really doesn't belong here. If anything,
> the drm_connector for writeback belongs to drm_crtc.

Why? We already have generic HDMI field inside drm_connector. I am
really hoping to be able to land DP parts next to it. In theory we can
have a DVI-specific entry there (e.g. with the subconnector type).
The idea is not to limit how the drivers subclass those structures.

I don't see a good case why WB should deviate from that design.

> If the issue is that some drivers need a custom drm_connector subclass,
> then I'd rather turn the connector field of drm_writeback_connector into
> a pointer.

Having a pointer requires additional ops in order to get drm_connector
from WB code and vice versa. Having drm_connector_wb inside
drm_connector saves us from those ops (which don't manifest for any
other kind of structure). Nor will it take any more space since union
will reuse space already taken up by HDMI part.

> 
> > I plan to add drm_connector_dp in a similar way, covering DP needs
> > (currently WIP).

-- 
With best wishes
Dmitry

Re: [RFC PATCH 6/8] drm/vkms: Adapt vkms writeback to new drm_writeback_connector

[Louis Chauvet]

Le 11/08/2025 à 13:23, Kandpal, Suraj a écrit :
>>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>>> @@ -213,7 +213,7 @@ struct vkms_crtc_state {
>>>     */
>>>    struct vkms_output {
>>>    	struct drm_crtc crtc;
>>> -	struct drm_writeback_connector wb_connector;
>>> +	struct drm_connector connector;
>>
>> Can you keep wb_connector here?
> 
> Reason for keeping drm_connector here is that drm_writeback_connector now resides within
> drm_connector so kzalloc for drm_writeback_connector wont allocate memory for drm_connector
> hence the reason for drm_connector.
> Unless you meant that I keep the variable name as wb_connector then yes can be done 😃

Ho yes sorry, that was the variable name yes!

> Regards,
> Suraj Kandpal
> 
>>
>>>    	struct drm_encoder wb_encoder;
>>>    	struct hrtimer vblank_hrtimer;
>>>    	ktime_t period_ns;
>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
>>> b/drivers/gpu/drm/vkms/vkms_writeback.c
>>> index 45d69a3b85f6..13c2a5c8f57a 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>>> @@ -102,13 +102,16 @@ static int vkms_wb_prepare_job(struct
>> drm_writeback_connector *wb_connector,
>>>    	return ret;
>>>    }
>>>
>>> -static void vkms_wb_cleanup_job(struct drm_writeback_connector
>>> *connector,
>>> +static void vkms_wb_cleanup_job(struct drm_writeback_connector
>>> +*wb_connector,
>>>    				struct drm_writeback_job *job)
>>>    {
>>>    	struct vkms_writeback_job *vkmsjob = job->priv;
>>> +	struct drm_connector *connector = container_of(wb_connector,
>>> +						       struct drm_connector,
>>> +						       writeback);
>>>    	struct vkms_output *vkms_output = container_of(connector,
>>>    						       struct vkms_output,
>>> -						       wb_connector);
>>> +						       connector);
>>>
>>>    	if (!job->fb)
>>>    		return;
>>> @@ -127,8 +130,8 @@ static void vkms_wb_atomic_commit(struct
>> drm_connector *conn,
>>>    	struct drm_connector_state *connector_state =
>> drm_atomic_get_new_connector_state(state,
>>>
>> 		 conn);
>>>    	struct vkms_output *output =
>> drm_crtc_to_vkms_output(connector_state->crtc);
>>> -	struct drm_writeback_connector *wb_conn = &output-
>>> wb_connector;
>>> -	struct drm_connector_state *conn_state = wb_conn->base.state;
>>> +	struct drm_writeback_connector *wb_conn = &output-
>>> connector.writeback;
>>> +	struct drm_connector_state *conn_state = output->connector.state;
>>>    	struct vkms_crtc_state *crtc_state = output->composer_state;
>>>    	struct drm_framebuffer *fb = connector_state->writeback_job->fb;
>>>    	u16 crtc_height = crtc_state->base.mode.vdisplay; @@ -166,7 +169,7
>>> @@ 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;
>>> +	struct drm_writeback_connector *wb =
>>> +&vkms_output->connector.writeback;
>>>    	int ret;
>>>
>>>    	ret = drmm_encoder_init(&vkmsdev->drm, &vkms_output-
>>> wb_encoder, @@
>>> -177,7 +180,7 @@ int vkms_enable_writeback_connector(struct vkms_device
>> *vkmsdev,
>>>    	vkms_output->wb_encoder.possible_clones |=
>>>    		drm_encoder_mask(&vkms_output->wb_encoder);
>>>
>>> -	drm_connector_helper_add(&wb->base,
>> &vkms_wb_conn_helper_funcs);
>>> +	drm_connector_helper_add(&vkms_output->connector,
>>> +&vkms_wb_conn_helper_funcs);
>>>
>>>    	return drmm_writeback_connector_init(&vkmsdev->drm, wb,
>>>    					     &vkms_wb_connector_funcs,
>>
>> --
>> Louis Chauvet, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> > > };
> >
> > I still don't like that. This really doesn't belong here. If anything,
> > the drm_connector for writeback belongs to drm_crtc.
> 
> Why? We already have generic HDMI field inside drm_connector. I am really
> hoping to be able to land DP parts next to it. In theory we can have a DVI-
> specific entry there (e.g. with the subconnector type).
> The idea is not to limit how the drivers subclass those structures.
> 
> I don't see a good case why WB should deviate from that design.
> 
> > If the issue is that some drivers need a custom drm_connector
> > subclass, then I'd rather turn the connector field of
> > drm_writeback_connector into a pointer.
> 
> Having a pointer requires additional ops in order to get drm_connector from
> WB code and vice versa. Having drm_connector_wb inside drm_connector
> saves us from those ops (which don't manifest for any other kind of structure).
> Nor will it take any more space since union will reuse space already taken up by
> HDMI part.
> 
> >

Seems like this thread has died. We need to get a conclusion on the design.
Laurent do you have any issue with the design given Dmitry's explanation as to why this
Design is good for drm_writeback_connector.

Regards,
Suraj Kandpal

> > > I plan to add drm_connector_dp in a similar way, covering DP needs
> > > (currently WIP).
> 
> --
> With best wishes
> Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Laurent Pinchart]

On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > };
> > >
> > > I still don't like that. This really doesn't belong here. If anything,
> > > the drm_connector for writeback belongs to drm_crtc.
> > 
> > Why? We already have generic HDMI field inside drm_connector. I am really
> > hoping to be able to land DP parts next to it. In theory we can have a DVI-
> > specific entry there (e.g. with the subconnector type).
> > The idea is not to limit how the drivers subclass those structures.
> > 
> > I don't see a good case why WB should deviate from that design.
> > 
> > > If the issue is that some drivers need a custom drm_connector
> > > subclass, then I'd rather turn the connector field of
> > > drm_writeback_connector into a pointer.
> > 
> > Having a pointer requires additional ops in order to get drm_connector from
> > WB code and vice versa. Having drm_connector_wb inside drm_connector
> > saves us from those ops (which don't manifest for any other kind of structure).
> > Nor will it take any more space since union will reuse space already taken up by
> > HDMI part.
> 
> Seems like this thread has died. We need to get a conclusion on the design.
> Laurent do you have any issue with the design given Dmitry's explanation as to why this
> Design is good for drm_writeback_connector.

I'm busy, I'll try to reply in the next few days.

> > > > I plan to add drm_connector_dp in a similar way, covering DP needs
> > > > (currently WIP).

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[liviu.dudau]

Hi,

On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > };
> > >
> > > I still don't like that. This really doesn't belong here. If anything,
> > > the drm_connector for writeback belongs to drm_crtc.
> > 
> > Why? We already have generic HDMI field inside drm_connector. I am really
> > hoping to be able to land DP parts next to it. In theory we can have a DVI-
> > specific entry there (e.g. with the subconnector type).
> > The idea is not to limit how the drivers subclass those structures.
> > 
> > I don't see a good case why WB should deviate from that design.
> > 
> > > If the issue is that some drivers need a custom drm_connector
> > > subclass, then I'd rather turn the connector field of
> > > drm_writeback_connector into a pointer.
> > 
> > Having a pointer requires additional ops in order to get drm_connector from
> > WB code and vice versa. Having drm_connector_wb inside drm_connector
> > saves us from those ops (which don't manifest for any other kind of structure).
> > Nor will it take any more space since union will reuse space already taken up by
> > HDMI part.
> > 
> > >
> 
> Seems like this thread has died. We need to get a conclusion on the design.
> Laurent do you have any issue with the design given Dmitry's explanation as to why this
> Design is good for drm_writeback_connector.

I'm with Laurent here. The idea for drm_connector (and a lot of drm structures) are to
be used as base "classes" for extended structures. I don't know why HDMI connector ended
up inside drm_connector as not all connectors have HDMI functionality, but that's a cleanup
for another day.

drm_writeback_connector uses the 'base' drm_connector only for a few things, mostly in
__drm_writeback_connector_init() and prepare_job()/cleanup_job(). In _init() we just setup
the properties and the encoder after we disable interlacing. prepare_job()/cleanup_job()
is another workaround to be to some custom ops some drivers might want for signalling. So
we should be able to convert the 'base' drm_connector to a pointer relatively easy. We shouldn't
need to get to the drm_connector from a drm_writeback_connector() outside drm_writeback.c.

Then it looks like what we need is a __drm_writeback_connector_init_with_connector() where we
can pass a base pointer and remember it. Maybe an extra parameter to existing init functions,
or a new one that skips the encoder initialisation entirely.

Best regards,
Liviu


> 
> Regards,
> Suraj Kandpal
> 
> > > > I plan to add drm_connector_dp in a similar way, covering DP needs
> > > > (currently WIP).
> > 
> > --
> > With best wishes
> > Dmitry

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> Hi,
> 
> On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > };
> > > >
> > > > I still don't like that. This really doesn't belong here. If anything,
> > > > the drm_connector for writeback belongs to drm_crtc.
> > > 
> > > Why? We already have generic HDMI field inside drm_connector. I am really
> > > hoping to be able to land DP parts next to it. In theory we can have a DVI-
> > > specific entry there (e.g. with the subconnector type).
> > > The idea is not to limit how the drivers subclass those structures.
> > > 
> > > I don't see a good case why WB should deviate from that design.
> > > 
> > > > If the issue is that some drivers need a custom drm_connector
> > > > subclass, then I'd rather turn the connector field of
> > > > drm_writeback_connector into a pointer.
> > > 
> > > Having a pointer requires additional ops in order to get drm_connector from
> > > WB code and vice versa. Having drm_connector_wb inside drm_connector
> > > saves us from those ops (which don't manifest for any other kind of structure).
> > > Nor will it take any more space since union will reuse space already taken up by
> > > HDMI part.
> > > 
> > > >
> > 
> > Seems like this thread has died. We need to get a conclusion on the design.
> > Laurent do you have any issue with the design given Dmitry's explanation as to why this
> > Design is good for drm_writeback_connector.
> 
> I'm with Laurent here. The idea for drm_connector (and a lot of drm structures) are to
> be used as base "classes" for extended structures. I don't know why HDMI connector ended
> up inside drm_connector as not all connectors have HDMI functionality, but that's a cleanup
> for another day.

Maybe Maxime can better comment on it, but I think it was made exactly
for the purpose of not limiting the driver's design. For example, a lot
of drivers subclass drm_connector via drm_bridge_connector. If
struct drm_connector_hdmi was a wrapper around struct drm_connector,
then it would have been impossible to use HDMI helpers for bridge
drivers, while current design freely allows any driver to utilize
corresponding library code.

> 
> drm_writeback_connector uses the 'base' drm_connector only for a few things, mostly in
> __drm_writeback_connector_init() and prepare_job()/cleanup_job(). In _init() we just setup
> the properties and the encoder after we disable interlacing. prepare_job()/cleanup_job()
> is another workaround to be to some custom ops some drivers might want for signalling. So
> we should be able to convert the 'base' drm_connector to a pointer relatively easy. We shouldn't
> need to get to the drm_connector from a drm_writeback_connector() outside drm_writeback.c.
> 
> Then it looks like what we need is a __drm_writeback_connector_init_with_connector() where we
> can pass a base pointer and remember it. Maybe an extra parameter to existing init functions,
> or a new one that skips the encoder initialisation entirely.

I've refactored out drm_encoder, that's not a big problem. The bigger
problem is the embedded 'drm_connector base' field. It's really use to
overlook that it's not initialized / not used.


-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[mripard]

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

Hi,

On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > Hi,
> > 
> > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > };
> > > > >
> > > > > I still don't like that. This really doesn't belong here. If anything,
> > > > > the drm_connector for writeback belongs to drm_crtc.
> > > > 
> > > > Why? We already have generic HDMI field inside drm_connector. I am really
> > > > hoping to be able to land DP parts next to it. In theory we can have a DVI-
> > > > specific entry there (e.g. with the subconnector type).
> > > > The idea is not to limit how the drivers subclass those structures.
> > > > 
> > > > I don't see a good case why WB should deviate from that design.
> > > > 
> > > > > If the issue is that some drivers need a custom drm_connector
> > > > > subclass, then I'd rather turn the connector field of
> > > > > drm_writeback_connector into a pointer.
> > > > 
> > > > Having a pointer requires additional ops in order to get drm_connector from
> > > > WB code and vice versa. Having drm_connector_wb inside drm_connector
> > > > saves us from those ops (which don't manifest for any other kind of structure).
> > > > Nor will it take any more space since union will reuse space already taken up by
> > > > HDMI part.
> > > > 
> > > > >
> > > 
> > > Seems like this thread has died. We need to get a conclusion on the design.
> > > Laurent do you have any issue with the design given Dmitry's explanation as to why this
> > > Design is good for drm_writeback_connector.
> > 
> > I'm with Laurent here. The idea for drm_connector (and a lot of drm structures) are to
> > be used as base "classes" for extended structures. I don't know why HDMI connector ended
> > up inside drm_connector as not all connectors have HDMI functionality, but that's a cleanup
> > for another day.
> 
> Maybe Maxime can better comment on it, but I think it was made exactly
> for the purpose of not limiting the driver's design. For example, a lot
> of drivers subclass drm_connector via drm_bridge_connector. If
> struct drm_connector_hdmi was a wrapper around struct drm_connector,
> then it would have been impossible to use HDMI helpers for bridge
> drivers, while current design freely allows any driver to utilize
> corresponding library code.

That's exactly why we ended up like this. With that design, we wouldn't
have been able to "inherit" two connector "classes": bridge_connector is
one, intel_connector another one.

See here for the rationale:
https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/

I don't think the "but we'll bloat drm_connector" makes sense either.
There's already a *lot* of things that aren't useful to every connector
(fwnode, display_info, edid in general, scaling, vrr, etc.)

And it's not like we allocate more than a handful of them during a
system's life.

Maxime

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

RE: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Kandpal, Suraj]

> Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> drm_writeback_connector structure
> 
> Hi,
> 
> On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > Hi,
> > >
> > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > };
> > > > > >
> > > > > > I still don't like that. This really doesn't belong here. If
> > > > > > anything, the drm_connector for writeback belongs to drm_crtc.
> > > > >
> > > > > Why? We already have generic HDMI field inside drm_connector. I
> > > > > am really hoping to be able to land DP parts next to it. In
> > > > > theory we can have a DVI- specific entry there (e.g. with the
> subconnector type).
> > > > > The idea is not to limit how the drivers subclass those structures.
> > > > >
> > > > > I don't see a good case why WB should deviate from that design.
> > > > >
> > > > > > If the issue is that some drivers need a custom drm_connector
> > > > > > subclass, then I'd rather turn the connector field of
> > > > > > drm_writeback_connector into a pointer.
> > > > >
> > > > > Having a pointer requires additional ops in order to get
> > > > > drm_connector from WB code and vice versa. Having
> > > > > drm_connector_wb inside drm_connector saves us from those ops
> (which don't manifest for any other kind of structure).
> > > > > Nor will it take any more space since union will reuse space
> > > > > already taken up by HDMI part.
> > > > >
> > > > > >
> > > >
> > > > Seems like this thread has died. We need to get a conclusion on the
> design.
> > > > Laurent do you have any issue with the design given Dmitry's
> > > > explanation as to why this Design is good for drm_writeback_connector.
> > >
> > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > structures) are to be used as base "classes" for extended
> > > structures. I don't know why HDMI connector ended up inside
> > > drm_connector as not all connectors have HDMI functionality, but that's a
> cleanup for another day.
> >
> > Maybe Maxime can better comment on it, but I think it was made exactly
> > for the purpose of not limiting the driver's design. For example, a
> > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > then it would have been impossible to use HDMI helpers for bridge
> > drivers, while current design freely allows any driver to utilize
> > corresponding library code.
> 
> That's exactly why we ended up like this. With that design, we wouldn't have
> been able to "inherit" two connector "classes": bridge_connector is one,
> intel_connector another one.
> 
> See here for the rationale:
> https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> 
> I don't think the "but we'll bloat drm_connector" makes sense either.
> There's already a *lot* of things that aren't useful to every connector (fwnode,
> display_info, edid in general, scaling, vrr, etc.)
> 
> And it's not like we allocate more than a handful of them during a system's life.

So Are we okay with the approach mentioned here with the changes that have been proposed here like
Having drm_writeback_connector in union with drm_hdmi_connector
Also one more thing I would like to clarify here is how everyone would like the patches
patches where each patch changes both the drm core and all related drivers (ensures buildability but then review is tough for each driver).
Or patches where we have initial drm core changes and then each patch does the all changes in a driver in its own respective patch.

Regards,
Suraj Kandpal

> 
> Maxime

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Mon, Aug 25, 2025 at 06:26:48AM +0000, Kandpal, Suraj wrote:
> > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> > drm_writeback_connector structure
> > 
> > Hi,
> > 
> > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > > Hi,
> > > >
> > > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > > };
> > > > > > >
> > > > > > > I still don't like that. This really doesn't belong here. If
> > > > > > > anything, the drm_connector for writeback belongs to drm_crtc.
> > > > > >
> > > > > > Why? We already have generic HDMI field inside drm_connector. I
> > > > > > am really hoping to be able to land DP parts next to it. In
> > > > > > theory we can have a DVI- specific entry there (e.g. with the
> > subconnector type).
> > > > > > The idea is not to limit how the drivers subclass those structures.
> > > > > >
> > > > > > I don't see a good case why WB should deviate from that design.
> > > > > >
> > > > > > > If the issue is that some drivers need a custom drm_connector
> > > > > > > subclass, then I'd rather turn the connector field of
> > > > > > > drm_writeback_connector into a pointer.
> > > > > >
> > > > > > Having a pointer requires additional ops in order to get
> > > > > > drm_connector from WB code and vice versa. Having
> > > > > > drm_connector_wb inside drm_connector saves us from those ops
> > (which don't manifest for any other kind of structure).
> > > > > > Nor will it take any more space since union will reuse space
> > > > > > already taken up by HDMI part.
> > > > > >
> > > > > > >
> > > > >
> > > > > Seems like this thread has died. We need to get a conclusion on the
> > design.
> > > > > Laurent do you have any issue with the design given Dmitry's
> > > > > explanation as to why this Design is good for drm_writeback_connector.
> > > >
> > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > > structures) are to be used as base "classes" for extended
> > > > structures. I don't know why HDMI connector ended up inside
> > > > drm_connector as not all connectors have HDMI functionality, but that's a
> > cleanup for another day.
> > >
> > > Maybe Maxime can better comment on it, but I think it was made exactly
> > > for the purpose of not limiting the driver's design. For example, a
> > > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > > then it would have been impossible to use HDMI helpers for bridge
> > > drivers, while current design freely allows any driver to utilize
> > > corresponding library code.
> > 
> > That's exactly why we ended up like this. With that design, we wouldn't have
> > been able to "inherit" two connector "classes": bridge_connector is one,
> > intel_connector another one.
> > 
> > See here for the rationale:
> > https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> > 
> > I don't think the "but we'll bloat drm_connector" makes sense either.
> > There's already a *lot* of things that aren't useful to every connector (fwnode,
> > display_info, edid in general, scaling, vrr, etc.)
> > 
> > And it's not like we allocate more than a handful of them during a system's life.
> 
> So Are we okay with the approach mentioned here with the changes that have been proposed here like
> Having drm_writeback_connector in union with drm_hdmi_connector
> Also one more thing I would like to clarify here is how everyone would like the patches
> patches where each patch changes both the drm core and all related drivers (ensures buildability but then review is tough for each driver).
> Or patches where we have initial drm core changes and then each patch does the all changes in a driver in its own respective patch.

The kernel must be bisectable. Which means that after each patch in the
series the kernel must build completely and work without additionally
introduced issues.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[mripard]

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

On Mon, Aug 25, 2025 at 06:26:48AM +0000, Kandpal, Suraj wrote:
> > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> > drm_writeback_connector structure
> > 
> > Hi,
> > 
> > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > > Hi,
> > > >
> > > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > > };
> > > > > > >
> > > > > > > I still don't like that. This really doesn't belong here. If
> > > > > > > anything, the drm_connector for writeback belongs to drm_crtc.
> > > > > >
> > > > > > Why? We already have generic HDMI field inside drm_connector. I
> > > > > > am really hoping to be able to land DP parts next to it. In
> > > > > > theory we can have a DVI- specific entry there (e.g. with the
> > subconnector type).
> > > > > > The idea is not to limit how the drivers subclass those structures.
> > > > > >
> > > > > > I don't see a good case why WB should deviate from that design.
> > > > > >
> > > > > > > If the issue is that some drivers need a custom drm_connector
> > > > > > > subclass, then I'd rather turn the connector field of
> > > > > > > drm_writeback_connector into a pointer.
> > > > > >
> > > > > > Having a pointer requires additional ops in order to get
> > > > > > drm_connector from WB code and vice versa. Having
> > > > > > drm_connector_wb inside drm_connector saves us from those ops
> > (which don't manifest for any other kind of structure).
> > > > > > Nor will it take any more space since union will reuse space
> > > > > > already taken up by HDMI part.
> > > > > >
> > > > > > >
> > > > >
> > > > > Seems like this thread has died. We need to get a conclusion on the
> > design.
> > > > > Laurent do you have any issue with the design given Dmitry's
> > > > > explanation as to why this Design is good for drm_writeback_connector.
> > > >
> > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > > structures) are to be used as base "classes" for extended
> > > > structures. I don't know why HDMI connector ended up inside
> > > > drm_connector as not all connectors have HDMI functionality, but that's a
> > cleanup for another day.
> > >
> > > Maybe Maxime can better comment on it, but I think it was made exactly
> > > for the purpose of not limiting the driver's design. For example, a
> > > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > > then it would have been impossible to use HDMI helpers for bridge
> > > drivers, while current design freely allows any driver to utilize
> > > corresponding library code.
> > 
> > That's exactly why we ended up like this. With that design, we wouldn't have
> > been able to "inherit" two connector "classes": bridge_connector is one,
> > intel_connector another one.
> > 
> > See here for the rationale:
> > https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> > 
> > I don't think the "but we'll bloat drm_connector" makes sense either.
> > There's already a *lot* of things that aren't useful to every connector (fwnode,
> > display_info, edid in general, scaling, vrr, etc.)
> > 
> > And it's not like we allocate more than a handful of them during a system's life.
> 
> So Are we okay with the approach mentioned here with the changes that have been proposed here like
> Having drm_writeback_connector in union with drm_hdmi_connector

I don't think we need a union here. It artificially creates the same
issue: we can't have two types for a connector if we do so.

> Also one more thing I would like to clarify here is how everyone would
> like the patches patches where each patch changes both the drm core
> and all related drivers (ensures buildability but then review is tough
> for each driver). Or patches where we have initial drm core changes
> and then each patch does the all changes in a driver in its own
> respective patch.

The latter should be preferred, but if you can't maintain bisectability
that way, then it's the most important and you should fall back to the
former.

Maxime

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

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[Dmitry Baryshkov]

On Tue, Aug 26, 2025 at 05:48:18PM +0200, mripard@kernel.org wrote:
> On Mon, Aug 25, 2025 at 06:26:48AM +0000, Kandpal, Suraj wrote:
> > > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> > > drm_writeback_connector structure
> > > 
> > > Hi,
> > > 
> > > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > > > };
> > > > > > > >
> > > > > > > > I still don't like that. This really doesn't belong here. If
> > > > > > > > anything, the drm_connector for writeback belongs to drm_crtc.
> > > > > > >
> > > > > > > Why? We already have generic HDMI field inside drm_connector. I
> > > > > > > am really hoping to be able to land DP parts next to it. In
> > > > > > > theory we can have a DVI- specific entry there (e.g. with the
> > > subconnector type).
> > > > > > > The idea is not to limit how the drivers subclass those structures.
> > > > > > >
> > > > > > > I don't see a good case why WB should deviate from that design.
> > > > > > >
> > > > > > > > If the issue is that some drivers need a custom drm_connector
> > > > > > > > subclass, then I'd rather turn the connector field of
> > > > > > > > drm_writeback_connector into a pointer.
> > > > > > >
> > > > > > > Having a pointer requires additional ops in order to get
> > > > > > > drm_connector from WB code and vice versa. Having
> > > > > > > drm_connector_wb inside drm_connector saves us from those ops
> > > (which don't manifest for any other kind of structure).
> > > > > > > Nor will it take any more space since union will reuse space
> > > > > > > already taken up by HDMI part.
> > > > > > >
> > > > > > > >
> > > > > >
> > > > > > Seems like this thread has died. We need to get a conclusion on the
> > > design.
> > > > > > Laurent do you have any issue with the design given Dmitry's
> > > > > > explanation as to why this Design is good for drm_writeback_connector.
> > > > >
> > > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > > > structures) are to be used as base "classes" for extended
> > > > > structures. I don't know why HDMI connector ended up inside
> > > > > drm_connector as not all connectors have HDMI functionality, but that's a
> > > cleanup for another day.
> > > >
> > > > Maybe Maxime can better comment on it, but I think it was made exactly
> > > > for the purpose of not limiting the driver's design. For example, a
> > > > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > > > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > > > then it would have been impossible to use HDMI helpers for bridge
> > > > drivers, while current design freely allows any driver to utilize
> > > > corresponding library code.
> > > 
> > > That's exactly why we ended up like this. With that design, we wouldn't have
> > > been able to "inherit" two connector "classes": bridge_connector is one,
> > > intel_connector another one.
> > > 
> > > See here for the rationale:
> > > https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> > > 
> > > I don't think the "but we'll bloat drm_connector" makes sense either.
> > > There's already a *lot* of things that aren't useful to every connector (fwnode,
> > > display_info, edid in general, scaling, vrr, etc.)
> > > 
> > > And it's not like we allocate more than a handful of them during a system's life.
> > 
> > So Are we okay with the approach mentioned here with the changes that have been proposed here like
> > Having drm_writeback_connector in union with drm_hdmi_connector
> 
> I don't think we need a union here. It artificially creates the same
> issue: we can't have two types for a connector if we do so.

Well... What kind of connector would be both HDMI and WriteBack? I think
they are mutually exclusive already.

> > Also one more thing I would like to clarify here is how everyone would
> > like the patches patches where each patch changes both the drm core
> > and all related drivers (ensures buildability but then review is tough
> > for each driver). Or patches where we have initial drm core changes
> > and then each patch does the all changes in a driver in its own
> > respective patch.
> 
> The latter should be preferred, but if you can't maintain bisectability
> that way, then it's the most important and you should fall back to the
> former.

I'd say, we should be trying our best in providing bisectability. It
really a PITA if one can not use `git bisect run`.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/8] drm: writeback: Refactor drm_writeback_connector structure

[mripard]

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

On Tue, Aug 26, 2025 at 07:08:17PM +0300, Dmitry Baryshkov wrote:
> On Tue, Aug 26, 2025 at 05:48:18PM +0200, mripard@kernel.org wrote:
> > On Mon, Aug 25, 2025 at 06:26:48AM +0000, Kandpal, Suraj wrote:
> > > > Subject: Re: [RFC PATCH 1/8] drm: writeback: Refactor
> > > > drm_writeback_connector structure
> > > > 
> > > > Hi,
> > > > 
> > > > On Sat, Aug 16, 2025 at 01:20:53AM +0300, Dmitry Baryshkov wrote:
> > > > > On Thu, Aug 14, 2025 at 05:13:54PM +0100, liviu.dudau@arm.com wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, Aug 13, 2025 at 10:04:22AM +0000, Kandpal, Suraj wrote:
> > > > > > > > > > };
> > > > > > > > >
> > > > > > > > > I still don't like that. This really doesn't belong here. If
> > > > > > > > > anything, the drm_connector for writeback belongs to drm_crtc.
> > > > > > > >
> > > > > > > > Why? We already have generic HDMI field inside drm_connector. I
> > > > > > > > am really hoping to be able to land DP parts next to it. In
> > > > > > > > theory we can have a DVI- specific entry there (e.g. with the
> > > > subconnector type).
> > > > > > > > The idea is not to limit how the drivers subclass those structures.
> > > > > > > >
> > > > > > > > I don't see a good case why WB should deviate from that design.
> > > > > > > >
> > > > > > > > > If the issue is that some drivers need a custom drm_connector
> > > > > > > > > subclass, then I'd rather turn the connector field of
> > > > > > > > > drm_writeback_connector into a pointer.
> > > > > > > >
> > > > > > > > Having a pointer requires additional ops in order to get
> > > > > > > > drm_connector from WB code and vice versa. Having
> > > > > > > > drm_connector_wb inside drm_connector saves us from those ops
> > > > (which don't manifest for any other kind of structure).
> > > > > > > > Nor will it take any more space since union will reuse space
> > > > > > > > already taken up by HDMI part.
> > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > > Seems like this thread has died. We need to get a conclusion on the
> > > > design.
> > > > > > > Laurent do you have any issue with the design given Dmitry's
> > > > > > > explanation as to why this Design is good for drm_writeback_connector.
> > > > > >
> > > > > > I'm with Laurent here. The idea for drm_connector (and a lot of drm
> > > > > > structures) are to be used as base "classes" for extended
> > > > > > structures. I don't know why HDMI connector ended up inside
> > > > > > drm_connector as not all connectors have HDMI functionality, but that's a
> > > > cleanup for another day.
> > > > >
> > > > > Maybe Maxime can better comment on it, but I think it was made exactly
> > > > > for the purpose of not limiting the driver's design. For example, a
> > > > > lot of drivers subclass drm_connector via drm_bridge_connector. If
> > > > > struct drm_connector_hdmi was a wrapper around struct drm_connector,
> > > > > then it would have been impossible to use HDMI helpers for bridge
> > > > > drivers, while current design freely allows any driver to utilize
> > > > > corresponding library code.
> > > > 
> > > > That's exactly why we ended up like this. With that design, we wouldn't have
> > > > been able to "inherit" two connector "classes": bridge_connector is one,
> > > > intel_connector another one.
> > > > 
> > > > See here for the rationale:
> > > > https://lore.kernel.org/dri-devel/ZOTDKHxn2bOg+Xmg@phenom.ffwll.local/
> > > > 
> > > > I don't think the "but we'll bloat drm_connector" makes sense either.
> > > > There's already a *lot* of things that aren't useful to every connector (fwnode,
> > > > display_info, edid in general, scaling, vrr, etc.)
> > > > 
> > > > And it's not like we allocate more than a handful of them during a system's life.
> > > 
> > > So Are we okay with the approach mentioned here with the changes that have been proposed here like
> > > Having drm_writeback_connector in union with drm_hdmi_connector
> > 
> > I don't think we need a union here. It artificially creates the same
> > issue: we can't have two types for a connector if we do so.
> 
> Well... What kind of connector would be both HDMI and WriteBack? I think
> they are mutually exclusive already.
> 
> > > Also one more thing I would like to clarify here is how everyone would
> > > like the patches patches where each patch changes both the drm core
> > > and all related drivers (ensures buildability but then review is tough
> > > for each driver). Or patches where we have initial drm core changes
> > > and then each patch does the all changes in a driver in its own
> > > respective patch.
> > 
> > The latter should be preferred, but if you can't maintain bisectability
> > that way, then it's the most important and you should fall back to the
> > former.
> 
> I'd say, we should be trying our best in providing bisectability. It
> really a PITA if one can not use `git bisect run`.

Yeah, I believe we are saying the same thing :)

Maxime

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