[PATCH v3 0/3] Allow drm_writeback_connector to accept pointer to drm_encoder

[PATCH v3 0/3] Allow drm_writeback_connector to accept pointer to drm_encoder

[Abhinav Kumar]

There are some vendor drivers for which the writeback encoder shares
hardware resources such as clocks and interrupts with the rest of the
display pipeline. In addition, there can be use-cases where the
writeback encoder could be a shared encoder between the physical display
path and the writeback path.

To accommodate for such cases, change the drm_writeback_connector to
accept a pointer to drm_encoder.

For existing users of drm_writeback_connector there will not be any
change in functionality due to this change.

This approach was first posted by Suraj Kandpal here [1] for both
encoder and connector. But after discussions [2], the consensus was
reached to split this change for the drm_encoder first and the
drm_connector part can be reworked in a subsequent change later.

Validation of this change was done using igt_writeback tests on
MSM based RB5 board using the changes posted here [3].

For all other chipsets, these changes were compile-tested.

[1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-suraj.kandpal@intel.com/
[2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/
[3] https://patchwork.freedesktop.org/series/99724/

changes in v3:
	-  squash the vendor changes into the same commit so
       that each patch in the series can compile individually	

Abhinav Kumar (3):
  drm: allow real encoder to be passed for drm_writeback_connector
  drm: introduce drm_writeback_connector_init_with_encoder API
  drm/vc4: change vc4 driver to use
    drm_writeback_connector_init_with_encoder()

 .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
 drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
 drivers/gpu/drm/drm_writeback.c                    | 145 +++++++++++++++------
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
 drivers/gpu/drm/vc4/vc4_txp.c                      |  35 +++--
 drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
 include/drm/drm_writeback.h                        |  27 +++-
 7 files changed, 165 insertions(+), 58 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.

In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.

To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector using a new
drm_writeback_connector_init_with_encoder() API.

In addition, to preserve the same call flows for the existing
users of drm_writeback_connector_init(), also allow passing
possible_crtcs as a parameter so that encoder can be initialized
with it.

changes in v3:
	- allow passing possible_crtcs for existing users of
	  drm_writeback_connector_init()
	- squash the vendor changes into the same commit so
	  that each patch in the series can compile individually

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
 drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
 drivers/gpu/drm/drm_writeback.c                    | 103 +++++++++++++--------
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
 drivers/gpu/drm/vc4/vc4_txp.c                      |  19 ++--
 drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
 include/drm/drm_writeback.h                        |  22 ++++-
 7 files changed, 103 insertions(+), 57 deletions(-)

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 e465cc4..40774e6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 	kwb_conn->wb_layer = kcrtc->master->wb_layer;
 
 	wb_conn = &kwb_conn->base;
-	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
 
 	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
 					       kwb_conn->wb_layer->layer_type,
@@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
 	err = drm_writeback_connector_init(&kms->base, wb_conn,
 					   &komeda_wb_connector_funcs,
 					   &komeda_wb_encoder_helper_funcs,
-					   formats, n_formats);
+					   formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
 	komeda_put_fourcc_list(formats);
 	if (err) {
 		kfree(kwb_conn);
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index f5847a7..b882066 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
 	struct malidp_drm *malidp = drm->dev_private;
 	u32 *formats;
 	int ret, n_formats;
+	uint32_t possible_crtcs;
 
 	if (!malidp->dev->hw->enable_memwrite)
 		return 0;
 
-	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+	possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
 	drm_connector_helper_add(&malidp->mw_connector.base,
 				 &malidp_mw_connector_helper_funcs);
 
@@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
 	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
 					   &malidp_mw_connector_funcs,
 					   &malidp_mw_encoder_helper_funcs,
-					   formats, n_formats);
+					   formats, n_formats, possible_crtcs);
 	kfree(formats);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..17c1471 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,36 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-/**
- * drm_writeback_connector_init - Initialize a writeback connector and its properties
- * @dev: DRM device
- * @wb_connector: Writeback connector to initialize
- * @con_funcs: Connector funcs vtable
- * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
- * @formats: Array of supported pixel formats for the writeback engine
- * @n_formats: Length of the formats array
- *
- * This function creates the writeback-connector-specific properties if they
- * have not been already created, initializes the connector as
- * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
- * values. It will also create an internal encoder associated with the
- * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
- * the encoder helper.
- *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors.
- *
- * Returns: 0 on success, or a negative error code
- */
-int drm_writeback_connector_init(struct drm_device *dev,
-				 struct drm_writeback_connector *wb_connector,
-				 const struct drm_connector_funcs *con_funcs,
-				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
-				 const u32 *formats, int n_formats)
+static int drm_writeback_connector_setup(struct drm_device *dev,
+		struct drm_writeback_connector *wb_connector,
+		const struct drm_connector_funcs *con_funcs, const u32 *formats,
+		int n_formats)
 {
 	struct drm_property_blob *blob;
-	struct drm_connector *connector = &wb_connector->base;
 	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_connector *connector = &wb_connector->base;
+
 	int ret = create_writeback_properties(dev);
 
 	if (ret != 0)
@@ -186,15 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
 
 	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
 					formats);
-	if (IS_ERR(blob))
-		return PTR_ERR(blob);
-
-	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
-	ret = drm_encoder_init(dev, &wb_connector->encoder,
-			       &drm_writeback_encoder_funcs,
-			       DRM_MODE_ENCODER_VIRTUAL, NULL);
-	if (ret)
-		goto fail;
+	if (IS_ERR(blob)) {
+		ret = PTR_ERR(blob);
+		return ret;
+	}
 
 	connector->interlace_allowed = 0;
 
@@ -204,7 +178,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 		goto connector_fail;
 
 	ret = drm_connector_attach_encoder(connector,
-						&wb_connector->encoder);
+						wb_connector->encoder);
 	if (ret)
 		goto attach_fail;
 
@@ -233,11 +207,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
 attach_fail:
 	drm_connector_cleanup(connector);
 connector_fail:
-	drm_encoder_cleanup(&wb_connector->encoder);
-fail:
 	drm_property_blob_put(blob);
 	return ret;
 }
+
+/**
+ * drm_writeback_connector_init - Initialize a writeback connector and its properties
+ * using an internal encoder
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @con_funcs: Connector funcs vtable
+ * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ * @possible_crtcs: possible crtcs for the internal writeback encoder
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values. It will also create an internal encoder associated with the
+ * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
+ * the encoder helper.
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init(struct drm_device *dev,
+		struct drm_writeback_connector *wb_connector,
+		const struct drm_connector_funcs *con_funcs,
+		const struct drm_encoder_helper_funcs *enc_helper_funcs,
+		const u32 *formats, int n_formats, uint32_t possible_crtcs)
+{
+	int ret = 0;
+
+	wb_connector->encoder = &wb_connector->internal_encoder;
+
+	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+
+	wb_connector->encoder->possible_crtcs = possible_crtcs;
+
+	ret = drm_encoder_init(dev, wb_connector->encoder,
+			       &drm_writeback_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret)
+		return ret;
+
+	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
+			n_formats);
+
+	if (ret)
+		drm_encoder_cleanup(wb_connector->encoder);
+
+	return ret;
+}
 EXPORT_SYMBOL(drm_writeback_connector_init);
 
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d125..1a610c0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
 			   struct rcar_du_crtc *rcrtc)
 {
 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
+	uint32_t possible_crtcs;
 
-	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
+	possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
 	drm_connector_helper_add(&wb_conn->base,
 				 &rcar_du_wb_conn_helper_funcs);
 
@@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
 					    &rcar_du_wb_conn_funcs,
 					    &rcar_du_wb_enc_helper_funcs,
 					    writeback_formats,
-					    ARRAY_SIZE(writeback_formats));
+					    ARRAY_SIZE(writeback_formats), possible_crtcs);
 }
 
 void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 9809ca3..32c4fb0 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
 
 	struct platform_device *pdev;
 
+	struct drm_encoder drm_enc;
+
 	struct drm_writeback_connector connector;
 
 	void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
 
 static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
 {
-	return container_of(encoder, struct vc4_txp, connector.encoder);
+	return container_of(encoder, struct vc4_txp, drm_enc);
 }
 
 static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	struct vc4_txp *txp;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
+	struct drm_writeback_connector *wb_conn;
 	int ret, irq;
 
 	irq = platform_get_irq(pdev, 0);
@@ -492,12 +495,14 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	txp->regset.regs = txp_regs;
 	txp->regset.nregs = ARRAY_SIZE(txp_regs);
 
-	drm_connector_helper_add(&txp->connector.base,
+	wb_conn = &txp->connector;
+	wb_conn->encoder = &txp->drm_enc;
+
+	drm_connector_helper_add(&wb_conn->base,
 				 &vc4_txp_connector_helper_funcs);
-	ret = drm_writeback_connector_init(drm, &txp->connector,
-					   &vc4_txp_connector_funcs,
-					   &vc4_txp_encoder_helper_funcs,
-					   drm_fmts, ARRAY_SIZE(drm_fmts));
+	ret = drm_writeback_connector_init(drm, wb_conn,
+			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
+			drm_fmts, ARRAY_SIZE(drm_fmts), 0);
 	if (ret)
 		return ret;
 
@@ -506,7 +511,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	encoder = &txp->connector.encoder;
+	encoder = txp->connector.encoder;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227..381e7b7 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
 {
 	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
 
-	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 
 	return drm_writeback_connector_init(&vkmsdev->drm, wb,
 					    &vkms_wb_connector_funcs,
 					    &vkms_wb_encoder_helper_funcs,
 					    vkms_wb_formats,
-					    ARRAY_SIZE(vkms_wb_formats));
+					    ARRAY_SIZE(vkms_wb_formats), 1);
 }
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 9697d27..5e002bc 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,13 +25,29 @@ struct drm_writeback_connector {
 	struct drm_connector base;
 
 	/**
-	 * @encoder: Internal encoder used by the connector to fulfill
+	 * @encoder: handle to drm_encoder used by the connector to fulfill
 	 * the DRM framework requirements. The users of the
 	 * @drm_writeback_connector control the behaviour of the @encoder
 	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
 	 * function.
+	 *
+	 * For some vendor drivers, the hardware resources are shared between
+	 * writeback encoder and rest of the display pipeline.
+	 * To accommodate such cases, encoder is a handle to the real encoder
+	 * hardware.
+	 *
+	 * For current existing writeback users, this shall continue to be the
+	 * embedded encoder for the writeback connector.
+	 *
+	 */
+	struct drm_encoder *encoder;
+
+	/**
+	 * @internal_encoder: internal encoder used by writeback when
+	 * a real encoder is not provided by the vendor drm drivers.
+	 * @encoder will be assigned to this for those cases.
 	 */
-	struct drm_encoder encoder;
+	struct drm_encoder internal_encoder;
 
 	/**
 	 * @pixel_formats_blob_ptr:
@@ -150,7 +166,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 				 struct drm_writeback_connector *wb_connector,
 				 const struct drm_connector_funcs *con_funcs,
 				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
-				 const u32 *formats, int n_formats);
+				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
 
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb);
-- 
2.7.4

[PATCH v3 2/3] drm: introduce drm_writeback_connector_init_with_encoder API

[Abhinav Kumar]

For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/drm_writeback.c | 42 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_writeback.h     |  5 +++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 17c1471..373b7e9 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -265,6 +265,48 @@ int drm_writeback_connector_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_writeback_connector_init);
 
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback connector and its properties
+ * using the encoder which already assigned and initialized
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @con_funcs: Connector funcs vtable
+ * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * This function assumes that the drm_writebac_connector's encoder has already been
+ * created and initialized before invoking this function.
+ *
+ * In addition, this function also assumes that callers of this API will manage
+ * assigning the encoder helper functions, possible_crtcs and any other encoder
+ * specific operation which is otherwise handled by drm_writeback_connector_init().
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
+		struct drm_writeback_connector *wb_connector,
+		const struct drm_connector_funcs *con_funcs, const u32 *formats,
+		int n_formats)
+{
+	int ret = 0;
+
+	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
+			n_formats);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb)
 {
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 5e002bc..5e752c8 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -168,6 +168,11 @@ int drm_writeback_connector_init(struct drm_device *dev,
 				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
 				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
 
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
+		struct drm_writeback_connector *wb_connector,
+		const struct drm_connector_funcs *con_funcs, const u32 *formats,
+		int n_formats);
+
 int drm_writeback_set_fb(struct drm_connector_state *conn_state,
 			 struct drm_framebuffer *fb);
 
-- 
2.7.4

[PATCH v3 3/3] drm/vc4: change vc4 driver to use drm_writeback_connector_init_with_encoder()

[Abhinav Kumar]

vc4 driver currently embeds the drm_encoder into struct vc4_txp
and later on uses container_of to retrieve the vc4_txp from
the drm_encoder.

Since drm_encoder has now been made a pointer inside
drm_writeback_connector, make vc4 driver use the new API
so that the embedded encoder model can be retained in the
driver and there is no change in functionality.

changes in v3:
    - none

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/vc4/vc4_txp.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 32c4fb0..36dffcf 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -370,6 +370,10 @@ static const struct drm_encoder_helper_funcs vc4_txp_encoder_helper_funcs = {
 	.disable = vc4_txp_encoder_disable,
 };
 
+static const struct drm_encoder_funcs vc4_txp_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static int vc4_txp_enable_vblank(struct drm_crtc *crtc)
 {
 	return 0;
@@ -498,13 +502,23 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	wb_conn = &txp->connector;
 	wb_conn->encoder = &txp->drm_enc;
 
+	drm_encoder_helper_add(wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
+
+	ret = drm_encoder_init(drm, wb_conn->encoder,
+			&vc4_txp_encoder_funcs,
+			DRM_MODE_ENCODER_VIRTUAL, NULL);
+
+	if (ret)
+		return ret;
+
 	drm_connector_helper_add(&wb_conn->base,
 				 &vc4_txp_connector_helper_funcs);
-	ret = drm_writeback_connector_init(drm, wb_conn,
-			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
-			drm_fmts, ARRAY_SIZE(drm_fmts), 0);
-	if (ret)
+	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
+			&vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
+	if (ret) {
+		drm_encoder_cleanup(wb_conn->encoder);
 		return ret;
+	}
 
 	ret = vc4_crtc_init(drm, vc4_crtc,
 			    &vc4_txp_crtc_funcs, &vc4_txp_crtc_helper_funcs);
-- 
2.7.4

Re: [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav,

Thank you for the patch.

On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
> 
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
> 
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector using a new
> drm_writeback_connector_init_with_encoder() API.

The commit message doesn't match the commit.

> In addition, to preserve the same call flows for the existing
> users of drm_writeback_connector_init(), also allow passing
> possible_crtcs as a parameter so that encoder can be initialized
> with it.
> 
> changes in v3:
> 	- allow passing possible_crtcs for existing users of
> 	  drm_writeback_connector_init()
> 	- squash the vendor changes into the same commit so
> 	  that each patch in the series can compile individually
> 
> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
>  drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
>  drivers/gpu/drm/drm_writeback.c                    | 103 +++++++++++++--------
>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
>  drivers/gpu/drm/vc4/vc4_txp.c                      |  19 ++--
>  drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
>  include/drm/drm_writeback.h                        |  22 ++++-
>  7 files changed, 103 insertions(+), 57 deletions(-)
> 
> 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 e465cc4..40774e6 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>  	kwb_conn->wb_layer = kcrtc->master->wb_layer;
>  
>  	wb_conn = &kwb_conn->base;
> -	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
>  
>  	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
>  					       kwb_conn->wb_layer->layer_type,
> @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>  	err = drm_writeback_connector_init(&kms->base, wb_conn,
>  					   &komeda_wb_connector_funcs,
>  					   &komeda_wb_encoder_helper_funcs,
> -					   formats, n_formats);
> +					   formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
>  	komeda_put_fourcc_list(formats);
>  	if (err) {
>  		kfree(kwb_conn);
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index f5847a7..b882066 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
>  	struct malidp_drm *malidp = drm->dev_private;
>  	u32 *formats;
>  	int ret, n_formats;
> +	uint32_t possible_crtcs;
>  
>  	if (!malidp->dev->hw->enable_memwrite)
>  		return 0;
>  
> -	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
> +	possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>  	drm_connector_helper_add(&malidp->mw_connector.base,
>  				 &malidp_mw_connector_helper_funcs);
>  
> @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
>  	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
>  					   &malidp_mw_connector_funcs,
>  					   &malidp_mw_encoder_helper_funcs,
> -					   formats, n_formats);
> +					   formats, n_formats, possible_crtcs);

Do you need the local variable ?

>  	kfree(formats);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dccf4504..17c1471 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -149,36 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>  
> -/**
> - * drm_writeback_connector_init - Initialize a writeback connector and its properties
> - * @dev: DRM device
> - * @wb_connector: Writeback connector to initialize
> - * @con_funcs: Connector funcs vtable
> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
> - * @formats: Array of supported pixel formats for the writeback engine
> - * @n_formats: Length of the formats array
> - *
> - * This function creates the writeback-connector-specific properties if they
> - * have not been already created, initializes the connector as
> - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> - * values. It will also create an internal encoder associated with the
> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
> - * the encoder helper.
> - *
> - * Drivers should always use this function instead of drm_connector_init() to
> - * set up writeback connectors.
> - *
> - * Returns: 0 on success, or a negative error code
> - */
> -int drm_writeback_connector_init(struct drm_device *dev,
> -				 struct drm_writeback_connector *wb_connector,
> -				 const struct drm_connector_funcs *con_funcs,
> -				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
> -				 const u32 *formats, int n_formats)
> +static int drm_writeback_connector_setup(struct drm_device *dev,
> +		struct drm_writeback_connector *wb_connector,
> +		const struct drm_connector_funcs *con_funcs, const u32 *formats,
> +		int n_formats)
>  {
>  	struct drm_property_blob *blob;
> -	struct drm_connector *connector = &wb_connector->base;
>  	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_connector *connector = &wb_connector->base;
> +
>  	int ret = create_writeback_properties(dev);
>  
>  	if (ret != 0)
> @@ -186,15 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  
>  	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>  					formats);
> -	if (IS_ERR(blob))
> -		return PTR_ERR(blob);
> -
> -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
> -			       &drm_writeback_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> -	if (ret)
> -		goto fail;
> +	if (IS_ERR(blob)) {
> +		ret = PTR_ERR(blob);
> +		return ret;
> +	}
>  
>  	connector->interlace_allowed = 0;
>  
> @@ -204,7 +178,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  		goto connector_fail;
>  
>  	ret = drm_connector_attach_encoder(connector,
> -						&wb_connector->encoder);
> +						wb_connector->encoder);
>  	if (ret)
>  		goto attach_fail;
>  
> @@ -233,11 +207,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  attach_fail:
>  	drm_connector_cleanup(connector);
>  connector_fail:
> -	drm_encoder_cleanup(&wb_connector->encoder);
> -fail:
>  	drm_property_blob_put(blob);
>  	return ret;
>  }
> +
> +/**
> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
> + * using an internal encoder
> + *
> + * @dev: DRM device
> + * @wb_connector: Writeback connector to initialize
> + * @con_funcs: Connector funcs vtable
> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
> + * @formats: Array of supported pixel formats for the writeback engine
> + * @n_formats: Length of the formats array
> + * @possible_crtcs: possible crtcs for the internal writeback encoder
> + *
> + * This function creates the writeback-connector-specific properties if they
> + * have not been already created, initializes the connector as
> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> + * values. It will also create an internal encoder associated with the
> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
> + * the encoder helper.
> + *
> + * Drivers should always use this function instead of drm_connector_init() to
> + * set up writeback connectors.
> + *
> + * Returns: 0 on success, or a negative error code
> + */
> +int drm_writeback_connector_init(struct drm_device *dev,
> +		struct drm_writeback_connector *wb_connector,
> +		const struct drm_connector_funcs *con_funcs,
> +		const struct drm_encoder_helper_funcs *enc_helper_funcs,
> +		const u32 *formats, int n_formats, uint32_t possible_crtcs)
> +{
> +	int ret = 0;
> +
> +	wb_connector->encoder = &wb_connector->internal_encoder;
> +
> +	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> +
> +	wb_connector->encoder->possible_crtcs = possible_crtcs;
> +
> +	ret = drm_encoder_init(dev, wb_connector->encoder,
> +			       &drm_writeback_encoder_funcs,
> +			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
> +			n_formats);
> +
> +	if (ret)
> +		drm_encoder_cleanup(wb_connector->encoder);
> +
> +	return ret;
> +}

All this doesn't belong to this patch. Just add the possible_crtcs
argument to drm_writeback_connector_init() in 1/3, nothing else. One
functional change, one patch.

>  EXPORT_SYMBOL(drm_writeback_connector_init);
>  
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> index c79d125..1a610c0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> @@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>  			   struct rcar_du_crtc *rcrtc)
>  {
>  	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> +	uint32_t possible_crtcs;
>  
> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> +	possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>  	drm_connector_helper_add(&wb_conn->base,
>  				 &rcar_du_wb_conn_helper_funcs);
>  
> @@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>  					    &rcar_du_wb_conn_funcs,
>  					    &rcar_du_wb_enc_helper_funcs,
>  					    writeback_formats,
> -					    ARRAY_SIZE(writeback_formats));
> +					    ARRAY_SIZE(writeback_formats), possible_crtcs);

Same here, do you need the intermediate variable ?

And please add a line break after ARRAY_SIZE().

>  }
>  
>  void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 9809ca3..32c4fb0 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -151,6 +151,8 @@ struct vc4_txp {
>  
>  	struct platform_device *pdev;
>  
> +	struct drm_encoder drm_enc;
> +
>  	struct drm_writeback_connector connector;
>  
>  	void __iomem *regs;
> @@ -159,7 +161,7 @@ struct vc4_txp {
>  
>  static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>  {
> -	return container_of(encoder, struct vc4_txp, connector.encoder);
> +	return container_of(encoder, struct vc4_txp, drm_enc);
>  }
>  
>  static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	struct vc4_txp *txp;
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
> +	struct drm_writeback_connector *wb_conn;
>  	int ret, irq;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -492,12 +495,14 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	txp->regset.regs = txp_regs;
>  	txp->regset.nregs = ARRAY_SIZE(txp_regs);
>  
> -	drm_connector_helper_add(&txp->connector.base,
> +	wb_conn = &txp->connector;
> +	wb_conn->encoder = &txp->drm_enc;
> +
> +	drm_connector_helper_add(&wb_conn->base,
>  				 &vc4_txp_connector_helper_funcs);
> -	ret = drm_writeback_connector_init(drm, &txp->connector,
> -					   &vc4_txp_connector_funcs,
> -					   &vc4_txp_encoder_helper_funcs,
> -					   drm_fmts, ARRAY_SIZE(drm_fmts));
> +	ret = drm_writeback_connector_init(drm, wb_conn,
> +			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
> +			drm_fmts, ARRAY_SIZE(drm_fmts), 0);

Most of this also doesn't belong to this patch.

>  	if (ret)
>  		return ret;
>  
> @@ -506,7 +511,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	encoder = &txp->connector.encoder;
> +	encoder = txp->connector.encoder;
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>  
>  	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 8694227..381e7b7 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
>  {
>  	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
>  
> -	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
>  	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>  
>  	return drm_writeback_connector_init(&vkmsdev->drm, wb,
>  					    &vkms_wb_connector_funcs,
>  					    &vkms_wb_encoder_helper_funcs,
>  					    vkms_wb_formats,
> -					    ARRAY_SIZE(vkms_wb_formats));
> +					    ARRAY_SIZE(vkms_wb_formats), 1);
>  }
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 9697d27..5e002bc 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -25,13 +25,29 @@ struct drm_writeback_connector {
>  	struct drm_connector base;
>  
>  	/**
> -	 * @encoder: Internal encoder used by the connector to fulfill
> +	 * @encoder: handle to drm_encoder used by the connector to fulfill
>  	 * the DRM framework requirements. The users of the
>  	 * @drm_writeback_connector control the behaviour of the @encoder
>  	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>  	 * function.
> +	 *
> +	 * For some vendor drivers, the hardware resources are shared between
> +	 * writeback encoder and rest of the display pipeline.
> +	 * To accommodate such cases, encoder is a handle to the real encoder
> +	 * hardware.
> +	 *
> +	 * For current existing writeback users, this shall continue to be the
> +	 * embedded encoder for the writeback connector.
> +	 *
> +	 */
> +	struct drm_encoder *encoder;
> +
> +	/**
> +	 * @internal_encoder: internal encoder used by writeback when
> +	 * a real encoder is not provided by the vendor drm drivers.
> +	 * @encoder will be assigned to this for those cases.
>  	 */
> -	struct drm_encoder encoder;
> +	struct drm_encoder internal_encoder;

None of this belong here.

>  
>  	/**
>  	 * @pixel_formats_blob_ptr:
> @@ -150,7 +166,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  				 struct drm_writeback_connector *wb_connector,
>  				 const struct drm_connector_funcs *con_funcs,
>  				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
> -				 const u32 *formats, int n_formats);
> +				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
>  
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  			 struct drm_framebuffer *fb);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

Thanks for the review.

On 3/17/2022 1:51 AM, Laurent Pinchart wrote:
> Hi Abhinav,
> 
> Thank you for the patch.
> 
> On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
>> For some vendor driver implementations, display hardware can
>> be shared between the encoder used for writeback and the physical
>> display.
>>
>> In addition resources such as clocks and interrupts can
>> also be shared between writeback and the real encoder.
>>
>> To accommodate such vendor drivers and hardware, allow
>> real encoder to be passed for drm_writeback_connector using a new
>> drm_writeback_connector_init_with_encoder() API.
> 
> The commit message doesn't match the commit.
Sorry, while splitting the change , I missed this part of the commit 
text. Will fix it up.
> 
>> In addition, to preserve the same call flows for the existing
>> users of drm_writeback_connector_init(), also allow passing
>> possible_crtcs as a parameter so that encoder can be initialized
>> with it.
>>
>> changes in v3:
>> 	- allow passing possible_crtcs for existing users of
>> 	  drm_writeback_connector_init()
>> 	- squash the vendor changes into the same commit so
>> 	  that each patch in the series can compile individually
>>
>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
>>   drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
>>   drivers/gpu/drm/drm_writeback.c                    | 103 +++++++++++++--------
>>   drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
>>   drivers/gpu/drm/vc4/vc4_txp.c                      |  19 ++--
>>   drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
>>   include/drm/drm_writeback.h                        |  22 ++++-
>>   7 files changed, 103 insertions(+), 57 deletions(-)
>>
>> 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 e465cc4..40774e6 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>> @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>   	kwb_conn->wb_layer = kcrtc->master->wb_layer;
>>   
>>   	wb_conn = &kwb_conn->base;
>> -	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
>>   
>>   	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
>>   					       kwb_conn->wb_layer->layer_type,
>> @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>   	err = drm_writeback_connector_init(&kms->base, wb_conn,
>>   					   &komeda_wb_connector_funcs,
>>   					   &komeda_wb_encoder_helper_funcs,
>> -					   formats, n_formats);
>> +					   formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
>>   	komeda_put_fourcc_list(formats);
>>   	if (err) {
>>   		kfree(kwb_conn);
>> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
>> index f5847a7..b882066 100644
>> --- a/drivers/gpu/drm/arm/malidp_mw.c
>> +++ b/drivers/gpu/drm/arm/malidp_mw.c
>> @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>   	struct malidp_drm *malidp = drm->dev_private;
>>   	u32 *formats;
>>   	int ret, n_formats;
>> +	uint32_t possible_crtcs;
>>   
>>   	if (!malidp->dev->hw->enable_memwrite)
>>   		return 0;
>>   
>> -	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>> +	possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>>   	drm_connector_helper_add(&malidp->mw_connector.base,
>>   				 &malidp_mw_connector_helper_funcs);
>>   
>> @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>   	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
>>   					   &malidp_mw_connector_funcs,
>>   					   &malidp_mw_encoder_helper_funcs,
>> -					   formats, n_formats);
>> +					   formats, n_formats, possible_crtcs);
> 
> Do you need the local variable ?

Yes, we can dtop this. I just used this instead of "1 << 
drm_crtc_index(&malidp->crtc)" to simplify it.
No strong preference.

> 
>>   	kfree(formats);
>>   	if (ret)
>>   		return ret;
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index dccf4504..17c1471 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -149,36 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
>>   	.destroy = drm_encoder_cleanup,
>>   };
>>   
>> -/**
>> - * drm_writeback_connector_init - Initialize a writeback connector and its properties
>> - * @dev: DRM device
>> - * @wb_connector: Writeback connector to initialize
>> - * @con_funcs: Connector funcs vtable
>> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>> - * @formats: Array of supported pixel formats for the writeback engine
>> - * @n_formats: Length of the formats array
>> - *
>> - * This function creates the writeback-connector-specific properties if they
>> - * have not been already created, initializes the connector as
>> - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>> - * values. It will also create an internal encoder associated with the
>> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>> - * the encoder helper.
>> - *
>> - * Drivers should always use this function instead of drm_connector_init() to
>> - * set up writeback connectors.
>> - *
>> - * Returns: 0 on success, or a negative error code
>> - */
>> -int drm_writeback_connector_init(struct drm_device *dev,
>> -				 struct drm_writeback_connector *wb_connector,
>> -				 const struct drm_connector_funcs *con_funcs,
>> -				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
>> -				 const u32 *formats, int n_formats)
>> +static int drm_writeback_connector_setup(struct drm_device *dev,
>> +		struct drm_writeback_connector *wb_connector,
>> +		const struct drm_connector_funcs *con_funcs, const u32 *formats,
>> +		int n_formats)
>>   {
>>   	struct drm_property_blob *blob;
>> -	struct drm_connector *connector = &wb_connector->base;
>>   	struct drm_mode_config *config = &dev->mode_config;
>> +	struct drm_connector *connector = &wb_connector->base;
>> +
>>   	int ret = create_writeback_properties(dev);
>>   
>>   	if (ret != 0)
>> @@ -186,15 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   
>>   	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>>   					formats);
>> -	if (IS_ERR(blob))
>> -		return PTR_ERR(blob);
>> -
>> -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
>> -			       &drm_writeback_encoder_funcs,
>> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>> -	if (ret)
>> -		goto fail;
>> +	if (IS_ERR(blob)) {
>> +		ret = PTR_ERR(blob);
>> +		return ret;
>> +	}
>>   
>>   	connector->interlace_allowed = 0;
>>   
>> @@ -204,7 +178,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   		goto connector_fail;
>>   
>>   	ret = drm_connector_attach_encoder(connector,
>> -						&wb_connector->encoder);
>> +						wb_connector->encoder);
>>   	if (ret)
>>   		goto attach_fail;
>>   
>> @@ -233,11 +207,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   attach_fail:
>>   	drm_connector_cleanup(connector);
>>   connector_fail:
>> -	drm_encoder_cleanup(&wb_connector->encoder);
>> -fail:
>>   	drm_property_blob_put(blob);
>>   	return ret;
>>   }
>> +
>> +/**
>> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
>> + * using an internal encoder
>> + *
>> + * @dev: DRM device
>> + * @wb_connector: Writeback connector to initialize
>> + * @con_funcs: Connector funcs vtable
>> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>> + * @formats: Array of supported pixel formats for the writeback engine
>> + * @n_formats: Length of the formats array
>> + * @possible_crtcs: possible crtcs for the internal writeback encoder
>> + *
>> + * This function creates the writeback-connector-specific properties if they
>> + * have not been already created, initializes the connector as
>> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>> + * values. It will also create an internal encoder associated with the
>> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>> + * the encoder helper.
>> + *
>> + * Drivers should always use this function instead of drm_connector_init() to
>> + * set up writeback connectors.
>> + *
>> + * Returns: 0 on success, or a negative error code
>> + */
>> +int drm_writeback_connector_init(struct drm_device *dev,
>> +		struct drm_writeback_connector *wb_connector,
>> +		const struct drm_connector_funcs *con_funcs,
>> +		const struct drm_encoder_helper_funcs *enc_helper_funcs,
>> +		const u32 *formats, int n_formats, uint32_t possible_crtcs)
>> +{
>> +	int ret = 0;
>> +
>> +	wb_connector->encoder = &wb_connector->internal_encoder;
>> +
>> +	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>> +
>> +	wb_connector->encoder->possible_crtcs = possible_crtcs;
>> +
>> +	ret = drm_encoder_init(dev, wb_connector->encoder,
>> +			       &drm_writeback_encoder_funcs,
>> +			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
>> +			n_formats);
>> +
>> +	if (ret)
>> +		drm_encoder_cleanup(wb_connector->encoder);
>> +
>> +	return ret;
>> +}
> 
> All this doesn't belong to this patch. Just add the possible_crtcs
> argument to drm_writeback_connector_init() in 1/3, nothing else. One
> functional change, one patch.

Ok, so let me re-factor the series like this:

1) possible_crtcs argument along with other vendor changes
2) changing encoder to a pointer with changes to vc4
3) adding a new API drm_writeback_connector_init_with_encoder() with the 
cleanup in the drm_writeback
4) changing vc4 to use the new API

Let me know what you think.

> 
>>   EXPORT_SYMBOL(drm_writeback_connector_init);
>>   
>>   int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> index c79d125..1a610c0 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>> @@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>   			   struct rcar_du_crtc *rcrtc)
>>   {
>>   	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>> +	uint32_t possible_crtcs;
>>   
>> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>> +	possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>   	drm_connector_helper_add(&wb_conn->base,
>>   				 &rcar_du_wb_conn_helper_funcs);
>>   
>> @@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>   					    &rcar_du_wb_conn_funcs,
>>   					    &rcar_du_wb_enc_helper_funcs,
>>   					    writeback_formats,
>> -					    ARRAY_SIZE(writeback_formats));
>> +					    ARRAY_SIZE(writeback_formats), possible_crtcs);
> 
> Same here, do you need the intermediate variable ?

> 
> And please add a line break after ARRAY_SIZE().
Ok will do
> 
>>   }
>>   
>>   void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>> index 9809ca3..32c4fb0 100644
>> --- a/drivers/gpu/drm/vc4/vc4_txp.c
>> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
>> @@ -151,6 +151,8 @@ struct vc4_txp {
>>   
>>   	struct platform_device *pdev;
>>   
>> +	struct drm_encoder drm_enc;
>> +
>>   	struct drm_writeback_connector connector;
>>   
>>   	void __iomem *regs;
>> @@ -159,7 +161,7 @@ struct vc4_txp {
>>   
>>   static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>>   {
>> -	return container_of(encoder, struct vc4_txp, connector.encoder);
>> +	return container_of(encoder, struct vc4_txp, drm_enc);
>>   }
>>   
>>   static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
>> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   	struct vc4_txp *txp;
>>   	struct drm_crtc *crtc;
>>   	struct drm_encoder *encoder;
>> +	struct drm_writeback_connector *wb_conn;
>>   	int ret, irq;
>>   
>>   	irq = platform_get_irq(pdev, 0);
>> @@ -492,12 +495,14 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   	txp->regset.regs = txp_regs;
>>   	txp->regset.nregs = ARRAY_SIZE(txp_regs);
>>   
>> -	drm_connector_helper_add(&txp->connector.base,
>> +	wb_conn = &txp->connector;
>> +	wb_conn->encoder = &txp->drm_enc;
>> +
>> +	drm_connector_helper_add(&wb_conn->base,
>>   				 &vc4_txp_connector_helper_funcs);
>> -	ret = drm_writeback_connector_init(drm, &txp->connector,
>> -					   &vc4_txp_connector_funcs,
>> -					   &vc4_txp_encoder_helper_funcs,
>> -					   drm_fmts, ARRAY_SIZE(drm_fmts));
>> +	ret = drm_writeback_connector_init(drm, wb_conn,
>> +			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
>> +			drm_fmts, ARRAY_SIZE(drm_fmts), 0);
> 
> Most of this also doesn't belong to this patch.
> 
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -506,7 +511,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   	if (ret)
>>   		return ret;
>>   
>> -	encoder = &txp->connector.encoder;
>> +	encoder = txp->connector.encoder;
>>   	encoder->possible_crtcs = drm_crtc_mask(crtc);
>>   
>>   	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
>> index 8694227..381e7b7 100644
>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>> @@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
>>   {
>>   	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
>>   
>> -	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
>>   	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>>   
>>   	return drm_writeback_connector_init(&vkmsdev->drm, wb,
>>   					    &vkms_wb_connector_funcs,
>>   					    &vkms_wb_encoder_helper_funcs,
>>   					    vkms_wb_formats,
>> -					    ARRAY_SIZE(vkms_wb_formats));
>> +					    ARRAY_SIZE(vkms_wb_formats), 1);
>>   }
>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>> index 9697d27..5e002bc 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -25,13 +25,29 @@ struct drm_writeback_connector {
>>   	struct drm_connector base;
>>   
>>   	/**
>> -	 * @encoder: Internal encoder used by the connector to fulfill
>> +	 * @encoder: handle to drm_encoder used by the connector to fulfill
>>   	 * the DRM framework requirements. The users of the
>>   	 * @drm_writeback_connector control the behaviour of the @encoder
>>   	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>>   	 * function.
>> +	 *
>> +	 * For some vendor drivers, the hardware resources are shared between
>> +	 * writeback encoder and rest of the display pipeline.
>> +	 * To accommodate such cases, encoder is a handle to the real encoder
>> +	 * hardware.
>> +	 *
>> +	 * For current existing writeback users, this shall continue to be the
>> +	 * embedded encoder for the writeback connector.
>> +	 *
>> +	 */
>> +	struct drm_encoder *encoder;
>> +
>> +	/**
>> +	 * @internal_encoder: internal encoder used by writeback when
>> +	 * a real encoder is not provided by the vendor drm drivers.
>> +	 * @encoder will be assigned to this for those cases.
>>   	 */
>> -	struct drm_encoder encoder;
>> +	struct drm_encoder internal_encoder;
> 
> None of this belong here.
Alright, will split to the next patch.
> 
>>   
>>   	/**
>>   	 * @pixel_formats_blob_ptr:
>> @@ -150,7 +166,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   				 struct drm_writeback_connector *wb_connector,
>>   				 const struct drm_connector_funcs *con_funcs,
>>   				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
>> -				 const u32 *formats, int n_formats);
>> +				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
>>   
>>   int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>>   			 struct drm_framebuffer *fb);
> 

Re: [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Liviu Dudau]

On Thu, Mar 17, 2022 at 10:26:38AM -0700, Abhinav Kumar wrote:
> Hi Laurent
> 
> Thanks for the review.
> 
> On 3/17/2022 1:51 AM, Laurent Pinchart wrote:
> > Hi Abhinav,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
> > > For some vendor driver implementations, display hardware can
> > > be shared between the encoder used for writeback and the physical
> > > display.
> > > 
> > > In addition resources such as clocks and interrupts can
> > > also be shared between writeback and the real encoder.
> > > 
> > > To accommodate such vendor drivers and hardware, allow
> > > real encoder to be passed for drm_writeback_connector using a new
> > > drm_writeback_connector_init_with_encoder() API.
> > 
> > The commit message doesn't match the commit.
> Sorry, while splitting the change , I missed this part of the commit text.
> Will fix it up.
> > 
> > > In addition, to preserve the same call flows for the existing
> > > users of drm_writeback_connector_init(), also allow passing
> > > possible_crtcs as a parameter so that encoder can be initialized
> > > with it.
> > > 
> > > changes in v3:
> > > 	- allow passing possible_crtcs for existing users of
> > > 	  drm_writeback_connector_init()
> > > 	- squash the vendor changes into the same commit so
> > > 	  that each patch in the series can compile individually
> > > 
> > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > ---
> > >   .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
> > >   drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
> > >   drivers/gpu/drm/drm_writeback.c                    | 103 +++++++++++++--------
> > >   drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
> > >   drivers/gpu/drm/vc4/vc4_txp.c                      |  19 ++--
> > >   drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
> > >   include/drm/drm_writeback.h                        |  22 ++++-
> > >   7 files changed, 103 insertions(+), 57 deletions(-)
> > > 
> > > 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 e465cc4..40774e6 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
> > >   	kwb_conn->wb_layer = kcrtc->master->wb_layer;
> > >   	wb_conn = &kwb_conn->base;
> > > -	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
> > >   	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
> > >   					       kwb_conn->wb_layer->layer_type,
> > > @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
> > >   	err = drm_writeback_connector_init(&kms->base, wb_conn,
> > >   					   &komeda_wb_connector_funcs,
> > >   					   &komeda_wb_encoder_helper_funcs,
> > > -					   formats, n_formats);
> > > +					   formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
> > >   	komeda_put_fourcc_list(formats);
> > >   	if (err) {
> > >   		kfree(kwb_conn);
> > > diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> > > index f5847a7..b882066 100644
> > > --- a/drivers/gpu/drm/arm/malidp_mw.c
> > > +++ b/drivers/gpu/drm/arm/malidp_mw.c
> > > @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
> > >   	struct malidp_drm *malidp = drm->dev_private;
> > >   	u32 *formats;
> > >   	int ret, n_formats;
> > > +	uint32_t possible_crtcs;
> > >   	if (!malidp->dev->hw->enable_memwrite)
> > >   		return 0;
> > > -	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
> > > +	possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
> > >   	drm_connector_helper_add(&malidp->mw_connector.base,
> > >   				 &malidp_mw_connector_helper_funcs);
> > > @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
> > >   	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
> > >   					   &malidp_mw_connector_funcs,
> > >   					   &malidp_mw_encoder_helper_funcs,
> > > -					   formats, n_formats);
> > > +					   formats, n_formats, possible_crtcs);
> > 
> > Do you need the local variable ?
> 
> Yes, we can dtop this. I just used this instead of "1 <<
> drm_crtc_index(&malidp->crtc)" to simplify it.
> No strong preference.
> 
> > 
> > >   	kfree(formats);
> > >   	if (ret)
> > >   		return ret;
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index dccf4504..17c1471 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -149,36 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
> > >   	.destroy = drm_encoder_cleanup,
> > >   };
> > > -/**
> > > - * drm_writeback_connector_init - Initialize a writeback connector and its properties
> > > - * @dev: DRM device
> > > - * @wb_connector: Writeback connector to initialize
> > > - * @con_funcs: Connector funcs vtable
> > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
> > > - * @formats: Array of supported pixel formats for the writeback engine
> > > - * @n_formats: Length of the formats array
> > > - *
> > > - * This function creates the writeback-connector-specific properties if they
> > > - * have not been already created, initializes the connector as
> > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> > > - * values. It will also create an internal encoder associated with the
> > > - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
> > > - * the encoder helper.
> > > - *
> > > - * Drivers should always use this function instead of drm_connector_init() to
> > > - * set up writeback connectors.
> > > - *
> > > - * Returns: 0 on success, or a negative error code
> > > - */
> > > -int drm_writeback_connector_init(struct drm_device *dev,
> > > -				 struct drm_writeback_connector *wb_connector,
> > > -				 const struct drm_connector_funcs *con_funcs,
> > > -				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
> > > -				 const u32 *formats, int n_formats)
> > > +static int drm_writeback_connector_setup(struct drm_device *dev,
> > > +		struct drm_writeback_connector *wb_connector,
> > > +		const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > > +		int n_formats)
> > >   {
> > >   	struct drm_property_blob *blob;
> > > -	struct drm_connector *connector = &wb_connector->base;
> > >   	struct drm_mode_config *config = &dev->mode_config;
> > > +	struct drm_connector *connector = &wb_connector->base;
> > > +
> > >   	int ret = create_writeback_properties(dev);
> > >   	if (ret != 0)
> > > @@ -186,15 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >   	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
> > >   					formats);
> > > -	if (IS_ERR(blob))
> > > -		return PTR_ERR(blob);
> > > -
> > > -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > > -	ret = drm_encoder_init(dev, &wb_connector->encoder,
> > > -			       &drm_writeback_encoder_funcs,
> > > -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > -	if (ret)
> > > -		goto fail;
> > > +	if (IS_ERR(blob)) {
> > > +		ret = PTR_ERR(blob);
> > > +		return ret;
> > > +	}
> > >   	connector->interlace_allowed = 0;
> > > @@ -204,7 +178,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >   		goto connector_fail;
> > >   	ret = drm_connector_attach_encoder(connector,
> > > -						&wb_connector->encoder);
> > > +						wb_connector->encoder);
> > >   	if (ret)
> > >   		goto attach_fail;
> > > @@ -233,11 +207,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >   attach_fail:
> > >   	drm_connector_cleanup(connector);
> > >   connector_fail:
> > > -	drm_encoder_cleanup(&wb_connector->encoder);
> > > -fail:
> > >   	drm_property_blob_put(blob);
> > >   	return ret;
> > >   }
> > > +
> > > +/**
> > > + * drm_writeback_connector_init - Initialize a writeback connector and its properties
> > > + * using an internal encoder
> > > + *
> > > + * @dev: DRM device
> > > + * @wb_connector: Writeback connector to initialize
> > > + * @con_funcs: Connector funcs vtable
> > > + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
> > > + * @formats: Array of supported pixel formats for the writeback engine
> > > + * @n_formats: Length of the formats array
> > > + * @possible_crtcs: possible crtcs for the internal writeback encoder
> > > + *
> > > + * This function creates the writeback-connector-specific properties if they
> > > + * have not been already created, initializes the connector as
> > > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> > > + * values. It will also create an internal encoder associated with the
> > > + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
> > > + * the encoder helper.
> > > + *
> > > + * Drivers should always use this function instead of drm_connector_init() to
> > > + * set up writeback connectors.
> > > + *
> > > + * Returns: 0 on success, or a negative error code
> > > + */
> > > +int drm_writeback_connector_init(struct drm_device *dev,
> > > +		struct drm_writeback_connector *wb_connector,
> > > +		const struct drm_connector_funcs *con_funcs,
> > > +		const struct drm_encoder_helper_funcs *enc_helper_funcs,
> > > +		const u32 *formats, int n_formats, uint32_t possible_crtcs)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	wb_connector->encoder = &wb_connector->internal_encoder;
> > > +
> > > +	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > > +
> > > +	wb_connector->encoder->possible_crtcs = possible_crtcs;
> > > +
> > > +	ret = drm_encoder_init(dev, wb_connector->encoder,
> > > +			       &drm_writeback_encoder_funcs,
> > > +			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
> > > +			n_formats);
> > > +
> > > +	if (ret)
> > > +		drm_encoder_cleanup(wb_connector->encoder);
> > > +
> > > +	return ret;
> > > +}
> > 
> > All this doesn't belong to this patch. Just add the possible_crtcs
> > argument to drm_writeback_connector_init() in 1/3, nothing else. One
> > functional change, one patch.
> 
> Ok, so let me re-factor the series like this:
> 
> 1) possible_crtcs argument along with other vendor changes
> 2) changing encoder to a pointer with changes to vc4
> 3) adding a new API drm_writeback_connector_init_with_encoder() with the
> cleanup in the drm_writeback
> 4) changing vc4 to use the new API

I think the order should be 1, 3, 4, 2 (without the vc4 changes).

Best regards,
Liviu

> 
> Let me know what you think.
> 
> > 
> > >   EXPORT_SYMBOL(drm_writeback_connector_init);
> > >   int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > index c79d125..1a610c0 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> > > @@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> > >   			   struct rcar_du_crtc *rcrtc)
> > >   {
> > >   	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> > > +	uint32_t possible_crtcs;
> > > -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> > > +	possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> > >   	drm_connector_helper_add(&wb_conn->base,
> > >   				 &rcar_du_wb_conn_helper_funcs);
> > > @@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> > >   					    &rcar_du_wb_conn_funcs,
> > >   					    &rcar_du_wb_enc_helper_funcs,
> > >   					    writeback_formats,
> > > -					    ARRAY_SIZE(writeback_formats));
> > > +					    ARRAY_SIZE(writeback_formats), possible_crtcs);
> > 
> > Same here, do you need the intermediate variable ?
> 
> > 
> > And please add a line break after ARRAY_SIZE().
> Ok will do
> > 
> > >   }
> > >   void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> > > index 9809ca3..32c4fb0 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_txp.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> > > @@ -151,6 +151,8 @@ struct vc4_txp {
> > >   	struct platform_device *pdev;
> > > +	struct drm_encoder drm_enc;
> > > +
> > >   	struct drm_writeback_connector connector;
> > >   	void __iomem *regs;
> > > @@ -159,7 +161,7 @@ struct vc4_txp {
> > >   static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
> > >   {
> > > -	return container_of(encoder, struct vc4_txp, connector.encoder);
> > > +	return container_of(encoder, struct vc4_txp, drm_enc);
> > >   }
> > >   static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
> > > @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> > >   	struct vc4_txp *txp;
> > >   	struct drm_crtc *crtc;
> > >   	struct drm_encoder *encoder;
> > > +	struct drm_writeback_connector *wb_conn;
> > >   	int ret, irq;
> > >   	irq = platform_get_irq(pdev, 0);
> > > @@ -492,12 +495,14 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> > >   	txp->regset.regs = txp_regs;
> > >   	txp->regset.nregs = ARRAY_SIZE(txp_regs);
> > > -	drm_connector_helper_add(&txp->connector.base,
> > > +	wb_conn = &txp->connector;
> > > +	wb_conn->encoder = &txp->drm_enc;
> > > +
> > > +	drm_connector_helper_add(&wb_conn->base,
> > >   				 &vc4_txp_connector_helper_funcs);
> > > -	ret = drm_writeback_connector_init(drm, &txp->connector,
> > > -					   &vc4_txp_connector_funcs,
> > > -					   &vc4_txp_encoder_helper_funcs,
> > > -					   drm_fmts, ARRAY_SIZE(drm_fmts));
> > > +	ret = drm_writeback_connector_init(drm, wb_conn,
> > > +			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
> > > +			drm_fmts, ARRAY_SIZE(drm_fmts), 0);
> > 
> > Most of this also doesn't belong to this patch.
> > 
> > >   	if (ret)
> > >   		return ret;
> > > @@ -506,7 +511,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> > >   	if (ret)
> > >   		return ret;
> > > -	encoder = &txp->connector.encoder;
> > > +	encoder = txp->connector.encoder;
> > >   	encoder->possible_crtcs = drm_crtc_mask(crtc);
> > >   	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
> > > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > index 8694227..381e7b7 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > > @@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> > >   {
> > >   	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> > > -	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> > >   	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
> > >   	return drm_writeback_connector_init(&vkmsdev->drm, wb,
> > >   					    &vkms_wb_connector_funcs,
> > >   					    &vkms_wb_encoder_helper_funcs,
> > >   					    vkms_wb_formats,
> > > -					    ARRAY_SIZE(vkms_wb_formats));
> > > +					    ARRAY_SIZE(vkms_wb_formats), 1);
> > >   }
> > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > > index 9697d27..5e002bc 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -25,13 +25,29 @@ struct drm_writeback_connector {
> > >   	struct drm_connector base;
> > >   	/**
> > > -	 * @encoder: Internal encoder used by the connector to fulfill
> > > +	 * @encoder: handle to drm_encoder used by the connector to fulfill
> > >   	 * the DRM framework requirements. The users of the
> > >   	 * @drm_writeback_connector control the behaviour of the @encoder
> > >   	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
> > >   	 * function.
> > > +	 *
> > > +	 * For some vendor drivers, the hardware resources are shared between
> > > +	 * writeback encoder and rest of the display pipeline.
> > > +	 * To accommodate such cases, encoder is a handle to the real encoder
> > > +	 * hardware.
> > > +	 *
> > > +	 * For current existing writeback users, this shall continue to be the
> > > +	 * embedded encoder for the writeback connector.
> > > +	 *
> > > +	 */
> > > +	struct drm_encoder *encoder;
> > > +
> > > +	/**
> > > +	 * @internal_encoder: internal encoder used by writeback when
> > > +	 * a real encoder is not provided by the vendor drm drivers.
> > > +	 * @encoder will be assigned to this for those cases.
> > >   	 */
> > > -	struct drm_encoder encoder;
> > > +	struct drm_encoder internal_encoder;
> > 
> > None of this belong here.
> Alright, will split to the next patch.
> > 
> > >   	/**
> > >   	 * @pixel_formats_blob_ptr:
> > > @@ -150,7 +166,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >   				 struct drm_writeback_connector *wb_connector,
> > >   				 const struct drm_connector_funcs *con_funcs,
> > >   				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
> > > -				 const u32 *formats, int n_formats);
> > > +				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
> > >   int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > >   			 struct drm_framebuffer *fb);
> > 

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

Re: [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Liviu

On 3/21/2022 11:07 AM, Liviu Dudau wrote:
> On Thu, Mar 17, 2022 at 10:26:38AM -0700, Abhinav Kumar wrote:
>> Hi Laurent
>>
>> Thanks for the review.
>>
>> On 3/17/2022 1:51 AM, Laurent Pinchart wrote:
>>> Hi Abhinav,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
>>>> For some vendor driver implementations, display hardware can
>>>> be shared between the encoder used for writeback and the physical
>>>> display.
>>>>
>>>> In addition resources such as clocks and interrupts can
>>>> also be shared between writeback and the real encoder.
>>>>
>>>> To accommodate such vendor drivers and hardware, allow
>>>> real encoder to be passed for drm_writeback_connector using a new
>>>> drm_writeback_connector_init_with_encoder() API.
>>>
>>> The commit message doesn't match the commit.
>> Sorry, while splitting the change , I missed this part of the commit text.
>> Will fix it up.
>>>
>>>> In addition, to preserve the same call flows for the existing
>>>> users of drm_writeback_connector_init(), also allow passing
>>>> possible_crtcs as a parameter so that encoder can be initialized
>>>> with it.
>>>>
>>>> changes in v3:
>>>> 	- allow passing possible_crtcs for existing users of
>>>> 	  drm_writeback_connector_init()
>>>> 	- squash the vendor changes into the same commit so
>>>> 	  that each patch in the series can compile individually
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
>>>>    drivers/gpu/drm/arm/malidp_mw.c                    |   5 +-
>>>>    drivers/gpu/drm/drm_writeback.c                    | 103 +++++++++++++--------
>>>>    drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   5 +-
>>>>    drivers/gpu/drm/vc4/vc4_txp.c                      |  19 ++--
>>>>    drivers/gpu/drm/vkms/vkms_writeback.c              |   3 +-
>>>>    include/drm/drm_writeback.h                        |  22 ++++-
>>>>    7 files changed, 103 insertions(+), 57 deletions(-)
>>>>
>>>> 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 e465cc4..40774e6 100644
>>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>>>> @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>>>    	kwb_conn->wb_layer = kcrtc->master->wb_layer;
>>>>    	wb_conn = &kwb_conn->base;
>>>> -	wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
>>>>    	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
>>>>    					       kwb_conn->wb_layer->layer_type,
>>>> @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>>>    	err = drm_writeback_connector_init(&kms->base, wb_conn,
>>>>    					   &komeda_wb_connector_funcs,
>>>>    					   &komeda_wb_encoder_helper_funcs,
>>>> -					   formats, n_formats);
>>>> +					   formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
>>>>    	komeda_put_fourcc_list(formats);
>>>>    	if (err) {
>>>>    		kfree(kwb_conn);
>>>> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
>>>> index f5847a7..b882066 100644
>>>> --- a/drivers/gpu/drm/arm/malidp_mw.c
>>>> +++ b/drivers/gpu/drm/arm/malidp_mw.c
>>>> @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>>>    	struct malidp_drm *malidp = drm->dev_private;
>>>>    	u32 *formats;
>>>>    	int ret, n_formats;
>>>> +	uint32_t possible_crtcs;
>>>>    	if (!malidp->dev->hw->enable_memwrite)
>>>>    		return 0;
>>>> -	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>>>> +	possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>>>>    	drm_connector_helper_add(&malidp->mw_connector.base,
>>>>    				 &malidp_mw_connector_helper_funcs);
>>>> @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>>>    	ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
>>>>    					   &malidp_mw_connector_funcs,
>>>>    					   &malidp_mw_encoder_helper_funcs,
>>>> -					   formats, n_formats);
>>>> +					   formats, n_formats, possible_crtcs);
>>>
>>> Do you need the local variable ?
>>
>> Yes, we can dtop this. I just used this instead of "1 <<
>> drm_crtc_index(&malidp->crtc)" to simplify it.
>> No strong preference.
>>
>>>
>>>>    	kfree(formats);
>>>>    	if (ret)
>>>>    		return ret;
>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>> index dccf4504..17c1471 100644
>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>> @@ -149,36 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
>>>>    	.destroy = drm_encoder_cleanup,
>>>>    };
>>>> -/**
>>>> - * drm_writeback_connector_init - Initialize a writeback connector and its properties
>>>> - * @dev: DRM device
>>>> - * @wb_connector: Writeback connector to initialize
>>>> - * @con_funcs: Connector funcs vtable
>>>> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>>>> - * @formats: Array of supported pixel formats for the writeback engine
>>>> - * @n_formats: Length of the formats array
>>>> - *
>>>> - * This function creates the writeback-connector-specific properties if they
>>>> - * have not been already created, initializes the connector as
>>>> - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>>>> - * values. It will also create an internal encoder associated with the
>>>> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>>>> - * the encoder helper.
>>>> - *
>>>> - * Drivers should always use this function instead of drm_connector_init() to
>>>> - * set up writeback connectors.
>>>> - *
>>>> - * Returns: 0 on success, or a negative error code
>>>> - */
>>>> -int drm_writeback_connector_init(struct drm_device *dev,
>>>> -				 struct drm_writeback_connector *wb_connector,
>>>> -				 const struct drm_connector_funcs *con_funcs,
>>>> -				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
>>>> -				 const u32 *formats, int n_formats)
>>>> +static int drm_writeback_connector_setup(struct drm_device *dev,
>>>> +		struct drm_writeback_connector *wb_connector,
>>>> +		const struct drm_connector_funcs *con_funcs, const u32 *formats,
>>>> +		int n_formats)
>>>>    {
>>>>    	struct drm_property_blob *blob;
>>>> -	struct drm_connector *connector = &wb_connector->base;
>>>>    	struct drm_mode_config *config = &dev->mode_config;
>>>> +	struct drm_connector *connector = &wb_connector->base;
>>>> +
>>>>    	int ret = create_writeback_properties(dev);
>>>>    	if (ret != 0)
>>>> @@ -186,15 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>    	blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>>>>    					formats);
>>>> -	if (IS_ERR(blob))
>>>> -		return PTR_ERR(blob);
>>>> -
>>>> -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>>>> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
>>>> -			       &drm_writeback_encoder_funcs,
>>>> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>> -	if (ret)
>>>> -		goto fail;
>>>> +	if (IS_ERR(blob)) {
>>>> +		ret = PTR_ERR(blob);
>>>> +		return ret;
>>>> +	}
>>>>    	connector->interlace_allowed = 0;
>>>> @@ -204,7 +178,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>    		goto connector_fail;
>>>>    	ret = drm_connector_attach_encoder(connector,
>>>> -						&wb_connector->encoder);
>>>> +						wb_connector->encoder);
>>>>    	if (ret)
>>>>    		goto attach_fail;
>>>> @@ -233,11 +207,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>    attach_fail:
>>>>    	drm_connector_cleanup(connector);
>>>>    connector_fail:
>>>> -	drm_encoder_cleanup(&wb_connector->encoder);
>>>> -fail:
>>>>    	drm_property_blob_put(blob);
>>>>    	return ret;
>>>>    }
>>>> +
>>>> +/**
>>>> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
>>>> + * using an internal encoder
>>>> + *
>>>> + * @dev: DRM device
>>>> + * @wb_connector: Writeback connector to initialize
>>>> + * @con_funcs: Connector funcs vtable
>>>> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>>>> + * @formats: Array of supported pixel formats for the writeback engine
>>>> + * @n_formats: Length of the formats array
>>>> + * @possible_crtcs: possible crtcs for the internal writeback encoder
>>>> + *
>>>> + * This function creates the writeback-connector-specific properties if they
>>>> + * have not been already created, initializes the connector as
>>>> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>>>> + * values. It will also create an internal encoder associated with the
>>>> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>>>> + * the encoder helper.
>>>> + *
>>>> + * Drivers should always use this function instead of drm_connector_init() to
>>>> + * set up writeback connectors.
>>>> + *
>>>> + * Returns: 0 on success, or a negative error code
>>>> + */
>>>> +int drm_writeback_connector_init(struct drm_device *dev,
>>>> +		struct drm_writeback_connector *wb_connector,
>>>> +		const struct drm_connector_funcs *con_funcs,
>>>> +		const struct drm_encoder_helper_funcs *enc_helper_funcs,
>>>> +		const u32 *formats, int n_formats, uint32_t possible_crtcs)
>>>> +{
>>>> +	int ret = 0;
>>>> +
>>>> +	wb_connector->encoder = &wb_connector->internal_encoder;
>>>> +
>>>> +	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>>>> +
>>>> +	wb_connector->encoder->possible_crtcs = possible_crtcs;
>>>> +
>>>> +	ret = drm_encoder_init(dev, wb_connector->encoder,
>>>> +			       &drm_writeback_encoder_funcs,
>>>> +			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
>>>> +			n_formats);
>>>> +
>>>> +	if (ret)
>>>> +		drm_encoder_cleanup(wb_connector->encoder);
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> All this doesn't belong to this patch. Just add the possible_crtcs
>>> argument to drm_writeback_connector_init() in 1/3, nothing else. One
>>> functional change, one patch.
>>
>> Ok, so let me re-factor the series like this:
>>
>> 1) possible_crtcs argument along with other vendor changes
>> 2) changing encoder to a pointer with changes to vc4
>> 3) adding a new API drm_writeback_connector_init_with_encoder() with the
>> cleanup in the drm_writeback
>> 4) changing vc4 to use the new API
> 
> I think the order should be 1, 3, 4, 2 (without the vc4 changes).
> 
> Best regards,
> Liviu
> 
You are right. I should re-order this way to address the very valid 
comments posted on patch 2.

Thanks

Abhinav
>>
>> Let me know what you think.
>>
>>>
>>>>    EXPORT_SYMBOL(drm_writeback_connector_init);
>>>>    int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> index c79d125..1a610c0 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> @@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>>    			   struct rcar_du_crtc *rcrtc)
>>>>    {
>>>>    	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>>>> +	uint32_t possible_crtcs;
>>>> -	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>> +	possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>>    	drm_connector_helper_add(&wb_conn->base,
>>>>    				 &rcar_du_wb_conn_helper_funcs);
>>>> @@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>>    					    &rcar_du_wb_conn_funcs,
>>>>    					    &rcar_du_wb_enc_helper_funcs,
>>>>    					    writeback_formats,
>>>> -					    ARRAY_SIZE(writeback_formats));
>>>> +					    ARRAY_SIZE(writeback_formats), possible_crtcs);
>>>
>>> Same here, do you need the intermediate variable ?
>>
>>>
>>> And please add a line break after ARRAY_SIZE().
>> Ok will do
>>>
>>>>    }
>>>>    void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>>>> index 9809ca3..32c4fb0 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_txp.c
>>>> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
>>>> @@ -151,6 +151,8 @@ struct vc4_txp {
>>>>    	struct platform_device *pdev;
>>>> +	struct drm_encoder drm_enc;
>>>> +
>>>>    	struct drm_writeback_connector connector;
>>>>    	void __iomem *regs;
>>>> @@ -159,7 +161,7 @@ struct vc4_txp {
>>>>    static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>>>>    {
>>>> -	return container_of(encoder, struct vc4_txp, connector.encoder);
>>>> +	return container_of(encoder, struct vc4_txp, drm_enc);
>>>>    }
>>>>    static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
>>>> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>    	struct vc4_txp *txp;
>>>>    	struct drm_crtc *crtc;
>>>>    	struct drm_encoder *encoder;
>>>> +	struct drm_writeback_connector *wb_conn;
>>>>    	int ret, irq;
>>>>    	irq = platform_get_irq(pdev, 0);
>>>> @@ -492,12 +495,14 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>    	txp->regset.regs = txp_regs;
>>>>    	txp->regset.nregs = ARRAY_SIZE(txp_regs);
>>>> -	drm_connector_helper_add(&txp->connector.base,
>>>> +	wb_conn = &txp->connector;
>>>> +	wb_conn->encoder = &txp->drm_enc;
>>>> +
>>>> +	drm_connector_helper_add(&wb_conn->base,
>>>>    				 &vc4_txp_connector_helper_funcs);
>>>> -	ret = drm_writeback_connector_init(drm, &txp->connector,
>>>> -					   &vc4_txp_connector_funcs,
>>>> -					   &vc4_txp_encoder_helper_funcs,
>>>> -					   drm_fmts, ARRAY_SIZE(drm_fmts));
>>>> +	ret = drm_writeback_connector_init(drm, wb_conn,
>>>> +			&vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
>>>> +			drm_fmts, ARRAY_SIZE(drm_fmts), 0);
>>>
>>> Most of this also doesn't belong to this patch.
>>>
>>>>    	if (ret)
>>>>    		return ret;
>>>> @@ -506,7 +511,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>    	if (ret)
>>>>    		return ret;
>>>> -	encoder = &txp->connector.encoder;
>>>> +	encoder = txp->connector.encoder;
>>>>    	encoder->possible_crtcs = drm_crtc_mask(crtc);
>>>>    	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> index 8694227..381e7b7 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> @@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
>>>>    {
>>>>    	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
>>>> -	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
>>>>    	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>>>>    	return drm_writeback_connector_init(&vkmsdev->drm, wb,
>>>>    					    &vkms_wb_connector_funcs,
>>>>    					    &vkms_wb_encoder_helper_funcs,
>>>>    					    vkms_wb_formats,
>>>> -					    ARRAY_SIZE(vkms_wb_formats));
>>>> +					    ARRAY_SIZE(vkms_wb_formats), 1);
>>>>    }
>>>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>>>> index 9697d27..5e002bc 100644
>>>> --- a/include/drm/drm_writeback.h
>>>> +++ b/include/drm/drm_writeback.h
>>>> @@ -25,13 +25,29 @@ struct drm_writeback_connector {
>>>>    	struct drm_connector base;
>>>>    	/**
>>>> -	 * @encoder: Internal encoder used by the connector to fulfill
>>>> +	 * @encoder: handle to drm_encoder used by the connector to fulfill
>>>>    	 * the DRM framework requirements. The users of the
>>>>    	 * @drm_writeback_connector control the behaviour of the @encoder
>>>>    	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>>>>    	 * function.
>>>> +	 *
>>>> +	 * For some vendor drivers, the hardware resources are shared between
>>>> +	 * writeback encoder and rest of the display pipeline.
>>>> +	 * To accommodate such cases, encoder is a handle to the real encoder
>>>> +	 * hardware.
>>>> +	 *
>>>> +	 * For current existing writeback users, this shall continue to be the
>>>> +	 * embedded encoder for the writeback connector.
>>>> +	 *
>>>> +	 */
>>>> +	struct drm_encoder *encoder;
>>>> +
>>>> +	/**
>>>> +	 * @internal_encoder: internal encoder used by writeback when
>>>> +	 * a real encoder is not provided by the vendor drm drivers.
>>>> +	 * @encoder will be assigned to this for those cases.
>>>>    	 */
>>>> -	struct drm_encoder encoder;
>>>> +	struct drm_encoder internal_encoder;
>>>
>>> None of this belong here.
>> Alright, will split to the next patch.
>>>
>>>>    	/**
>>>>    	 * @pixel_formats_blob_ptr:
>>>> @@ -150,7 +166,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>    				 struct drm_writeback_connector *wb_connector,
>>>>    				 const struct drm_connector_funcs *con_funcs,
>>>>    				 const struct drm_encoder_helper_funcs *enc_helper_funcs,
>>>> -				 const u32 *formats, int n_formats);
>>>> +				 const u32 *formats, int n_formats, uint32_t possible_crtcs);
>>>>    int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>>>>    			 struct drm_framebuffer *fb);
>>>
>