[PATCH v5 0/4] Allow drm_writeback_connector to accept pointer to drm_encoder

[PATCH v5 0/4] 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 v5:
	- re-order the series to make sure the encoder initialization
	  is not broken due to incorrect order of changes

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

 .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
 drivers/gpu/drm/arm/malidp_mw.c                    |   4 +-
 drivers/gpu/drm/drm_writeback.c                    | 143 +++++++++++++++------
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |   4 +-
 drivers/gpu/drm/vc4/vc4_txp.c                      |  36 ++++--
 drivers/gpu/drm/vkms/vkms_writeback.c              |   4 +-
 include/drm/drm_writeback.h                        |  25 +++-
 7 files changed, 161 insertions(+), 58 deletions(-)

-- 
2.7.4

[PATCH v5 1/4] drm: allow passing possible_crtcs to drm_writeback_connector_init()

[Abhinav Kumar]

Clients of drm_writeback_connector_init() initialize the
possible_crtcs and then invoke the call to this API.

To simplify things, allow passing possible_crtcs as a parameter
to drm_writeback_connector_init() and make changes to the
other drm drivers to make them compatible with this change.

changes in v5:
     - None

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Acked-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +--
 drivers/gpu/drm/arm/malidp_mw.c                          | 4 ++--
 drivers/gpu/drm/drm_writeback.c                          | 6 +++++-
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c              | 4 ++--
 drivers/gpu/drm/vc4/vc4_txp.c                            | 3 ++-
 drivers/gpu/drm/vkms/vkms_writeback.c                    | 4 ++--
 include/drm/drm_writeback.h                              | 2 +-
 7 files changed, 15 insertions(+), 11 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..e54921d 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -212,7 +212,6 @@ int malidp_mw_connector_init(struct drm_device *drm)
 	if (!malidp->dev->hw->enable_memwrite)
 		return 0;
 
-	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
 	drm_connector_helper_add(&malidp->mw_connector.base,
 				 &malidp_mw_connector_helper_funcs);
 
@@ -223,7 +222,8 @@ 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,
+					  (1 << drm_crtc_index(&malidp->crtc)));
 	kfree(formats);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..dc2ef12 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -157,6 +157,7 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
  * @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
@@ -174,7 +175,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)
 {
 	struct drm_property_blob *blob;
 	struct drm_connector *connector = &wb_connector->base;
@@ -190,6 +191,9 @@ int drm_writeback_connector_init(struct drm_device *dev,
 		return PTR_ERR(blob);
 
 	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);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d125..fcfb0b3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -200,7 +200,6 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
 {
 	struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
 
-	wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
 	drm_connector_helper_add(&wb_conn->base,
 				 &rcar_du_wb_conn_helper_funcs);
 
@@ -208,7 +207,8 @@ 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),
+					   (1 << drm_crtc_index(&rcrtc->crtc)));
 }
 
 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..3447eb6 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -497,7 +497,8 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	ret = drm_writeback_connector_init(drm, &txp->connector,
 					   &vc4_txp_connector_funcs,
 					   &vc4_txp_encoder_helper_funcs,
-					   drm_fmts, ARRAY_SIZE(drm_fmts));
+					   drm_fmts, ARRAY_SIZE(drm_fmts),
+					   0);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227..6d01e55 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -140,12 +140,12 @@ 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..db6214f 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -150,7 +150,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 v5 2/4] 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.

changes in v5:
	- reorder this change to come before in the series
	  to avoid incorrect functionality in subsequent changes
	- continue using struct drm_encoder instead of
	  struct drm_encoder * and switch it in next change

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

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..abe78b9 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,37 +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
- * @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)
+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)
@@ -187,18 +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);
-
-	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)
-		goto fail;
+	if (IS_ERR(blob)) {
+		ret = PTR_ERR(blob);
+		return ret;
+	}
 
 	connector->interlace_allowed = 0;
 
@@ -237,13 +207,102 @@ 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
+ * @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;
+
+	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);
 
+/**
+ * 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 db6214f..0093bab 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -152,6 +152,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 v5 3/4] 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.

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 v5:
	- reorder this change to come before in the series
      to avoid incorrect sequence in subsequent changes
	- continue using struct drm_encoder instead of
	  struct drm_encoder * and switch it in next change

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

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3447eb6..5490162 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -368,6 +368,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;
@@ -467,6 +471,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,16 +497,27 @@ 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,
-				 &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),
-					   0);
+	wb_conn = &txp->connector;
+
+	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_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);
 	if (ret)
-- 
2.7.4

[PATCH v5 4/4] 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.

changes in v5:
	- re-order the change to come last in the series
	- rework necessary changes as part of the re-order

Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/drm_writeback.c | 12 +++++++-----
 drivers/gpu/drm/vc4/vc4_txp.c   | 13 ++++++++-----
 include/drm/drm_writeback.h     | 18 ++++++++++++++++--
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index abe78b9..d0672f5 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -178,7 +178,7 @@ static int drm_writeback_connector_setup(struct drm_device *dev,
 		goto connector_fail;
 
 	ret = drm_connector_attach_encoder(connector,
-						&wb_connector->encoder);
+						wb_connector->encoder);
 	if (ret)
 		goto attach_fail;
 
@@ -241,11 +241,13 @@ int drm_writeback_connector_init(struct drm_device *dev,
 {
 	int ret = 0;
 
-	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
+	wb_connector->encoder = &wb_connector->internal_encoder;
 
-	wb_connector->encoder.possible_crtcs = possible_crtcs;
+	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
 
-	ret = drm_encoder_init(dev, &wb_connector->encoder,
+	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)
@@ -255,7 +257,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 			n_formats);
 
 	if (ret)
-		drm_encoder_cleanup(&wb_connector->encoder);
+		drm_encoder_cleanup(wb_connector->encoder);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5490162..3d24ef5 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)
@@ -498,10 +500,11 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	txp->regset.nregs = ARRAY_SIZE(txp_regs);
 
 	wb_conn = &txp->connector;
+	wb_conn->encoder = &txp->drm_enc;
 
-	drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
+	drm_encoder_helper_add(wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
 
-	ret = drm_encoder_init(drm, &wb_conn->encoder,
+	ret = drm_encoder_init(drm, wb_conn->encoder,
 			&vc4_txp_encoder_funcs,
 			DRM_MODE_ENCODER_VIRTUAL, NULL);
 
@@ -514,7 +517,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	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);
+		drm_encoder_cleanup(wb_conn->encoder);
 		return ret;
 	}
 
@@ -523,7 +526,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/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 0093bab..ed35c3d 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,15 +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;
+	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 internal_encoder;
+	/**
 	 * @pixel_formats_blob_ptr:
 	 *
 	 * DRM blob property data for the pixel formats list on writeback
-- 
2.7.4

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Liviu Dudau]

On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> 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.
> 
> changes in v5:
> 	- reorder this change to come before in the series
> 	  to avoid incorrect functionality in subsequent changes
> 	- continue using struct drm_encoder instead of
> 	  struct drm_encoder * and switch it in next change
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Hi Abhinav,

> ---
>  drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>  include/drm/drm_writeback.h     |   5 ++
>  2 files changed, 106 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..abe78b9 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -149,37 +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
> - * @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)
> +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;
> +

Point of this reordering the statements is...?

>  	int ret = create_writeback_properties(dev);
>  
>  	if (ret != 0)
> @@ -187,18 +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);
> -
> -	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)
> -		goto fail;
> +	if (IS_ERR(blob)) {
> +		ret = PTR_ERR(blob);
> +		return ret;
> +	}

I don't see why you have changed the earlier code to store the result of PTR_ERR into
ret other than to help your debugging. I suggest that you keep the existing code that
returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

>  
>  	connector->interlace_allowed = 0;
>  
> @@ -237,13 +207,102 @@ 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
> + * @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;
> +
> +	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);
>  
> +/**
> + * 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

spelling: writeback

> + * 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.

.... if they want to manage themselves the lifetime of the associated encoder.

We're not trying to replace drm_writeback_connector_init() function here, only to
provide an alternative function to call for special cases.

> + *
> + * 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)

Where is the encoder parameter? Isn't that the reason why the function is called
`_with_encoder`?

I think there might have been too many version of these patchsets and things are
starting to be confusing. Please revisit the series without rushing and come up with
a plan of action. My understanding of watching this series has been that you're
trying to come up with a function that does *connector* initialisation but skips the
*encoder* initialisation because it might have been already done by the driver. The
idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
the encoder initialisation part. Then `drm_writeback_connector_init()` only
initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
There is no need to have the `drm_writeback_connector_setup()` function at all.

Best regards,
Liviu
 

> +{
> +	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 db6214f..0093bab 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -152,6 +152,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
> 

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

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

Thanks for the review.

On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
>> 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.
>>
>> changes in v5:
>> 	- reorder this change to come before in the series
>> 	  to avoid incorrect functionality in subsequent changes
>> 	- continue using struct drm_encoder instead of
>> 	  struct drm_encoder * and switch it in next change
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> Hi Abhinav,
> 
>> ---
>>   drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>>   include/drm/drm_writeback.h     |   5 ++
>>   2 files changed, 106 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index dc2ef12..abe78b9 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -149,37 +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
>> - * @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)
>> +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;
>> +
> 
> Point of this reordering the statements is...?
This diff can be avoided. There was no reason to reorder this. I will 
remove this re-order.
> 
>>   	int ret = create_writeback_properties(dev);
>>   
>>   	if (ret != 0)
>> @@ -187,18 +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);
>> -
>> -	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)
>> -		goto fail;
>> +	if (IS_ERR(blob)) {
>> +		ret = PTR_ERR(blob);
>> +		return ret;
>> +	}
> 
> I don't see why you have changed the earlier code to store the result of PTR_ERR into
> ret other than to help your debugging. I suggest that you keep the existing code that
> returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
Sure, i can fix this for a smaller diff stat.
> 
>>   
>>   	connector->interlace_allowed = 0;
>>   
>> @@ -237,13 +207,102 @@ 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
>> + * @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;
>> +
>> +	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);
>>   
>> +/**
>> + * 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
> 
> spelling: writeback
Ack. will fix this.
> 
>> + * 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.
> 
> .... if they want to manage themselves the lifetime of the associated encoder.
> 
> We're not trying to replace drm_writeback_connector_init() function here, only to
> provide an alternative function to call for special cases.

Yes, we are trying to provide an alternative function call for special 
case where the encoder is a shared encoder and/or where the resources of 
the writeback encoder are shared with other hardware blocks.

I can add that comment to this function's doc.

> 
>> + *
>> + * 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)
> 
> Where is the encoder parameter? Isn't that the reason why the function is called
> `_with_encoder`?
The encoder parameter is skipped here because this function assumes that 
wb_connector->encoder has already been initialized, setup with functions 
and its possible_crts have already been set prior to calling this 
function like the vc4 example shows. So this function doesnt need an 
explicit encoder parameter. Let me know if thats a concern.
> 
> I think there might have been too many version of these patchsets and things are
> starting to be confusing. Please revisit the series without rushing and come up with
> a plan of action. My understanding of watching this series has been that you're
> trying to come up with a function that does *connector* initialisation but skips the
> *encoder* initialisation because it might have been already done by the driver. The
> idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
> that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
> the encoder initialisation part. Then `drm_writeback_connector_init()` only
> initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
> There is no need to have the `drm_writeback_connector_setup()` function at all.
> 
> Best regards,
> Liviu
> 

I agree there have been a 4 revisions prior to this because of the way 
this affects the existing writeback drivers. So initial revision was a 
bit intrusive into other drivers (which was just mostly a take over from 
the previous patchset posted by the Co-developer ) and since rev3 we 
have decided to have a separate API 
drm_writeback_connector_init_with_encoder() so that existing clients are 
unaffected and it works seamlessly under the hood.

Only clients which already embed an encoder (vc4) and the new ones which 
have special encoder requirements like the MSM driver for which I am 
preparing these changes for will use the new API.

Apologies for the revisions, but thanks to some great feedback from you 
and laurent its shaping up nicely and reaching its conclusion I feel.

So i think this revision is pretty close to being clean thanks to the 
feedback you gave on rev4. Between rev4 and this one I didnt drastically 
change the design other than re-ordering the changes to avoid the 
intermediate patches having an incorrect state for the vc4 encoder. So 
all your comments related to the encoder_cleanup() and vc4's encoder not 
getting initialized would have gotten addressed but overall concept 
remained same.

You are right, we are trying to come up with a function which does 
connector initialization but skips the encoder part because that has 
already been done in the client side of this API ( hence _with_encoder() 
name ). The "_with_encoder" indicates that the caller already has an 
encoder for the writeback connector which is being passed so there is no 
need to pass the encoder again.

I thought this addresses all the concerns posted in the previous series.

So are you suggesting something like below?

1) rename drm_writeback_connector_setup() to 
drm_writeback_connector_init_with_encoder()
(essentially thats what will end up happening )

2) Inside drm_writeback_connector_init() check:

drm_writeback_connector_init(.....)
{
    if (!wb_conn->encoder)
	initialize the encoder

    call drm_writeback_**_init_with_encoder() 				
}

This will also work but have the foll concerns/questions:

1) Will drm_writeback_connector_init_with_encoder() be exported if we 
change as per your suggestion or will all clients continue to use 
drm_writeback_connector_init() only? We wanted to have a separate 
function for new clients.

2) How will we detect that encoder needs to be initialized without it 
being a pointer which happens in the later change. So ordering becomes 
an issue.

Thats why I thought this is the best way to address the comments and 
keep the functionality intact with each change.

Let me know if I have misunderstood some comment or idea.



> 
>> +{
>> +	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 db6214f..0093bab 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -152,6 +152,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
>>
> 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Liviu Dudau]

On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hello,

> 
> Thanks for the review.
> 
> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > 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.
> > > 
> > > changes in v5:
> > > 	- reorder this change to come before in the series
> > > 	  to avoid incorrect functionality in subsequent changes
> > > 	- continue using struct drm_encoder instead of
> > > 	  struct drm_encoder * and switch it in next change
> > > 
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > 
> > Hi Abhinav,
> > 
> > > ---
> > >   drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
> > >   include/drm/drm_writeback.h     |   5 ++
> > >   2 files changed, 106 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index dc2ef12..abe78b9 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -149,37 +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
> > > - * @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)
> > > +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;
> > > +
> > 
> > Point of this reordering the statements is...?
> This diff can be avoided. There was no reason to reorder this. I will remove
> this re-order.
> > 
> > >   	int ret = create_writeback_properties(dev);
> > >   	if (ret != 0)
> > > @@ -187,18 +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);
> > > -
> > > -	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)
> > > -		goto fail;
> > > +	if (IS_ERR(blob)) {
> > > +		ret = PTR_ERR(blob);
> > > +		return ret;
> > > +	}
> > 
> > I don't see why you have changed the earlier code to store the result of PTR_ERR into
> > ret other than to help your debugging. I suggest that you keep the existing code that
> > returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
> Sure, i can fix this for a smaller diff stat.
> > 
> > >   	connector->interlace_allowed = 0;
> > > @@ -237,13 +207,102 @@ 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
> > > + * @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;
> > > +
> > > +	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);
> > > +/**
> > > + * 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
> > 
> > spelling: writeback
> Ack. will fix this.
> > 
> > > + * 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.
> > 
> > .... if they want to manage themselves the lifetime of the associated encoder.
> > 
> > We're not trying to replace drm_writeback_connector_init() function here, only to
> > provide an alternative function to call for special cases.
> 
> Yes, we are trying to provide an alternative function call for special case
> where the encoder is a shared encoder and/or where the resources of the
> writeback encoder are shared with other hardware blocks.
> 
> I can add that comment to this function's doc.
> 
> > 
> > > + *
> > > + * 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)
> > 
> > Where is the encoder parameter? Isn't that the reason why the function is called
> > `_with_encoder`?
> The encoder parameter is skipped here because this function assumes that
> wb_connector->encoder has already been initialized, setup with functions and
> its possible_crts have already been set prior to calling this function like
> the vc4 example shows. So this function doesnt need an explicit encoder
> parameter. Let me know if thats a concern.
> > 
> > I think there might have been too many version of these patchsets and things are
> > starting to be confusing. Please revisit the series without rushing and come up with
> > a plan of action. My understanding of watching this series has been that you're
> > trying to come up with a function that does *connector* initialisation but skips the
> > *encoder* initialisation because it might have been already done by the driver. The
> > idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
> > that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
> > the encoder initialisation part. Then `drm_writeback_connector_init()` only
> > initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
> > There is no need to have the `drm_writeback_connector_setup()` function at all.
> > 
> > Best regards,
> > Liviu
> > 
> 
> I agree there have been a 4 revisions prior to this because of the way this
> affects the existing writeback drivers. So initial revision was a bit
> intrusive into other drivers (which was just mostly a take over from the
> previous patchset posted by the Co-developer ) and since rev3 we have
> decided to have a separate API drm_writeback_connector_init_with_encoder()
> so that existing clients are unaffected and it works seamlessly under the
> hood.
> 
> Only clients which already embed an encoder (vc4) and the new ones which
> have special encoder requirements like the MSM driver for which I am
> preparing these changes for will use the new API.
> 
> Apologies for the revisions, but thanks to some great feedback from you and
> laurent its shaping up nicely and reaching its conclusion I feel.

I think it is only natural that there will be some iterations. If I remember
correctly, the initial writeback series has something like 13 revisions :)

> 
> So i think this revision is pretty close to being clean thanks to the
> feedback you gave on rev4. Between rev4 and this one I didnt drastically
> change the design other than re-ordering the changes to avoid the
> intermediate patches having an incorrect state for the vc4 encoder. So all
> your comments related to the encoder_cleanup() and vc4's encoder not getting
> initialized would have gotten addressed but overall concept remained same.
> 
> You are right, we are trying to come up with a function which does connector
> initialization but skips the encoder part because that has already been done
> in the client side of this API ( hence _with_encoder() name ). The
> "_with_encoder" indicates that the caller already has an encoder for the
> writeback connector which is being passed so there is no need to pass the
> encoder again.
> 
> I thought this addresses all the concerns posted in the previous series.
> 
> So are you suggesting something like below?
> 
> 1) rename drm_writeback_connector_setup() to
> drm_writeback_connector_init_with_encoder()
> (essentially thats what will end up happening )

Correct. Without the pointless reordering of code it should be about 3 lines of code
that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).

> 
> 2) Inside drm_writeback_connector_init() check:
> 
> drm_writeback_connector_init(.....)
> {
>    if (!wb_conn->encoder)
> 	initialize the encoder

No, the assumption of drm_writeback_connector_init() is that we will provide the
encoder, so no need to check that one is already provided. What you could do is:

     WARN_ON(wb_conn->encoder);

before we overwrite the encoder. That way we will get a nice warning in the kernel
log if someone tries to call drm_writeback_connector_init() with a preset encoder.

> 
>    call drm_writeback_**_init_with_encoder() 				
> }
> 
> This will also work but have the foll concerns/questions:
> 
> 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
> as per your suggestion or will all clients continue to use
> drm_writeback_connector_init() only? We wanted to have a separate function
> for new clients.

Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.

> 
> 2) How will we detect that encoder needs to be initialized without it being
> a pointer which happens in the later change. So ordering becomes an issue.

I think the init problem is simple. You either call drm_writeback_connector_init()
and the framework provides you with an encoder, or you call
drm_writeback_connector_init_with_encoder() and the framework will use yours. The
problems will show up on the cleanup and exit codes, where we need to be able to skip
the cleanup if the encoder pointer is not the internal one. I think a simple

    if (connector->encoder == &connector->internal_encoder)
          do_encoder_cleanup_work_here()

should work.

> 
> Thats why I thought this is the best way to address the comments and keep
> the functionality intact with each change.
> 
> Let me know if I have misunderstood some comment or idea.

I hope that with these clarifications we are both on the same page.

Best regards,
Liviu



> 
> 
> 
> > 
> > > +{
> > > +	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 db6214f..0093bab 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -152,6 +152,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
> > > 
> > 

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

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

Thanks for the response.

On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
>> Hi Liviu
> 
> Hello,
> 
>>
>> Thanks for the review.
>>
>> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
>>> On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
>>>> 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.
>>>>
>>>> changes in v5:
>>>> 	- reorder this change to come before in the series
>>>> 	  to avoid incorrect functionality in subsequent changes
>>>> 	- continue using struct drm_encoder instead of
>>>> 	  struct drm_encoder * and switch it in next change
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>> Hi Abhinav,
>>>
>>>> ---
>>>>    drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>>>>    include/drm/drm_writeback.h     |   5 ++
>>>>    2 files changed, 106 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>> index dc2ef12..abe78b9 100644
>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>> @@ -149,37 +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
>>>> - * @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)
>>>> +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;
>>>> +
>>>
>>> Point of this reordering the statements is...?
>> This diff can be avoided. There was no reason to reorder this. I will remove
>> this re-order.
>>>
>>>>    	int ret = create_writeback_properties(dev);
>>>>    	if (ret != 0)
>>>> @@ -187,18 +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);
>>>> -
>>>> -	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)
>>>> -		goto fail;
>>>> +	if (IS_ERR(blob)) {
>>>> +		ret = PTR_ERR(blob);
>>>> +		return ret;
>>>> +	}
>>>
>>> I don't see why you have changed the earlier code to store the result of PTR_ERR into
>>> ret other than to help your debugging. I suggest that you keep the existing code that
>>> returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
>> Sure, i can fix this for a smaller diff stat.
>>>
>>>>    	connector->interlace_allowed = 0;
>>>> @@ -237,13 +207,102 @@ 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
>>>> + * @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;
>>>> +
>>>> +	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);
>>>> +/**
>>>> + * 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
>>>
>>> spelling: writeback
>> Ack. will fix this.
>>>
>>>> + * 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.
>>>
>>> .... if they want to manage themselves the lifetime of the associated encoder.
>>>
>>> We're not trying to replace drm_writeback_connector_init() function here, only to
>>> provide an alternative function to call for special cases.
>>
>> Yes, we are trying to provide an alternative function call for special case
>> where the encoder is a shared encoder and/or where the resources of the
>> writeback encoder are shared with other hardware blocks.
>>
>> I can add that comment to this function's doc.
>>
>>>
>>>> + *
>>>> + * 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)
>>>
>>> Where is the encoder parameter? Isn't that the reason why the function is called
>>> `_with_encoder`?
>> The encoder parameter is skipped here because this function assumes that
>> wb_connector->encoder has already been initialized, setup with functions and
>> its possible_crts have already been set prior to calling this function like
>> the vc4 example shows. So this function doesnt need an explicit encoder
>> parameter. Let me know if thats a concern.
>>>
>>> I think there might have been too many version of these patchsets and things are
>>> starting to be confusing. Please revisit the series without rushing and come up with
>>> a plan of action. My understanding of watching this series has been that you're
>>> trying to come up with a function that does *connector* initialisation but skips the
>>> *encoder* initialisation because it might have been already done by the driver. The
>>> idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
>>> that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
>>> the encoder initialisation part. Then `drm_writeback_connector_init()` only
>>> initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
>>> There is no need to have the `drm_writeback_connector_setup()` function at all.
>>>
>>> Best regards,
>>> Liviu
>>>
>>
>> I agree there have been a 4 revisions prior to this because of the way this
>> affects the existing writeback drivers. So initial revision was a bit
>> intrusive into other drivers (which was just mostly a take over from the
>> previous patchset posted by the Co-developer ) and since rev3 we have
>> decided to have a separate API drm_writeback_connector_init_with_encoder()
>> so that existing clients are unaffected and it works seamlessly under the
>> hood.
>>
>> Only clients which already embed an encoder (vc4) and the new ones which
>> have special encoder requirements like the MSM driver for which I am
>> preparing these changes for will use the new API.
>>
>> Apologies for the revisions, but thanks to some great feedback from you and
>> laurent its shaping up nicely and reaching its conclusion I feel.
> 
> I think it is only natural that there will be some iterations. If I remember
> correctly, the initial writeback series has something like 13 revisions :)
> 
>>
>> So i think this revision is pretty close to being clean thanks to the
>> feedback you gave on rev4. Between rev4 and this one I didnt drastically
>> change the design other than re-ordering the changes to avoid the
>> intermediate patches having an incorrect state for the vc4 encoder. So all
>> your comments related to the encoder_cleanup() and vc4's encoder not getting
>> initialized would have gotten addressed but overall concept remained same.
>>
>> You are right, we are trying to come up with a function which does connector
>> initialization but skips the encoder part because that has already been done
>> in the client side of this API ( hence _with_encoder() name ). The
>> "_with_encoder" indicates that the caller already has an encoder for the
>> writeback connector which is being passed so there is no need to pass the
>> encoder again.
>>
>> I thought this addresses all the concerns posted in the previous series.
>>
>> So are you suggesting something like below?
>>
>> 1) rename drm_writeback_connector_setup() to
>> drm_writeback_connector_init_with_encoder()
>> (essentially thats what will end up happening )
> 
> Correct. Without the pointless reordering of code it should be about 3 lines of code
> that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
> 

But isnt thats how it already looks.

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

So the only change you are requesting is that, instead of having a new 
drm_writeback_connector_setup(), just call 
drm_writeback_connector_init_with_encoder().

It will essentially look like

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_init_with_encoder(dev, wb_connector, 
con_funcs, formats,
             n_formats);

drm_writeback_connector_init_with_encoder() will still not have an 
encoder parameter because of what I wrote previously.

Hope this is what you had in mind as well.

>>
>> 2) Inside drm_writeback_connector_init() check:
>>
>> drm_writeback_connector_init(.....)
>> {
>>     if (!wb_conn->encoder)
>> 	initialize the encoder
> 
> No, the assumption of drm_writeback_connector_init() is that we will provide the
> encoder, so no need to check that one is already provided. What you could do is:
> 
>       WARN_ON(wb_conn->encoder);
> 

Got it, i will add a warning inside drm_writeback_connector_init() to 
emphasize that its only meant for cases where an encoder is not provided.

> before we overwrite the encoder. That way we will get a nice warning in the kernel
> log if someone tries to call drm_writeback_connector_init() with a preset encoder.
> 
>>
>>     call drm_writeback_**_init_with_encoder() 				
>> }
>>
>> This will also work but have the foll concerns/questions:
>>
>> 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
>> as per your suggestion or will all clients continue to use
>> drm_writeback_connector_init() only? We wanted to have a separate function
>> for new clients.
> 
> Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
> 
Alright, so vc4 and new vendors which provide their own encoder will use 
this one. So no changes to the rest of the series.

>>
>> 2) How will we detect that encoder needs to be initialized without it being
>> a pointer which happens in the later change. So ordering becomes an issue.
> 
> I think the init problem is simple. You either call drm_writeback_connector_init()
> and the framework provides you with an encoder, or you call
> drm_writeback_connector_init_with_encoder() and the framework will use yours. The
> problems will show up on the cleanup and exit codes, where we need to be able to skip
> the cleanup if the encoder pointer is not the internal one. I think a simple
> 
>      if (connector->encoder == &connector->internal_encoder)
>            do_encoder_cleanup_work_here()
> 
> should work.

Sorry, I am missing something here.

Even in this current patch, the drm_encoder_cleanup() is done only 
inside drm_writeback_connector_init() where internal_encoder is used.

For drm_writeback_connector_init_with_encoder(), we dont do the cleanup 
and expect the caller to do it like vc4 does in the next patch.

So why do we need such a condition?

> 
>>
>> Thats why I thought this is the best way to address the comments and keep
>> the functionality intact with each change.
>>
>> Let me know if I have misunderstood some comment or idea.
> 
> I hope that with these clarifications we are both on the same page.
> 
> Best regards,
> Liviu
> 
> 
> 
>>
>>
>>
>>>
>>>> +{
>>>> +	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 db6214f..0093bab 100644
>>>> --- a/include/drm/drm_writeback.h
>>>> +++ b/include/drm/drm_writeback.h
>>>> @@ -152,6 +152,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
>>>>
>>>
> 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Liviu Dudau]

On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hello,

> 
> Thanks for the response.
> 
> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> > On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> > > Hi Liviu
> > 
> > Hello,
> > 
> > > 
> > > Thanks for the review.
> > > 
> > > On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > > > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > > > 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.
> > > > > 
> > > > > changes in v5:
> > > > > 	- reorder this change to come before in the series
> > > > > 	  to avoid incorrect functionality in subsequent changes
> > > > > 	- continue using struct drm_encoder instead of
> > > > > 	  struct drm_encoder * and switch it in next change
> > > > > 
> > > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > > 
> > > > Hi Abhinav,
> > > > 
> > > > > ---
> > > > >    drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
> > > > >    include/drm/drm_writeback.h     |   5 ++
> > > > >    2 files changed, 106 insertions(+), 42 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > > > index dc2ef12..abe78b9 100644
> > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > @@ -149,37 +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
> > > > > - * @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)
> > > > > +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;
> > > > > +
> > > > 
> > > > Point of this reordering the statements is...?
> > > This diff can be avoided. There was no reason to reorder this. I will remove
> > > this re-order.
> > > > 
> > > > >    	int ret = create_writeback_properties(dev);
> > > > >    	if (ret != 0)
> > > > > @@ -187,18 +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);
> > > > > -
> > > > > -	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)
> > > > > -		goto fail;
> > > > > +	if (IS_ERR(blob)) {
> > > > > +		ret = PTR_ERR(blob);
> > > > > +		return ret;
> > > > > +	}
> > > > 
> > > > I don't see why you have changed the earlier code to store the result of PTR_ERR into
> > > > ret other than to help your debugging. I suggest that you keep the existing code that
> > > > returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
> > > Sure, i can fix this for a smaller diff stat.
> > > > 
> > > > >    	connector->interlace_allowed = 0;
> > > > > @@ -237,13 +207,102 @@ 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
> > > > > + * @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;
> > > > > +
> > > > > +	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);
> > > > > +/**
> > > > > + * 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
> > > > 
> > > > spelling: writeback
> > > Ack. will fix this.
> > > > 
> > > > > + * 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.
> > > > 
> > > > .... if they want to manage themselves the lifetime of the associated encoder.
> > > > 
> > > > We're not trying to replace drm_writeback_connector_init() function here, only to
> > > > provide an alternative function to call for special cases.
> > > 
> > > Yes, we are trying to provide an alternative function call for special case
> > > where the encoder is a shared encoder and/or where the resources of the
> > > writeback encoder are shared with other hardware blocks.
> > > 
> > > I can add that comment to this function's doc.
> > > 
> > > > 
> > > > > + *
> > > > > + * 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)
> > > > 
> > > > Where is the encoder parameter? Isn't that the reason why the function is called
> > > > `_with_encoder`?
> > > The encoder parameter is skipped here because this function assumes that
> > > wb_connector->encoder has already been initialized, setup with functions and
> > > its possible_crts have already been set prior to calling this function like
> > > the vc4 example shows. So this function doesnt need an explicit encoder
> > > parameter. Let me know if thats a concern.
> > > > 
> > > > I think there might have been too many version of these patchsets and things are
> > > > starting to be confusing. Please revisit the series without rushing and come up with
> > > > a plan of action. My understanding of watching this series has been that you're
> > > > trying to come up with a function that does *connector* initialisation but skips the
> > > > *encoder* initialisation because it might have been already done by the driver. The
> > > > idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
> > > > that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
> > > > the encoder initialisation part. Then `drm_writeback_connector_init()` only
> > > > initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
> > > > There is no need to have the `drm_writeback_connector_setup()` function at all.
> > > > 
> > > > Best regards,
> > > > Liviu
> > > > 
> > > 
> > > I agree there have been a 4 revisions prior to this because of the way this
> > > affects the existing writeback drivers. So initial revision was a bit
> > > intrusive into other drivers (which was just mostly a take over from the
> > > previous patchset posted by the Co-developer ) and since rev3 we have
> > > decided to have a separate API drm_writeback_connector_init_with_encoder()
> > > so that existing clients are unaffected and it works seamlessly under the
> > > hood.
> > > 
> > > Only clients which already embed an encoder (vc4) and the new ones which
> > > have special encoder requirements like the MSM driver for which I am
> > > preparing these changes for will use the new API.
> > > 
> > > Apologies for the revisions, but thanks to some great feedback from you and
> > > laurent its shaping up nicely and reaching its conclusion I feel.
> > 
> > I think it is only natural that there will be some iterations. If I remember
> > correctly, the initial writeback series has something like 13 revisions :)
> > 
> > > 
> > > So i think this revision is pretty close to being clean thanks to the
> > > feedback you gave on rev4. Between rev4 and this one I didnt drastically
> > > change the design other than re-ordering the changes to avoid the
> > > intermediate patches having an incorrect state for the vc4 encoder. So all
> > > your comments related to the encoder_cleanup() and vc4's encoder not getting
> > > initialized would have gotten addressed but overall concept remained same.
> > > 
> > > You are right, we are trying to come up with a function which does connector
> > > initialization but skips the encoder part because that has already been done
> > > in the client side of this API ( hence _with_encoder() name ). The
> > > "_with_encoder" indicates that the caller already has an encoder for the
> > > writeback connector which is being passed so there is no need to pass the
> > > encoder again.
> > > 
> > > I thought this addresses all the concerns posted in the previous series.
> > > 
> > > So are you suggesting something like below?
> > > 
> > > 1) rename drm_writeback_connector_setup() to
> > > drm_writeback_connector_init_with_encoder()
> > > (essentially thats what will end up happening )
> > 
> > Correct. Without the pointless reordering of code it should be about 3 lines of code
> > that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
> > 
> 
> But isnt thats how it already looks.
> 
> 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);
> 
> So the only change you are requesting is that, instead of having a new
> drm_writeback_connector_setup(), just call
> drm_writeback_connector_init_with_encoder().
> 
> It will essentially look like
> 
> 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;

(1)

> 
>     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_init_with_encoder(dev, wb_connector,
> con_funcs, formats,
>             n_formats);

Yes, this is exactly what I had in mind.


> 
> drm_writeback_connector_init_with_encoder() will still not have an encoder
> parameter because of what I wrote previously.

But in your patch drm_writeback_connector_init_with_encoder() still has an
encoder_funcs pointer which is useless, as the expectations are (AFAII) that the
whole encoder init dance has already happened. And while you have a point that you
can set the encoder pointer in the connector before calling
drm_writeback_connector_init_with_encoder() I think it would be easier to read if you
pass the encoder explicitly in the parameter list and skip the assignment in (1) and
do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not
wrong, I just think we should be explicit so that code is easier to read.

> 
> Hope this is what you had in mind as well.
> 
> > > 
> > > 2) Inside drm_writeback_connector_init() check:
> > > 
> > > drm_writeback_connector_init(.....)
> > > {
> > >     if (!wb_conn->encoder)
> > > 	initialize the encoder
> > 
> > No, the assumption of drm_writeback_connector_init() is that we will provide the
> > encoder, so no need to check that one is already provided. What you could do is:
> > 
> >       WARN_ON(wb_conn->encoder);
> > 
> 
> Got it, i will add a warning inside drm_writeback_connector_init() to
> emphasize that its only meant for cases where an encoder is not provided.
> 
> > before we overwrite the encoder. That way we will get a nice warning in the kernel
> > log if someone tries to call drm_writeback_connector_init() with a preset encoder.
> > 
> > > 
> > >     call drm_writeback_**_init_with_encoder() 				
> > > }
> > > 
> > > This will also work but have the foll concerns/questions:
> > > 
> > > 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
> > > as per your suggestion or will all clients continue to use
> > > drm_writeback_connector_init() only? We wanted to have a separate function
> > > for new clients.
> > 
> > Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
> > 
> Alright, so vc4 and new vendors which provide their own encoder will use
> this one. So no changes to the rest of the series.
> 
> > > 
> > > 2) How will we detect that encoder needs to be initialized without it being
> > > a pointer which happens in the later change. So ordering becomes an issue.
> > 
> > I think the init problem is simple. You either call drm_writeback_connector_init()
> > and the framework provides you with an encoder, or you call
> > drm_writeback_connector_init_with_encoder() and the framework will use yours. The
> > problems will show up on the cleanup and exit codes, where we need to be able to skip
> > the cleanup if the encoder pointer is not the internal one. I think a simple
> > 
> >      if (connector->encoder == &connector->internal_encoder)
> >            do_encoder_cleanup_work_here()
> > 
> > should work.
> 
> Sorry, I am missing something here.
> 
> Even in this current patch, the drm_encoder_cleanup() is done only inside
> drm_writeback_connector_init() where internal_encoder is used.
> 
> For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and
> expect the caller to do it like vc4 does in the next patch.
> 
> So why do we need such a condition?

You're right. I was thinking that at cleanup time we also need to do work with the
right encoder, but that should accomplished by passing the right .destroy hook in the
drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the
initialisation correctly it should all work fine, please disregard my comments.

Best regards,
Liviu


> 
> > 
> > > 
> > > Thats why I thought this is the best way to address the comments and keep
> > > the functionality intact with each change.
> > > 
> > > Let me know if I have misunderstood some comment or idea.
> > 
> > I hope that with these clarifications we are both on the same page.
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > > +{
> > > > > +	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 db6214f..0093bab 100644
> > > > > --- a/include/drm/drm_writeback.h
> > > > > +++ b/include/drm/drm_writeback.h
> > > > > @@ -152,6 +152,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
> > > > > 
> > > > 
> > 

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

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

On 3/25/2022 3:19 AM, Liviu Dudau wrote:
> On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
>> Hi Liviu
> 
> Hello,
> 
>>
>> Thanks for the response.
>>
>> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
>>> On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
>>>> Hi Liviu
>>>
>>> Hello,
>>>
>>>>
>>>> Thanks for the review.
>>>>
>>>> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
>>>>> On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
>>>>>> 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.
>>>>>>
>>>>>> changes in v5:
>>>>>> 	- reorder this change to come before in the series
>>>>>> 	  to avoid incorrect functionality in subsequent changes
>>>>>> 	- continue using struct drm_encoder instead of
>>>>>> 	  struct drm_encoder * and switch it in next change
>>>>>>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>
>>>>> Hi Abhinav,
>>>>>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>>>>>>     include/drm/drm_writeback.h     |   5 ++
>>>>>>     2 files changed, 106 insertions(+), 42 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>>>> index dc2ef12..abe78b9 100644
>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>> @@ -149,37 +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
>>>>>> - * @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)
>>>>>> +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;
>>>>>> +
>>>>>
>>>>> Point of this reordering the statements is...?
>>>> This diff can be avoided. There was no reason to reorder this. I will remove
>>>> this re-order.
>>>>>
>>>>>>     	int ret = create_writeback_properties(dev);
>>>>>>     	if (ret != 0)
>>>>>> @@ -187,18 +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);
>>>>>> -
>>>>>> -	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)
>>>>>> -		goto fail;
>>>>>> +	if (IS_ERR(blob)) {
>>>>>> +		ret = PTR_ERR(blob);
>>>>>> +		return ret;
>>>>>> +	}
>>>>>
>>>>> I don't see why you have changed the earlier code to store the result of PTR_ERR into
>>>>> ret other than to help your debugging. I suggest that you keep the existing code that
>>>>> returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
>>>> Sure, i can fix this for a smaller diff stat.
>>>>>
>>>>>>     	connector->interlace_allowed = 0;
>>>>>> @@ -237,13 +207,102 @@ 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
>>>>>> + * @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;
>>>>>> +
>>>>>> +	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);
>>>>>> +/**
>>>>>> + * 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
>>>>>
>>>>> spelling: writeback
>>>> Ack. will fix this.
>>>>>
>>>>>> + * 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.
>>>>>
>>>>> .... if they want to manage themselves the lifetime of the associated encoder.
>>>>>
>>>>> We're not trying to replace drm_writeback_connector_init() function here, only to
>>>>> provide an alternative function to call for special cases.
>>>>
>>>> Yes, we are trying to provide an alternative function call for special case
>>>> where the encoder is a shared encoder and/or where the resources of the
>>>> writeback encoder are shared with other hardware blocks.
>>>>
>>>> I can add that comment to this function's doc.
>>>>
>>>>>
>>>>>> + *
>>>>>> + * 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)
>>>>>
>>>>> Where is the encoder parameter? Isn't that the reason why the function is called
>>>>> `_with_encoder`?
>>>> The encoder parameter is skipped here because this function assumes that
>>>> wb_connector->encoder has already been initialized, setup with functions and
>>>> its possible_crts have already been set prior to calling this function like
>>>> the vc4 example shows. So this function doesnt need an explicit encoder
>>>> parameter. Let me know if thats a concern.
>>>>>
>>>>> I think there might have been too many version of these patchsets and things are
>>>>> starting to be confusing. Please revisit the series without rushing and come up with
>>>>> a plan of action. My understanding of watching this series has been that you're
>>>>> trying to come up with a function that does *connector* initialisation but skips the
>>>>> *encoder* initialisation because it might have been already done by the driver. The
>>>>> idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
>>>>> that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
>>>>> the encoder initialisation part. Then `drm_writeback_connector_init()` only
>>>>> initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
>>>>> There is no need to have the `drm_writeback_connector_setup()` function at all.
>>>>>
>>>>> Best regards,
>>>>> Liviu
>>>>>
>>>>
>>>> I agree there have been a 4 revisions prior to this because of the way this
>>>> affects the existing writeback drivers. So initial revision was a bit
>>>> intrusive into other drivers (which was just mostly a take over from the
>>>> previous patchset posted by the Co-developer ) and since rev3 we have
>>>> decided to have a separate API drm_writeback_connector_init_with_encoder()
>>>> so that existing clients are unaffected and it works seamlessly under the
>>>> hood.
>>>>
>>>> Only clients which already embed an encoder (vc4) and the new ones which
>>>> have special encoder requirements like the MSM driver for which I am
>>>> preparing these changes for will use the new API.
>>>>
>>>> Apologies for the revisions, but thanks to some great feedback from you and
>>>> laurent its shaping up nicely and reaching its conclusion I feel.
>>>
>>> I think it is only natural that there will be some iterations. If I remember
>>> correctly, the initial writeback series has something like 13 revisions :)
>>>
>>>>
>>>> So i think this revision is pretty close to being clean thanks to the
>>>> feedback you gave on rev4. Between rev4 and this one I didnt drastically
>>>> change the design other than re-ordering the changes to avoid the
>>>> intermediate patches having an incorrect state for the vc4 encoder. So all
>>>> your comments related to the encoder_cleanup() and vc4's encoder not getting
>>>> initialized would have gotten addressed but overall concept remained same.
>>>>
>>>> You are right, we are trying to come up with a function which does connector
>>>> initialization but skips the encoder part because that has already been done
>>>> in the client side of this API ( hence _with_encoder() name ). The
>>>> "_with_encoder" indicates that the caller already has an encoder for the
>>>> writeback connector which is being passed so there is no need to pass the
>>>> encoder again.
>>>>
>>>> I thought this addresses all the concerns posted in the previous series.
>>>>
>>>> So are you suggesting something like below?
>>>>
>>>> 1) rename drm_writeback_connector_setup() to
>>>> drm_writeback_connector_init_with_encoder()
>>>> (essentially thats what will end up happening )
>>>
>>> Correct. Without the pointless reordering of code it should be about 3 lines of code
>>> that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
>>>
>>
>> But isnt thats how it already looks.
>>
>> 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);
>>
>> So the only change you are requesting is that, instead of having a new
>> drm_writeback_connector_setup(), just call
>> drm_writeback_connector_init_with_encoder().
>>
>> It will essentially look like
>>
>> 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;
> 
> (1)
> 
>>
>>      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_init_with_encoder(dev, wb_connector,
>> con_funcs, formats,
>>              n_formats);
> 
> Yes, this is exactly what I had in mind.

Thank you for confirming.

> 
> 
>>
>> drm_writeback_connector_init_with_encoder() will still not have an encoder
>> parameter because of what I wrote previously.
> 
> But in your patch drm_writeback_connector_init_with_encoder() still has an
> encoder_funcs pointer which is useless, as the expectations are (AFAII) that the
> whole encoder init dance has already happened. And while you have a point that you
> can set the encoder pointer in the connector before calling
> drm_writeback_connector_init_with_encoder() I think it would be easier to read if you
> pass the encoder explicitly in the parameter list and skip the assignment in (1) and
> do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not
> wrong, I just think we should be explicit so that code is easier to read.

Sorry, but I am missing something here.

Where is the encoder_funcs pointer in 
drm_writeback_connector_init_with_encoder()? It only has 
drm_connector_funcs pointer. Because, like I mentioned in the earlier 
comment, the callers of this API would have already setup the encoder 
with the required functions/possible_crtcs etc.

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


So are you suggesting I pass the drm_encoder as a parameter to 
drm_writeback_connector_init_with_encoder as well?

That has two potential issues:

1) If we move just the assignment of wb_connector->encoder to inside 
drm_writeback_connector_init_with_encoder, we cannot do these before 
calling drm_writeback_connector_init_with_encoder() as 
wb_connector->encoder will be NULL for the cases which use 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);

2) We can only do these changes once wb_connector->encoder has been 
changed to a pointer which happens in patch 4 of this series
https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5

I was told to split the functionality of adding the new API in a 
separate patch by laurent.

To keep changes independent and functionally correct I thought this is 
the best split.

So the cleanup you are suggesting can be done in patch 4 of this series 
but still I am not sure how to get around concern (1) because the 
encoder has to be fully initialized before attaching to a wb_connector.


> 
>>
>> Hope this is what you had in mind as well.
>>
>>>>
>>>> 2) Inside drm_writeback_connector_init() check:
>>>>
>>>> drm_writeback_connector_init(.....)
>>>> {
>>>>      if (!wb_conn->encoder)
>>>> 	initialize the encoder
>>>
>>> No, the assumption of drm_writeback_connector_init() is that we will provide the
>>> encoder, so no need to check that one is already provided. What you could do is:
>>>
>>>        WARN_ON(wb_conn->encoder);
>>>
>>
>> Got it, i will add a warning inside drm_writeback_connector_init() to
>> emphasize that its only meant for cases where an encoder is not provided.
>>
>>> before we overwrite the encoder. That way we will get a nice warning in the kernel
>>> log if someone tries to call drm_writeback_connector_init() with a preset encoder.
>>>
>>>>
>>>>      call drm_writeback_**_init_with_encoder() 				
>>>> }
>>>>
>>>> This will also work but have the foll concerns/questions:
>>>>
>>>> 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
>>>> as per your suggestion or will all clients continue to use
>>>> drm_writeback_connector_init() only? We wanted to have a separate function
>>>> for new clients.
>>>
>>> Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
>>>
>> Alright, so vc4 and new vendors which provide their own encoder will use
>> this one. So no changes to the rest of the series.
>>
>>>>
>>>> 2) How will we detect that encoder needs to be initialized without it being
>>>> a pointer which happens in the later change. So ordering becomes an issue.
>>>
>>> I think the init problem is simple. You either call drm_writeback_connector_init()
>>> and the framework provides you with an encoder, or you call
>>> drm_writeback_connector_init_with_encoder() and the framework will use yours. The
>>> problems will show up on the cleanup and exit codes, where we need to be able to skip
>>> the cleanup if the encoder pointer is not the internal one. I think a simple
>>>
>>>       if (connector->encoder == &connector->internal_encoder)
>>>             do_encoder_cleanup_work_here()
>>>
>>> should work.
>>
>> Sorry, I am missing something here.
>>
>> Even in this current patch, the drm_encoder_cleanup() is done only inside
>> drm_writeback_connector_init() where internal_encoder is used.
>>
>> For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and
>> expect the caller to do it like vc4 does in the next patch.
>>
>> So why do we need such a condition?
> 
> You're right. I was thinking that at cleanup time we also need to do work with the
> right encoder, but that should accomplished by passing the right .destroy hook in the
> drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the
> initialisation correctly it should all work fine, please disregard my comments.
> 
> Best regards,
> Liviu
> 
> 
>>
>>>
>>>>
>>>> Thats why I thought this is the best way to address the comments and keep
>>>> the functionality intact with each change.
>>>>
>>>> Let me know if I have misunderstood some comment or idea.
>>>
>>> I hope that with these clarifications we are both on the same page.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>> +{
>>>>>> +	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 db6214f..0093bab 100644
>>>>>> --- a/include/drm/drm_writeback.h
>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>> @@ -152,6 +152,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
>>>>>>
>>>>>
>>>
> 

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

Gentle reminder ... Can you please help to clarify the last set of 
questions so that I can work on the next version?

Thanks

Abhinav

On 3/25/2022 9:31 AM, Abhinav Kumar wrote:
> Hi Liviu
> 
> On 3/25/2022 3:19 AM, Liviu Dudau wrote:
>> On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
>>> Hi Liviu
>>
>> Hello,
>>
>>>
>>> Thanks for the response.
>>>
>>> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
>>>> On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
>>>>> Hi Liviu
>>>>
>>>> Hello,
>>>>
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
>>>>>> On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> changes in v5:
>>>>>>>     - reorder this change to come before in the series
>>>>>>>       to avoid incorrect functionality in subsequent changes
>>>>>>>     - continue using struct drm_encoder instead of
>>>>>>>       struct drm_encoder * and switch it in next change
>>>>>>>
>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>
>>>>>> Hi Abhinav,
>>>>>>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/drm_writeback.c | 143 
>>>>>>> ++++++++++++++++++++++++++++------------
>>>>>>>     include/drm/drm_writeback.h     |   5 ++
>>>>>>>     2 files changed, 106 insertions(+), 42 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c 
>>>>>>> b/drivers/gpu/drm/drm_writeback.c
>>>>>>> index dc2ef12..abe78b9 100644
>>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>>> @@ -149,37 +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
>>>>>>> - * @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)
>>>>>>> +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;
>>>>>>> +
>>>>>>
>>>>>> Point of this reordering the statements is...?
>>>>> This diff can be avoided. There was no reason to reorder this. I 
>>>>> will remove
>>>>> this re-order.
>>>>>>
>>>>>>>         int ret = create_writeback_properties(dev);
>>>>>>>         if (ret != 0)
>>>>>>> @@ -187,18 +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);
>>>>>>> -
>>>>>>> -    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)
>>>>>>> -        goto fail;
>>>>>>> +    if (IS_ERR(blob)) {
>>>>>>> +        ret = PTR_ERR(blob);
>>>>>>> +        return ret;
>>>>>>> +    }
>>>>>>
>>>>>> I don't see why you have changed the earlier code to store the 
>>>>>> result of PTR_ERR into
>>>>>> ret other than to help your debugging. I suggest that you keep the 
>>>>>> existing code that
>>>>>> returns PTR_ERR(blob) directly and you will have a nicer diff stat 
>>>>>> as well.
>>>>> Sure, i can fix this for a smaller diff stat.
>>>>>>
>>>>>>>         connector->interlace_allowed = 0;
>>>>>>> @@ -237,13 +207,102 @@ 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
>>>>>>> + * @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;
>>>>>>> +
>>>>>>> +    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);
>>>>>>> +/**
>>>>>>> + * 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
>>>>>>
>>>>>> spelling: writeback
>>>>> Ack. will fix this.
>>>>>>
>>>>>>> + * 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.
>>>>>>
>>>>>> .... if they want to manage themselves the lifetime of the 
>>>>>> associated encoder.
>>>>>>
>>>>>> We're not trying to replace drm_writeback_connector_init() 
>>>>>> function here, only to
>>>>>> provide an alternative function to call for special cases.
>>>>>
>>>>> Yes, we are trying to provide an alternative function call for 
>>>>> special case
>>>>> where the encoder is a shared encoder and/or where the resources of 
>>>>> the
>>>>> writeback encoder are shared with other hardware blocks.
>>>>>
>>>>> I can add that comment to this function's doc.
>>>>>
>>>>>>
>>>>>>> + *
>>>>>>> + * 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)
>>>>>>
>>>>>> Where is the encoder parameter? Isn't that the reason why the 
>>>>>> function is called
>>>>>> `_with_encoder`?
>>>>> The encoder parameter is skipped here because this function assumes 
>>>>> that
>>>>> wb_connector->encoder has already been initialized, setup with 
>>>>> functions and
>>>>> its possible_crts have already been set prior to calling this 
>>>>> function like
>>>>> the vc4 example shows. So this function doesnt need an explicit 
>>>>> encoder
>>>>> parameter. Let me know if thats a concern.
>>>>>>
>>>>>> I think there might have been too many version of these patchsets 
>>>>>> and things are
>>>>>> starting to be confusing. Please revisit the series without 
>>>>>> rushing and come up with
>>>>>> a plan of action. My understanding of watching this series has 
>>>>>> been that you're
>>>>>> trying to come up with a function that does *connector* 
>>>>>> initialisation but skips the
>>>>>> *encoder* initialisation because it might have been already done 
>>>>>> by the driver. The
>>>>>> idea will be then to have a function 
>>>>>> `drm_writeback_connector_init_with_encoder()`
>>>>>> that does *all* the work that `drm_writeback_connector_init()` 
>>>>>> does at the moment minus
>>>>>> the encoder initialisation part. Then 
>>>>>> `drm_writeback_connector_init()` only
>>>>>> initialises the internal encoder and calls 
>>>>>> `drm_writeback_connector_init_with_encoder()`.
>>>>>> There is no need to have the `drm_writeback_connector_setup()` 
>>>>>> function at all.
>>>>>>
>>>>>> Best regards,
>>>>>> Liviu
>>>>>>
>>>>>
>>>>> I agree there have been a 4 revisions prior to this because of the 
>>>>> way this
>>>>> affects the existing writeback drivers. So initial revision was a bit
>>>>> intrusive into other drivers (which was just mostly a take over 
>>>>> from the
>>>>> previous patchset posted by the Co-developer ) and since rev3 we have
>>>>> decided to have a separate API 
>>>>> drm_writeback_connector_init_with_encoder()
>>>>> so that existing clients are unaffected and it works seamlessly 
>>>>> under the
>>>>> hood.
>>>>>
>>>>> Only clients which already embed an encoder (vc4) and the new ones 
>>>>> which
>>>>> have special encoder requirements like the MSM driver for which I am
>>>>> preparing these changes for will use the new API.
>>>>>
>>>>> Apologies for the revisions, but thanks to some great feedback from 
>>>>> you and
>>>>> laurent its shaping up nicely and reaching its conclusion I feel.
>>>>
>>>> I think it is only natural that there will be some iterations. If I 
>>>> remember
>>>> correctly, the initial writeback series has something like 13 
>>>> revisions :)
>>>>
>>>>>
>>>>> So i think this revision is pretty close to being clean thanks to the
>>>>> feedback you gave on rev4. Between rev4 and this one I didnt 
>>>>> drastically
>>>>> change the design other than re-ordering the changes to avoid the
>>>>> intermediate patches having an incorrect state for the vc4 encoder. 
>>>>> So all
>>>>> your comments related to the encoder_cleanup() and vc4's encoder 
>>>>> not getting
>>>>> initialized would have gotten addressed but overall concept 
>>>>> remained same.
>>>>>
>>>>> You are right, we are trying to come up with a function which does 
>>>>> connector
>>>>> initialization but skips the encoder part because that has already 
>>>>> been done
>>>>> in the client side of this API ( hence _with_encoder() name ). The
>>>>> "_with_encoder" indicates that the caller already has an encoder 
>>>>> for the
>>>>> writeback connector which is being passed so there is no need to 
>>>>> pass the
>>>>> encoder again.
>>>>>
>>>>> I thought this addresses all the concerns posted in the previous 
>>>>> series.
>>>>>
>>>>> So are you suggesting something like below?
>>>>>
>>>>> 1) rename drm_writeback_connector_setup() to
>>>>> drm_writeback_connector_init_with_encoder()
>>>>> (essentially thats what will end up happening )
>>>>
>>>> Correct. Without the pointless reordering of code it should be about 
>>>> 3 lines of code
>>>> that get removed (the calls to drm_encoder_helper_add() and 
>>>> drm_encoder_init()).
>>>>
>>>
>>> But isnt thats how it already looks.
>>>
>>> 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);
>>>
>>> So the only change you are requesting is that, instead of having a new
>>> drm_writeback_connector_setup(), just call
>>> drm_writeback_connector_init_with_encoder().
>>>
>>> It will essentially look like
>>>
>>> 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;
>>
>> (1)
>>
>>>
>>>      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_init_with_encoder(dev, wb_connector,
>>> con_funcs, formats,
>>>              n_formats);
>>
>> Yes, this is exactly what I had in mind.
> 
> Thank you for confirming.
> 
>>
>>
>>>
>>> drm_writeback_connector_init_with_encoder() will still not have an 
>>> encoder
>>> parameter because of what I wrote previously.
>>
>> But in your patch drm_writeback_connector_init_with_encoder() still 
>> has an
>> encoder_funcs pointer which is useless, as the expectations are 
>> (AFAII) that the
>> whole encoder init dance has already happened. And while you have a 
>> point that you
>> can set the encoder pointer in the connector before calling
>> drm_writeback_connector_init_with_encoder() I think it would be easier 
>> to read if you
>> pass the encoder explicitly in the parameter list and skip the 
>> assignment in (1) and
>> do it inside drm_writeback_connector_init_with_encoder(). Again, your 
>> code is not
>> wrong, I just think we should be explicit so that code is easier to read.
> 
> Sorry, but I am missing something here.
> 
> Where is the encoder_funcs pointer in 
> drm_writeback_connector_init_with_encoder()? It only has 
> drm_connector_funcs pointer. Because, like I mentioned in the earlier 
> comment, the callers of this API would have already setup the encoder 
> with the required functions/possible_crtcs etc.
> 
> 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;
> }
> 
> 
> So are you suggesting I pass the drm_encoder as a parameter to 
> drm_writeback_connector_init_with_encoder as well?
> 
> That has two potential issues:
> 
> 1) If we move just the assignment of wb_connector->encoder to inside 
> drm_writeback_connector_init_with_encoder, we cannot do these before 
> calling drm_writeback_connector_init_with_encoder() as 
> wb_connector->encoder will be NULL for the cases which use 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);
> 
> 2) We can only do these changes once wb_connector->encoder has been 
> changed to a pointer which happens in patch 4 of this series
> https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5
> 
> I was told to split the functionality of adding the new API in a 
> separate patch by laurent.
> 
> To keep changes independent and functionally correct I thought this is 
> the best split.
> 
> So the cleanup you are suggesting can be done in patch 4 of this series 
> but still I am not sure how to get around concern (1) because the 
> encoder has to be fully initialized before attaching to a wb_connector.
> 
> 
>>
>>>
>>> Hope this is what you had in mind as well.
>>>
>>>>>
>>>>> 2) Inside drm_writeback_connector_init() check:
>>>>>
>>>>> drm_writeback_connector_init(.....)
>>>>> {
>>>>>      if (!wb_conn->encoder)
>>>>>     initialize the encoder
>>>>
>>>> No, the assumption of drm_writeback_connector_init() is that we will 
>>>> provide the
>>>> encoder, so no need to check that one is already provided. What you 
>>>> could do is:
>>>>
>>>>        WARN_ON(wb_conn->encoder);
>>>>
>>>
>>> Got it, i will add a warning inside drm_writeback_connector_init() to
>>> emphasize that its only meant for cases where an encoder is not 
>>> provided.
>>>
>>>> before we overwrite the encoder. That way we will get a nice warning 
>>>> in the kernel
>>>> log if someone tries to call drm_writeback_connector_init() with a 
>>>> preset encoder.
>>>>
>>>>>
>>>>>      call drm_writeback_**_init_with_encoder()
>>>>> }
>>>>>
>>>>> This will also work but have the foll concerns/questions:
>>>>>
>>>>> 1) Will drm_writeback_connector_init_with_encoder() be exported if 
>>>>> we change
>>>>> as per your suggestion or will all clients continue to use
>>>>> drm_writeback_connector_init() only? We wanted to have a separate 
>>>>> function
>>>>> for new clients.
>>>>
>>>> Yes, we will need to export 
>>>> drm_writeback_connector_init_with_encoder() as well.
>>>>
>>> Alright, so vc4 and new vendors which provide their own encoder will use
>>> this one. So no changes to the rest of the series.
>>>
>>>>>
>>>>> 2) How will we detect that encoder needs to be initialized without 
>>>>> it being
>>>>> a pointer which happens in the later change. So ordering becomes an 
>>>>> issue.
>>>>
>>>> I think the init problem is simple. You either call 
>>>> drm_writeback_connector_init()
>>>> and the framework provides you with an encoder, or you call
>>>> drm_writeback_connector_init_with_encoder() and the framework will 
>>>> use yours. The
>>>> problems will show up on the cleanup and exit codes, where we need 
>>>> to be able to skip
>>>> the cleanup if the encoder pointer is not the internal one. I think 
>>>> a simple
>>>>
>>>>       if (connector->encoder == &connector->internal_encoder)
>>>>             do_encoder_cleanup_work_here()
>>>>
>>>> should work.
>>>
>>> Sorry, I am missing something here.
>>>
>>> Even in this current patch, the drm_encoder_cleanup() is done only 
>>> inside
>>> drm_writeback_connector_init() where internal_encoder is used.
>>>
>>> For drm_writeback_connector_init_with_encoder(), we dont do the 
>>> cleanup and
>>> expect the caller to do it like vc4 does in the next patch.
>>>
>>> So why do we need such a condition?
>>
>> You're right. I was thinking that at cleanup time we also need to do 
>> work with the
>> right encoder, but that should accomplished by passing the right 
>> .destroy hook in the
>> drm_encoder_funcs pointer via drm_encoder_init. So if the special 
>> drivers to the
>> initialisation correctly it should all work fine, please disregard my 
>> comments.
>>
>> Best regards,
>> Liviu
>>
>>
>>>
>>>>
>>>>>
>>>>> Thats why I thought this is the best way to address the comments 
>>>>> and keep
>>>>> the functionality intact with each change.
>>>>>
>>>>> Let me know if I have misunderstood some comment or idea.
>>>>
>>>> I hope that with these clarifications we are both on the same page.
>>>>
>>>> Best regards,
>>>> Liviu
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> +{
>>>>>>> +    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 db6214f..0093bab 100644
>>>>>>> --- a/include/drm/drm_writeback.h
>>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>>> @@ -152,6 +152,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
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Liviu Dudau]

On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hi Abhinav,

Sorry for the delay, got busy with other things at the beginning of the week.

> 
> On 3/25/2022 3:19 AM, Liviu Dudau wrote:
> > On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
> > > Hi Liviu
> > 
> > Hello,
> > 
> > > 
> > > Thanks for the response.
> > > 
> > > On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> > > > On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> > > > > Hi Liviu
> > > > 
> > > > Hello,
> > > > 
> > > > > 
> > > > > Thanks for the review.
> > > > > 
> > > > > On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > > > > > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > > > > > 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.
> > > > > > > 
> > > > > > > changes in v5:
> > > > > > > 	- reorder this change to come before in the series
> > > > > > > 	  to avoid incorrect functionality in subsequent changes
> > > > > > > 	- continue using struct drm_encoder instead of
> > > > > > > 	  struct drm_encoder * and switch it in next change
> > > > > > > 
> > > > > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > > > > 
> > > > > > Hi Abhinav,
> > > > > > 
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
> > > > > > >     include/drm/drm_writeback.h     |   5 ++
> > > > > > >     2 files changed, 106 insertions(+), 42 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > > > > > index dc2ef12..abe78b9 100644
> > > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > > @@ -149,37 +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
> > > > > > > - * @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)
> > > > > > > +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;
> > > > > > > +
> > > > > > 
> > > > > > Point of this reordering the statements is...?
> > > > > This diff can be avoided. There was no reason to reorder this. I will remove
> > > > > this re-order.
> > > > > > 
> > > > > > >     	int ret = create_writeback_properties(dev);
> > > > > > >     	if (ret != 0)
> > > > > > > @@ -187,18 +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);
> > > > > > > -
> > > > > > > -	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)
> > > > > > > -		goto fail;
> > > > > > > +	if (IS_ERR(blob)) {
> > > > > > > +		ret = PTR_ERR(blob);
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > 
> > > > > > I don't see why you have changed the earlier code to store the result of PTR_ERR into
> > > > > > ret other than to help your debugging. I suggest that you keep the existing code that
> > > > > > returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
> > > > > Sure, i can fix this for a smaller diff stat.
> > > > > > 
> > > > > > >     	connector->interlace_allowed = 0;
> > > > > > > @@ -237,13 +207,102 @@ 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
> > > > > > > + * @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;
> > > > > > > +
> > > > > > > +	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);
> > > > > > > +/**
> > > > > > > + * 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

This is the encoder helper funcs that I was talking about. Maybe it is better if you
send a new, cleaner version and we can continue the conversation/review there?

Best regards,
Liviu


> > > > > > > + * @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
> > > > > > 
> > > > > > spelling: writeback
> > > > > Ack. will fix this.
> > > > > > 
> > > > > > > + * 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.
> > > > > > 
> > > > > > .... if they want to manage themselves the lifetime of the associated encoder.
> > > > > > 
> > > > > > We're not trying to replace drm_writeback_connector_init() function here, only to
> > > > > > provide an alternative function to call for special cases.
> > > > > 
> > > > > Yes, we are trying to provide an alternative function call for special case
> > > > > where the encoder is a shared encoder and/or where the resources of the
> > > > > writeback encoder are shared with other hardware blocks.
> > > > > 
> > > > > I can add that comment to this function's doc.
> > > > > 
> > > > > > 
> > > > > > > + *
> > > > > > > + * 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)
> > > > > > 
> > > > > > Where is the encoder parameter? Isn't that the reason why the function is called
> > > > > > `_with_encoder`?
> > > > > The encoder parameter is skipped here because this function assumes that
> > > > > wb_connector->encoder has already been initialized, setup with functions and
> > > > > its possible_crts have already been set prior to calling this function like
> > > > > the vc4 example shows. So this function doesnt need an explicit encoder
> > > > > parameter. Let me know if thats a concern.
> > > > > > 
> > > > > > I think there might have been too many version of these patchsets and things are
> > > > > > starting to be confusing. Please revisit the series without rushing and come up with
> > > > > > a plan of action. My understanding of watching this series has been that you're
> > > > > > trying to come up with a function that does *connector* initialisation but skips the
> > > > > > *encoder* initialisation because it might have been already done by the driver. The
> > > > > > idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
> > > > > > that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
> > > > > > the encoder initialisation part. Then `drm_writeback_connector_init()` only
> > > > > > initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
> > > > > > There is no need to have the `drm_writeback_connector_setup()` function at all.
> > > > > > 
> > > > > > Best regards,
> > > > > > Liviu
> > > > > > 
> > > > > 
> > > > > I agree there have been a 4 revisions prior to this because of the way this
> > > > > affects the existing writeback drivers. So initial revision was a bit
> > > > > intrusive into other drivers (which was just mostly a take over from the
> > > > > previous patchset posted by the Co-developer ) and since rev3 we have
> > > > > decided to have a separate API drm_writeback_connector_init_with_encoder()
> > > > > so that existing clients are unaffected and it works seamlessly under the
> > > > > hood.
> > > > > 
> > > > > Only clients which already embed an encoder (vc4) and the new ones which
> > > > > have special encoder requirements like the MSM driver for which I am
> > > > > preparing these changes for will use the new API.
> > > > > 
> > > > > Apologies for the revisions, but thanks to some great feedback from you and
> > > > > laurent its shaping up nicely and reaching its conclusion I feel.
> > > > 
> > > > I think it is only natural that there will be some iterations. If I remember
> > > > correctly, the initial writeback series has something like 13 revisions :)
> > > > 
> > > > > 
> > > > > So i think this revision is pretty close to being clean thanks to the
> > > > > feedback you gave on rev4. Between rev4 and this one I didnt drastically
> > > > > change the design other than re-ordering the changes to avoid the
> > > > > intermediate patches having an incorrect state for the vc4 encoder. So all
> > > > > your comments related to the encoder_cleanup() and vc4's encoder not getting
> > > > > initialized would have gotten addressed but overall concept remained same.
> > > > > 
> > > > > You are right, we are trying to come up with a function which does connector
> > > > > initialization but skips the encoder part because that has already been done
> > > > > in the client side of this API ( hence _with_encoder() name ). The
> > > > > "_with_encoder" indicates that the caller already has an encoder for the
> > > > > writeback connector which is being passed so there is no need to pass the
> > > > > encoder again.
> > > > > 
> > > > > I thought this addresses all the concerns posted in the previous series.
> > > > > 
> > > > > So are you suggesting something like below?
> > > > > 
> > > > > 1) rename drm_writeback_connector_setup() to
> > > > > drm_writeback_connector_init_with_encoder()
> > > > > (essentially thats what will end up happening )
> > > > 
> > > > Correct. Without the pointless reordering of code it should be about 3 lines of code
> > > > that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
> > > > 
> > > 
> > > But isnt thats how it already looks.
> > > 
> > > 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);
> > > 
> > > So the only change you are requesting is that, instead of having a new
> > > drm_writeback_connector_setup(), just call
> > > drm_writeback_connector_init_with_encoder().
> > > 
> > > It will essentially look like
> > > 
> > > 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;
> > 
> > (1)
> > 
> > > 
> > >      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_init_with_encoder(dev, wb_connector,
> > > con_funcs, formats,
> > >              n_formats);
> > 
> > Yes, this is exactly what I had in mind.
> 
> Thank you for confirming.
> 
> > 
> > 
> > > 
> > > drm_writeback_connector_init_with_encoder() will still not have an encoder
> > > parameter because of what I wrote previously.
> > 
> > But in your patch drm_writeback_connector_init_with_encoder() still has an
> > encoder_funcs pointer which is useless, as the expectations are (AFAII) that the
> > whole encoder init dance has already happened. And while you have a point that you
> > can set the encoder pointer in the connector before calling
> > drm_writeback_connector_init_with_encoder() I think it would be easier to read if you
> > pass the encoder explicitly in the parameter list and skip the assignment in (1) and
> > do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not
> > wrong, I just think we should be explicit so that code is easier to read.
> 
> Sorry, but I am missing something here.
> 
> Where is the encoder_funcs pointer in
> drm_writeback_connector_init_with_encoder()? It only has drm_connector_funcs
> pointer. Because, like I mentioned in the earlier comment, the callers of
> this API would have already setup the encoder with the required
> functions/possible_crtcs etc.
> 
> 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;
> }
> 
> 
> So are you suggesting I pass the drm_encoder as a parameter to
> drm_writeback_connector_init_with_encoder as well?
> 
> That has two potential issues:
> 
> 1) If we move just the assignment of wb_connector->encoder to inside
> drm_writeback_connector_init_with_encoder, we cannot do these before calling
> drm_writeback_connector_init_with_encoder() as wb_connector->encoder will be
> NULL for the cases which use 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);

Where can you not do this? Inside drm_writeback_connector_init_with_encoder() you
don't need to do that, as the function is intended to initialise a writeback
connector using a provided encoder that has been already initialised. Inside
drm_writeback_connector_init() you will call these functions using the internal
encoder pointer that you have added in the series.

> 
> 2) We can only do these changes once wb_connector->encoder has been changed
> to a pointer which happens in patch 4 of this series
> https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5
> 
> I was told to split the functionality of adding the new API in a separate
> patch by laurent.
> 
> To keep changes independent and functionally correct I thought this is the
> best split.
> 
> So the cleanup you are suggesting can be done in patch 4 of this series but
> still I am not sure how to get around concern (1) because the encoder has to
> be fully initialized before attaching to a wb_connector.

I think if you keep the vc4 patch in patch 4 and move the rest in here it will make
more sense. Again, I think a new series cleaned up might help others to review as
well (specially Laurent).

Best regards,
Liviu


> 
> 
> > 
> > > 
> > > Hope this is what you had in mind as well.
> > > 
> > > > > 
> > > > > 2) Inside drm_writeback_connector_init() check:
> > > > > 
> > > > > drm_writeback_connector_init(.....)
> > > > > {
> > > > >      if (!wb_conn->encoder)
> > > > > 	initialize the encoder
> > > > 
> > > > No, the assumption of drm_writeback_connector_init() is that we will provide the
> > > > encoder, so no need to check that one is already provided. What you could do is:
> > > > 
> > > >        WARN_ON(wb_conn->encoder);
> > > > 
> > > 
> > > Got it, i will add a warning inside drm_writeback_connector_init() to
> > > emphasize that its only meant for cases where an encoder is not provided.
> > > 
> > > > before we overwrite the encoder. That way we will get a nice warning in the kernel
> > > > log if someone tries to call drm_writeback_connector_init() with a preset encoder.
> > > > 
> > > > > 
> > > > >      call drm_writeback_**_init_with_encoder() 				
> > > > > }
> > > > > 
> > > > > This will also work but have the foll concerns/questions:
> > > > > 
> > > > > 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
> > > > > as per your suggestion or will all clients continue to use
> > > > > drm_writeback_connector_init() only? We wanted to have a separate function
> > > > > for new clients.
> > > > 
> > > > Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
> > > > 
> > > Alright, so vc4 and new vendors which provide their own encoder will use
> > > this one. So no changes to the rest of the series.
> > > 
> > > > > 
> > > > > 2) How will we detect that encoder needs to be initialized without it being
> > > > > a pointer which happens in the later change. So ordering becomes an issue.
> > > > 
> > > > I think the init problem is simple. You either call drm_writeback_connector_init()
> > > > and the framework provides you with an encoder, or you call
> > > > drm_writeback_connector_init_with_encoder() and the framework will use yours. The
> > > > problems will show up on the cleanup and exit codes, where we need to be able to skip
> > > > the cleanup if the encoder pointer is not the internal one. I think a simple
> > > > 
> > > >       if (connector->encoder == &connector->internal_encoder)
> > > >             do_encoder_cleanup_work_here()
> > > > 
> > > > should work.
> > > 
> > > Sorry, I am missing something here.
> > > 
> > > Even in this current patch, the drm_encoder_cleanup() is done only inside
> > > drm_writeback_connector_init() where internal_encoder is used.
> > > 
> > > For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and
> > > expect the caller to do it like vc4 does in the next patch.
> > > 
> > > So why do we need such a condition?
> > 
> > You're right. I was thinking that at cleanup time we also need to do work with the
> > right encoder, but that should accomplished by passing the right .destroy hook in the
> > drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the
> > initialisation correctly it should all work fine, please disregard my comments.
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Thats why I thought this is the best way to address the comments and keep
> > > > > the functionality intact with each change.
> > > > > 
> > > > > Let me know if I have misunderstood some comment or idea.
> > > > 
> > > > I hope that with these clarifications we are both on the same page.
> > > > 
> > > > Best regards,
> > > > Liviu
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > +{
> > > > > > > +	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 db6214f..0093bab 100644
> > > > > > > --- a/include/drm/drm_writeback.h
> > > > > > > +++ b/include/drm/drm_writeback.h
> > > > > > > @@ -152,6 +152,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
> > > > > > > 
> > > > > > 
> > > > 
> > 

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

Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

Thanks for the response.

Let me fix up and spin a new version.

Abhinav


On 3/31/2022 3:01 AM, Liviu Dudau wrote:
> On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:
>> Hi Liviu
> 
> Hi Abhinav,
> 
> Sorry for the delay, got busy with other things at the beginning of the week.
> 
>>
>> On 3/25/2022 3:19 AM, Liviu Dudau wrote:
>>> On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
>>>> Hi Liviu
>>>
>>> Hello,
>>>
>>>>
>>>> Thanks for the response.
>>>>
>>>> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
>>>>> On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
>>>>>> Hi Liviu
>>>>>
>>>>> Hello,
>>>>>
>>>>>>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
>>>>>>> On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> changes in v5:
>>>>>>>> 	- reorder this change to come before in the series
>>>>>>>> 	  to avoid incorrect functionality in subsequent changes
>>>>>>>> 	- continue using struct drm_encoder instead of
>>>>>>>> 	  struct drm_encoder * and switch it in next change
>>>>>>>>
>>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>>
>>>>>>> Hi Abhinav,
>>>>>>>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>>>>>>>>      include/drm/drm_writeback.h     |   5 ++
>>>>>>>>      2 files changed, 106 insertions(+), 42 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>>>>>> index dc2ef12..abe78b9 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>>>> @@ -149,37 +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
>>>>>>>> - * @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)
>>>>>>>> +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;
>>>>>>>> +
>>>>>>>
>>>>>>> Point of this reordering the statements is...?
>>>>>> This diff can be avoided. There was no reason to reorder this. I will remove
>>>>>> this re-order.
>>>>>>>
>>>>>>>>      	int ret = create_writeback_properties(dev);
>>>>>>>>      	if (ret != 0)
>>>>>>>> @@ -187,18 +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);
>>>>>>>> -
>>>>>>>> -	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)
>>>>>>>> -		goto fail;
>>>>>>>> +	if (IS_ERR(blob)) {
>>>>>>>> +		ret = PTR_ERR(blob);
>>>>>>>> +		return ret;
>>>>>>>> +	}
>>>>>>>
>>>>>>> I don't see why you have changed the earlier code to store the result of PTR_ERR into
>>>>>>> ret other than to help your debugging. I suggest that you keep the existing code that
>>>>>>> returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
>>>>>> Sure, i can fix this for a smaller diff stat.
>>>>>>>
>>>>>>>>      	connector->interlace_allowed = 0;
>>>>>>>> @@ -237,13 +207,102 @@ 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
>>>>>>>> + * @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;
>>>>>>>> +
>>>>>>>> +	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);
>>>>>>>> +/**
>>>>>>>> + * 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
> 
> This is the encoder helper funcs that I was talking about. Maybe it is better if you
> send a new, cleaner version and we can continue the conversation/review there?
> 
> Best regards,
> Liviu
The doc is wrong here, I will fix this and spin a new version.
> 
> 
>>>>>>>> + * @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
>>>>>>>
>>>>>>> spelling: writeback
>>>>>> Ack. will fix this.
>>>>>>>
>>>>>>>> + * 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.
>>>>>>>
>>>>>>> .... if they want to manage themselves the lifetime of the associated encoder.
>>>>>>>
>>>>>>> We're not trying to replace drm_writeback_connector_init() function here, only to
>>>>>>> provide an alternative function to call for special cases.
>>>>>>
>>>>>> Yes, we are trying to provide an alternative function call for special case
>>>>>> where the encoder is a shared encoder and/or where the resources of the
>>>>>> writeback encoder are shared with other hardware blocks.
>>>>>>
>>>>>> I can add that comment to this function's doc.
>>>>>>
>>>>>>>
>>>>>>>> + *
>>>>>>>> + * 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)
>>>>>>>
>>>>>>> Where is the encoder parameter? Isn't that the reason why the function is called
>>>>>>> `_with_encoder`?
>>>>>> The encoder parameter is skipped here because this function assumes that
>>>>>> wb_connector->encoder has already been initialized, setup with functions and
>>>>>> its possible_crts have already been set prior to calling this function like
>>>>>> the vc4 example shows. So this function doesnt need an explicit encoder
>>>>>> parameter. Let me know if thats a concern.
>>>>>>>
>>>>>>> I think there might have been too many version of these patchsets and things are
>>>>>>> starting to be confusing. Please revisit the series without rushing and come up with
>>>>>>> a plan of action. My understanding of watching this series has been that you're
>>>>>>> trying to come up with a function that does *connector* initialisation but skips the
>>>>>>> *encoder* initialisation because it might have been already done by the driver. The
>>>>>>> idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
>>>>>>> that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
>>>>>>> the encoder initialisation part. Then `drm_writeback_connector_init()` only
>>>>>>> initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
>>>>>>> There is no need to have the `drm_writeback_connector_setup()` function at all.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Liviu
>>>>>>>
>>>>>>
>>>>>> I agree there have been a 4 revisions prior to this because of the way this
>>>>>> affects the existing writeback drivers. So initial revision was a bit
>>>>>> intrusive into other drivers (which was just mostly a take over from the
>>>>>> previous patchset posted by the Co-developer ) and since rev3 we have
>>>>>> decided to have a separate API drm_writeback_connector_init_with_encoder()
>>>>>> so that existing clients are unaffected and it works seamlessly under the
>>>>>> hood.
>>>>>>
>>>>>> Only clients which already embed an encoder (vc4) and the new ones which
>>>>>> have special encoder requirements like the MSM driver for which I am
>>>>>> preparing these changes for will use the new API.
>>>>>>
>>>>>> Apologies for the revisions, but thanks to some great feedback from you and
>>>>>> laurent its shaping up nicely and reaching its conclusion I feel.
>>>>>
>>>>> I think it is only natural that there will be some iterations. If I remember
>>>>> correctly, the initial writeback series has something like 13 revisions :)
>>>>>
>>>>>>
>>>>>> So i think this revision is pretty close to being clean thanks to the
>>>>>> feedback you gave on rev4. Between rev4 and this one I didnt drastically
>>>>>> change the design other than re-ordering the changes to avoid the
>>>>>> intermediate patches having an incorrect state for the vc4 encoder. So all
>>>>>> your comments related to the encoder_cleanup() and vc4's encoder not getting
>>>>>> initialized would have gotten addressed but overall concept remained same.
>>>>>>
>>>>>> You are right, we are trying to come up with a function which does connector
>>>>>> initialization but skips the encoder part because that has already been done
>>>>>> in the client side of this API ( hence _with_encoder() name ). The
>>>>>> "_with_encoder" indicates that the caller already has an encoder for the
>>>>>> writeback connector which is being passed so there is no need to pass the
>>>>>> encoder again.
>>>>>>
>>>>>> I thought this addresses all the concerns posted in the previous series.
>>>>>>
>>>>>> So are you suggesting something like below?
>>>>>>
>>>>>> 1) rename drm_writeback_connector_setup() to
>>>>>> drm_writeback_connector_init_with_encoder()
>>>>>> (essentially thats what will end up happening )
>>>>>
>>>>> Correct. Without the pointless reordering of code it should be about 3 lines of code
>>>>> that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
>>>>>
>>>>
>>>> But isnt thats how it already looks.
>>>>
>>>> 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);
>>>>
>>>> So the only change you are requesting is that, instead of having a new
>>>> drm_writeback_connector_setup(), just call
>>>> drm_writeback_connector_init_with_encoder().
>>>>
>>>> It will essentially look like
>>>>
>>>> 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;
>>>
>>> (1)
>>>
>>>>
>>>>       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_init_with_encoder(dev, wb_connector,
>>>> con_funcs, formats,
>>>>               n_formats);
>>>
>>> Yes, this is exactly what I had in mind.
>>
>> Thank you for confirming.
>>
>>>
>>>
>>>>
>>>> drm_writeback_connector_init_with_encoder() will still not have an encoder
>>>> parameter because of what I wrote previously.
>>>
>>> But in your patch drm_writeback_connector_init_with_encoder() still has an
>>> encoder_funcs pointer which is useless, as the expectations are (AFAII) that the
>>> whole encoder init dance has already happened. And while you have a point that you
>>> can set the encoder pointer in the connector before calling
>>> drm_writeback_connector_init_with_encoder() I think it would be easier to read if you
>>> pass the encoder explicitly in the parameter list and skip the assignment in (1) and
>>> do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not
>>> wrong, I just think we should be explicit so that code is easier to read.
>>
>> Sorry, but I am missing something here.
>>
>> Where is the encoder_funcs pointer in
>> drm_writeback_connector_init_with_encoder()? It only has drm_connector_funcs
>> pointer. Because, like I mentioned in the earlier comment, the callers of
>> this API would have already setup the encoder with the required
>> functions/possible_crtcs etc.
>>
>> 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;
>> }
>>
>>
>> So are you suggesting I pass the drm_encoder as a parameter to
>> drm_writeback_connector_init_with_encoder as well?
>>
>> That has two potential issues:
>>
>> 1) If we move just the assignment of wb_connector->encoder to inside
>> drm_writeback_connector_init_with_encoder, we cannot do these before calling
>> drm_writeback_connector_init_with_encoder() as wb_connector->encoder will be
>> NULL for the cases which use 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);
> 
> Where can you not do this? Inside drm_writeback_connector_init_with_encoder() you
> don't need to do that, as the function is intended to initialise a writeback
> connector using a provided encoder that has been already initialised. Inside
> drm_writeback_connector_init() you will call these functions using the internal
> encoder pointer that you have added in the series.
> 
>>
>> 2) We can only do these changes once wb_connector->encoder has been changed
>> to a pointer which happens in patch 4 of this series
>> https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5
>>
>> I was told to split the functionality of adding the new API in a separate
>> patch by laurent.
>>
>> To keep changes independent and functionally correct I thought this is the
>> best split.
>>
>> So the cleanup you are suggesting can be done in patch 4 of this series but
>> still I am not sure how to get around concern (1) because the encoder has to
>> be fully initialized before attaching to a wb_connector.
> 
> I think if you keep the vc4 patch in patch 4 and move the rest in here it will make
> more sense. Again, I think a new series cleaned up might help others to review as
> well (specially Laurent).
> 
> Best regards,
> Liviu
> 
> 
>>
>>
>>>
>>>>
>>>> Hope this is what you had in mind as well.
>>>>
>>>>>>
>>>>>> 2) Inside drm_writeback_connector_init() check:
>>>>>>
>>>>>> drm_writeback_connector_init(.....)
>>>>>> {
>>>>>>       if (!wb_conn->encoder)
>>>>>> 	initialize the encoder
>>>>>
>>>>> No, the assumption of drm_writeback_connector_init() is that we will provide the
>>>>> encoder, so no need to check that one is already provided. What you could do is:
>>>>>
>>>>>         WARN_ON(wb_conn->encoder);
>>>>>
>>>>
>>>> Got it, i will add a warning inside drm_writeback_connector_init() to
>>>> emphasize that its only meant for cases where an encoder is not provided.
>>>>
>>>>> before we overwrite the encoder. That way we will get a nice warning in the kernel
>>>>> log if someone tries to call drm_writeback_connector_init() with a preset encoder.
>>>>>
>>>>>>
>>>>>>       call drm_writeback_**_init_with_encoder() 				
>>>>>> }
>>>>>>
>>>>>> This will also work but have the foll concerns/questions:
>>>>>>
>>>>>> 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
>>>>>> as per your suggestion or will all clients continue to use
>>>>>> drm_writeback_connector_init() only? We wanted to have a separate function
>>>>>> for new clients.
>>>>>
>>>>> Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
>>>>>
>>>> Alright, so vc4 and new vendors which provide their own encoder will use
>>>> this one. So no changes to the rest of the series.
>>>>
>>>>>>
>>>>>> 2) How will we detect that encoder needs to be initialized without it being
>>>>>> a pointer which happens in the later change. So ordering becomes an issue.
>>>>>
>>>>> I think the init problem is simple. You either call drm_writeback_connector_init()
>>>>> and the framework provides you with an encoder, or you call
>>>>> drm_writeback_connector_init_with_encoder() and the framework will use yours. The
>>>>> problems will show up on the cleanup and exit codes, where we need to be able to skip
>>>>> the cleanup if the encoder pointer is not the internal one. I think a simple
>>>>>
>>>>>        if (connector->encoder == &connector->internal_encoder)
>>>>>              do_encoder_cleanup_work_here()
>>>>>
>>>>> should work.
>>>>
>>>> Sorry, I am missing something here.
>>>>
>>>> Even in this current patch, the drm_encoder_cleanup() is done only inside
>>>> drm_writeback_connector_init() where internal_encoder is used.
>>>>
>>>> For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and
>>>> expect the caller to do it like vc4 does in the next patch.
>>>>
>>>> So why do we need such a condition?
>>>
>>> You're right. I was thinking that at cleanup time we also need to do work with the
>>> right encoder, but that should accomplished by passing the right .destroy hook in the
>>> drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the
>>> initialisation correctly it should all work fine, please disregard my comments.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Thats why I thought this is the best way to address the comments and keep
>>>>>> the functionality intact with each change.
>>>>>>
>>>>>> Let me know if I have misunderstood some comment or idea.
>>>>>
>>>>> I hope that with these clarifications we are both on the same page.
>>>>>
>>>>> Best regards,
>>>>> Liviu
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> +	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 db6214f..0093bab 100644
>>>>>>>> --- a/include/drm/drm_writeback.h
>>>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>>>> @@ -152,6 +152,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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>