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

[PATCH v6 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 v6:
	- remove drm_writeback_connector_setup() and instead
      directly call drm_writeback_connector_init_with_encoder()
	- pass encoder as a parameter explicitly to the new API
      for better readability  


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                    | 78 ++++++++++++++++++----
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c        |  4 +-
 drivers/gpu/drm/vc4/vc4_txp.c                      | 35 +++++++---
 drivers/gpu/drm/vkms/vkms_writeback.c              |  4 +-
 include/drm/drm_writeback.h                        | 29 +++++++-
 7 files changed, 126 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH v6 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 v6:
     - 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 v6 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 v6:
	- remove drm_writeback_connector_setup() and instead
	  directly call drm_writeback_connector_init_with_encoder()
	- fix a drm_writeback_connector typo and function doc which
	  incorrectly shows that the function accepts enc_helper_funcs
	- pass encoder as a parameter explicitly to the new API
	  for better readability

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

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..797223c 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -177,6 +177,62 @@ 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 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_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
+			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
+ * @enc: handle to the already initialized drm encoder
+ * @con_funcs: Connector funcs vtable
+ * @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_writeback_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 if they want to manage themselves the lifetime of the
+ * associated encoder.
+ *
+ * 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, struct drm_encoder *enc,
+		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;
@@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
 	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;
 
 	connector->interlace_allowed = 0;
 
@@ -207,8 +254,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 	if (ret)
 		goto connector_fail;
 
-	ret = drm_connector_attach_encoder(connector,
-						&wb_connector->encoder);
+	ret = drm_connector_attach_encoder(connector, enc);
 	if (ret)
 		goto attach_fail;
 
@@ -237,12 +283,10 @@ 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;
 }
-EXPORT_SYMBOL(drm_writeback_connector_init);
+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..4795024 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -152,6 +152,12 @@ 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,
+				struct drm_encoder *enc,
+				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 v6 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 v6:
	- pass the encoder to drm_writeback_connector_init_with_encoder()

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..5e53f02 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, &wb_conn->encoder,
+			&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 v6 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 v6:
	- assign the encoder inside
	  drm_writeback_connector_init_with_encoder() for
	  better readability
	- improve some documentation for internal encoder

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

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
 {
 	int ret = 0;
 
-	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
+	drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
 
-	wb_connector->encoder.possible_crtcs = possible_crtcs;
+	wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
 
-	ret = drm_encoder_init(dev, &wb_connector->encoder,
+	ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
 			       &drm_writeback_encoder_funcs,
 			       DRM_MODE_ENCODER_VIRTUAL, NULL);
 	if (ret)
 		return ret;
 
-	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
-			con_funcs, formats, n_formats);
+	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+			&wb_connector->internal_encoder, con_funcs, formats, n_formats);
 
 	if (ret)
-		drm_encoder_cleanup(&wb_connector->encoder);
+		drm_encoder_cleanup(&wb_connector->internal_encoder);
 
 	return ret;
 }
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
 	struct drm_mode_config *config = &dev->mode_config;
 	int ret = create_writeback_properties(dev);
 
+	/*
+	 * Assign the encoder passed to this API to the wb_connector's encoder.
+	 * For drm_writeback_connector_init(), this shall be the internal_encoder
+	 */
+	wb_connector->encoder = enc;
+
 	if (ret != 0)
 		return ret;
 
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 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)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 
 	wb_conn = &txp->connector;
 
-	drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
+	drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
 
-	ret = drm_encoder_init(drm, &wb_conn->encoder,
+	ret = drm_encoder_init(drm, &txp->drm_enc,
 			&vc4_txp_encoder_funcs,
 			DRM_MODE_ENCODER_VIRTUAL, NULL);
 
@@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
 	drm_connector_helper_add(&wb_conn->base,
 				 &vc4_txp_connector_helper_funcs);
 
-	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
+	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
 			&vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
 	if (ret) {
-		drm_encoder_cleanup(&wb_conn->encoder);
+		drm_encoder_cleanup(&txp->drm_enc);
 		return ret;
 	}
 
@@ -523,7 +525,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 4795024..3f5c330 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,15 +25,32 @@ 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
+	 * drm_writeback_connector_init() is used.
+	 * @encoder will be assigned to this for those cases
+	 *
+	 * This will be unused when drm_writeback_connector_init_with_encoder()
+	 * is used.
+	 */
+	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 v6 1/4] drm: allow passing possible_crtcs to drm_writeback_connector_init()

[Laurent Pinchart]

Hello Abhinav,

Thank you for the patch.

On Thu, Mar 31, 2022 at 05:12:10PM -0700, Abhinav Kumar wrote:
> 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 v6:
>      - 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)));

Please wrap long lines to match the coding style of existing drivers.

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

Wrong indentation, and outer parentheses are not needed.

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

For in-kernel code, you can use u32 instead of uint32_t.

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

Wrong indentation, and outer parentheses are not needed.

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

Line wrap here too.

>  
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  			 struct drm_framebuffer *fb);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav,

Thank you for the patch.

On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
> 
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
> 
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
> 
> changes in v6:
> 	- assign the encoder inside
> 	  drm_writeback_connector_init_with_encoder() for
> 	  better readability
> 	- improve some documentation for internal encoder
> 
> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
>  include/drm/drm_writeback.h     | 21 +++++++++++++++++++--

Please split this in two patches, one for the DRM core and one for the
VC4 driver. This applies to most patches as a general rule, with the
main exception being API refactoring that requires changing the
implementation and all its users in a single patch.

>  3 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 797223c..7f72109 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  {
>  	int ret = 0;
>  
> -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> +	drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
>  
> -	wb_connector->encoder.possible_crtcs = possible_crtcs;
> +	wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>  
> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
> +	ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>  			       &drm_writeback_encoder_funcs,
>  			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
> -			con_funcs, formats, n_formats);
> +	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> +			&wb_connector->internal_encoder, con_funcs, formats, n_formats);
>  
>  	if (ret)
> -		drm_encoder_cleanup(&wb_connector->encoder);
> +		drm_encoder_cleanup(&wb_connector->internal_encoder);
>  
>  	return ret;
>  }
> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>  	struct drm_mode_config *config = &dev->mode_config;
>  	int ret = create_writeback_properties(dev);
>  
> +	/*
> +	 * Assign the encoder passed to this API to the wb_connector's encoder.
> +	 * For drm_writeback_connector_init(), this shall be the internal_encoder
> +	 */
> +	wb_connector->encoder = enc;
> +
>  	if (ret != 0)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 5e53f02..a9b4f83 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)
> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  
>  	wb_conn = &txp->connector;
>  
> -	drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
> +	drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
>  
> -	ret = drm_encoder_init(drm, &wb_conn->encoder,
> +	ret = drm_encoder_init(drm, &txp->drm_enc,
>  			&vc4_txp_encoder_funcs,
>  			DRM_MODE_ENCODER_VIRTUAL, NULL);
>  
> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	drm_connector_helper_add(&wb_conn->base,
>  				 &vc4_txp_connector_helper_funcs);
>  
> -	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
> +	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
>  			&vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
>  	if (ret) {
> -		drm_encoder_cleanup(&wb_conn->encoder);
> +		drm_encoder_cleanup(&txp->drm_enc);
>  		return ret;
>  	}
>  
> @@ -523,7 +525,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 4795024..3f5c330 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -25,15 +25,32 @@ 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
> +	 * drm_writeback_connector_init() is used.
> +	 * @encoder will be assigned to this for those cases
> +	 *
> +	 * This will be unused when drm_writeback_connector_init_with_encoder()
> +	 * is used.
> +	 */
> +	struct drm_encoder internal_encoder;
> +	/**
>  	 * @pixel_formats_blob_ptr:
>  	 *
>  	 * DRM blob property data for the pixel formats list on writeback
> -- 
> 2.7.4
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

Thanks for the review.

One question below.

On 4/1/2022 8:38 AM, Laurent Pinchart wrote:
> Hi Abhinav,
> 
> Thank you for the patch.
> 
> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
>> For some vendor driver implementations, display hardware can
>> be shared between the encoder used for writeback and the physical
>> display.
>>
>> In addition resources such as clocks and interrupts can
>> also be shared between writeback and the real encoder.
>>
>> To accommodate such vendor drivers and hardware, allow
>> real encoder to be passed for drm_writeback_connector.
>>
>> changes in v6:
>> 	- assign the encoder inside
>> 	  drm_writeback_connector_init_with_encoder() for
>> 	  better readability
>> 	- improve some documentation for internal encoder
>>
>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>>   drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
>>   include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
> 
> Please split this in two patches, one for the DRM core and one for the
> VC4 driver. This applies to most patches as a general rule, with the
> main exception being API refactoring that requires changing the
> implementation and all its users in a single patch.

I also wanted to do that but that would break compilation of this change 
because of vc4 (explained below) so I had to club it in this patch.

If you can provide some suggestion on how to address vc4's dependency I 
can break this up.

> 
>>   3 files changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index 797223c..7f72109 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   {
>>   	int ret = 0;
>>   
>> -	drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>> +	drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
>>   
>> -	wb_connector->encoder.possible_crtcs = possible_crtcs;
>> +	wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>>   
>> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
>> +	ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>>   			       &drm_writeback_encoder_funcs,
>>   			       DRM_MODE_ENCODER_VIRTUAL, NULL);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>> -			con_funcs, formats, n_formats);
>> +	ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
>> +			&wb_connector->internal_encoder, con_funcs, formats, n_formats);
>>   
>>   	if (ret)
>> -		drm_encoder_cleanup(&wb_connector->encoder);
>> +		drm_encoder_cleanup(&wb_connector->internal_encoder);
>>   
>>   	return ret;
>>   }
>> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>>   	struct drm_mode_config *config = &dev->mode_config;
>>   	int ret = create_writeback_properties(dev);
>>   
>> +	/*
>> +	 * Assign the encoder passed to this API to the wb_connector's encoder.
>> +	 * For drm_writeback_connector_init(), this shall be the internal_encoder
>> +	 */
>> +	wb_connector->encoder = enc;
>> +
>>   	if (ret != 0)
>>   		return ret;
>>   
>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>> index 5e53f02..a9b4f83 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);

vc4 directly references connector.encoder here.

>> +	return container_of(encoder, struct vc4_txp, drm_enc);
>>   }
>>   
>>   static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   
>>   	wb_conn = &txp->connector;
>>   
>> -	drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
>> +	drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
>>   
>> -	ret = drm_encoder_init(drm, &wb_conn->encoder,
>> +	ret = drm_encoder_init(drm, &txp->drm_enc,
>>   			&vc4_txp_encoder_funcs,
>>   			DRM_MODE_ENCODER_VIRTUAL, NULL);
>>   
>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   	drm_connector_helper_add(&wb_conn->base,
>>   				 &vc4_txp_connector_helper_funcs);
>>   
>> -	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
>> +	ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
>>   			&vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
>>   	if (ret) {
>> -		drm_encoder_cleanup(&wb_conn->encoder);
>> +		drm_encoder_cleanup(&txp->drm_enc);
>>   		return ret;
>>   	}
>>   
>> @@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>   	if (ret)
>>   		return ret;
>>   
>> -	encoder = &txp->connector.encoder;

Also here.

>> +	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 4795024..3f5c330 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -25,15 +25,32 @@ 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
>> +	 * drm_writeback_connector_init() is used.
>> +	 * @encoder will be assigned to this for those cases
>> +	 *
>> +	 * This will be unused when drm_writeback_connector_init_with_encoder()
>> +	 * is used.
>> +	 */
>> +	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 v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Rob Clark]

On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Abhinav,
>
> Thank you for the patch.
>
> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> > For some vendor driver implementations, display hardware can
> > be shared between the encoder used for writeback and the physical
> > display.
> >
> > In addition resources such as clocks and interrupts can
> > also be shared between writeback and the real encoder.
> >
> > To accommodate such vendor drivers and hardware, allow
> > real encoder to be passed for drm_writeback_connector.
> >
> > changes in v6:
> >       - assign the encoder inside
> >         drm_writeback_connector_init_with_encoder() for
> >         better readability
> >       - improve some documentation for internal encoder
> >
> > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > ---
> >  drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
> >  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
> >  include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
>
> Please split this in two patches, one for the DRM core and one for the
> VC4 driver. This applies to most patches as a general rule, with the
> main exception being API refactoring that requires changing the
> implementation and all its users in a single patch.

But this *is* API refactoring ;-)

BR,
-R

> >  3 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index 797223c..7f72109 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >  {
> >       int ret = 0;
> >
> > -     drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > +     drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
> >
> > -     wb_connector->encoder.possible_crtcs = possible_crtcs;
> > +     wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> >
> > -     ret = drm_encoder_init(dev, &wb_connector->encoder,
> > +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
> >                              &drm_writeback_encoder_funcs,
> >                              DRM_MODE_ENCODER_VIRTUAL, NULL);
> >       if (ret)
> >               return ret;
> >
> > -     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
> > -                     con_funcs, formats, n_formats);
> > +     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > +                     &wb_connector->internal_encoder, con_funcs, formats, n_formats);
> >
> >       if (ret)
> > -             drm_encoder_cleanup(&wb_connector->encoder);
> > +             drm_encoder_cleanup(&wb_connector->internal_encoder);
> >
> >       return ret;
> >  }
> > @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> >       struct drm_mode_config *config = &dev->mode_config;
> >       int ret = create_writeback_properties(dev);
> >
> > +     /*
> > +      * Assign the encoder passed to this API to the wb_connector's encoder.
> > +      * For drm_writeback_connector_init(), this shall be the internal_encoder
> > +      */
> > +     wb_connector->encoder = enc;
> > +
> >       if (ret != 0)
> >               return ret;
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> > index 5e53f02..a9b4f83 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)
> > @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> >
> >       wb_conn = &txp->connector;
> >
> > -     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
> > +     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
> >
> > -     ret = drm_encoder_init(drm, &wb_conn->encoder,
> > +     ret = drm_encoder_init(drm, &txp->drm_enc,
> >                       &vc4_txp_encoder_funcs,
> >                       DRM_MODE_ENCODER_VIRTUAL, NULL);
> >
> > @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> >       drm_connector_helper_add(&wb_conn->base,
> >                                &vc4_txp_connector_helper_funcs);
> >
> > -     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
> > +     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
> >                       &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
> >       if (ret) {
> > -             drm_encoder_cleanup(&wb_conn->encoder);
> > +             drm_encoder_cleanup(&txp->drm_enc);
> >               return ret;
> >       }
> >
> > @@ -523,7 +525,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 4795024..3f5c330 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -25,15 +25,32 @@ 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
> > +      * drm_writeback_connector_init() is used.
> > +      * @encoder will be assigned to this for those cases
> > +      *
> > +      * This will be unused when drm_writeback_connector_init_with_encoder()
> > +      * is used.
> > +      */
> > +     struct drm_encoder internal_encoder;
> > +     /**
> >        * @pixel_formats_blob_ptr:
> >        *
> >        * DRM blob property data for the pixel formats list on writeback
> > --
> > 2.7.4
> >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Laurent Pinchart]

Hi Rob and Abhinav,

On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
> > On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> > > For some vendor driver implementations, display hardware can
> > > be shared between the encoder used for writeback and the physical
> > > display.
> > >
> > > In addition resources such as clocks and interrupts can
> > > also be shared between writeback and the real encoder.
> > >
> > > To accommodate such vendor drivers and hardware, allow
> > > real encoder to be passed for drm_writeback_connector.
> > >
> > > changes in v6:
> > >       - assign the encoder inside
> > >         drm_writeback_connector_init_with_encoder() for
> > >         better readability
> > >       - improve some documentation for internal encoder
> > >
> > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > > Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
> > >  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
> > >  include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
> >
> > Please split this in two patches, one for the DRM core and one for the
> > VC4 driver. This applies to most patches as a general rule, with the
> > main exception being API refactoring that requires changing the
> > implementation and all its users in a single patch.
> 
> But this *is* API refactoring ;-)

Partly at least :-) Looking at the API change itself, wouldn't we
minimize the extra changes to vc4 if we moved this patch before 3/4 ?

> > >  3 files changed, 39 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > > index 797223c..7f72109 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >  {
> > >       int ret = 0;
> > >
> > > -     drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > > +     drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
> > >
> > > -     wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > +     wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> > >
> > > -     ret = drm_encoder_init(dev, &wb_connector->encoder,
> > > +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
> > >                              &drm_writeback_encoder_funcs,
> > >                              DRM_MODE_ENCODER_VIRTUAL, NULL);
> > >       if (ret)
> > >               return ret;
> > >
> > > -     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
> > > -                     con_funcs, formats, n_formats);
> > > +     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> > > +                     &wb_connector->internal_encoder, con_funcs, formats, n_formats);
> > >
> > >       if (ret)
> > > -             drm_encoder_cleanup(&wb_connector->encoder);
> > > +             drm_encoder_cleanup(&wb_connector->internal_encoder);
> > >
> > >       return ret;
> > >  }
> > > @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > >       struct drm_mode_config *config = &dev->mode_config;
> > >       int ret = create_writeback_properties(dev);
> > >
> > > +     /*
> > > +      * Assign the encoder passed to this API to the wb_connector's encoder.
> > > +      * For drm_writeback_connector_init(), this shall be the internal_encoder
> > > +      */
> > > +     wb_connector->encoder = enc;
> > > +
> > >       if (ret != 0)
> > >               return ret;
> > >
> > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> > > index 5e53f02..a9b4f83 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)
> > > @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> > >
> > >       wb_conn = &txp->connector;
> > >
> > > -     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
> > > +     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
> > >
> > > -     ret = drm_encoder_init(drm, &wb_conn->encoder,
> > > +     ret = drm_encoder_init(drm, &txp->drm_enc,
> > >                       &vc4_txp_encoder_funcs,
> > >                       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > >
> > > @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> > >       drm_connector_helper_add(&wb_conn->base,
> > >                                &vc4_txp_connector_helper_funcs);
> > >
> > > -     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
> > > +     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
> > >                       &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
> > >       if (ret) {
> > > -             drm_encoder_cleanup(&wb_conn->encoder);
> > > +             drm_encoder_cleanup(&txp->drm_enc);
> > >               return ret;
> > >       }
> > >
> > > @@ -523,7 +525,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 4795024..3f5c330 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -25,15 +25,32 @@ 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
> > > +      * drm_writeback_connector_init() is used.
> > > +      * @encoder will be assigned to this for those cases
> > > +      *
> > > +      * This will be unused when drm_writeback_connector_init_with_encoder()
> > > +      * is used.
> > > +      */
> > > +     struct drm_encoder internal_encoder;
> > > +     /**
> > >        * @pixel_formats_blob_ptr:
> > >        *
> > >        * DRM blob property data for the pixel formats list on writeback

-- 
Regards,

Laurent Pinchart

Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
> Hi Rob and Abhinav,
> 
> On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
>> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
>>> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
>>>> For some vendor driver implementations, display hardware can
>>>> be shared between the encoder used for writeback and the physical
>>>> display.
>>>>
>>>> In addition resources such as clocks and interrupts can
>>>> also be shared between writeback and the real encoder.
>>>>
>>>> To accommodate such vendor drivers and hardware, allow
>>>> real encoder to be passed for drm_writeback_connector.
>>>>
>>>> changes in v6:
>>>>        - assign the encoder inside
>>>>          drm_writeback_connector_init_with_encoder() for
>>>>          better readability
>>>>        - improve some documentation for internal encoder
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>>>>   drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
>>>>   include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
>>>
>>> Please split this in two patches, one for the DRM core and one for the
>>> VC4 driver. This applies to most patches as a general rule, with the
>>> main exception being API refactoring that requires changing the
>>> implementation and all its users in a single patch.
>>
>> But this *is* API refactoring ;-)
> 
> Partly at least :-) Looking at the API change itself, wouldn't we
> minimize the extra changes to vc4 if we moved this patch before 3/4 ?

I can move all the changes done in vc4 except below part to the change 
3/4 itself because that way I can show usage of vc4->drm_enc with the 
new API. Let me know if that works.

The only part which will remain is the below one:

@@ -523,7 +525,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);

Since i dont know vc4 driver very well, I was not sure of a good way to 
decouple this dependency.

Let me know if that works.

Thanks

Abhinav

> 
>>>>   3 files changed, 39 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>> index 797223c..7f72109 100644
>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>   {
>>>>        int ret = 0;
>>>>
>>>> -     drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>>>> +     drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
>>>>
>>>> -     wb_connector->encoder.possible_crtcs = possible_crtcs;
>>>> +     wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>>>>
>>>> -     ret = drm_encoder_init(dev, &wb_connector->encoder,
>>>> +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>>>>                               &drm_writeback_encoder_funcs,
>>>>                               DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>        if (ret)
>>>>                return ret;
>>>>
>>>> -     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>>>> -                     con_funcs, formats, n_formats);
>>>> +     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
>>>> +                     &wb_connector->internal_encoder, con_funcs, formats, n_formats);
>>>>
>>>>        if (ret)
>>>> -             drm_encoder_cleanup(&wb_connector->encoder);
>>>> +             drm_encoder_cleanup(&wb_connector->internal_encoder);
>>>>
>>>>        return ret;
>>>>   }
>>>> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>>>>        struct drm_mode_config *config = &dev->mode_config;
>>>>        int ret = create_writeback_properties(dev);
>>>>
>>>> +     /*
>>>> +      * Assign the encoder passed to this API to the wb_connector's encoder.
>>>> +      * For drm_writeback_connector_init(), this shall be the internal_encoder
>>>> +      */
>>>> +     wb_connector->encoder = enc;
>>>> +
>>>>        if (ret != 0)
>>>>                return ret;
>>>>
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>>>> index 5e53f02..a9b4f83 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)
>>>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>
>>>>        wb_conn = &txp->connector;
>>>>
>>>> -     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
>>>> +     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
>>>>
>>>> -     ret = drm_encoder_init(drm, &wb_conn->encoder,
>>>> +     ret = drm_encoder_init(drm, &txp->drm_enc,
>>>>                        &vc4_txp_encoder_funcs,
>>>>                        DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>
>>>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>        drm_connector_helper_add(&wb_conn->base,
>>>>                                 &vc4_txp_connector_helper_funcs);
>>>>
>>>> -     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
>>>> +     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
>>>>                        &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
>>>>        if (ret) {
>>>> -             drm_encoder_cleanup(&wb_conn->encoder);
>>>> +             drm_encoder_cleanup(&txp->drm_enc);
>>>>                return ret;
>>>>        }
>>>>
>>>> @@ -523,7 +525,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 4795024..3f5c330 100644
>>>> --- a/include/drm/drm_writeback.h
>>>> +++ b/include/drm/drm_writeback.h
>>>> @@ -25,15 +25,32 @@ 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
>>>> +      * drm_writeback_connector_init() is used.
>>>> +      * @encoder will be assigned to this for those cases
>>>> +      *
>>>> +      * This will be unused when drm_writeback_connector_init_with_encoder()
>>>> +      * is used.
>>>> +      */
>>>> +     struct drm_encoder internal_encoder;
>>>> +     /**
>>>>         * @pixel_formats_blob_ptr:
>>>>         *
>>>>         * DRM blob property data for the pixel formats list on writeback
> 

Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav,

On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
> On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
> > On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
> >> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
> >>> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
> >>>> For some vendor driver implementations, display hardware can
> >>>> be shared between the encoder used for writeback and the physical
> >>>> display.
> >>>>
> >>>> In addition resources such as clocks and interrupts can
> >>>> also be shared between writeback and the real encoder.
> >>>>
> >>>> To accommodate such vendor drivers and hardware, allow
> >>>> real encoder to be passed for drm_writeback_connector.
> >>>>
> >>>> changes in v6:
> >>>>        - assign the encoder inside
> >>>>          drm_writeback_connector_init_with_encoder() for
> >>>>          better readability
> >>>>        - improve some documentation for internal encoder
> >>>>
> >>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>> ---
> >>>>   drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
> >>>>   drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
> >>>>   include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
> >>>
> >>> Please split this in two patches, one for the DRM core and one for the
> >>> VC4 driver. This applies to most patches as a general rule, with the
> >>> main exception being API refactoring that requires changing the
> >>> implementation and all its users in a single patch.
> >>
> >> But this *is* API refactoring ;-)
> > 
> > Partly at least :-) Looking at the API change itself, wouldn't we
> > minimize the extra changes to vc4 if we moved this patch before 3/4 ?
> 
> I can move all the changes done in vc4 except below part to the change 
> 3/4 itself because that way I can show usage of vc4->drm_enc with the 
> new API. Let me know if that works.

What I meant is moving the API refactoring from 4/4 before the current
3/4, with minimal changes to vc4 there (only to avoid introducing a
bisection breakge), and then move most of the vc4 changes from this
patch to the current 3/4 (which will become 4/4). If that's what you
meant too, it sounds good to me.

> The only part which will remain is the below one:
> 
> @@ -523,7 +525,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);
> 
> Since i dont know vc4 driver very well, I was not sure of a good way to 
> decouple this dependency.
> 
> Let me know if that works.
>
> >>>>   3 files changed, 39 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> >>>> index 797223c..7f72109 100644
> >>>> --- a/drivers/gpu/drm/drm_writeback.c
> >>>> +++ b/drivers/gpu/drm/drm_writeback.c
> >>>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >>>>   {
> >>>>        int ret = 0;
> >>>>
> >>>> -     drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> >>>> +     drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
> >>>>
> >>>> -     wb_connector->encoder.possible_crtcs = possible_crtcs;
> >>>> +     wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> >>>>
> >>>> -     ret = drm_encoder_init(dev, &wb_connector->encoder,
> >>>> +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
> >>>>                               &drm_writeback_encoder_funcs,
> >>>>                               DRM_MODE_ENCODER_VIRTUAL, NULL);
> >>>>        if (ret)
> >>>>                return ret;
> >>>>
> >>>> -     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
> >>>> -                     con_funcs, formats, n_formats);
> >>>> +     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> >>>> +                     &wb_connector->internal_encoder, con_funcs, formats, n_formats);
> >>>>
> >>>>        if (ret)
> >>>> -             drm_encoder_cleanup(&wb_connector->encoder);
> >>>> +             drm_encoder_cleanup(&wb_connector->internal_encoder);
> >>>>
> >>>>        return ret;
> >>>>   }
> >>>> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> >>>>        struct drm_mode_config *config = &dev->mode_config;
> >>>>        int ret = create_writeback_properties(dev);
> >>>>
> >>>> +     /*
> >>>> +      * Assign the encoder passed to this API to the wb_connector's encoder.
> >>>> +      * For drm_writeback_connector_init(), this shall be the internal_encoder
> >>>> +      */
> >>>> +     wb_connector->encoder = enc;
> >>>> +
> >>>>        if (ret != 0)
> >>>>                return ret;
> >>>>
> >>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> >>>> index 5e53f02..a9b4f83 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)
> >>>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> >>>>
> >>>>        wb_conn = &txp->connector;
> >>>>
> >>>> -     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
> >>>> +     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
> >>>>
> >>>> -     ret = drm_encoder_init(drm, &wb_conn->encoder,
> >>>> +     ret = drm_encoder_init(drm, &txp->drm_enc,
> >>>>                        &vc4_txp_encoder_funcs,
> >>>>                        DRM_MODE_ENCODER_VIRTUAL, NULL);
> >>>>
> >>>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
> >>>>        drm_connector_helper_add(&wb_conn->base,
> >>>>                                 &vc4_txp_connector_helper_funcs);
> >>>>
> >>>> -     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
> >>>> +     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
> >>>>                        &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
> >>>>        if (ret) {
> >>>> -             drm_encoder_cleanup(&wb_conn->encoder);
> >>>> +             drm_encoder_cleanup(&txp->drm_enc);
> >>>>                return ret;
> >>>>        }
> >>>>
> >>>> @@ -523,7 +525,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 4795024..3f5c330 100644
> >>>> --- a/include/drm/drm_writeback.h
> >>>> +++ b/include/drm/drm_writeback.h
> >>>> @@ -25,15 +25,32 @@ 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
> >>>> +      * drm_writeback_connector_init() is used.
> >>>> +      * @encoder will be assigned to this for those cases
> >>>> +      *
> >>>> +      * This will be unused when drm_writeback_connector_init_with_encoder()
> >>>> +      * is used.
> >>>> +      */
> >>>> +     struct drm_encoder internal_encoder;
> >>>> +     /**
> >>>>         * @pixel_formats_blob_ptr:
> >>>>         *
> >>>>         * DRM blob property data for the pixel formats list on writeback

-- 
Regards,

Laurent Pinchart

Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

On 4/5/2022 10:02 AM, Laurent Pinchart wrote:
> Hi Abhinav,
> 
> On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
>> On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
>>> On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
>>>> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
>>>>> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
>>>>>> For some vendor driver implementations, display hardware can
>>>>>> be shared between the encoder used for writeback and the physical
>>>>>> display.
>>>>>>
>>>>>> In addition resources such as clocks and interrupts can
>>>>>> also be shared between writeback and the real encoder.
>>>>>>
>>>>>> To accommodate such vendor drivers and hardware, allow
>>>>>> real encoder to be passed for drm_writeback_connector.
>>>>>>
>>>>>> changes in v6:
>>>>>>         - assign the encoder inside
>>>>>>           drm_writeback_connector_init_with_encoder() for
>>>>>>           better readability
>>>>>>         - improve some documentation for internal encoder
>>>>>>
>>>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>>>>>>    drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
>>>>>>    include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
>>>>>
>>>>> Please split this in two patches, one for the DRM core and one for the
>>>>> VC4 driver. This applies to most patches as a general rule, with the
>>>>> main exception being API refactoring that requires changing the
>>>>> implementation and all its users in a single patch.
>>>>
>>>> But this *is* API refactoring ;-)
>>>
>>> Partly at least :-) Looking at the API change itself, wouldn't we
>>> minimize the extra changes to vc4 if we moved this patch before 3/4 ?
>>
>> I can move all the changes done in vc4 except below part to the change
>> 3/4 itself because that way I can show usage of vc4->drm_enc with the
>> new API. Let me know if that works.
> 
> What I meant is moving the API refactoring from 4/4 before the current
> 3/4, with minimal changes to vc4 there (only to avoid introducing a
> bisection breakge), and then move most of the vc4 changes from this
> patch to the current 3/4 (which will become 4/4). If that's what you
> meant too, it sounds good to me.

The API refactoring part in this patch is tied closely with changing the 
wb_connector's encoder to a pointer which breaks vc4.

I have not made any additional refactoring changes here.

So I am not sure how to decouple this more.

Thanks

Abhinav


> 
>> The only part which will remain is the below one:
>>
>> @@ -523,7 +525,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);
>>
>> Since i dont know vc4 driver very well, I was not sure of a good way to
>> decouple this dependency.
>>
>> Let me know if that works.
>>
>>>>>>    3 files changed, 39 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>>>> index 797223c..7f72109 100644
>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>>>    {
>>>>>>         int ret = 0;
>>>>>>
>>>>>> -     drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>>>>>> +     drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
>>>>>>
>>>>>> -     wb_connector->encoder.possible_crtcs = possible_crtcs;
>>>>>> +     wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>>>>>>
>>>>>> -     ret = drm_encoder_init(dev, &wb_connector->encoder,
>>>>>> +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>>>>>>                                &drm_writeback_encoder_funcs,
>>>>>>                                DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>>>         if (ret)
>>>>>>                 return ret;
>>>>>>
>>>>>> -     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>>>>>> -                     con_funcs, formats, n_formats);
>>>>>> +     ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
>>>>>> +                     &wb_connector->internal_encoder, con_funcs, formats, n_formats);
>>>>>>
>>>>>>         if (ret)
>>>>>> -             drm_encoder_cleanup(&wb_connector->encoder);
>>>>>> +             drm_encoder_cleanup(&wb_connector->internal_encoder);
>>>>>>
>>>>>>         return ret;
>>>>>>    }
>>>>>> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>>>>>>         struct drm_mode_config *config = &dev->mode_config;
>>>>>>         int ret = create_writeback_properties(dev);
>>>>>>
>>>>>> +     /*
>>>>>> +      * Assign the encoder passed to this API to the wb_connector's encoder.
>>>>>> +      * For drm_writeback_connector_init(), this shall be the internal_encoder
>>>>>> +      */
>>>>>> +     wb_connector->encoder = enc;
>>>>>> +
>>>>>>         if (ret != 0)
>>>>>>                 return ret;
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>>>>>> index 5e53f02..a9b4f83 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)
>>>>>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>>>
>>>>>>         wb_conn = &txp->connector;
>>>>>>
>>>>>> -     drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
>>>>>> +     drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
>>>>>>
>>>>>> -     ret = drm_encoder_init(drm, &wb_conn->encoder,
>>>>>> +     ret = drm_encoder_init(drm, &txp->drm_enc,
>>>>>>                         &vc4_txp_encoder_funcs,
>>>>>>                         DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>>>
>>>>>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>>>>>         drm_connector_helper_add(&wb_conn->base,
>>>>>>                                  &vc4_txp_connector_helper_funcs);
>>>>>>
>>>>>> -     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
>>>>>> +     ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
>>>>>>                         &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
>>>>>>         if (ret) {
>>>>>> -             drm_encoder_cleanup(&wb_conn->encoder);
>>>>>> +             drm_encoder_cleanup(&txp->drm_enc);
>>>>>>                 return ret;
>>>>>>         }
>>>>>>
>>>>>> @@ -523,7 +525,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 4795024..3f5c330 100644
>>>>>> --- a/include/drm/drm_writeback.h
>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>> @@ -25,15 +25,32 @@ 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
>>>>>> +      * drm_writeback_connector_init() is used.
>>>>>> +      * @encoder will be assigned to this for those cases
>>>>>> +      *
>>>>>> +      * This will be unused when drm_writeback_connector_init_with_encoder()
>>>>>> +      * is used.
>>>>>> +      */
>>>>>> +     struct drm_encoder internal_encoder;
>>>>>> +     /**
>>>>>>          * @pixel_formats_blob_ptr:
>>>>>>          *
>>>>>>          * DRM blob property data for the pixel formats list on writeback
> 

Re: [Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

On 4/5/2022 10:08 AM, Abhinav Kumar wrote:
> Hi Laurent
> 
> On 4/5/2022 10:02 AM, Laurent Pinchart wrote:
>> Hi Abhinav,
>>
>> On Tue, Apr 05, 2022 at 09:53:57AM -0700, Abhinav Kumar wrote:
>>> On 4/5/2022 9:47 AM, Laurent Pinchart wrote:
>>>> On Mon, Apr 04, 2022 at 11:43:37AM -0700, Rob Clark wrote:
>>>>> On Fri, Apr 1, 2022 at 8:38 AM Laurent Pinchart wrote:
>>>>>> On Thu, Mar 31, 2022 at 05:12:13PM -0700, Abhinav Kumar wrote:
>>>>>>> For some vendor driver implementations, display hardware can
>>>>>>> be shared between the encoder used for writeback and the physical
>>>>>>> display.
>>>>>>>
>>>>>>> In addition resources such as clocks and interrupts can
>>>>>>> also be shared between writeback and the real encoder.
>>>>>>>
>>>>>>> To accommodate such vendor drivers and hardware, allow
>>>>>>> real encoder to be passed for drm_writeback_connector.
>>>>>>>
>>>>>>> changes in v6:
>>>>>>>         - assign the encoder inside
>>>>>>>           drm_writeback_connector_init_with_encoder() for
>>>>>>>           better readability
>>>>>>>         - improve some documentation for internal encoder
>>>>>>>
>>>>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com>
>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>>>>>>>    drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++------
>>>>>>>    include/drm/drm_writeback.h     | 21 +++++++++++++++++++--
>>>>>>
>>>>>> Please split this in two patches, one for the DRM core and one for 
>>>>>> the
>>>>>> VC4 driver. This applies to most patches as a general rule, with the
>>>>>> main exception being API refactoring that requires changing the
>>>>>> implementation and all its users in a single patch.
>>>>>
>>>>> But this *is* API refactoring ;-)
>>>>
>>>> Partly at least :-) Looking at the API change itself, wouldn't we
>>>> minimize the extra changes to vc4 if we moved this patch before 3/4 ?
>>>
>>> I can move all the changes done in vc4 except below part to the change
>>> 3/4 itself because that way I can show usage of vc4->drm_enc with the
>>> new API. Let me know if that works.
>>
>> What I meant is moving the API refactoring from 4/4 before the current
>> 3/4, with minimal changes to vc4 there (only to avoid introducing a
>> bisection breakge), and then move most of the vc4 changes from this
>> patch to the current 3/4 (which will become 4/4). If that's what you
>> meant too, it sounds good to me.
> 
> The API refactoring part in this patch is tied closely with changing the 
> wb_connector's encoder to a pointer which breaks vc4.
> 
> I have not made any additional refactoring changes here.
> 
> So I am not sure how to decouple this more.
> 
> Thanks
> 
> Abhinav

Looking at this more, even if we split the patch to smaller set of core 
changes, we might be able to make them individually compile but 
functionality will be broken because the wb_connector->encoder has to be 
a valid one and if we break the core functionality further it will break 
this.

@@ -238,6 +238,12 @@  int 
drm_writeback_connector_init_with_encoder(struct drm_device *dev,
  	struct drm_mode_config *config = &dev->mode_config;
  	int ret = create_writeback_properties(dev);

+	/*
+	 * Assign the encoder passed to this API to the wb_connector's encoder.
+	 * For drm_writeback_connector_init(), this shall be the internal_encoder
+	 */
+	wb_connector->encoder = enc;
+
  	if (ret != 0)
  		return ret;

So to keep both compilation and functionality intact at every change, i 
am not sure how to break this up more.

> 
> 
>>
>>> The only part which will remain is the below one:
>>>
>>> @@ -523,7 +525,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);
>>>
>>> Since i dont know vc4 driver very well, I was not sure of a good way to
>>> decouple this dependency.
>>>
>>> Let me know if that works.
>>>
>>>>>>>    3 files changed, 39 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c 
>>>>>>> b/drivers/gpu/drm/drm_writeback.c
>>>>>>> index 797223c..7f72109 100644
>>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct 
>>>>>>> drm_device *dev,
>>>>>>>    {
>>>>>>>         int ret = 0;
>>>>>>>
>>>>>>> -     drm_encoder_helper_add(&wb_connector->encoder, 
>>>>>>> enc_helper_funcs);
>>>>>>> +     drm_encoder_helper_add(&wb_connector->internal_encoder, 
>>>>>>> enc_helper_funcs);
>>>>>>>
>>>>>>> -     wb_connector->encoder.possible_crtcs = possible_crtcs;
>>>>>>> +     wb_connector->internal_encoder.possible_crtcs = 
>>>>>>> possible_crtcs;
>>>>>>>
>>>>>>> -     ret = drm_encoder_init(dev, &wb_connector->encoder,
>>>>>>> +     ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>>>>>>>                                &drm_writeback_encoder_funcs,
>>>>>>>                                DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>>>>         if (ret)
>>>>>>>                 return ret;
>>>>>>>
>>>>>>> -     ret = drm_writeback_connector_init_with_encoder(dev, 
>>>>>>> wb_connector, &wb_connector->encoder,
>>>>>>> -                     con_funcs, formats, n_formats);
>>>>>>> +     ret = drm_writeback_connector_init_with_encoder(dev, 
>>>>>>> wb_connector,
>>>>>>> +                     &wb_connector->internal_encoder, con_funcs, 
>>>>>>> formats, n_formats);
>>>>>>>
>>>>>>>         if (ret)
>>>>>>> -             drm_encoder_cleanup(&wb_connector->encoder);
>>>>>>> +             drm_encoder_cleanup(&wb_connector->internal_encoder);
>>>>>>>
>>>>>>>         return ret;
>>>>>>>    }
>>>>>>> @@ -238,6 +238,12 @@ int 
>>>>>>> drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>>>>>>>         struct drm_mode_config *config = &dev->mode_config;
>>>>>>>         int ret = create_writeback_properties(dev);
>>>>>>>
>>>>>>> +     /*
>>>>>>> +      * Assign the encoder passed to this API to the 
>>>>>>> wb_connector's encoder.
>>>>>>> +      * For drm_writeback_connector_init(), this shall be the 
>>>>>>> internal_encoder
>>>>>>> +      */
>>>>>>> +     wb_connector->encoder = enc;
>>>>>>> +
>>>>>>>         if (ret != 0)
>>>>>>>                 return ret;
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c 
>>>>>>> b/drivers/gpu/drm/vc4/vc4_txp.c
>>>>>>> index 5e53f02..a9b4f83 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)
>>>>>>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, 
>>>>>>> struct device *master, void *data)
>>>>>>>
>>>>>>>         wb_conn = &txp->connector;
>>>>>>>
>>>>>>> -     drm_encoder_helper_add(&wb_conn->encoder, 
>>>>>>> &vc4_txp_encoder_helper_funcs);
>>>>>>> +     drm_encoder_helper_add(&txp->drm_enc, 
>>>>>>> &vc4_txp_encoder_helper_funcs);
>>>>>>>
>>>>>>> -     ret = drm_encoder_init(drm, &wb_conn->encoder,
>>>>>>> +     ret = drm_encoder_init(drm, &txp->drm_enc,
>>>>>>>                         &vc4_txp_encoder_funcs,
>>>>>>>                         DRM_MODE_ENCODER_VIRTUAL, NULL);
>>>>>>>
>>>>>>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, 
>>>>>>> struct device *master, void *data)
>>>>>>>         drm_connector_helper_add(&wb_conn->base,
>>>>>>>                                  &vc4_txp_connector_helper_funcs);
>>>>>>>
>>>>>>> -     ret = drm_writeback_connector_init_with_encoder(drm, 
>>>>>>> wb_conn, &wb_conn->encoder,
>>>>>>> +     ret = drm_writeback_connector_init_with_encoder(drm, 
>>>>>>> wb_conn, &txp->drm_enc,
>>>>>>>                         &vc4_txp_connector_funcs, drm_fmts, 
>>>>>>> ARRAY_SIZE(drm_fmts));
>>>>>>>         if (ret) {
>>>>>>> -             drm_encoder_cleanup(&wb_conn->encoder);
>>>>>>> +             drm_encoder_cleanup(&txp->drm_enc);
>>>>>>>                 return ret;
>>>>>>>         }
>>>>>>>
>>>>>>> @@ -523,7 +525,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 4795024..3f5c330 100644
>>>>>>> --- a/include/drm/drm_writeback.h
>>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>>> @@ -25,15 +25,32 @@ 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
>>>>>>> +      * drm_writeback_connector_init() is used.
>>>>>>> +      * @encoder will be assigned to this for those cases
>>>>>>> +      *
>>>>>>> +      * This will be unused when 
>>>>>>> drm_writeback_connector_init_with_encoder()
>>>>>>> +      * is used.
>>>>>>> +      */
>>>>>>> +     struct drm_encoder internal_encoder;
>>>>>>> +     /**
>>>>>>>          * @pixel_formats_blob_ptr:
>>>>>>>          *
>>>>>>>          * DRM blob property data for the pixel formats list on 
>>>>>>> writeback
>>

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

[Liviu Dudau]

Hi Abhinav,

On Thu, Mar 31, 2022 at 05:12:11PM -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 v6:
> 	- remove drm_writeback_connector_setup() and instead
> 	  directly call drm_writeback_connector_init_with_encoder()
> 	- fix a drm_writeback_connector typo and function doc which
> 	  incorrectly shows that the function accepts enc_helper_funcs
> 	- pass encoder as a parameter explicitly to the new API
> 	  for better readability

Side comment: if you plan to have the log of changes in the commit message then I
would keep a list of all the changes. Otherwise, putting the log after the -- line
below would still convey the information but will also ensure that when merged it
will not show up in the commit message.

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 72 +++++++++++++++++++++++++++++++++--------
>  include/drm/drm_writeback.h     |  6 ++++
>  2 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..797223c 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -177,6 +177,62 @@ 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 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_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
> +			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
> + * @enc: handle to the already initialized drm encoder
> + * @con_funcs: Connector funcs vtable
> + * @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_writeback_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().

I would stop after "... specific operation".

Otherwise, looks good to me.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu


> + *
> + * 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.
> + *
> + * 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, struct drm_encoder *enc,
> +		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;
> @@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  	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;
>  
>  	connector->interlace_allowed = 0;
>  
> @@ -207,8 +254,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  	if (ret)
>  		goto connector_fail;
>  
> -	ret = drm_connector_attach_encoder(connector,
> -						&wb_connector->encoder);
> +	ret = drm_connector_attach_encoder(connector, enc);
>  	if (ret)
>  		goto attach_fail;
>  
> @@ -237,12 +283,10 @@ 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;
>  }
> -EXPORT_SYMBOL(drm_writeback_connector_init);
> +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..4795024 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -152,6 +152,12 @@ 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,
> +				struct drm_encoder *enc,
> +				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 v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

Hi Liviu

Thank you for your reviews. They were very useful to get this in shape.

Some comments below.

Abhinav

On 4/6/2022 7:55 AM, Liviu Dudau wrote:
> Hi Abhinav,
> 
> On Thu, Mar 31, 2022 at 05:12:11PM -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 v6:
>> 	- remove drm_writeback_connector_setup() and instead
>> 	  directly call drm_writeback_connector_init_with_encoder()
>> 	- fix a drm_writeback_connector typo and function doc which
>> 	  incorrectly shows that the function accepts enc_helper_funcs
>> 	- pass encoder as a parameter explicitly to the new API
>> 	  for better readability
> 
> Side comment: if you plan to have the log of changes in the commit message then I
> would keep a list of all the changes. Otherwise, putting the log after the -- line
> below would still convey the information but will also ensure that when merged it
> will not show up in the commit message.

For DRM, I believe the convention was to have it above the --- ?

But yes, I will certainly include the full change log across the 
revisions. Still waiting for the reviews on the next 2 patches to reach 
a conclusion and when I post it again I will add it.

> 
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/drm_writeback.c | 72 +++++++++++++++++++++++++++++++++--------
>>   include/drm/drm_writeback.h     |  6 ++++
>>   2 files changed, 64 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index dc2ef12..797223c 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -177,6 +177,62 @@ 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 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_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>> +			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
>> + * @enc: handle to the already initialized drm encoder
>> + * @con_funcs: Connector funcs vtable
>> + * @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_writeback_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().
> 
> I would stop after "... specific operation".
Alright, will do. Thanks.
> 
> Otherwise, looks good to me.
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 
> 
>> + *
>> + * 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.
>> + *
>> + * 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, struct drm_encoder *enc,
>> +		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;
>> @@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   	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;
>>   
>>   	connector->interlace_allowed = 0;
>>   
>> @@ -207,8 +254,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>   	if (ret)
>>   		goto connector_fail;
>>   
>> -	ret = drm_connector_attach_encoder(connector,
>> -						&wb_connector->encoder);
>> +	ret = drm_connector_attach_encoder(connector, enc);
>>   	if (ret)
>>   		goto attach_fail;
>>   
>> @@ -237,12 +283,10 @@ 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;
>>   }
>> -EXPORT_SYMBOL(drm_writeback_connector_init);
>> +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..4795024 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -152,6 +152,12 @@ 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,
>> +				struct drm_encoder *enc,
>> +				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
>>
>