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

[PATCH 0/5] 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.

Patches 1-2 prepare the RGB part, which has been used only with the VOP(1)
so far.

Patch 3 is a cleanup patch and aims to make the creation and destruction of
the CRTCs and planes more readable.

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

Patch 5 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 (5):
  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  | 50 +++++++---
 drivers/gpu/drm/rockchip/rockchip_rgb.c       | 21 +++--
 drivers/gpu/drm/rockchip/rockchip_rgb.h       |  6 +-
 5 files changed, 147 insertions(+), 26 deletions(-)


base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
-- 
2.30.2

[PATCH 1/5] 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>
---
 drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index 75eb7cca3d82..16201a5cf1e8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -18,17 +18,17 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
+#include <dt-bindings/soc/rockchip,vop2.h>
+
 #include "rockchip_drm_drv.h"
 #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 +125,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 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
 		goto err_free_encoder;
 	}
 
+	rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;
+
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret < 0) {
 		DRM_DEV_ERROR(drm_dev->dev,
@@ -182,6 +184,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 2/5] 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>
---
 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 c356de5dd220..f7335f9cac73 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 16201a5cf1e8..ed6ccd1db465 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -74,7 +74,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;
@@ -92,7 +93,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);
 
@@ -105,8 +106,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 3/5] 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>
---
 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 105a548d0abe..94fddbf70ff6 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;
@@ -2371,15 +2371,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] = {
@@ -2683,7 +2693,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;
 
@@ -2697,19 +2707,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 4/5] 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 to the video port 2 of the VOP2. Add support for this output
block.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 94fddbf70ff6..16041c79d228 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[];
 };
@@ -2697,11 +2701,25 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
+	vop2->rgb = rockchip_rgb_init(dev, &vop2->vps[2].crtc, vop2->drm, 2);
+	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)
@@ -2710,6 +2728,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 5/5] 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>
---
 .../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 1/5] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder

[Sascha Hauer]

On Wed, Nov 30, 2022 at 03:02:13PM +0100, Michael Riesch wrote:
> 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>
> ---
>  drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> index 75eb7cca3d82..16201a5cf1e8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> @@ -18,17 +18,17 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +
>  #include "rockchip_drm_drv.h"
>  #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 +125,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 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>  		goto err_free_encoder;
>  	}
>  
> +	rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;

This is vop2 specific. This code is used with the vop1 as well, so it
doesn't look like it belongs here, at least not hidden in a patch titled
"embed drm_encoder into rockchip_encoder".

Normally the crtc_endpoint_id is set by the encoder, coming from the
encoder node, asking the question "To which port on the VOP am I
connected to?"

Here the situation is different. We are called from the VOP and the
question instead is: "Is there something connected to VPx endpoint id
ROCKCHIP_VOP2_EP_RGB0?"

You might need a vop2 specific entry to this code.

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 4/5] drm/rockchip: vop2: add support for the rgb output block

[Sascha Hauer]

On Wed, Nov 30, 2022 at 03:02:16PM +0100, Michael Riesch wrote:
> The Rockchip VOP2 features an internal RGB output block, which can be
> attached to the video port 2 of the VOP2. Add support for this output
> block.
> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 21 ++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 94fddbf70ff6..16041c79d228 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[];
>  };
> @@ -2697,11 +2701,25 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> +	vop2->rgb = rockchip_rgb_init(dev, &vop2->vps[2].crtc, vop2->drm, 2);

Here you assume that the RGB output can only be connected to VP2, but it
could be connected to any other VP as well, and we can find the
description where it actually shall be connected in the device tree.

As mentioned in my comment to patch 1, the question is "Is there
something connected to VPx at endpoint ROCKCHIP_VOP2_EP_RGB0?"

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 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs

[Dan Carpenter]

Hi Michael,

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Riesch/drm-rockchip-vop2-add-support-for-the-rgb-output-block/20221130-220346
base:   b7b275e60bcd5f89771e865a8239325f86d9927d
patch link:    https://lore.kernel.org/r/20221130140217.3196414-4-michael.riesch%40wolfvision.net
patch subject: [PATCH 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs
config: parisc-randconfig-m031-20221225
compiler: hppa-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

New smatch warnings:
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330 vop2_create_crtcs() error: uninitialized symbol 'possible_crtcs'.

vim +/possible_crtcs +2330 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

fb83276e59f2d6 Michael Riesch 2022-11-30  2249  static int vop2_create_crtcs(struct vop2 *vop2)
604be85547ce4d Andy Yan       2022-04-22  2250  {
604be85547ce4d Andy Yan       2022-04-22  2251  	const struct vop2_data *vop2_data = vop2->data;
604be85547ce4d Andy Yan       2022-04-22  2252  	struct drm_device *drm = vop2->drm;
604be85547ce4d Andy Yan       2022-04-22  2253  	struct device *dev = vop2->dev;
604be85547ce4d Andy Yan       2022-04-22  2254  	struct drm_plane *plane;
604be85547ce4d Andy Yan       2022-04-22  2255  	struct device_node *port;
604be85547ce4d Andy Yan       2022-04-22  2256  	struct vop2_video_port *vp;
604be85547ce4d Andy Yan       2022-04-22  2257  	int i, nvp, nvps = 0;
604be85547ce4d Andy Yan       2022-04-22  2258  	int ret;
604be85547ce4d Andy Yan       2022-04-22  2259  
604be85547ce4d Andy Yan       2022-04-22  2260  	for (i = 0; i < vop2_data->nr_vps; i++) {
604be85547ce4d Andy Yan       2022-04-22  2261  		const struct vop2_video_port_data *vp_data;
604be85547ce4d Andy Yan       2022-04-22  2262  		struct device_node *np;
604be85547ce4d Andy Yan       2022-04-22  2263  		char dclk_name[9];
604be85547ce4d Andy Yan       2022-04-22  2264  
604be85547ce4d Andy Yan       2022-04-22  2265  		vp_data = &vop2_data->vp[i];
604be85547ce4d Andy Yan       2022-04-22  2266  		vp = &vop2->vps[i];
604be85547ce4d Andy Yan       2022-04-22  2267  		vp->vop2 = vop2;
604be85547ce4d Andy Yan       2022-04-22  2268  		vp->id = vp_data->id;
604be85547ce4d Andy Yan       2022-04-22  2269  		vp->regs = vp_data->regs;
604be85547ce4d Andy Yan       2022-04-22  2270  		vp->data = vp_data;
604be85547ce4d Andy Yan       2022-04-22  2271  
604be85547ce4d Andy Yan       2022-04-22  2272  		snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp->id);
604be85547ce4d Andy Yan       2022-04-22  2273  		vp->dclk = devm_clk_get(vop2->dev, dclk_name);
604be85547ce4d Andy Yan       2022-04-22  2274  		if (IS_ERR(vp->dclk)) {
604be85547ce4d Andy Yan       2022-04-22  2275  			drm_err(vop2->drm, "failed to get %s\n", dclk_name);
604be85547ce4d Andy Yan       2022-04-22  2276  			return PTR_ERR(vp->dclk);
604be85547ce4d Andy Yan       2022-04-22  2277  		}
604be85547ce4d Andy Yan       2022-04-22  2278  
604be85547ce4d Andy Yan       2022-04-22  2279  		np = of_graph_get_remote_node(dev->of_node, i, -1);
604be85547ce4d Andy Yan       2022-04-22  2280  		if (!np) {
604be85547ce4d Andy Yan       2022-04-22  2281  			drm_dbg(vop2->drm, "%s: No remote for vp%d\n", __func__, i);
604be85547ce4d Andy Yan       2022-04-22  2282  			continue;
604be85547ce4d Andy Yan       2022-04-22  2283  		}
604be85547ce4d Andy Yan       2022-04-22  2284  		of_node_put(np);
604be85547ce4d Andy Yan       2022-04-22  2285  
604be85547ce4d Andy Yan       2022-04-22  2286  		port = of_graph_get_port_by_id(dev->of_node, i);
604be85547ce4d Andy Yan       2022-04-22  2287  		if (!port) {
604be85547ce4d Andy Yan       2022-04-22  2288  			drm_err(vop2->drm, "no port node found for video_port%d\n", i);
604be85547ce4d Andy Yan       2022-04-22  2289  			return -ENOENT;
604be85547ce4d Andy Yan       2022-04-22  2290  		}
604be85547ce4d Andy Yan       2022-04-22  2291  
604be85547ce4d Andy Yan       2022-04-22  2292  		vp->crtc.port = port;
604be85547ce4d Andy Yan       2022-04-22  2293  		nvps++;
604be85547ce4d Andy Yan       2022-04-22  2294  	}
604be85547ce4d Andy Yan       2022-04-22  2295  
604be85547ce4d Andy Yan       2022-04-22  2296  	nvp = 0;
604be85547ce4d Andy Yan       2022-04-22  2297  	for (i = 0; i < vop2->registered_num_wins; i++) {
604be85547ce4d Andy Yan       2022-04-22  2298  		struct vop2_win *win = &vop2->win[i];
604be85547ce4d Andy Yan       2022-04-22  2299  		u32 possible_crtcs;
604be85547ce4d Andy Yan       2022-04-22  2300  
604be85547ce4d Andy Yan       2022-04-22  2301  		if (vop2->data->soc_id == 3566) {
604be85547ce4d Andy Yan       2022-04-22  2302  			/*
604be85547ce4d Andy Yan       2022-04-22  2303  			 * On RK3566 these windows don't have an independent
604be85547ce4d Andy Yan       2022-04-22  2304  			 * framebuffer. They share the framebuffer with smart0,
604be85547ce4d Andy Yan       2022-04-22  2305  			 * esmart0 and cluster0 respectively.
604be85547ce4d Andy Yan       2022-04-22  2306  			 */
604be85547ce4d Andy Yan       2022-04-22  2307  			switch (win->data->phys_id) {
604be85547ce4d Andy Yan       2022-04-22  2308  			case ROCKCHIP_VOP2_SMART1:
604be85547ce4d Andy Yan       2022-04-22  2309  			case ROCKCHIP_VOP2_ESMART1:
604be85547ce4d Andy Yan       2022-04-22  2310  			case ROCKCHIP_VOP2_CLUSTER1:
604be85547ce4d Andy Yan       2022-04-22  2311  				continue;
604be85547ce4d Andy Yan       2022-04-22  2312  			}
604be85547ce4d Andy Yan       2022-04-22  2313  		}
604be85547ce4d Andy Yan       2022-04-22  2314  
604be85547ce4d Andy Yan       2022-04-22  2315  		if (win->type == DRM_PLANE_TYPE_PRIMARY) {
604be85547ce4d Andy Yan       2022-04-22  2316  			vp = find_vp_without_primary(vop2);
604be85547ce4d Andy Yan       2022-04-22  2317  			if (vp) {
604be85547ce4d Andy Yan       2022-04-22  2318  				possible_crtcs = BIT(nvp);
604be85547ce4d Andy Yan       2022-04-22  2319  				vp->primary_plane = win;
604be85547ce4d Andy Yan       2022-04-22  2320  				nvp++;
604be85547ce4d Andy Yan       2022-04-22  2321  			} else {
604be85547ce4d Andy Yan       2022-04-22  2322  				/* change the unused primary window to overlay window */
604be85547ce4d Andy Yan       2022-04-22  2323  				win->type = DRM_PLANE_TYPE_OVERLAY;
604be85547ce4d Andy Yan       2022-04-22  2324  			}
604be85547ce4d Andy Yan       2022-04-22  2325  		}
604be85547ce4d Andy Yan       2022-04-22  2326  
604be85547ce4d Andy Yan       2022-04-22  2327  		if (win->type == DRM_PLANE_TYPE_OVERLAY)
604be85547ce4d Andy Yan       2022-04-22  2328  			possible_crtcs = (1 << nvps) - 1;

What about if win->type is not equal to either DRM_PLANE_TYPE_PRIMARY or
DRM_PLANE_TYPE_OVERLAY?  That's what the checker is worried about.

604be85547ce4d Andy Yan       2022-04-22  2329  
604be85547ce4d Andy Yan       2022-04-22 @2330  		ret = vop2_plane_init(vop2, win, possible_crtcs);
                                                                                                 ^^^^^^^^^^^^^^

604be85547ce4d Andy Yan       2022-04-22  2331  		if (ret) {
604be85547ce4d Andy Yan       2022-04-22  2332  			drm_err(vop2->drm, "failed to init plane %s: %d\n",
604be85547ce4d Andy Yan       2022-04-22  2333  				win->data->name, ret);
604be85547ce4d Andy Yan       2022-04-22  2334  			return ret;
604be85547ce4d Andy Yan       2022-04-22  2335  		}
604be85547ce4d Andy Yan       2022-04-22  2336  	}
604be85547ce4d Andy Yan       2022-04-22  2337  
604be85547ce4d Andy Yan       2022-04-22  2338  	for (i = 0; i < vop2_data->nr_vps; i++) {

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs

[Michael Riesch]

Hi Dan,

On 1/3/23 09:07, Dan Carpenter wrote:
> Hi Michael,
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Riesch/drm-rockchip-vop2-add-support-for-the-rgb-output-block/20221130-220346
> base:   b7b275e60bcd5f89771e865a8239325f86d9927d
> patch link:    https://lore.kernel.org/r/20221130140217.3196414-4-michael.riesch%40wolfvision.net
> patch subject: [PATCH 3/5] drm/rockchip: vop2: use symmetric function pair vop2_{create,destroy}_crtcs
> config: parisc-randconfig-m031-20221225
> compiler: hppa-linux-gcc (GCC) 12.1.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>

Thanks for the report.

> New smatch warnings:
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330 vop2_create_crtcs() error: uninitialized symbol 'possible_crtcs'.
> 
> vim +/possible_crtcs +2330 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> 
> fb83276e59f2d6 Michael Riesch 2022-11-30  2249  static int vop2_create_crtcs(struct vop2 *vop2)
> 604be85547ce4d Andy Yan       2022-04-22  2250  {
> 604be85547ce4d Andy Yan       2022-04-22  2251  	const struct vop2_data *vop2_data = vop2->data;
> 604be85547ce4d Andy Yan       2022-04-22  2252  	struct drm_device *drm = vop2->drm;
> 604be85547ce4d Andy Yan       2022-04-22  2253  	struct device *dev = vop2->dev;
> 604be85547ce4d Andy Yan       2022-04-22  2254  	struct drm_plane *plane;
> 604be85547ce4d Andy Yan       2022-04-22  2255  	struct device_node *port;
> 604be85547ce4d Andy Yan       2022-04-22  2256  	struct vop2_video_port *vp;
> 604be85547ce4d Andy Yan       2022-04-22  2257  	int i, nvp, nvps = 0;
> 604be85547ce4d Andy Yan       2022-04-22  2258  	int ret;
> 604be85547ce4d Andy Yan       2022-04-22  2259  
> 604be85547ce4d Andy Yan       2022-04-22  2260  	for (i = 0; i < vop2_data->nr_vps; i++) {
> 604be85547ce4d Andy Yan       2022-04-22  2261  		const struct vop2_video_port_data *vp_data;
> 604be85547ce4d Andy Yan       2022-04-22  2262  		struct device_node *np;
> 604be85547ce4d Andy Yan       2022-04-22  2263  		char dclk_name[9];
> 604be85547ce4d Andy Yan       2022-04-22  2264  
> 604be85547ce4d Andy Yan       2022-04-22  2265  		vp_data = &vop2_data->vp[i];
> 604be85547ce4d Andy Yan       2022-04-22  2266  		vp = &vop2->vps[i];
> 604be85547ce4d Andy Yan       2022-04-22  2267  		vp->vop2 = vop2;
> 604be85547ce4d Andy Yan       2022-04-22  2268  		vp->id = vp_data->id;
> 604be85547ce4d Andy Yan       2022-04-22  2269  		vp->regs = vp_data->regs;
> 604be85547ce4d Andy Yan       2022-04-22  2270  		vp->data = vp_data;
> 604be85547ce4d Andy Yan       2022-04-22  2271  
> 604be85547ce4d Andy Yan       2022-04-22  2272  		snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp->id);
> 604be85547ce4d Andy Yan       2022-04-22  2273  		vp->dclk = devm_clk_get(vop2->dev, dclk_name);
> 604be85547ce4d Andy Yan       2022-04-22  2274  		if (IS_ERR(vp->dclk)) {
> 604be85547ce4d Andy Yan       2022-04-22  2275  			drm_err(vop2->drm, "failed to get %s\n", dclk_name);
> 604be85547ce4d Andy Yan       2022-04-22  2276  			return PTR_ERR(vp->dclk);
> 604be85547ce4d Andy Yan       2022-04-22  2277  		}
> 604be85547ce4d Andy Yan       2022-04-22  2278  
> 604be85547ce4d Andy Yan       2022-04-22  2279  		np = of_graph_get_remote_node(dev->of_node, i, -1);
> 604be85547ce4d Andy Yan       2022-04-22  2280  		if (!np) {
> 604be85547ce4d Andy Yan       2022-04-22  2281  			drm_dbg(vop2->drm, "%s: No remote for vp%d\n", __func__, i);
> 604be85547ce4d Andy Yan       2022-04-22  2282  			continue;
> 604be85547ce4d Andy Yan       2022-04-22  2283  		}
> 604be85547ce4d Andy Yan       2022-04-22  2284  		of_node_put(np);
> 604be85547ce4d Andy Yan       2022-04-22  2285  
> 604be85547ce4d Andy Yan       2022-04-22  2286  		port = of_graph_get_port_by_id(dev->of_node, i);
> 604be85547ce4d Andy Yan       2022-04-22  2287  		if (!port) {
> 604be85547ce4d Andy Yan       2022-04-22  2288  			drm_err(vop2->drm, "no port node found for video_port%d\n", i);
> 604be85547ce4d Andy Yan       2022-04-22  2289  			return -ENOENT;
> 604be85547ce4d Andy Yan       2022-04-22  2290  		}
> 604be85547ce4d Andy Yan       2022-04-22  2291  
> 604be85547ce4d Andy Yan       2022-04-22  2292  		vp->crtc.port = port;
> 604be85547ce4d Andy Yan       2022-04-22  2293  		nvps++;
> 604be85547ce4d Andy Yan       2022-04-22  2294  	}
> 604be85547ce4d Andy Yan       2022-04-22  2295  
> 604be85547ce4d Andy Yan       2022-04-22  2296  	nvp = 0;
> 604be85547ce4d Andy Yan       2022-04-22  2297  	for (i = 0; i < vop2->registered_num_wins; i++) {
> 604be85547ce4d Andy Yan       2022-04-22  2298  		struct vop2_win *win = &vop2->win[i];
> 604be85547ce4d Andy Yan       2022-04-22  2299  		u32 possible_crtcs;
> 604be85547ce4d Andy Yan       2022-04-22  2300  
> 604be85547ce4d Andy Yan       2022-04-22  2301  		if (vop2->data->soc_id == 3566) {
> 604be85547ce4d Andy Yan       2022-04-22  2302  			/*
> 604be85547ce4d Andy Yan       2022-04-22  2303  			 * On RK3566 these windows don't have an independent
> 604be85547ce4d Andy Yan       2022-04-22  2304  			 * framebuffer. They share the framebuffer with smart0,
> 604be85547ce4d Andy Yan       2022-04-22  2305  			 * esmart0 and cluster0 respectively.
> 604be85547ce4d Andy Yan       2022-04-22  2306  			 */
> 604be85547ce4d Andy Yan       2022-04-22  2307  			switch (win->data->phys_id) {
> 604be85547ce4d Andy Yan       2022-04-22  2308  			case ROCKCHIP_VOP2_SMART1:
> 604be85547ce4d Andy Yan       2022-04-22  2309  			case ROCKCHIP_VOP2_ESMART1:
> 604be85547ce4d Andy Yan       2022-04-22  2310  			case ROCKCHIP_VOP2_CLUSTER1:
> 604be85547ce4d Andy Yan       2022-04-22  2311  				continue;
> 604be85547ce4d Andy Yan       2022-04-22  2312  			}
> 604be85547ce4d Andy Yan       2022-04-22  2313  		}
> 604be85547ce4d Andy Yan       2022-04-22  2314  
> 604be85547ce4d Andy Yan       2022-04-22  2315  		if (win->type == DRM_PLANE_TYPE_PRIMARY) {
> 604be85547ce4d Andy Yan       2022-04-22  2316  			vp = find_vp_without_primary(vop2);
> 604be85547ce4d Andy Yan       2022-04-22  2317  			if (vp) {
> 604be85547ce4d Andy Yan       2022-04-22  2318  				possible_crtcs = BIT(nvp);
> 604be85547ce4d Andy Yan       2022-04-22  2319  				vp->primary_plane = win;
> 604be85547ce4d Andy Yan       2022-04-22  2320  				nvp++;
> 604be85547ce4d Andy Yan       2022-04-22  2321  			} else {
> 604be85547ce4d Andy Yan       2022-04-22  2322  				/* change the unused primary window to overlay window */
> 604be85547ce4d Andy Yan       2022-04-22  2323  				win->type = DRM_PLANE_TYPE_OVERLAY;
> 604be85547ce4d Andy Yan       2022-04-22  2324  			}
> 604be85547ce4d Andy Yan       2022-04-22  2325  		}
> 604be85547ce4d Andy Yan       2022-04-22  2326  
> 604be85547ce4d Andy Yan       2022-04-22  2327  		if (win->type == DRM_PLANE_TYPE_OVERLAY)
> 604be85547ce4d Andy Yan       2022-04-22  2328  			possible_crtcs = (1 << nvps) - 1;
> 
> What about if win->type is not equal to either DRM_PLANE_TYPE_PRIMARY or
> DRM_PLANE_TYPE_OVERLAY?  That's what the checker is worried about.

If I am not mistaken this issue is present in mainline and my series
does neither introduce nor fix it. I can add a patch that sorts the
issue out to the v2 of my series, though.

As of now the driver only supports PRIMARY and OVERLAY planes, therefore
the current state is safe to use. But maybe CURSOR planes are supported
in the future.

Rewriting the two if's into a if/else structure and setting the
possible_crtcs variable to zero in the else case should do the trick. If
there aren't any objections to that approach, I'll spin a patch.

Best regards,
Michael

> 
> 604be85547ce4d Andy Yan       2022-04-22  2329  
> 604be85547ce4d Andy Yan       2022-04-22 @2330  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                                                                  ^^^^^^^^^^^^^^
> 
> 604be85547ce4d Andy Yan       2022-04-22  2331  		if (ret) {
> 604be85547ce4d Andy Yan       2022-04-22  2332  			drm_err(vop2->drm, "failed to init plane %s: %d\n",
> 604be85547ce4d Andy Yan       2022-04-22  2333  				win->data->name, ret);
> 604be85547ce4d Andy Yan       2022-04-22  2334  			return ret;
> 604be85547ce4d Andy Yan       2022-04-22  2335  		}
> 604be85547ce4d Andy Yan       2022-04-22  2336  	}
> 604be85547ce4d Andy Yan       2022-04-22  2337  
> 604be85547ce4d Andy Yan       2022-04-22  2338  	for (i = 0; i < vop2_data->nr_vps; i++) {
> 

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

[Michael Riesch]

Hi Heiko,

On 11/30/22 15:02, Michael Riesch wrote:
> 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>

This patch is somewhat independent of the other patches of the series.
In particular it is not affected by the comments that this series has
received so far. If there are no objections, you might consider applying it.

Thanks and best regards,
Michael

> ---
>  .../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>;
> +		};
> +	};
> +
>  };

Re: [PATCH 1/5] drm/rockchip: rgb: embed drm_encoder into rockchip_encoder

[Michael Riesch]

Hi Sascha,

Thanks for your comments!

On 12/7/22 07:45, Sascha Hauer wrote:
> On Wed, Nov 30, 2022 at 03:02:13PM +0100, Michael Riesch wrote:
>> 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>
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_rgb.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> index 75eb7cca3d82..16201a5cf1e8 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
>> @@ -18,17 +18,17 @@
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_simple_kms_helper.h>
>>  
>> +#include <dt-bindings/soc/rockchip,vop2.h>
>> +
>>  #include "rockchip_drm_drv.h"
>>  #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 +125,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 +161,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>>  		goto err_free_encoder;
>>  	}
>>  
>> +	rgb->encoder.crtc_endpoint_id = ROCKCHIP_VOP2_EP_RGB0;
> 
> This is vop2 specific. This code is used with the vop1 as well, so it
> doesn't look like it belongs here, at least not hidden in a patch titled
> "embed drm_encoder into rockchip_encoder".

OK, the very least I can do is to create an extra patch that sets the
crtc_endpoint_id.

> Normally the crtc_endpoint_id is set by the encoder, coming from the
> encoder node, asking the question "To which port on the VOP am I
> connected to?"
> 
> Here the situation is different. We are called from the VOP and the
> question instead is: "Is there something connected to VPx endpoint id
> ROCKCHIP_VOP2_EP_RGB0?"
> 
> You might need a vop2 specific entry to this code.

I just realized that rockchip_rgb_init parses the endpoint ID. If I am
not mistaken, we can directly store it in the crtc_endpoint_id member.

Seeing that this would be pretty generic, can it be included in one
patch (or should I still split this into a separate patch)?

Best regards,
Michael