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

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

> >  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

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

> 
> 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

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

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

> > > @@ -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

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

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

> > > };
> >
> > 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

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!  /
  ---------------
    ¯\_(ツ)_/¯

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

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

> 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

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

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

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

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

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

> > > > @@ -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