[PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block

[PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block

[Michael Riesch]

Hi all,

This series adds support for the RGB output block that can be found in the
Rockchip Video Output Processor (VOP) 2. Version 2 of this series
incorporates the feedback by Dan Carpenter and Sascha Hauer. Version 3
fixes a dumb mistake pointed out by Sascha :-) Thanks for your comments!

Patches 1-4 clean up the code and make it more general.

Patch 5 activates the support for the RGB output block in the VOP2 driver.

Patch 6 adds pinctrls for the 16-bit and 18-bit RGB data lines.

Tested on a custom board featuring the RK3568 SoC with a 18-bit RGB
display.

Looking forward to your comments!

Best regards,
Michael

Michael Riesch (6):
  drm/rockchip: vop2: initialize possible_crtcs properly
  drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
  drm/rockchip: rgb: add video_port parameter to init function
  drm/rockchip: vop2: use symmetric function pair
    vop2_{create,destroy}_crtcs
  drm/rockchip: vop2: add support for the rgb output block
  arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to
    rk356x

 .../boot/dts/rockchip/rk3568-pinctrl.dtsi     | 94 +++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 80 ++++++++++++----
 drivers/gpu/drm/rockchip/rockchip_rgb.c       | 19 ++--
 drivers/gpu/drm/rockchip/rockchip_rgb.h       |  6 +-
 5 files changed, 172 insertions(+), 29 deletions(-)


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.30.2

[PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly

[Michael Riesch]

The variable possible_crtcs is only initialized for primary and
overlay planes. Since the VOP2 driver only supports these plane
types at the moment, the current code is safe. However, in order
to provide a future-proof solution, fix the initialization of
the variable.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - no changes
v2:
 - new patch

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 8cecf81a5ae0..374ef821b453 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
 				/* change the unused primary window to overlay window */
 				win->type = DRM_PLANE_TYPE_OVERLAY;
 			}
-		}
-
-		if (win->type == DRM_PLANE_TYPE_OVERLAY)
+		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
 			possible_crtcs = (1 << nvps) - 1;
+		} else {
+			possible_crtcs = 0;
+		}
 
 		ret = vop2_plane_init(vop2, win, possible_crtcs);
 		if (ret) {
-- 
2.30.2

[PATCH v3 2/6] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder

[Michael Riesch]

Commit 540b8f271e53 ("drm/rockchip: Embed drm_encoder into
rockchip_decoder") provides the means to pass the endpoint ID to the
VOP2 driver, which sets the interface MUX accordingly. However, this
step has not yet been carried out for the RGB output block. Embed the
drm_encoder structure into the rockchip_encoder structure and set the
endpoint ID correctly.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - no changes
v2:
 - use endpoint id from device tree instead of hardcoded value

 drivers/gpu/drm/rockchip/rockchip_rgb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 75eb7cca3d82..5971df4302f2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -22,13 +22,11 @@
 #include "rockchip_drm_vop.h"
 #include "rockchip_rgb.h"
 
-#define encoder_to_rgb(c) container_of(c, struct rockchip_rgb, encoder)
-
 struct rockchip_rgb {
 	struct device *dev;
 	struct drm_device *drm_dev;
 	struct drm_bridge *bridge;
-	struct drm_encoder encoder;
+	struct rockchip_encoder encoder;
 	struct drm_connector connector;
 	int output_mode;
 };
@@ -125,7 +123,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 		return ERR_PTR(ret);
 	}
 
-	encoder = &rgb->encoder;
+	encoder = &rgb->encoder.encoder;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
 	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
@@ -161,6 +159,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 		goto err_free_encoder;
 	}
 
+	rgb->encoder.crtc_endpoint_id = endpoint_id;
+
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret < 0) {
 		DRM_DEV_ERROR(drm_dev->dev,
@@ -182,6 +182,6 @@ void rockchip_rgb_fini(struct rockchip_rgb *rgb)
 {
 	drm_panel_bridge_remove(rgb->bridge);
 	drm_connector_cleanup(&rgb->connector);
-	drm_encoder_cleanup(&rgb->encoder);
+	drm_encoder_cleanup(&rgb->encoder.encoder);
 }
 EXPORT_SYMBOL_GPL(rockchip_rgb_fini);
-- 
2.30.2

[PATCH v3 3/6] drm/rockchip: rgb: add video_port parameter to init function

[Michael Riesch]

The VOP2 driver has more than one video port, hence the hard-coded
port id will not work anymore. Add an extra parameter for the video
port id to the rockchip_rgb_init function.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - no changes
v2:
 - no changes

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +-
 drivers/gpu/drm/rockchip/rockchip_rgb.c     | 9 +++++----
 drivers/gpu/drm/rockchip/rockchip_rgb.h     | 6 ++++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fa1f4ee6d195..5d18dea5c8d6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -2221,7 +2221,7 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 		goto err_disable_pm_runtime;
 
 	if (vop->data->feature & VOP_FEATURE_INTERNAL_RGB) {
-		vop->rgb = rockchip_rgb_init(dev, &vop->crtc, vop->drm_dev);
+		vop->rgb = rockchip_rgb_init(dev, &vop->crtc, vop->drm_dev, 0);
 		if (IS_ERR(vop->rgb)) {
 			ret = PTR_ERR(vop->rgb);
 			goto err_disable_pm_runtime;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 5971df4302f2..c677b71ae516 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -72,7 +72,8 @@ struct drm_encoder_helper_funcs rockchip_rgb_encoder_helper_funcs = {
 
 struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 				       struct drm_crtc *crtc,
-				       struct drm_device *drm_dev)
+				       struct drm_device *drm_dev,
+				       int video_port)
 {
 	struct rockchip_rgb *rgb;
 	struct drm_encoder *encoder;
@@ -90,7 +91,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 	rgb->dev = dev;
 	rgb->drm_dev = drm_dev;
 
-	port = of_graph_get_port_by_id(dev->of_node, 0);
+	port = of_graph_get_port_by_id(dev->of_node, video_port);
 	if (!port)
 		return ERR_PTR(-EINVAL);
 
@@ -103,8 +104,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 			continue;
 
 		child_count++;
-		ret = drm_of_find_panel_or_bridge(dev->of_node, 0, endpoint_id,
-						  &panel, &bridge);
+		ret = drm_of_find_panel_or_bridge(dev->of_node, video_port,
+						  endpoint_id, &panel, &bridge);
 		if (!ret) {
 			of_node_put(endpoint);
 			break;
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.h b/drivers/gpu/drm/rockchip/rockchip_rgb.h
index 27b9635124bc..1bd4e20e91eb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.h
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.h
@@ -8,12 +8,14 @@
 #ifdef CONFIG_ROCKCHIP_RGB
 struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 				       struct drm_crtc *crtc,
-				       struct drm_device *drm_dev);
+				       struct drm_device *drm_dev,
+				       int video_port);
 void rockchip_rgb_fini(struct rockchip_rgb *rgb);
 #else
 static inline struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 						     struct drm_crtc *crtc,
-						     struct drm_device *drm_dev)
+						     struct drm_device *drm_dev,
+						     int video_port)
 {
 	return NULL;
 }
-- 
2.30.2

[PATCH v3 4/6] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs

[Michael Riesch]

Let the function name vop2_create_crtcs reflect that the function creates
multiple CRTCS. Also, use a symmetric function pair to create and destroy
the CRTCs and the corresponding planes.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - no changes
v2:
 - no changes

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 31 ++++++++++----------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 374ef821b453..06fcdfa7b885 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2246,7 +2246,7 @@ static struct vop2_video_port *find_vp_without_primary(struct vop2 *vop2)
 
 #define NR_LAYERS 6
 
-static int vop2_create_crtc(struct vop2 *vop2)
+static int vop2_create_crtcs(struct vop2 *vop2)
 {
 	const struct vop2_data *vop2_data = vop2->data;
 	struct drm_device *drm = vop2->drm;
@@ -2372,15 +2372,25 @@ static int vop2_create_crtc(struct vop2 *vop2)
 	return 0;
 }
 
-static void vop2_destroy_crtc(struct drm_crtc *crtc)
+static void vop2_destroy_crtcs(struct vop2 *vop2)
 {
-	of_node_put(crtc->port);
+	struct drm_device *drm = vop2->drm;
+	struct list_head *crtc_list = &drm->mode_config.crtc_list;
+	struct list_head *plane_list = &drm->mode_config.plane_list;
+	struct drm_crtc *crtc, *tmpc;
+	struct drm_plane *plane, *tmpp;
+
+	list_for_each_entry_safe(plane, tmpp, plane_list, head)
+		drm_plane_cleanup(plane);
 
 	/*
 	 * Destroy CRTC after vop2_plane_destroy() since vop2_disable_plane()
 	 * references the CRTC.
 	 */
-	drm_crtc_cleanup(crtc);
+	list_for_each_entry_safe(crtc, tmpc, crtc_list, head) {
+		of_node_put(crtc->port);
+		drm_crtc_cleanup(crtc);
+	}
 }
 
 static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
@@ -2684,7 +2694,7 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	ret = vop2_create_crtc(vop2);
+	ret = vop2_create_crtcs(vop2);
 	if (ret)
 		return ret;
 
@@ -2698,19 +2708,10 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 static void vop2_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct vop2 *vop2 = dev_get_drvdata(dev);
-	struct drm_device *drm = vop2->drm;
-	struct list_head *plane_list = &drm->mode_config.plane_list;
-	struct list_head *crtc_list = &drm->mode_config.crtc_list;
-	struct drm_crtc *crtc, *tmpc;
-	struct drm_plane *plane, *tmpp;
 
 	pm_runtime_disable(dev);
 
-	list_for_each_entry_safe(plane, tmpp, plane_list, head)
-		drm_plane_cleanup(plane);
-
-	list_for_each_entry_safe(crtc, tmpc, crtc_list, head)
-		vop2_destroy_crtc(crtc);
+	vop2_destroy_crtcs(vop2);
 }
 
 const struct component_ops vop2_component_ops = {
-- 
2.30.2

[PATCH v3 5/6] drm/rockchip: vop2: add support for the rgb output block

[Michael Riesch]

The Rockchip VOP2 features an internal RGB output block, which can be
attached any video port of the VOP2. Add support for this output block.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - fix commit messages (still assumed video port 2)
 - fix condition to make 0 a valid video port
v2:
 - move away from wrong assumption that the RGB block is always
   connected to video port 2 -> check devicetree to find RGB block

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44 ++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 06fcdfa7b885..f38ffd0ccd9f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -39,6 +39,7 @@
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_vop2.h"
+#include "rockchip_rgb.h"
 
 /*
  * VOP2 architecture
@@ -212,6 +213,9 @@ struct vop2 {
 	struct clk *hclk;
 	struct clk *aclk;
 
+	/* optional internal rgb encoder */
+	struct rockchip_rgb *rgb;
+
 	/* must be put at the end of the struct */
 	struct vop2_win win[];
 };
@@ -2393,6 +2397,25 @@ static void vop2_destroy_crtcs(struct vop2 *vop2)
 	}
 }
 
+static int vop2_find_rgb_encoder(struct vop2 *vop2)
+{
+	struct device_node *node = vop2->dev->of_node;
+	struct device_node *endpoint;
+	int i;
+
+	for (i = 0; i < vop2->data->nr_vps; i++) {
+		endpoint = of_graph_get_endpoint_by_regs(node, i,
+							 ROCKCHIP_VOP2_EP_RGB0);
+		if (!endpoint)
+			continue;
+
+		of_node_put(endpoint);
+		return i;
+	}
+
+	return -ENOENT;
+}
+
 static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
 	[VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
 	[VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
@@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
+	ret = vop2_find_rgb_encoder(vop2);
+	if (ret >= 0) {
+		vop2->rgb = rockchip_rgb_init(dev, &vop2->vps[ret].crtc,
+					      vop2->drm, ret);
+		if (IS_ERR(vop2->rgb)) {
+			if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
+				ret = PTR_ERR(vop2->rgb);
+				goto err_crtcs;
+			}
+			vop2->rgb = NULL;
+		}
+	}
+
 	rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
 
 	pm_runtime_enable(&pdev->dev);
 
 	return 0;
+
+err_crtcs:
+	vop2_destroy_crtcs(vop2);
+
+	return ret;
 }
 
 static void vop2_unbind(struct device *dev, struct device *master, void *data)
@@ -2711,6 +2752,9 @@ static void vop2_unbind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_disable(dev);
 
+	if (vop2->rgb)
+		rockchip_rgb_fini(vop2->rgb);
+
 	vop2_destroy_crtcs(vop2);
 }
 
-- 
2.30.2

[PATCH v3 6/6] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x

[Michael Riesch]

The rk3568-pinctrl.dtsi only defines the 24-bit RGB interface. Add separate
nodes for the 16-bit and 18-bit version, respectively. While at it, split
off the clock/sync signals from the data signals.

The exact mapping of the data pins was discussed here:
https://lore.kernel.org/linux-rockchip/f33a0488-528c-99de-3279-3c0346a03fd6@wolfvision.net/T/

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
v3:
 - no changes
v2:
 - no changes

 .../boot/dts/rockchip/rk3568-pinctrl.dtsi     | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
index 8f90c66dd9e9..0a979bfb63d9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi
@@ -3117,4 +3117,98 @@ tsadc_pin: tsadc-pin {
 				<0 RK_PA1 0 &pcfg_pull_none>;
 		};
 	};
+
+	lcdc {
+		/omit-if-no-ref/
+		lcdc_clock: lcdc-clock {
+			rockchip,pins =
+				/* lcdc_clk */
+				<3 RK_PA0 1 &pcfg_pull_none>,
+				/* lcdc_den */
+				<3 RK_PC3 1 &pcfg_pull_none>,
+				/* lcdc_hsync */
+				<3 RK_PC1 1 &pcfg_pull_none>,
+				/* lcdc_vsync */
+				<3 RK_PC2 1 &pcfg_pull_none>;
+		};
+
+		/omit-if-no-ref/
+		lcdc_data16: lcdc-data16 {
+			rockchip,pins =
+				/* lcdc_d3 */
+				<2 RK_PD3 1 &pcfg_pull_none>,
+				/* lcdc_d4 */
+				<2 RK_PD4 1 &pcfg_pull_none>,
+				/* lcdc_d5 */
+				<2 RK_PD5 1 &pcfg_pull_none>,
+				/* lcdc_d6 */
+				<2 RK_PD6 1 &pcfg_pull_none>,
+				/* lcdc_d7 */
+				<2 RK_PD7 1 &pcfg_pull_none>,
+				/* lcdc_d10 */
+				<3 RK_PA3 1 &pcfg_pull_none>,
+				/* lcdc_d11 */
+				<3 RK_PA4 1 &pcfg_pull_none>,
+				/* lcdc_d12 */
+				<3 RK_PA5 1 &pcfg_pull_none>,
+				/* lcdc_d13 */
+				<3 RK_PA6 1 &pcfg_pull_none>,
+				/* lcdc_d14 */
+				<3 RK_PA7 1 &pcfg_pull_none>,
+				/* lcdc_d15 */
+				<3 RK_PB0 1 &pcfg_pull_none>,
+				/* lcdc_d19 */
+				<3 RK_PB4 1 &pcfg_pull_none>,
+				/* lcdc_d20 */
+				<3 RK_PB5 1 &pcfg_pull_none>,
+				/* lcdc_d21 */
+				<3 RK_PB6 1 &pcfg_pull_none>,
+				/* lcdc_d22 */
+				<3 RK_PB7 1 &pcfg_pull_none>,
+				/* lcdc_d23 */
+				<3 RK_PC0 1 &pcfg_pull_none>;
+		};
+
+		/omit-if-no-ref/
+		lcdc_data18: lcdc-data18 {
+			rockchip,pins =
+				/* lcdc_d2 */
+				<2 RK_PD2 1 &pcfg_pull_none>,
+				/* lcdc_d3 */
+				<2 RK_PD3 1 &pcfg_pull_none>,
+				/* lcdc_d4 */
+				<2 RK_PD4 1 &pcfg_pull_none>,
+				/* lcdc_d5 */
+				<2 RK_PD5 1 &pcfg_pull_none>,
+				/* lcdc_d6 */
+				<2 RK_PD6 1 &pcfg_pull_none>,
+				/* lcdc_d7 */
+				<2 RK_PD7 1 &pcfg_pull_none>,
+				/* lcdc_d10 */
+				<3 RK_PA3 1 &pcfg_pull_none>,
+				/* lcdc_d11 */
+				<3 RK_PA4 1 &pcfg_pull_none>,
+				/* lcdc_d12 */
+				<3 RK_PA5 1 &pcfg_pull_none>,
+				/* lcdc_d13 */
+				<3 RK_PA6 1 &pcfg_pull_none>,
+				/* lcdc_d14 */
+				<3 RK_PA7 1 &pcfg_pull_none>,
+				/* lcdc_d15 */
+				<3 RK_PB0 1 &pcfg_pull_none>,
+				/* lcdc_d18 */
+				<3 RK_PB3 1 &pcfg_pull_none>,
+				/* lcdc_d19 */
+				<3 RK_PB4 1 &pcfg_pull_none>,
+				/* lcdc_d20 */
+				<3 RK_PB5 1 &pcfg_pull_none>,
+				/* lcdc_d21 */
+				<3 RK_PB6 1 &pcfg_pull_none>,
+				/* lcdc_d22 */
+				<3 RK_PB7 1 &pcfg_pull_none>,
+				/* lcdc_d23 */
+				<3 RK_PC0 1 &pcfg_pull_none>;
+		};
+	};
+
 };
-- 
2.30.2

Re: [PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block

[Sascha Hauer]

On Tue, Jan 24, 2023 at 06:47:00AM +0100, Michael Riesch wrote:
> Hi all,
> 
> This series adds support for the RGB output block that can be found in the
> Rockchip Video Output Processor (VOP) 2. Version 2 of this series
> incorporates the feedback by Dan Carpenter and Sascha Hauer. Version 3
> fixes a dumb mistake pointed out by Sascha :-) Thanks for your comments!
> 
> Patches 1-4 clean up the code and make it more general.
> 
> Patch 5 activates the support for the RGB output block in the VOP2 driver.
> 
> Patch 6 adds pinctrls for the 16-bit and 18-bit RGB data lines.
> 
> Tested on a custom board featuring the RK3568 SoC with a 18-bit RGB
> display.
> 
> Looking forward to your comments!

For the series:

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 5/6] drm/rockchip: vop2: add support for the rgb output block

[Michael Riesch]

On 1/24/23 06:47, Michael Riesch wrote:
> The Rockchip VOP2 features an internal RGB output block, which can be
> attached any video port of the VOP2. Add support for this output block.

s/attached any/attached to any/ of course. Can this be fixed when the
patch is applied?

Michael

> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> v3:
>  - fix commit messages (still assumed video port 2)
>  - fix condition to make 0 a valid video port
> v2:
>  - move away from wrong assumption that the RGB block is always
>    connected to video port 2 -> check devicetree to find RGB block
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 44 ++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 06fcdfa7b885..f38ffd0ccd9f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -39,6 +39,7 @@
>  #include "rockchip_drm_gem.h"
>  #include "rockchip_drm_fb.h"
>  #include "rockchip_drm_vop2.h"
> +#include "rockchip_rgb.h"
>  
>  /*
>   * VOP2 architecture
> @@ -212,6 +213,9 @@ struct vop2 {
>  	struct clk *hclk;
>  	struct clk *aclk;
>  
> +	/* optional internal rgb encoder */
> +	struct rockchip_rgb *rgb;
> +
>  	/* must be put at the end of the struct */
>  	struct vop2_win win[];
>  };
> @@ -2393,6 +2397,25 @@ static void vop2_destroy_crtcs(struct vop2 *vop2)
>  	}
>  }
>  
> +static int vop2_find_rgb_encoder(struct vop2 *vop2)
> +{
> +	struct device_node *node = vop2->dev->of_node;
> +	struct device_node *endpoint;
> +	int i;
> +
> +	for (i = 0; i < vop2->data->nr_vps; i++) {
> +		endpoint = of_graph_get_endpoint_by_regs(node, i,
> +							 ROCKCHIP_VOP2_EP_RGB0);
> +		if (!endpoint)
> +			continue;
> +
> +		of_node_put(endpoint);
> +		return i;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static struct reg_field vop2_cluster_regs[VOP2_WIN_MAX_REG] = {
>  	[VOP2_WIN_ENABLE] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 0, 0),
>  	[VOP2_WIN_FORMAT] = REG_FIELD(RK3568_CLUSTER_WIN_CTRL0, 1, 5),
> @@ -2698,11 +2721,29 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> +	ret = vop2_find_rgb_encoder(vop2);
> +	if (ret >= 0) {
> +		vop2->rgb = rockchip_rgb_init(dev, &vop2->vps[ret].crtc,
> +					      vop2->drm, ret);
> +		if (IS_ERR(vop2->rgb)) {
> +			if (PTR_ERR(vop2->rgb) == -EPROBE_DEFER) {
> +				ret = PTR_ERR(vop2->rgb);
> +				goto err_crtcs;
> +			}
> +			vop2->rgb = NULL;
> +		}
> +	}
> +
>  	rockchip_drm_dma_init_device(vop2->drm, vop2->dev);
>  
>  	pm_runtime_enable(&pdev->dev);
>  
>  	return 0;
> +
> +err_crtcs:
> +	vop2_destroy_crtcs(vop2);
> +
> +	return ret;
>  }
>  
>  static void vop2_unbind(struct device *dev, struct device *master, void *data)
> @@ -2711,6 +2752,9 @@ static void vop2_unbind(struct device *dev, struct device *master, void *data)
>  
>  	pm_runtime_disable(dev);
>  
> +	if (vop2->rgb)
> +		rockchip_rgb_fini(vop2->rgb);
> +
>  	vop2_destroy_crtcs(vop2);
>  }
>  

Re: (subset) [PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block

[Heiko Stuebner]

On Tue, 24 Jan 2023 06:47:00 +0100, Michael Riesch wrote:
> This series adds support for the RGB output block that can be found in the
> Rockchip Video Output Processor (VOP) 2. Version 2 of this series
> incorporates the feedback by Dan Carpenter and Sascha Hauer. Version 3
> fixes a dumb mistake pointed out by Sascha :-) Thanks for your comments!
> 
> Patches 1-4 clean up the code and make it more general.
> 
> [...]

Applied, thanks!

[6/6] arm64: dts: rockchip: add pinctrls for 16-bit/18-bit rgb interface to rk356x
      commit: 381b6d432f6eb00e1faff763f55e67519af9fa23

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: (subset) [PATCH v3 0/6] drm/rockchip: vop2: add support for the rgb output block

[Heiko Stuebner]

On Tue, 24 Jan 2023 06:47:00 +0100, Michael Riesch wrote:
> This series adds support for the RGB output block that can be found in the
> Rockchip Video Output Processor (VOP) 2. Version 2 of this series
> incorporates the feedback by Dan Carpenter and Sascha Hauer. Version 3
> fixes a dumb mistake pointed out by Sascha :-) Thanks for your comments!
> 
> Patches 1-4 clean up the code and make it more general.
> 
> [...]

Applied, thanks!

[1/6] drm/rockchip: vop2: initialize possible_crtcs properly
      commit: 368419a2d429e2438bef333959732c640310bdc7
[2/6] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder
      commit: f8a852f1f86391127ab57b1c41fe0e62bc14f27c
[3/6] drm/rockchip: rgb: add video_port parameter to init function
      commit: 03db8f25cf16c579fe75fd2230bbe64c221bfe25
[4/6] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs
      commit: cddddc066b056e7bb629a8c4d99c9c4a8ca70a6a
[5/6] drm/rockchip: vop2: add support for the rgb output block
      commit: c66c6d7c47058a72a00b50d7f5c4538e3fa49b1c

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly

[Nathan Chancellor]

Hi Michael,

On Tue, Jan 24, 2023 at 06:47:01AM +0100, Michael Riesch wrote:
> The variable possible_crtcs is only initialized for primary and
> overlay planes. Since the VOP2 driver only supports these plane
> types at the moment, the current code is safe. However, in order
> to provide a future-proof solution, fix the initialization of
> the variable.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
> v3:
>  - no changes
> v2:
>  - new patch
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 8cecf81a5ae0..374ef821b453 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
>  				/* change the unused primary window to overlay window */
>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>  			}
> -		}
> -
> -		if (win->type == DRM_PLANE_TYPE_OVERLAY)
> +		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>  			possible_crtcs = (1 << nvps) - 1;
> +		} else {
> +			possible_crtcs = 0;
> +		}
>  
>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>  		if (ret) {
> -- 
> 2.30.2
> 

This patch is now in -next as commit 368419a2d429 ("drm/rockchip: vop2:
initialize possible_crtcs properly") and it actually appears to
introduce a path where possible_crtcs could be used uninitialized.

  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:8: error: variable 'possible_crtcs' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
                          if (vp) {
                              ^~
  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330:36: note: uninitialized use occurs here
                  ret = vop2_plane_init(vop2, win, possible_crtcs);
                                                   ^~~~~~~~~~~~~~
  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:4: note: remove the 'if' if its condition is always true
                          if (vp) {
                          ^~~~~~~~
  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2298:21: note: initialize the variable 'possible_crtcs' to silence this warning
                  u32 possible_crtcs;
                                    ^
                                     = 0
  1 error generated.

Prior to this change, if that else path was hit, clang recognized based on
the assignment that the next if statement would always be true. Now, if
the else path is taken, the possible_crtcs assignment will be missed. Is
that intentional?

Cheers,
Nathan

Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs properly

[Michael Riesch]

Hi Nathan,

On 3/14/23 17:08, Nathan Chancellor wrote:
> Hi Michael,
> 
> On Tue, Jan 24, 2023 at 06:47:01AM +0100, Michael Riesch wrote:
>> The variable possible_crtcs is only initialized for primary and
>> overlay planes. Since the VOP2 driver only supports these plane
>> types at the moment, the current code is safe. However, in order
>> to provide a future-proof solution, fix the initialization of
>> the variable.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <error27@gmail.com>
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>> v3:
>>  - no changes
>> v2:
>>  - new patch
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 8cecf81a5ae0..374ef821b453 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
>>  				/* change the unused primary window to overlay window */
>>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>>  			}
>> -		}
>> -
>> -		if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>>  			possible_crtcs = (1 << nvps) - 1;
>> +		} else {
>> +			possible_crtcs = 0;
>> +		}
>>  
>>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>>  		if (ret) {
>> -- 
>> 2.30.2
>>
> 
> This patch is now in -next as commit 368419a2d429 ("drm/rockchip: vop2:
> initialize possible_crtcs properly") and it actually appears to
> introduce a path where possible_crtcs could be used uninitialized.
> 
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:8: error: variable 'possible_crtcs' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>                           if (vp) {
>                               ^~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330:36: note: uninitialized use occurs here
>                   ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                    ^~~~~~~~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:4: note: remove the 'if' if its condition is always true
>                           if (vp) {
>                           ^~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2298:21: note: initialize the variable 'possible_crtcs' to silence this warning
>                   u32 possible_crtcs;
>                                     ^
>                                      = 0
>   1 error generated.
> 
> Prior to this change, if that else path was hit, clang recognized based on
> the assignment that the next if statement would always be true. Now, if
> the else path is taken, the possible_crtcs assignment will be missed. Is
> that intentional?

As it turns out, the approach in my patch does not cover all paths. I'll
submit a follow-up patch that initializes possible_crtcs = 0 and drops
the else path. This should solve the issue for good.

Regards, Michael