[PATCH v2 00/12] Add VOP2 support on rk3588

[PATCH v2 00/12] Add VOP2 support on rk3588

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

This patch sets aims at enable the VOP2 support on rk3588.

Main feature of VOP2 on rk3588:
Four video ports:
VP0 Max 4096x2160
VP1 Max 4096x2160
VP2 Max 4096x2160
VP3 Max 2048x1080

4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
4 4K Esmart windows with line RGB/YUV support

The current version support all the 8 windows with all the suppported
plane format.

And we don't have a upstreamed encoder/connector(HDMI/DP) for rk3588
yet, Cristian from collabora is working on adding upstream support for
HDMI on rk3588.

My current test is runing with a HDMI driver pick from downstream bsp kernel.

A branch based on linux-6.7 rc2 containing all the series and
HDMI driver(not compatible with mainline rk3568 hdmi) picked from downstream bsp kernel
is available [0].

[0]https://github.com/andyshrk/linux/tree/rk3588-vop2-upstream-linux-6.7-rc2-2023-11-22

Changes in v2:
- fix errors when running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
- split dt-bindings form vop driver patch
- add rk3588_ prefix for functions which are rk3588 only
- make some calculation as fixed value and keep calculation formula as
  comment
- check return value for some cru calculation functions.
- check return value for syscon_regmap_lookup_by_phandle
- add NV20/NV30 for esmart plane

Andy Yan (12):
  drm/rockchip: move output interface related definition to
    rockchip_drm_drv.h
  Revert "drm/rockchip: vop2: Use regcache_sync() to fix suspend/resume"
  drm/rockchip: vop2: set half_block_en bit in all mode
  drm/rockchip: vop2: clear afbc en and transform bit for cluster window
    at linear mode
  drm/rockchip: vop2: Set YUV/RGB overlay mode
  drm/rockchip: vop2: rename grf to sys_grf
  dt-bindings: soc: rockchip: add rk3588 vop/vo syscon
  dt-bindings: display: vop2: Add rk3588 support
  dt-bindings: soc: vop2: Add more endpoint definition
  drm/rockchip: vop2: Add support for rk3588
  drm/rockchip: vop2: Add debugfs support
  arm64: dts: rockchip: Add vop on rk3588

 .../display/rockchip/rockchip-vop2.yaml       |  27 +
 .../devicetree/bindings/soc/rockchip/grf.yaml |   2 +
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     |  96 ++
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |   1 -
 drivers/gpu/drm/rockchip/cdn-dp-core.c        |   1 -
 .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   |   1 -
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c   |   1 -
 drivers/gpu/drm/rockchip/inno_hdmi.c          |   1 -
 drivers/gpu/drm/rockchip/rk3066_hdmi.c        |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |  18 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  12 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 845 +++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h  |  82 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c      |   1 -
 drivers/gpu/drm/rockchip/rockchip_rgb.c       |   1 -
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c  | 221 +++++
 include/dt-bindings/soc/rockchip,vop2.h       |   4 +
 17 files changed, 1257 insertions(+), 58 deletions(-)

-- 
2.34.1

[PATCH v2 01/12] drm/rockchip: move output interface related definition to rockchip_drm_drv.h

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

The output interface related definition can shared between
vop and vop2, move them to rockchip_drm_drv.h can avoid duplicated
definition.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

(no changes since v1)

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |  1 -
 drivers/gpu/drm/rockchip/cdn-dp-core.c          |  1 -
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c |  1 -
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |  1 -
 drivers/gpu/drm/rockchip/inno_hdmi.c            |  1 -
 drivers/gpu/drm/rockchip/rk3066_hdmi.c          |  1 -
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h     | 17 +++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h     | 12 +-----------
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h    | 16 +---------------
 drivers/gpu/drm/rockchip/rockchip_lvds.c        |  1 -
 drivers/gpu/drm/rockchip/rockchip_rgb.c         |  1 -
 11 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 84aa811ca1e9..bd08d57486fe 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -30,7 +30,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 
 #define RK3288_GRF_SOC_CON6		0x25c
 #define RK3288_EDP_LCDC_SEL		BIT(5)
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 21254e4e107a..a855c45ae7f3 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -24,7 +24,6 @@
 
 #include "cdn-dp-core.h"
 #include "cdn-dp-reg.h"
-#include "rockchip_drm_vop.h"
 
 static inline struct cdn_dp_device *connector_to_dp(struct drm_connector *connector)
 {
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 6396f9324dab..4cc8ed8f4fbd 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -26,7 +26,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 
 #define DSI_PHY_RSTZ			0xa0
 #define PHY_DISFORCEPLL			0
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 341550199111..fe33092abbe7 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -18,7 +18,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 
 #define RK3228_GRF_SOC_CON2		0x0408
 #define RK3228_HDMI_SDAIN_MSK		BIT(14)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 6e5b922a121e..f6d819803c0e 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -23,7 +23,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 
 #include "inno_hdmi.h"
 
diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
index fa6e592e0276..78136d0c5a65 100644
--- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
+++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
@@ -17,7 +17,6 @@
 #include "rk3066_hdmi.h"
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 
 #define DEFAULT_PLLA_RATE 30000000
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index aeb03a57240f..3d8ab2defa1b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -20,6 +20,23 @@
 #define ROCKCHIP_MAX_CONNECTOR	2
 #define ROCKCHIP_MAX_CRTC	4
 
+/*
+ * display output interface supported by rockchip lcdc
+ */
+#define ROCKCHIP_OUT_MODE_P888		0
+#define ROCKCHIP_OUT_MODE_BT1120	0
+#define ROCKCHIP_OUT_MODE_P666		1
+#define ROCKCHIP_OUT_MODE_P565		2
+#define ROCKCHIP_OUT_MODE_BT656		5
+#define ROCKCHIP_OUT_MODE_S888		8
+#define ROCKCHIP_OUT_MODE_S888_DUMMY	12
+#define ROCKCHIP_OUT_MODE_YUV420	14
+/* for use special outface */
+#define ROCKCHIP_OUT_MODE_AAAA		15
+
+/* output flags */
+#define ROCKCHIP_OUTPUT_DSI_DUAL	BIT(0)
+
 struct drm_device;
 struct drm_connector;
 struct iommu_domain;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 4b2daefeb8c1..43d9c9191b7a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -277,17 +277,7 @@ struct vop_data {
 /* dst alpha ctrl define */
 #define DST_FACTOR_M0(x)		(((x) & 0x7) << 6)
 
-/*
- * display output interface supported by rockchip lcdc
- */
-#define ROCKCHIP_OUT_MODE_P888	0
-#define ROCKCHIP_OUT_MODE_P666	1
-#define ROCKCHIP_OUT_MODE_P565	2
-/* for use special outface */
-#define ROCKCHIP_OUT_MODE_AAAA	15
-
-/* output flags */
-#define ROCKCHIP_OUTPUT_DSI_DUAL	BIT(0)
+
 
 enum alpha_mode {
 	ALPHA_STRAIGHT,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
index 56fd31e05238..7175f46a2014 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -7,10 +7,9 @@
 #ifndef _ROCKCHIP_DRM_VOP2_H
 #define _ROCKCHIP_DRM_VOP2_H
 
-#include "rockchip_drm_vop.h"
-
 #include <linux/regmap.h>
 #include <drm/drm_modes.h>
+#include "rockchip_drm_vop.h"
 
 #define VOP_FEATURE_OUTPUT_10BIT        BIT(0)
 
@@ -166,19 +165,6 @@ struct vop2_data {
 #define WB_YRGB_FIFO_FULL_INTR		BIT(18)
 #define WB_COMPLETE_INTR		BIT(19)
 
-/*
- * display output interface supported by rockchip lcdc
- */
-#define ROCKCHIP_OUT_MODE_P888		0
-#define ROCKCHIP_OUT_MODE_BT1120	0
-#define ROCKCHIP_OUT_MODE_P666		1
-#define ROCKCHIP_OUT_MODE_P565		2
-#define ROCKCHIP_OUT_MODE_BT656		5
-#define ROCKCHIP_OUT_MODE_S888		8
-#define ROCKCHIP_OUT_MODE_S888_DUMMY	12
-#define ROCKCHIP_OUT_MODE_YUV420	14
-/* for use special outface */
-#define ROCKCHIP_OUT_MODE_AAAA		15
 
 enum vop_csc_format {
 	CSC_BT601L,
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index f0f47e9abf5a..59341654ec32 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -27,7 +27,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 #include "rockchip_lvds.h"
 
 #define DISPLAY_OUTPUT_RGB		0
diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
index c677b71ae516..dbfbde24698e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
@@ -19,7 +19,6 @@
 #include <drm/drm_simple_kms_helper.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_vop.h"
 #include "rockchip_rgb.h"
 
 struct rockchip_rgb {
-- 
2.34.1

Re: [PATCH v2 01/12] drm/rockchip: move output interface related definition to rockchip_drm_drv.h

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:53:49PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> The output interface related definition can shared between
> vop and vop2, move them to rockchip_drm_drv.h can avoid duplicated
> definition.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

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

Sascha

> 
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c |  1 -
>  drivers/gpu/drm/rockchip/cdn-dp-core.c          |  1 -
>  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c |  1 -
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c     |  1 -
>  drivers/gpu/drm/rockchip/inno_hdmi.c            |  1 -
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c          |  1 -
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     | 17 +++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h     | 12 +-----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.h    | 16 +---------------
>  drivers/gpu/drm/rockchip/rockchip_lvds.c        |  1 -
>  drivers/gpu/drm/rockchip/rockchip_rgb.c         |  1 -
>  11 files changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 84aa811ca1e9..bd08d57486fe 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -30,7 +30,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  
>  #define RK3288_GRF_SOC_CON6		0x25c
>  #define RK3288_EDP_LCDC_SEL		BIT(5)
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 21254e4e107a..a855c45ae7f3 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> @@ -24,7 +24,6 @@
>  
>  #include "cdn-dp-core.h"
>  #include "cdn-dp-reg.h"
> -#include "rockchip_drm_vop.h"
>  
>  static inline struct cdn_dp_device *connector_to_dp(struct drm_connector *connector)
>  {
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 6396f9324dab..4cc8ed8f4fbd 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -26,7 +26,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  
>  #define DSI_PHY_RSTZ			0xa0
>  #define PHY_DISFORCEPLL			0
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 341550199111..fe33092abbe7 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -18,7 +18,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  
>  #define RK3228_GRF_SOC_CON2		0x0408
>  #define RK3228_HDMI_SDAIN_MSK		BIT(14)
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 6e5b922a121e..f6d819803c0e 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -23,7 +23,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  
>  #include "inno_hdmi.h"
>  
> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> index fa6e592e0276..78136d0c5a65 100644
> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> @@ -17,7 +17,6 @@
>  #include "rk3066_hdmi.h"
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  
>  #define DEFAULT_PLLA_RATE 30000000
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index aeb03a57240f..3d8ab2defa1b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -20,6 +20,23 @@
>  #define ROCKCHIP_MAX_CONNECTOR	2
>  #define ROCKCHIP_MAX_CRTC	4
>  
> +/*
> + * display output interface supported by rockchip lcdc
> + */
> +#define ROCKCHIP_OUT_MODE_P888		0
> +#define ROCKCHIP_OUT_MODE_BT1120	0
> +#define ROCKCHIP_OUT_MODE_P666		1
> +#define ROCKCHIP_OUT_MODE_P565		2
> +#define ROCKCHIP_OUT_MODE_BT656		5
> +#define ROCKCHIP_OUT_MODE_S888		8
> +#define ROCKCHIP_OUT_MODE_S888_DUMMY	12
> +#define ROCKCHIP_OUT_MODE_YUV420	14
> +/* for use special outface */
> +#define ROCKCHIP_OUT_MODE_AAAA		15
> +
> +/* output flags */
> +#define ROCKCHIP_OUTPUT_DSI_DUAL	BIT(0)
> +
>  struct drm_device;
>  struct drm_connector;
>  struct iommu_domain;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 4b2daefeb8c1..43d9c9191b7a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -277,17 +277,7 @@ struct vop_data {
>  /* dst alpha ctrl define */
>  #define DST_FACTOR_M0(x)		(((x) & 0x7) << 6)
>  
> -/*
> - * display output interface supported by rockchip lcdc
> - */
> -#define ROCKCHIP_OUT_MODE_P888	0
> -#define ROCKCHIP_OUT_MODE_P666	1
> -#define ROCKCHIP_OUT_MODE_P565	2
> -/* for use special outface */
> -#define ROCKCHIP_OUT_MODE_AAAA	15
> -
> -/* output flags */
> -#define ROCKCHIP_OUTPUT_DSI_DUAL	BIT(0)
> +
>  
>  enum alpha_mode {
>  	ALPHA_STRAIGHT,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> index 56fd31e05238..7175f46a2014 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
> @@ -7,10 +7,9 @@
>  #ifndef _ROCKCHIP_DRM_VOP2_H
>  #define _ROCKCHIP_DRM_VOP2_H
>  
> -#include "rockchip_drm_vop.h"
> -
>  #include <linux/regmap.h>
>  #include <drm/drm_modes.h>
> +#include "rockchip_drm_vop.h"
>  
>  #define VOP_FEATURE_OUTPUT_10BIT        BIT(0)
>  
> @@ -166,19 +165,6 @@ struct vop2_data {
>  #define WB_YRGB_FIFO_FULL_INTR		BIT(18)
>  #define WB_COMPLETE_INTR		BIT(19)
>  
> -/*
> - * display output interface supported by rockchip lcdc
> - */
> -#define ROCKCHIP_OUT_MODE_P888		0
> -#define ROCKCHIP_OUT_MODE_BT1120	0
> -#define ROCKCHIP_OUT_MODE_P666		1
> -#define ROCKCHIP_OUT_MODE_P565		2
> -#define ROCKCHIP_OUT_MODE_BT656		5
> -#define ROCKCHIP_OUT_MODE_S888		8
> -#define ROCKCHIP_OUT_MODE_S888_DUMMY	12
> -#define ROCKCHIP_OUT_MODE_YUV420	14
> -/* for use special outface */
> -#define ROCKCHIP_OUT_MODE_AAAA		15
>  
>  enum vop_csc_format {
>  	CSC_BT601L,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index f0f47e9abf5a..59341654ec32 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -27,7 +27,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  #include "rockchip_lvds.h"
>  
>  #define DISPLAY_OUTPUT_RGB		0
> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> index c677b71ae516..dbfbde24698e 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> @@ -19,7 +19,6 @@
>  #include <drm/drm_simple_kms_helper.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_vop.h"
>  #include "rockchip_rgb.h"
>  
>  struct rockchip_rgb {
> -- 
> 2.34.1
> 
> 
> 

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

[PATCH v2 02/12] Revert "drm/rockchip: vop2: Use regcache_sync() to fix suspend/resume"

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

This reverts commit b63a553e8f5aa6574eeb535a551817a93c426d8c.

regcache_sync will try to reload the configuration in regcache to
hardware, but the registers of 4 Cluster windows and Esmart1/2/3 on
the upcoming rk3588 can not be set successfully before internal PD
power on.

Also it's better to keep the hardware register as it is before we really
enable it.

So let's revert this version, and keep the first version:
commit afa965a45e01 ("drm/rockchip: vop2: fix suspend/resume")

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 312da5783362..57784d0a22a6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -217,6 +217,8 @@ struct vop2 {
 	struct vop2_win win[];
 };
 
+static const struct regmap_config vop2_regmap_config;
+
 static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
 {
 	return container_of(crtc, struct vop2_video_port, crtc);
@@ -883,7 +885,11 @@ static void vop2_enable(struct vop2 *vop2)
 		return;
 	}
 
-	regcache_sync(vop2->map);
+	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
+	if (ret) {
+		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
+		return;
+	}
 
 	if (vop2->data->soc_id == 3566)
 		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
@@ -913,8 +919,6 @@ static void vop2_disable(struct vop2 *vop2)
 
 	pm_runtime_put_sync(vop2->dev);
 
-	regcache_mark_dirty(vop2->map);
-
 	clk_disable_unprepare(vop2->aclk);
 	clk_disable_unprepare(vop2->hclk);
 }
-- 
2.34.1

Re: [PATCH v2 02/12] Revert "drm/rockchip: vop2: Use regcache_sync() to fix suspend/resume"

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:54:00PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> This reverts commit b63a553e8f5aa6574eeb535a551817a93c426d8c.
> 
> regcache_sync will try to reload the configuration in regcache to
> hardware, but the registers of 4 Cluster windows and Esmart1/2/3 on
> the upcoming rk3588 can not be set successfully before internal PD
> power on.
> 
> Also it's better to keep the hardware register as it is before we really
> enable it.
> 
> So let's revert this version, and keep the first version:
> commit afa965a45e01 ("drm/rockchip: vop2: fix suspend/resume")
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

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

Sascha

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 312da5783362..57784d0a22a6 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -217,6 +217,8 @@ struct vop2 {
>  	struct vop2_win win[];
>  };
>  
> +static const struct regmap_config vop2_regmap_config;
> +
>  static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
>  {
>  	return container_of(crtc, struct vop2_video_port, crtc);
> @@ -883,7 +885,11 @@ static void vop2_enable(struct vop2 *vop2)
>  		return;
>  	}
>  
> -	regcache_sync(vop2->map);
> +	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
> +	if (ret) {
> +		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
> +		return;
> +	}
>  
>  	if (vop2->data->soc_id == 3566)
>  		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
> @@ -913,8 +919,6 @@ static void vop2_disable(struct vop2 *vop2)
>  
>  	pm_runtime_put_sync(vop2->dev);
>  
> -	regcache_mark_dirty(vop2->map);
> -
>  	clk_disable_unprepare(vop2->aclk);
>  	clk_disable_unprepare(vop2->hclk);
>  }
> -- 
> 2.34.1
> 
> 

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

[PATCH v2 03/12] drm/rockchip: vop2: set half_block_en bit in all mode

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

At first we thought the half_block_en bit in AFBCD_CTRL register
only work in afbc mode. But the fact is that it control the line
buffer in all mode(afbc/tile/line), so we need configure it in
all case.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 57784d0a22a6..639dfebc6bd1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -521,6 +521,18 @@ static bool rockchip_vop2_mod_supported(struct drm_plane *plane, u32 format,
 	return vop2_convert_afbc_format(format) >= 0;
 }
 
+/*
+ * 0: Full mode, 16 lines for one tail
+ * 1: half block mode, 8 lines one tail
+ */
+static bool vop2_half_block_enable(struct drm_plane_state *pstate)
+{
+	if (pstate->rotation & (DRM_MODE_ROTATE_270 | DRM_MODE_ROTATE_90))
+		return false;
+	else
+		return true;
+}
+
 static u32 vop2_afbc_transform_offset(struct drm_plane_state *pstate,
 				      bool afbc_half_block_en)
 {
@@ -1144,6 +1156,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	bool rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
 	struct rockchip_gem_object *rk_obj;
 	unsigned long offset;
+	bool half_block_en;
 	bool afbc_en;
 	dma_addr_t yrgb_mst;
 	dma_addr_t uv_mst;
@@ -1236,6 +1249,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 	dsp_info = (dsp_h - 1) << 16 | ((dsp_w - 1) & 0xffff);
 
 	format = vop2_convert_format(fb->format->format);
+	half_block_en = vop2_half_block_enable(pstate);
 
 	drm_dbg(vop2->drm, "vp%d update %s[%dx%d->%dx%d@%dx%d] fmt[%p4cc_%s] addr[%pad]\n",
 		vp->id, win->data->name, actual_w, actual_h, dsp_w, dsp_h,
@@ -1243,6 +1257,9 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		&fb->format->format,
 		afbc_en ? "AFBC" : "", &yrgb_mst);
 
+	if (vop2_cluster_window(win))
+		vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, half_block_en);
+
 	if (afbc_en) {
 		u32 stride;
 
@@ -1283,13 +1300,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
 		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
 		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
-		if (pstate->rotation & (DRM_MODE_ROTATE_270 | DRM_MODE_ROTATE_90)) {
-			vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, 0);
-			transform_offset = vop2_afbc_transform_offset(pstate, false);
-		} else {
-			vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, 1);
-			transform_offset = vop2_afbc_transform_offset(pstate, true);
-		}
+		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
 		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
 		vop2_win_write(win, VOP2_WIN_AFBC_PIC_SIZE, act_info);
 		vop2_win_write(win, VOP2_WIN_AFBC_TRANSFORM_OFFSET, transform_offset);
-- 
2.34.1

Re: [PATCH v2 03/12] drm/rockchip: vop2: set half_block_en bit in all mode

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:54:13PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> At first we thought the half_block_en bit in AFBCD_CTRL register
> only work in afbc mode. But the fact is that it control the line
> buffer in all mode(afbc/tile/line), so we need configure it in
> all case.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

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

Sascha

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 25 ++++++++++++++------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 57784d0a22a6..639dfebc6bd1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -521,6 +521,18 @@ static bool rockchip_vop2_mod_supported(struct drm_plane *plane, u32 format,
>  	return vop2_convert_afbc_format(format) >= 0;
>  }
>  
> +/*
> + * 0: Full mode, 16 lines for one tail
> + * 1: half block mode, 8 lines one tail
> + */
> +static bool vop2_half_block_enable(struct drm_plane_state *pstate)
> +{
> +	if (pstate->rotation & (DRM_MODE_ROTATE_270 | DRM_MODE_ROTATE_90))
> +		return false;
> +	else
> +		return true;
> +}
> +
>  static u32 vop2_afbc_transform_offset(struct drm_plane_state *pstate,
>  				      bool afbc_half_block_en)
>  {
> @@ -1144,6 +1156,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  	bool rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
>  	struct rockchip_gem_object *rk_obj;
>  	unsigned long offset;
> +	bool half_block_en;
>  	bool afbc_en;
>  	dma_addr_t yrgb_mst;
>  	dma_addr_t uv_mst;
> @@ -1236,6 +1249,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  	dsp_info = (dsp_h - 1) << 16 | ((dsp_w - 1) & 0xffff);
>  
>  	format = vop2_convert_format(fb->format->format);
> +	half_block_en = vop2_half_block_enable(pstate);
>  
>  	drm_dbg(vop2->drm, "vp%d update %s[%dx%d->%dx%d@%dx%d] fmt[%p4cc_%s] addr[%pad]\n",
>  		vp->id, win->data->name, actual_w, actual_h, dsp_w, dsp_h,
> @@ -1243,6 +1257,9 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  		&fb->format->format,
>  		afbc_en ? "AFBC" : "", &yrgb_mst);
>  
> +	if (vop2_cluster_window(win))
> +		vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, half_block_en);
> +
>  	if (afbc_en) {
>  		u32 stride;
>  
> @@ -1283,13 +1300,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
>  		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
>  		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
> -		if (pstate->rotation & (DRM_MODE_ROTATE_270 | DRM_MODE_ROTATE_90)) {
> -			vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, 0);
> -			transform_offset = vop2_afbc_transform_offset(pstate, false);
> -		} else {
> -			vop2_win_write(win, VOP2_WIN_AFBC_HALF_BLOCK_EN, 1);
> -			transform_offset = vop2_afbc_transform_offset(pstate, true);
> -		}
> +		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
>  		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
>  		vop2_win_write(win, VOP2_WIN_AFBC_PIC_SIZE, act_info);
>  		vop2_win_write(win, VOP2_WIN_AFBC_TRANSFORM_OFFSET, transform_offset);
> -- 
> 2.34.1
> 
> 
> 

-- 
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 v2 03/12] drm/rockchip: vop2: set half_block_en bit in all mode

[Heiko Stübner]

Am Mittwoch, 22. November 2023, 13:54:13 CET schrieb Andy Yan:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> At first we thought the half_block_en bit in AFBCD_CTRL register
> only work in afbc mode. But the fact is that it control the line
> buffer in all mode(afbc/tile/line), so we need configure it in
> all case.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

This looks common to the rk3568 variant, right, so I guess this should
have a

Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")

perhaps?


Heiko

[PATCH v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

The enable bit and transform offset of cluster windows should be
cleared when it work at linear mode, or we may have a iommu fault
issue.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 639dfebc6bd1..a019cc9bbd54 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1312,6 +1312,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 		vop2_win_write(win, VOP2_WIN_AFBC_ROTATE_270, rotate_270);
 		vop2_win_write(win, VOP2_WIN_AFBC_ROTATE_90, rotate_90);
 	} else {
+		if (vop2_cluster_window(win)) {
+			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 0);
+			vop2_win_write(win, VOP2_WIN_AFBC_TRANSFORM_OFFSET, 0);
+		}
+
 		vop2_win_write(win, VOP2_WIN_YRGB_VIR, DIV_ROUND_UP(fb->pitches[0], 4));
 	}
 
-- 
2.34.1

Re: [PATCH v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:54:25PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> The enable bit and transform offset of cluster windows should be
> cleared when it work at linear mode, or we may have a iommu fault
> issue.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

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

Sascha

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 639dfebc6bd1..a019cc9bbd54 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1312,6 +1312,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  		vop2_win_write(win, VOP2_WIN_AFBC_ROTATE_270, rotate_270);
>  		vop2_win_write(win, VOP2_WIN_AFBC_ROTATE_90, rotate_90);
>  	} else {
> +		if (vop2_cluster_window(win)) {
> +			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 0);
> +			vop2_win_write(win, VOP2_WIN_AFBC_TRANSFORM_OFFSET, 0);
> +		}
> +
>  		vop2_win_write(win, VOP2_WIN_YRGB_VIR, DIV_ROUND_UP(fb->pitches[0], 4));
>  	}
>  
> -- 
> 2.34.1
> 
> 
> 

-- 
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 v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Heiko Stübner]

Am Mittwoch, 22. November 2023, 13:54:25 CET schrieb Andy Yan:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> The enable bit and transform offset of cluster windows should be
> cleared when it work at linear mode, or we may have a iommu fault
> issue.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

I guess same here?

Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")

Re: [PATCH v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Andy Yan]

Hi Heiko:

On 11/27/23 23:02, Heiko Stübner wrote:
> Am Mittwoch, 22. November 2023, 13:54:25 CET schrieb Andy Yan:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> The enable bit and transform offset of cluster windows should be
>> cleared when it work at linear mode, or we may have a iommu fault
>> issue.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> I guess same here?
>
> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")


I'm not sure if we need a Fixes tag here,  in fact this issue never happens on

rk3566/8 , because the cluster windows of rk356x only support afbc format,

they don't have a chance to switch between afbc and linear mode.

Of course, the lack support of linear mode of rk356x cluster windows is a thoughtless

of IC design, if it really support both afbc and linear format, we indeed need this fix.

The situation is the same as patch 03/12.

So I hope follow your advice, if it need a Fixes tag here.

>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Heiko Stübner]

Am Dienstag, 28. November 2023, 09:03:46 CET schrieb Andy Yan:
> Hi Heiko:
> 
> On 11/27/23 23:02, Heiko Stübner wrote:
> > Am Mittwoch, 22. November 2023, 13:54:25 CET schrieb Andy Yan:
> >> From: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> The enable bit and transform offset of cluster windows should be
> >> cleared when it work at linear mode, or we may have a iommu fault
> >> issue.
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> > I guess same here?
> >
> > Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
> 
> 
> I'm not sure if we need a Fixes tag here,  in fact this issue never happens on
> 
> rk3566/8 , because the cluster windows of rk356x only support afbc format,
> 
> they don't have a chance to switch between afbc and linear mode.
> 
> Of course, the lack support of linear mode of rk356x cluster windows is a thoughtless
> 
> of IC design, if it really support both afbc and linear format, we indeed need this fix.
> 
> The situation is the same as patch 03/12.
> 
> So I hope follow your advice, if it need a Fixes tag here.

ah ok, thanks for the explanation. Then I guess we don't need a fixes tag
when the rk3568 is not affected by this.

Same for the other patch. If you're re-sending you could add this information
to the commit message though. (existing support for rk3568 only supports
afbc cluster windows and is therefore not affected)


Thanks
Heiko

Re: [PATCH v2 04/12] drm/rockchip: vop2: clear afbc en and transform bit for cluster window at linear mode

[Andy Yan]

Hi Heiko:

On 11/28/23 16:30, Heiko Stübner wrote:
> Am Dienstag, 28. November 2023, 09:03:46 CET schrieb Andy Yan:
>> Hi Heiko:
>>
>> On 11/27/23 23:02, Heiko Stübner wrote:
>>> Am Mittwoch, 22. November 2023, 13:54:25 CET schrieb Andy Yan:
>>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> The enable bit and transform offset of cluster windows should be
>>>> cleared when it work at linear mode, or we may have a iommu fault
>>>> issue.
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>> I guess same here?
>>>
>>> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
>>
>> I'm not sure if we need a Fixes tag here,  in fact this issue never happens on
>>
>> rk3566/8 , because the cluster windows of rk356x only support afbc format,
>>
>> they don't have a chance to switch between afbc and linear mode.
>>
>> Of course, the lack support of linear mode of rk356x cluster windows is a thoughtless
>>
>> of IC design, if it really support both afbc and linear format, we indeed need this fix.
>>
>> The situation is the same as patch 03/12.
>>
>> So I hope follow your advice, if it need a Fixes tag here.
> ah ok, thanks for the explanation. Then I guess we don't need a fixes tag
> when the rk3568 is not affected by this.
>
> Same for the other patch. If you're re-sending you could add this information
> to the commit message though. (existing support for rk3568 only supports
> afbc cluster windows and is therefore not affected)


Ok, will done.

>
>
> Thanks
> Heiko
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 05/12] drm/rockchip: vop2: Set YUV/RGB overlay mode

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

Set overlay mode register according to the
output mode is yuv or rgb.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

 drivers/gpu/drm/rockchip/rockchip_drm_drv.h  |  1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 3d8ab2defa1b..7a58c5c9d4ec 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -51,6 +51,7 @@ struct rockchip_crtc_state {
 	u32 bus_format;
 	u32 bus_flags;
 	int color_space;
+	bool yuv_overlay;
 };
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index a019cc9bbd54..b32a291c5caa 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1612,6 +1612,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	vop2->enable_count++;
 
+	vcstate->yuv_overlay = is_yuv_output(vcstate->bus_format);
+
 	vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
 
 	polflags = 0;
@@ -1639,7 +1641,7 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 	if (vop2_output_uv_swap(vcstate->bus_format, vcstate->output_mode))
 		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RB_SWAP;
 
-	if (is_yuv_output(vcstate->bus_format))
+	if (vcstate->yuv_overlay)
 		dsp_ctrl |= RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y;
 
 	vop2_dither_setup(crtc, &dsp_ctrl);
@@ -1948,10 +1950,12 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 	u16 hdisplay;
 	u32 bg_dly;
 	u32 pre_scan_dly;
+	u32 ovl_ctrl;
 	int i;
 	struct vop2_video_port *vp0 = &vop2->vps[0];
 	struct vop2_video_port *vp1 = &vop2->vps[1];
 	struct vop2_video_port *vp2 = &vop2->vps[2];
+	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(vp->crtc.state);
 
 	adjusted_mode = &vp->crtc.state->adjusted_mode;
 	hsync_len = adjusted_mode->crtc_hsync_end - adjusted_mode->crtc_hsync_start;
@@ -1964,7 +1968,14 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 	pre_scan_dly = ((bg_dly + (hdisplay >> 1) - 1) << 16) | hsync_len;
 	vop2_vp_write(vp, RK3568_VP_PRE_SCAN_HTIMING, pre_scan_dly);
 
-	vop2_writel(vop2, RK3568_OVL_CTRL, 0);
+	ovl_ctrl = vop2_readl(vop2, RK3568_OVL_CTRL);
+	if (vcstate->yuv_overlay)
+		ovl_ctrl |= BIT(vp->id);
+	else
+		ovl_ctrl &= ~BIT(vp->id);
+
+	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);
+
 	port_sel = vop2_readl(vop2, RK3568_OVL_PORT_SEL);
 	port_sel &= RK3568_OVL_PORT_SEL__SEL_PORT;
 
@@ -2036,9 +2047,11 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 		layer_sel |= RK3568_OVL_LAYER_SEL__LAYER(nlayer + ofs, 5);
 	}
 
+	ovl_ctrl |= RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD;
+
 	vop2_writel(vop2, RK3568_OVL_LAYER_SEL, layer_sel);
 	vop2_writel(vop2, RK3568_OVL_PORT_SEL, port_sel);
-	vop2_writel(vop2, RK3568_OVL_CTRL, RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD);
+	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);
 }
 
 static void vop2_setup_dly_for_windows(struct vop2 *vop2)
-- 
2.34.1

Re: [PATCH v2 05/12] drm/rockchip: vop2: Set YUV/RGB overlay mode

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:54:38PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> Set overlay mode register according to the
> output mode is yuv or rgb.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h  |  1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index 3d8ab2defa1b..7a58c5c9d4ec 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -51,6 +51,7 @@ struct rockchip_crtc_state {
>  	u32 bus_format;
>  	u32 bus_flags;
>  	int color_space;
> +	bool yuv_overlay;

This struct already contains a bool type variable. Please add this one
next to it to keep the struct size smaller.

>  };
>  #define to_rockchip_crtc_state(s) \
>  		container_of(s, struct rockchip_crtc_state, base)
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index a019cc9bbd54..b32a291c5caa 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1612,6 +1612,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	vop2->enable_count++;
>  
> +	vcstate->yuv_overlay = is_yuv_output(vcstate->bus_format);
> +
>  	vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
>  
>  	polflags = 0;
> @@ -1639,7 +1641,7 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  	if (vop2_output_uv_swap(vcstate->bus_format, vcstate->output_mode))
>  		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RB_SWAP;
>  
> -	if (is_yuv_output(vcstate->bus_format))
> +	if (vcstate->yuv_overlay)
>  		dsp_ctrl |= RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y;
>  
>  	vop2_dither_setup(crtc, &dsp_ctrl);
> @@ -1948,10 +1950,12 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>  	u16 hdisplay;
>  	u32 bg_dly;
>  	u32 pre_scan_dly;
> +	u32 ovl_ctrl;
>  	int i;
>  	struct vop2_video_port *vp0 = &vop2->vps[0];
>  	struct vop2_video_port *vp1 = &vop2->vps[1];
>  	struct vop2_video_port *vp2 = &vop2->vps[2];
> +	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(vp->crtc.state);
>  
>  	adjusted_mode = &vp->crtc.state->adjusted_mode;
>  	hsync_len = adjusted_mode->crtc_hsync_end - adjusted_mode->crtc_hsync_start;
> @@ -1964,7 +1968,14 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>  	pre_scan_dly = ((bg_dly + (hdisplay >> 1) - 1) << 16) | hsync_len;
>  	vop2_vp_write(vp, RK3568_VP_PRE_SCAN_HTIMING, pre_scan_dly);
>  
> -	vop2_writel(vop2, RK3568_OVL_CTRL, 0);
> +	ovl_ctrl = vop2_readl(vop2, RK3568_OVL_CTRL);
> +	if (vcstate->yuv_overlay)
> +		ovl_ctrl |= BIT(vp->id);
> +	else
> +		ovl_ctrl &= ~BIT(vp->id);

Some

#define RK3568_OVL_CTRL__YUV_MODE(vp)	BIT(vp)

Would be nice.

> +
> +	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);

Is it necessary to write this register twice?

> +
>  	port_sel = vop2_readl(vop2, RK3568_OVL_PORT_SEL);
>  	port_sel &= RK3568_OVL_PORT_SEL__SEL_PORT;
>  
> @@ -2036,9 +2047,11 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>  		layer_sel |= RK3568_OVL_LAYER_SEL__LAYER(nlayer + ofs, 5);
>  	}
>  
> +	ovl_ctrl |= RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD;
> +
>  	vop2_writel(vop2, RK3568_OVL_LAYER_SEL, layer_sel);
>  	vop2_writel(vop2, RK3568_OVL_PORT_SEL, port_sel);
> -	vop2_writel(vop2, RK3568_OVL_CTRL, RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD);
> +	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);

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 v2 05/12] drm/rockchip: vop2: Set YUV/RGB overlay mode

[Andy Yan]

Hi Sasha:

On 11/27/23 22:16, Sascha Hauer wrote:
> On Wed, Nov 22, 2023 at 08:54:38PM +0800, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> Set overlay mode register according to the
>> output mode is yuv or rgb.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_drv.h  |  1 +
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 19 ++++++++++++++++---
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> index 3d8ab2defa1b..7a58c5c9d4ec 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
>> @@ -51,6 +51,7 @@ struct rockchip_crtc_state {
>>   	u32 bus_format;
>>   	u32 bus_flags;
>>   	int color_space;
>> +	bool yuv_overlay;
> This struct already contains a bool type variable. Please add this one
> next to it to keep the struct size smaller.


Okay, will do.

>
>>   };
>>   #define to_rockchip_crtc_state(s) \
>>   		container_of(s, struct rockchip_crtc_state, base)
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index a019cc9bbd54..b32a291c5caa 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -1612,6 +1612,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>   
>>   	vop2->enable_count++;
>>   
>> +	vcstate->yuv_overlay = is_yuv_output(vcstate->bus_format);
>> +
>>   	vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
>>   
>>   	polflags = 0;
>> @@ -1639,7 +1641,7 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>   	if (vop2_output_uv_swap(vcstate->bus_format, vcstate->output_mode))
>>   		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RB_SWAP;
>>   
>> -	if (is_yuv_output(vcstate->bus_format))
>> +	if (vcstate->yuv_overlay)
>>   		dsp_ctrl |= RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y;
>>   
>>   	vop2_dither_setup(crtc, &dsp_ctrl);
>> @@ -1948,10 +1950,12 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>>   	u16 hdisplay;
>>   	u32 bg_dly;
>>   	u32 pre_scan_dly;
>> +	u32 ovl_ctrl;
>>   	int i;
>>   	struct vop2_video_port *vp0 = &vop2->vps[0];
>>   	struct vop2_video_port *vp1 = &vop2->vps[1];
>>   	struct vop2_video_port *vp2 = &vop2->vps[2];
>> +	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(vp->crtc.state);
>>   
>>   	adjusted_mode = &vp->crtc.state->adjusted_mode;
>>   	hsync_len = adjusted_mode->crtc_hsync_end - adjusted_mode->crtc_hsync_start;
>> @@ -1964,7 +1968,14 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>>   	pre_scan_dly = ((bg_dly + (hdisplay >> 1) - 1) << 16) | hsync_len;
>>   	vop2_vp_write(vp, RK3568_VP_PRE_SCAN_HTIMING, pre_scan_dly);
>>   
>> -	vop2_writel(vop2, RK3568_OVL_CTRL, 0);
>> +	ovl_ctrl = vop2_readl(vop2, RK3568_OVL_CTRL);
>> +	if (vcstate->yuv_overlay)
>> +		ovl_ctrl |= BIT(vp->id);
>> +	else
>> +		ovl_ctrl &= ~BIT(vp->id);
> Some
>
> #define RK3568_OVL_CTRL__YUV_MODE(vp)	BIT(vp)
>
> Would be nice.


Okay, will do.

>> +
>> +	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);
> Is it necessary to write this register twice?

I don't think so. Just follow the original code write it here.

Anyway, I will just write once in next version.


And would you please check my response about debugfs patch[0] when it is convenient for you?

I want to know what you think, and prepare the next version.


[0]https://patchwork.kernel.org/project/dri-devel/patch/20231122125601.3455031-1-andyshrk@163.com/

>
>> +
>>   	port_sel = vop2_readl(vop2, RK3568_OVL_PORT_SEL);
>>   	port_sel &= RK3568_OVL_PORT_SEL__SEL_PORT;
>>   
>> @@ -2036,9 +2047,11 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>>   		layer_sel |= RK3568_OVL_LAYER_SEL__LAYER(nlayer + ofs, 5);
>>   	}
>>   
>> +	ovl_ctrl |= RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD;
>> +
>>   	vop2_writel(vop2, RK3568_OVL_LAYER_SEL, layer_sel);
>>   	vop2_writel(vop2, RK3568_OVL_PORT_SEL, port_sel);
>> -	vop2_writel(vop2, RK3568_OVL_CTRL, RK3568_OVL_CTRL__LAYERSEL_REGDONE_IMD);
>> +	vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);
> Sascha
>

[PATCH v2 06/12] drm/rockchip: vop2: rename grf to sys_grf

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

The vop2 need to reference more grf(system grf, vop grf, vo0/1 grf,etc)
in the upcoming rk3588.

So we rename the current system grf to sys_grf.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index b32a291c5caa..4bcc405bcf11 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -190,7 +190,7 @@ struct vop2 {
 	void __iomem *regs;
 	struct regmap *map;
 
-	struct regmap *grf;
+	struct regmap *sys_grf;
 
 	/* physical map length of vop2 register */
 	u32 len;
@@ -1503,9 +1503,9 @@ static void rk3568_set_intf_mux(struct vop2_video_port *vp, int id,
 		dip &= ~RK3568_DSP_IF_POL__RGB_LVDS_PIN_POL;
 		dip |= FIELD_PREP(RK3568_DSP_IF_POL__RGB_LVDS_PIN_POL, polflags);
 		if (polflags & POLFLAG_DCLK_INV)
-			regmap_write(vop2->grf, RK3568_GRF_VO_CON1, BIT(3 + 16) | BIT(3));
+			regmap_write(vop2->sys_grf, RK3568_GRF_VO_CON1, BIT(3 + 16) | BIT(3));
 		else
-			regmap_write(vop2->grf, RK3568_GRF_VO_CON1, BIT(3 + 16));
+			regmap_write(vop2->sys_grf, RK3568_GRF_VO_CON1, BIT(3 + 16));
 		break;
 	case ROCKCHIP_VOP2_EP_HDMI0:
 		die &= ~RK3568_SYS_DSP_INFACE_EN_HDMI_MUX;
@@ -2764,7 +2764,7 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 			return PTR_ERR(vop2->lut_regs);
 	}
 
-	vop2->grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
+	vop2->sys_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
 
 	vop2->hclk = devm_clk_get(vop2->dev, "hclk");
 	if (IS_ERR(vop2->hclk)) {
-- 
2.34.1

Re: [PATCH v2 06/12] drm/rockchip: vop2: rename grf to sys_grf

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:54:54PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> The vop2 need to reference more grf(system grf, vop grf, vo0/1 grf,etc)
> in the upcoming rk3588.
> 
> So we rename the current system grf to sys_grf.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

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

Sascha

> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index b32a291c5caa..4bcc405bcf11 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -190,7 +190,7 @@ struct vop2 {
>  	void __iomem *regs;
>  	struct regmap *map;
>  
> -	struct regmap *grf;
> +	struct regmap *sys_grf;
>  
>  	/* physical map length of vop2 register */
>  	u32 len;
> @@ -1503,9 +1503,9 @@ static void rk3568_set_intf_mux(struct vop2_video_port *vp, int id,
>  		dip &= ~RK3568_DSP_IF_POL__RGB_LVDS_PIN_POL;
>  		dip |= FIELD_PREP(RK3568_DSP_IF_POL__RGB_LVDS_PIN_POL, polflags);
>  		if (polflags & POLFLAG_DCLK_INV)
> -			regmap_write(vop2->grf, RK3568_GRF_VO_CON1, BIT(3 + 16) | BIT(3));
> +			regmap_write(vop2->sys_grf, RK3568_GRF_VO_CON1, BIT(3 + 16) | BIT(3));
>  		else
> -			regmap_write(vop2->grf, RK3568_GRF_VO_CON1, BIT(3 + 16));
> +			regmap_write(vop2->sys_grf, RK3568_GRF_VO_CON1, BIT(3 + 16));
>  		break;
>  	case ROCKCHIP_VOP2_EP_HDMI0:
>  		die &= ~RK3568_SYS_DSP_INFACE_EN_HDMI_MUX;
> @@ -2764,7 +2764,7 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
>  			return PTR_ERR(vop2->lut_regs);
>  	}
>  
> -	vop2->grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
> +	vop2->sys_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
>  
>  	vop2->hclk = devm_clk_get(vop2->dev, "hclk");
>  	if (IS_ERR(vop2->hclk)) {
> -- 
> 2.34.1
> 
> 
> 

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

[PATCH v2 07/12] dt-bindings: soc: rockchip: add rk3588 vop/vo syscon

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

Add VOP and VO GRF syscon compatibles for RK3588

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---

(no changes since v1)

 Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
index e4fa6a07b4fa..26db4f48ff62 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
@@ -28,6 +28,8 @@ properties:
               - rockchip,rk3588-sys-grf
               - rockchip,rk3588-pcie3-phy-grf
               - rockchip,rk3588-pcie3-pipe-grf
+              - rockchip,rk3588-vo-grf
+              - rockchip,rk3588-vop-grf
               - rockchip,rv1108-usbgrf
           - const: syscon
       - items:
-- 
2.34.1

[PATCH v2 08/12] dt-bindings: display: vop2: Add rk3588 support

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

The vop2 on rk3588 is similar to which on rk356x
but with 4 video ports and need to reference
more grf modules.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- fix errors when running 'make DT_CHECKER_FLAGS=-m dt_binding_check'

 .../display/rockchip/rockchip-vop2.yaml       | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
index b60b90472d42..24148d9b3b14 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
@@ -20,6 +20,7 @@ properties:
     enum:
       - rockchip,rk3566-vop
       - rockchip,rk3568-vop
+      - rockchip,rk3588-vop
 
   reg:
     items:
@@ -42,26 +43,47 @@ properties:
       frame start (VSYNC), line flag and other status interrupts.
 
   clocks:
+    minItems: 3
     items:
       - description: Clock for ddr buffer transfer.
       - description: Clock for the ahb bus to R/W the phy regs.
       - description: Pixel clock for video port 0.
       - description: Pixel clock for video port 1.
       - description: Pixel clock for video port 2.
+      - description: Pixel clock for video port 4.
+      - description: Peripheral clock for vop on rk3588.
 
   clock-names:
+    minItems: 3
     items:
       - const: aclk
       - const: hclk
       - const: dclk_vp0
       - const: dclk_vp1
       - const: dclk_vp2
+      - const: dclk_vp3
+      - const: pclk_vop
 
   rockchip,grf:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
       Phandle to GRF regs used for misc control
 
+  rockchip,vo-grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to VO GRF regs used for misc control, required for rk3588
+
+  rockchip,vop-grf:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to VOP GRF regs used for misc control, required for rk3588
+
+  rockchip,pmu:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to PMU regs used for misc control, required for rk3588
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -81,6 +103,11 @@ properties:
         description:
           Output endpoint of VP2
 
+      port@3:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Output endpoint of VP3
+
   iommus:
     maxItems: 1
 
-- 
2.34.1

Re: [PATCH v2 08/12] dt-bindings: display: vop2: Add rk3588 support

[Krzysztof Kozlowski]

On 22/11/2023 13:55, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> The vop2 on rk3588 is similar to which on rk356x
> but with 4 video ports and need to reference
> more grf modules.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - fix errors when running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> 
>  .../display/rockchip/rockchip-vop2.yaml       | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> index b60b90472d42..24148d9b3b14 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> @@ -20,6 +20,7 @@ properties:
>      enum:
>        - rockchip,rk3566-vop
>        - rockchip,rk3568-vop
> +      - rockchip,rk3588-vop
>  
>    reg:
>      items:
> @@ -42,26 +43,47 @@ properties:
>        frame start (VSYNC), line flag and other status interrupts.
>  
>    clocks:
> +    minItems: 3
>      items:
>        - description: Clock for ddr buffer transfer.
>        - description: Clock for the ahb bus to R/W the phy regs.
>        - description: Pixel clock for video port 0.
>        - description: Pixel clock for video port 1.
>        - description: Pixel clock for video port 2.
> +      - description: Pixel clock for video port 4.
> +      - description: Peripheral clock for vop on rk3588.
>  
>    clock-names:
> +    minItems: 3

You relax requirements for all existing variants here which is not
explained in commit msg. I assume this was not intentional, so you need
to re-constrain them in allOf:if:then.

See for example:
https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57
for some ideas.

>      items:
>        - const: aclk
>        - const: hclk
>        - const: dclk_vp0
>        - const: dclk_vp1
>        - const: dclk_vp2
> +      - const: dclk_vp3
> +      - const: pclk_vop
>  
>    rockchip,grf:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
>        Phandle to GRF regs used for misc control
>  
> +  rockchip,vo-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to VO GRF regs used for misc control, required for rk3588

Drop last sentence, instead add it to required in allOf:if:then.

Is this valid for other variants? If not, should be disallowed in
allOf:if:then: for them.

> +
> +  rockchip,vop-grf:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to VOP GRF regs used for misc control, required for rk3588
> +
> +  rockchip,pmu:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to PMU regs used for misc control, required for rk3588

For all these three: what is "misc control"? Way too vague. Everything
is a misc and everything can be control. You must be here specific and
much more descriptive.

> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  
> @@ -81,6 +103,11 @@ properties:
>          description:
>            Output endpoint of VP2
>  
> +      port@3:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Output endpoint of VP3

Valid for other variants?

Best regards,
Krzysztof

Re: [PATCH v2 08/12] dt-bindings: display: vop2: Add rk3588 support

[Andy Yan]

Hi Krzysztof:


On 11/23/23 03:07, Krzysztof Kozlowski wrote:
> On 22/11/2023 13:55, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> The vop2 on rk3588 is similar to which on rk356x
>> but with 4 video ports and need to reference
>> more grf modules.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - fix errors when running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>
>>   .../display/rockchip/rockchip-vop2.yaml       | 27 +++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
>> index b60b90472d42..24148d9b3b14 100644
>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
>> @@ -20,6 +20,7 @@ properties:
>>       enum:
>>         - rockchip,rk3566-vop
>>         - rockchip,rk3568-vop
>> +      - rockchip,rk3588-vop
>>   
>>     reg:
>>       items:
>> @@ -42,26 +43,47 @@ properties:
>>         frame start (VSYNC), line flag and other status interrupts.
>>   
>>     clocks:
>> +    minItems: 3
>>       items:
>>         - description: Clock for ddr buffer transfer.
>>         - description: Clock for the ahb bus to R/W the phy regs.
>>         - description: Pixel clock for video port 0.
>>         - description: Pixel clock for video port 1.
>>         - description: Pixel clock for video port 2.
>> +      - description: Pixel clock for video port 4.
>> +      - description: Peripheral clock for vop on rk3588.
>>   
>>     clock-names:
>> +    minItems: 3
> 
> You relax requirements for all existing variants here which is not
> explained in commit msg. I assume this was not intentional, so you need
> to re-constrain them in allOf:if:then.
> 
> See for example:
> https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57
> for some ideas.
> 
>>       items:
>>         - const: aclk
>>         - const: hclk
>>         - const: dclk_vp0
>>         - const: dclk_vp1
>>         - const: dclk_vp2
>> +      - const: dclk_vp3
>> +      - const: pclk_vop
>>   
>>     rockchip,grf:
>>       $ref: /schemas/types.yaml#/definitions/phandle
>>       description:
>>         Phandle to GRF regs used for misc control
>>   
>> +  rockchip,vo-grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to VO GRF regs used for misc control, required for rk3588
> 
> Drop last sentence, instead add it to required in allOf:if:then.
> 
> Is this valid for other variants? If not, should be disallowed in
> allOf:if:then: for them.

Only valid for rk3588 now.

> 
>> +
>> +  rockchip,vop-grf:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to VOP GRF regs used for misc control, required for rk3588
>> +
>> +  rockchip,pmu:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to PMU regs used for misc control, required for rk3588
> 
> For all these three: what is "misc control"? Way too vague. Everything
> is a misc and everything can be control. You must be here specific and
> much more descriptive.

improve in v3

> 
>> +
>>     ports:
>>       $ref: /schemas/graph.yaml#/properties/ports
>>   
>> @@ -81,6 +103,11 @@ properties:
>>           description:
>>             Output endpoint of VP2
>>   
>> +      port@3:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description:
>> +          Output endpoint of VP3
> 
> Valid for other variants?
> 

Only valid for rk3588 now.

Thanks for your review and guidance, I try to fix in v3 [0]

[0]https://patchwork.kernel.org/project/linux-rockchip/patch/20231130122418.13258-1-andyshrk@163.com/
> Best regards,
> Krzysztof
> 

[PATCH v2 09/12] dt-bindings: soc: vop2: Add more endpoint definition

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

There are 2 HDMI, 2 DP, 2 eDP on rk3588, so add
corresponding endpoint definition for it.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- split form vop driver patch

 include/dt-bindings/soc/rockchip,vop2.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/soc/rockchip,vop2.h b/include/dt-bindings/soc/rockchip,vop2.h
index 6e66a802b96a..668f199df9f0 100644
--- a/include/dt-bindings/soc/rockchip,vop2.h
+++ b/include/dt-bindings/soc/rockchip,vop2.h
@@ -10,5 +10,9 @@
 #define ROCKCHIP_VOP2_EP_LVDS0	5
 #define ROCKCHIP_VOP2_EP_MIPI1	6
 #define ROCKCHIP_VOP2_EP_LVDS1	7
+#define ROCKCHIP_VOP2_EP_HDMI1	8
+#define ROCKCHIP_VOP2_EP_EDP1	9
+#define ROCKCHIP_VOP2_EP_DP0	10
+#define ROCKCHIP_VOP2_EP_DP1	11
 
 #endif /* __DT_BINDINGS_ROCKCHIP_VOP2_H */
-- 
2.34.1

Re: [PATCH v2 09/12] dt-bindings: soc: vop2: Add more endpoint definition

[Krzysztof Kozlowski]

On 22/11/2023 13:55, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> There are 2 HDMI, 2 DP, 2 eDP on rk3588, so add
> corresponding endpoint definition for it.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Subject:

dt-bindings: soc: rockchip,vop2:
or
dt-bindings: rockchip,vop2:


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

[PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

VOP2 on rk3588:

Four video ports:
VP0 Max 4096x2160
VP1 Max 4096x2160
VP2 Max 4096x2160
VP3 Max 2048x1080

4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
4 4K Esmart windows with line RGB/YUV support

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v2:
- add rk3588_ prefix for functions which are rk3588 only
- make some calculation as fixed value and keep calculation formula as
  comment
- check return value for some cru calculation functions.
- check return value for syscon_regmap_lookup_by_phandle
- add NV20/NV30 for esmart plane

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  66 ++++
 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
 3 files changed, 660 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 4bcc405bcf11..9eecbe1f71f9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -191,6 +191,9 @@ struct vop2 {
 	struct regmap *map;
 
 	struct regmap *sys_grf;
+	struct regmap *vop_grf;
+	struct regmap *vo1_grf;
+	struct regmap *sys_pmu;
 
 	/* physical map length of vop2 register */
 	u32 len;
@@ -209,6 +212,7 @@ struct vop2 {
 	unsigned int enable_count;
 	struct clk *hclk;
 	struct clk *aclk;
+	struct clk *pclk;
 
 	/* optional internal rgb encoder */
 	struct rockchip_rgb *rgb;
@@ -217,6 +221,13 @@ struct vop2 {
 	struct vop2_win win[];
 };
 
+#define vop2_output_if_is_hdmi(x)	(x == ROCKCHIP_VOP2_EP_HDMI0 || x == ROCKCHIP_VOP2_EP_HDMI1)
+#define vop2_output_if_is_dp(x)		(x == ROCKCHIP_VOP2_EP_DP0 || x == ROCKCHIP_VOP2_EP_DP1)
+#define vop2_output_if_is_edp(x)	(x == ROCKCHIP_VOP2_EP_EDP0 || x == ROCKCHIP_VOP2_EP_EDP1)
+#define vop2_output_if_is_mipi(x)	(x == ROCKCHIP_VOP2_EP_MIPI0 || x == ROCKCHIP_VOP2_EP_MIPI1)
+#define vop2_output_if_is_lvds(x)	(x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
+#define vop2_output_if_is_dpi(x)	(x == ROCKCHIP_VOP2_EP_RGB0)
+
 static const struct regmap_config vop2_regmap_config;
 
 static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
@@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
 static void vop2_cfg_done(struct vop2_video_port *vp)
 {
 	struct vop2 *vop2 = vp->vop2;
+	u32 val;
+
+	val = BIT(vp->id) | (BIT(vp->id) << 16) |
+		RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
 
-	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
-			BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
+	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);
 }
 
 static void vop2_win_disable(struct vop2_win *win)
@@ -464,6 +478,17 @@ static bool vop2_output_uv_swap(u32 bus_format, u32 output_mode)
 		return false;
 }
 
+static bool vop2_output_rg_swap(struct vop2 *vop2, u32 bus_format)
+{
+	if (vop2->data->soc_id == 3588) {
+		if (bus_format == MEDIA_BUS_FMT_YUV8_1X24 ||
+		    bus_format == MEDIA_BUS_FMT_YUV10_1X30)
+			return true;
+	}
+
+	return false;
+}
+
 static bool is_yuv_output(u32 bus_format)
 {
 	switch (bus_format) {
@@ -868,13 +893,32 @@ static int vop2_core_clks_prepare_enable(struct vop2 *vop2)
 		goto err;
 	}
 
+	ret = clk_prepare_enable(vop2->pclk);
+	if (ret < 0) {
+		drm_err(vop2->drm, "failed to enable pclk - %d\n", ret);
+		goto err1;
+	}
+
 	return 0;
+err1:
+	clk_disable_unprepare(vop2->aclk);
 err:
 	clk_disable_unprepare(vop2->hclk);
 
 	return ret;
 }
 
+static void rk3588_vop2_power_domain_enable_all(struct vop2 *vop2)
+{
+	u32 pd;
+
+	pd = vop2_readl(vop2, RK3588_SYS_PD_CTRL);
+	pd &= ~(VOP2_PD_CLUSTER0 | VOP2_PD_CLUSTER1 | VOP2_PD_CLUSTER2 |
+		VOP2_PD_CLUSTER3 | VOP2_PD_ESMART);
+
+	vop2_writel(vop2, RK3588_SYS_PD_CTRL, pd);
+}
+
 static void vop2_enable(struct vop2 *vop2)
 {
 	int ret;
@@ -906,6 +950,9 @@ static void vop2_enable(struct vop2 *vop2)
 	if (vop2->data->soc_id == 3566)
 		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
 
+	if (vop2->data->soc_id == 3588)
+		rk3588_vop2_power_domain_enable_all(vop2);
+
 	vop2_writel(vop2, RK3568_REG_CFG_DONE, RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
 
 	/*
@@ -931,6 +978,7 @@ static void vop2_disable(struct vop2 *vop2)
 
 	pm_runtime_put_sync(vop2->dev);
 
+	clk_disable_unprepare(vop2->pclk);
 	clk_disable_unprepare(vop2->aclk);
 	clk_disable_unprepare(vop2->hclk);
 }
@@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
 			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
 		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
 		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
-		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
+		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
+		else
+			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
+
 		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
 		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
 		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
@@ -1486,10 +1538,10 @@ static void vop2_post_config(struct drm_crtc *crtc)
 	vop2_vp_write(vp, RK3568_VP_DSP_BG, 0);
 }
 
-static void rk3568_set_intf_mux(struct vop2_video_port *vp, int id,
-				u32 polflags)
+static unsigned long rk3568_set_intf_mux(struct vop2_video_port *vp, int id, u32 polflags)
 {
 	struct vop2 *vop2 = vp->vop2;
+	struct drm_crtc *crtc = &vp->crtc;
 	u32 die, dip;
 
 	die = vop2_readl(vop2, RK3568_DSP_IF_EN);
@@ -1551,13 +1603,271 @@ static void rk3568_set_intf_mux(struct vop2_video_port *vp, int id,
 		break;
 	default:
 		drm_err(vop2->drm, "Invalid interface id %d on vp%d\n", id, vp->id);
-		return;
+		return 0;
+	}
+
+	dip |= RK3568_DSP_IF_POL__CFG_DONE_IMD;
+
+	vop2_writel(vop2, RK3568_DSP_IF_EN, die);
+	vop2_writel(vop2, RK3568_DSP_IF_POL, dip);
+
+	return crtc->state->adjusted_mode.crtc_clock  * 1000LL;
+}
+
+/*
+ * calc the dclk on rk3588
+ * the available div of dclk is 1, 2, 4
+ */
+static unsigned long rk3588_calc_dclk(unsigned long child_clk, unsigned long max_dclk)
+{
+	if (child_clk * 4 <= max_dclk)
+		return child_clk * 4;
+	else if (child_clk * 2 <= max_dclk)
+		return child_clk * 2;
+	else if (child_clk <= max_dclk)
+		return child_clk;
+	else
+		return 0;
+}
+
+/*
+ * 4 pixclk/cycle on rk3588
+ * RGB/eDP/HDMI: if_pixclk >= dclk_core
+ * DP: dp_pixclk = dclk_out <= dclk_core
+ * DSI: mipi_pixclk <= dclk_out <= dclk_core
+ */
+static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
+					 int *dclk_core_div, int *dclk_out_div,
+					 int *if_pixclk_div, int *if_dclk_div)
+{
+	struct vop2 *vop2 = vp->vop2;
+	struct drm_crtc *crtc = &vp->crtc;
+	struct drm_display_mode *adjusted_mode = &crtc->state->adjusted_mode;
+	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
+	int output_mode = vcstate->output_mode;
+	unsigned long v_pixclk = adjusted_mode->crtc_clock * 1000LL; /* video timing pixclk */
+	unsigned long dclk_core_rate = v_pixclk >> 2;
+	unsigned long dclk_rate = v_pixclk;
+	unsigned long dclk_out_rate;
+	unsigned long if_dclk_rate;
+	unsigned long if_pixclk_rate;
+	int K = 1;
+
+	if (vop2_output_if_is_hdmi(id)) {
+		/*
+		 * K = 2: dclk_core = if_pixclk_rate > if_dclk_rate
+		 * K = 1: dclk_core = hdmie_edp_dclk > if_pixclk_rate
+		 */
+		if (output_mode == ROCKCHIP_OUT_MODE_YUV420) {
+			dclk_rate = dclk_rate >> 1;
+			K = 2;
+		}
+
+		if_pixclk_rate = (dclk_core_rate << 1) / K;
+		if_dclk_rate = dclk_core_rate / K;
+		/*
+		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
+		 * *if_dclk_div = dclk_rate / if_dclk_rate;
+		 */
+		 *if_pixclk_div = 2;
+		 *if_dclk_div = 4;
+	} else if (vop2_output_if_is_edp(id)) {
+		/*
+		 * edp_pixclk = edp_dclk > dclk_core
+		 */
+		if_pixclk_rate = v_pixclk / K;
+		dclk_rate = if_pixclk_rate * K;
+		/*
+		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
+		 * *if_dclk_div = *if_pixclk_div;
+		 */
+		*if_pixclk_div = K;
+		*if_dclk_div = K;
+	} else if (vop2_output_if_is_dp(id)) {
+		if (output_mode == ROCKCHIP_OUT_MODE_YUV420)
+			dclk_out_rate = v_pixclk >> 3;
+		else
+			dclk_out_rate = v_pixclk >> 2;
+
+		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
+		if (!dclk_rate) {
+			drm_err(vop2->drm, "DP dclk_out_rate out of range, dclk_out_rate: %ld KHZ\n",
+				dclk_out_rate);
+			return 0;
+		}
+		*dclk_out_div = dclk_rate / dclk_out_rate;
+	} else if (vop2_output_if_is_mipi(id)) {
+		if_pixclk_rate = dclk_core_rate / K;
+		/*
+		 * dclk_core = dclk_out * K = if_pixclk * K = v_pixclk / 4
+		 */
+		dclk_out_rate = if_pixclk_rate;
+		/*
+		 * dclk_rate = N * dclk_core_rate N = (1,2,4 ),
+		 * we get a little factor here
+		 */
+		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
+		if (!dclk_rate) {
+			drm_err(vop2->drm, "MIPI dclk out of range, dclk_out_rate: %ld KHZ\n",
+				dclk_out_rate);
+			return 0;
+		}
+		*dclk_out_div = dclk_rate / dclk_out_rate;
+		/*
+		 * mipi pixclk == dclk_out
+		 */
+		*if_pixclk_div = 1;
+	} else if (vop2_output_if_is_dpi(id)) {
+		dclk_rate = v_pixclk;
+	}
+
+	*dclk_core_div = dclk_rate / dclk_core_rate;
+	*if_pixclk_div = ilog2(*if_pixclk_div);
+	*if_dclk_div = ilog2(*if_dclk_div);
+	*dclk_core_div = ilog2(*dclk_core_div);
+	*dclk_out_div = ilog2(*dclk_out_div);
+
+	drm_dbg(vop2->drm, "dclk: %ld, pixclk_div: %d, dclk_div: %d\n",
+		dclk_rate, *if_pixclk_div, *if_dclk_div);
+
+	return dclk_rate;
+}
+
+/*
+ * MIPI port mux on rk3588:
+ * 0: Video Port2
+ * 1: Video Port3
+ * 3: Video Port 1(MIPI1 only)
+ */
+static u32 rk3588_get_mipi_port_mux(int vp_id)
+{
+	if (vp_id == 1)
+		return 3;
+	else if (vp_id == 3)
+		return 1;
+	else
+		return 0;
+}
+
+static u32 rk3588_get_hdmi_pol(u32 flags)
+{
+	u32 val;
+
+	val = (flags & DRM_MODE_FLAG_NHSYNC) ? BIT(HSYNC_POSITIVE) : 0;
+	val |= (flags & DRM_MODE_FLAG_NVSYNC) ? BIT(VSYNC_POSITIVE) : 0;
+
+	return val;
+}
+
+static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32 polflags)
+{
+	struct vop2 *vop2 = vp->vop2;
+	int dclk_core_div, dclk_out_div, if_pixclk_div, if_dclk_div;
+	unsigned long clock;
+	u32 die, dip, div, vp_clk_div, val;
+
+	clock = rk3588_calc_cru_cfg(vp, id, &dclk_core_div, &dclk_out_div,
+				    &if_pixclk_div, &if_dclk_div);
+	if (!clock)
+		return 0;
+
+	vp_clk_div = FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_CORE_DIV, dclk_core_div);
+	vp_clk_div |= FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_OUT_DIV, dclk_out_div);
+
+	die = vop2_readl(vop2, RK3568_DSP_IF_EN);
+	dip = vop2_readl(vop2, RK3568_DSP_IF_POL);
+	div = vop2_readl(vop2, RK3568_DSP_IF_CTRL);
+
+	switch (id) {
+	case ROCKCHIP_VOP2_EP_HDMI0:
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV, if_dclk_div);
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_HDMI0 |
+			    FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX, vp->id);
+		val = rk3588_get_hdmi_pol(polflags);
+		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 1, 1));
+		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0, HIWORD_UPDATE(val, 6, 5));
+		break;
+	case ROCKCHIP_VOP2_EP_HDMI1:
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV, if_dclk_div);
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI1_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI1_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_HDMI1 |
+			    FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI1_MUX, vp->id);
+		val = rk3588_get_hdmi_pol(polflags);
+		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 4, 4));
+		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0, HIWORD_UPDATE(val, 8, 7));
+		break;
+	case ROCKCHIP_VOP2_EP_EDP0:
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV, if_dclk_div);
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_EDP0 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX, vp->id);
+		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 0, 0));
+		break;
+	case ROCKCHIP_VOP2_EP_EDP1:
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV, if_dclk_div);
+		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI1_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_EDP1 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI1_MUX, vp->id);
+		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 3, 3));
+		break;
+	case ROCKCHIP_VOP2_EP_MIPI0:
+		div |= FIELD_PREP(RK3588_DSP_IF_MIPI0_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_MIPI0_MUX;
+		val = rk3588_get_mipi_port_mux(vp->id);
+		die |= RK3588_SYS_DSP_INFACE_EN_MIPI0 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_MIPI0_MUX, !!val);
+		break;
+	case ROCKCHIP_VOP2_EP_MIPI1:
+		div |= FIELD_PREP(RK3588_DSP_IF_MIPI1_PCLK_DIV, if_pixclk_div);
+		die &= ~RK3588_SYS_DSP_INFACE_EN_MIPI1_MUX;
+		val = rk3588_get_mipi_port_mux(vp->id);
+		die |= RK3588_SYS_DSP_INFACE_EN_MIPI1 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_MIPI1_MUX, val);
+		break;
+	case ROCKCHIP_VOP2_EP_DP0:
+		die &= ~RK3588_SYS_DSP_INFACE_EN_DP0_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_DP0 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_DP0_MUX, vp->id);
+		dip &= ~RK3588_DSP_IF_POL__DP0_PIN_POL;
+		dip |= FIELD_PREP(RK3588_DSP_IF_POL__DP0_PIN_POL, polflags);
+		break;
+	case ROCKCHIP_VOP2_EP_DP1:
+		die &= ~RK3588_SYS_DSP_INFACE_EN_MIPI1_MUX;
+		die |= RK3588_SYS_DSP_INFACE_EN_MIPI1 |
+			   FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_MIPI1_MUX, vp->id);
+		dip &= ~RK3588_DSP_IF_POL__DP1_PIN_POL;
+		dip |= FIELD_PREP(RK3588_DSP_IF_POL__DP1_PIN_POL, polflags);
+		break;
+	default:
+		drm_err(vop2->drm, "Invalid interface id %d on vp%d\n", id, vp->id);
+		return 0;
 	}
 
 	dip |= RK3568_DSP_IF_POL__CFG_DONE_IMD;
 
+	vop2_vp_write(vp, RK3588_VP_CLK_CTRL, vp_clk_div);
 	vop2_writel(vop2, RK3568_DSP_IF_EN, die);
+	vop2_writel(vop2, RK3568_DSP_IF_CTRL, div);
 	vop2_writel(vop2, RK3568_DSP_IF_POL, dip);
+
+	return clock;
+}
+
+static unsigned long vop2_set_intf_mux(struct vop2_video_port *vp, int ep_id, u32 polflags)
+{
+	struct vop2 *vop2 = vp->vop2;
+
+	if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+		return rk3568_set_intf_mux(vp, ep_id, polflags);
+	else if (vop2->data->soc_id == 3588)
+		return rk3588_set_intf_mux(vp, ep_id, polflags);
+	else
+		return 0;
 }
 
 static int us_to_vertical_line(struct drm_display_mode *mode, int us)
@@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
 		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
 
-		rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
+		/*
+		 * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
+		 * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
+		 * system cru may be the 1/2 or 1/4 of mode->clock.
+		 */
+		clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
 	}
 
+	if (!clock)
+		return;
+
 	if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
 	    !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
 		out_mode = ROCKCHIP_OUT_MODE_P888;
@@ -1640,6 +1958,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	if (vop2_output_uv_swap(vcstate->bus_format, vcstate->output_mode))
 		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RB_SWAP;
+	if (vop2_output_rg_swap(vop2, vcstate->bus_format))
+		dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_RG_SWAP;
 
 	if (vcstate->yuv_overlay)
 		dsp_ctrl |= RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y;
@@ -2016,6 +2336,14 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 			port_sel &= ~RK3568_OVL_PORT_SEL__CLUSTER1;
 			port_sel |= FIELD_PREP(RK3568_OVL_PORT_SEL__CLUSTER1, vp->id);
 			break;
+		case ROCKCHIP_VOP2_CLUSTER2:
+			port_sel &= ~RK3588_OVL_PORT_SEL__CLUSTER2;
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SEL__CLUSTER2, vp->id);
+			break;
+		case ROCKCHIP_VOP2_CLUSTER3:
+			port_sel &= ~RK3588_OVL_PORT_SEL__CLUSTER3;
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SEL__CLUSTER3, vp->id);
+			break;
 		case ROCKCHIP_VOP2_ESMART0:
 			port_sel &= ~RK3568_OVL_PORT_SEL__ESMART0;
 			port_sel |= FIELD_PREP(RK3568_OVL_PORT_SEL__ESMART0, vp->id);
@@ -2024,6 +2352,14 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 			port_sel &= ~RK3568_OVL_PORT_SEL__ESMART1;
 			port_sel |= FIELD_PREP(RK3568_OVL_PORT_SEL__ESMART1, vp->id);
 			break;
+		case ROCKCHIP_VOP2_ESMART2:
+			port_sel &= ~RK3588_OVL_PORT_SEL__ESMART2;
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SEL__ESMART2, vp->id);
+			break;
+		case ROCKCHIP_VOP2_ESMART3:
+			port_sel &= ~RK3588_OVL_PORT_SEL__ESMART3;
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SEL__ESMART3, vp->id);
+			break;
 		case ROCKCHIP_VOP2_SMART0:
 			port_sel &= ~RK3568_OVL_PORT_SEL__SMART0;
 			port_sel |= FIELD_PREP(RK3568_OVL_PORT_SEL__SMART0, vp->id);
@@ -2763,8 +3099,31 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		if (IS_ERR(vop2->lut_regs))
 			return PTR_ERR(vop2->lut_regs);
 	}
+	if (vop2_data->feature & VOP2_FEATURE_HAS_SYS_GRF) {
+		vop2->sys_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
+		if (IS_ERR(vop2->sys_grf))
+			return dev_err_probe(dev, PTR_ERR(vop2->sys_grf), "cannot get sys_grf");
+	}
 
-	vop2->sys_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
+	if (vop2_data->feature & VOP2_FEATURE_HAS_VOP_GRF) {
+		vop2->vop_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,vop-grf");
+		if (IS_ERR(vop2->vop_grf))
+			return dev_err_probe(dev, PTR_ERR(vop2->vop_grf), "cannot get vop_grf");
+
+	}
+
+	if (vop2_data->feature & VOP2_FEATURE_HAS_VO1_GRF) {
+		vop2->vo1_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,vo1-grf");
+		if (IS_ERR(vop2->vo1_grf))
+			return dev_err_probe(dev, PTR_ERR(vop2->vo1_grf), "cannot get vo1_grf");
+
+	}
+
+	if (vop2_data->feature & VOP2_FEATURE_HAS_SYS_PMU) {
+		vop2->sys_pmu = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,pmu");
+		if (IS_ERR(vop2->sys_pmu))
+			return dev_err_probe(dev, PTR_ERR(vop2->sys_pmu), "cannot get sys_pmu");
+	}
 
 	vop2->hclk = devm_clk_get(vop2->dev, "hclk");
 	if (IS_ERR(vop2->hclk)) {
@@ -2778,6 +3137,12 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		return PTR_ERR(vop2->aclk);
 	}
 
+	vop2->pclk = devm_clk_get_optional(vop2->dev, "pclk_vop");
+	if (IS_ERR(vop2->pclk)) {
+		drm_err(vop2->drm, "failed to get pclk source\n");
+		return PTR_ERR(vop2->pclk);
+	}
+
 	vop2->irq = platform_get_irq(pdev, 0);
 	if (vop2->irq < 0) {
 		drm_err(vop2->drm, "cannot find irq for vop2\n");
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
index 7175f46a2014..bab3d92f8219 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -13,9 +13,16 @@
 
 #define VOP_FEATURE_OUTPUT_10BIT        BIT(0)
 
+#define VOP2_FEATURE_HAS_SYS_GRF	BIT(0)
+#define VOP2_FEATURE_HAS_VO0_GRF	BIT(1)
+#define VOP2_FEATURE_HAS_VO1_GRF	BIT(2)
+#define VOP2_FEATURE_HAS_VOP_GRF	BIT(3)
+#define VOP2_FEATURE_HAS_SYS_PMU	BIT(5)
+
 #define WIN_FEATURE_AFBDC		BIT(0)
 #define WIN_FEATURE_CLUSTER		BIT(1)
 
+#define HIWORD_UPDATE(v, h, l)  ((GENMASK(h, l) << 16) | ((v) << (l)))
 /*
  *  the delay number of a window in different mode.
  */
@@ -38,6 +45,18 @@ enum vop2_scale_down_mode {
 	VOP2_SCALE_DOWN_AVG,
 };
 
+/*
+ * vop2 internal power domain id,
+ * should be all none zero, 0 will be treat as invalid;
+ */
+#define VOP2_PD_CLUSTER0	BIT(0)
+#define VOP2_PD_CLUSTER1	BIT(1)
+#define VOP2_PD_CLUSTER2	BIT(2)
+#define VOP2_PD_CLUSTER3	BIT(3)
+#define VOP2_PD_DSC_8K		BIT(5)
+#define VOP2_PD_DSC_4K		BIT(6)
+#define VOP2_PD_ESMART		BIT(7)
+
 enum vop2_win_regs {
 	VOP2_WIN_ENABLE,
 	VOP2_WIN_FORMAT,
@@ -138,6 +157,7 @@ struct vop2_video_port_data {
 
 struct vop2_data {
 	u8 nr_vps;
+	u64 feature;
 	const struct vop2_win_data *win;
 	const struct vop2_video_port_data *vp;
 	struct vop_rect max_input;
@@ -192,6 +212,11 @@ enum dst_factor_mode {
 };
 
 #define RK3568_GRF_VO_CON1			0x0364
+
+#define RK3588_GRF_SOC_CON1			0x0304
+#define RK3588_GRF_VOP_CON2			0x08
+#define RK3588_GRF_VO1_CON0			0x00
+
 /* System registers definition */
 #define RK3568_REG_CFG_DONE			0x000
 #define RK3568_VERSION_INFO			0x004
@@ -200,6 +225,7 @@ enum dst_factor_mode {
 #define RK3568_DSP_IF_EN			0x028
 #define RK3568_DSP_IF_CTRL			0x02c
 #define RK3568_DSP_IF_POL			0x030
+#define RK3588_SYS_PD_CTRL			0x034
 #define RK3568_WB_CTRL				0x40
 #define RK3568_WB_XSCAL_FACTOR			0x44
 #define RK3568_WB_YRGB_MST			0x48
@@ -223,6 +249,7 @@ enum dst_factor_mode {
 #define RK3568_VP_DSP_CTRL			0x00
 #define RK3568_VP_MIPI_CTRL			0x04
 #define RK3568_VP_COLOR_BAR_CTRL		0x08
+#define RK3588_VP_CLK_CTRL			0x0C
 #define RK3568_VP_3D_LUT_CTRL			0x10
 #define RK3568_VP_3D_LUT_MST			0x20
 #define RK3568_VP_DSP_BG			0x2C
@@ -357,13 +384,18 @@ enum dst_factor_mode {
 #define RK3568_VP_DSP_CTRL__DITHER_DOWN_EN		BIT(17)
 #define RK3568_VP_DSP_CTRL__PRE_DITHER_DOWN_EN		BIT(16)
 #define RK3568_VP_DSP_CTRL__POST_DSP_OUT_R2Y		BIT(15)
+#define RK3568_VP_DSP_CTRL__DSP_RG_SWAP			BIT(10)
 #define RK3568_VP_DSP_CTRL__DSP_RB_SWAP			BIT(9)
+#define RK3568_VP_DSP_CTRL__DSP_BG_SWAP			BIT(8)
 #define RK3568_VP_DSP_CTRL__DSP_INTERLACE		BIT(7)
 #define RK3568_VP_DSP_CTRL__DSP_FILED_POL		BIT(6)
 #define RK3568_VP_DSP_CTRL__P2I_EN			BIT(5)
 #define RK3568_VP_DSP_CTRL__CORE_DCLK_DIV		BIT(4)
 #define RK3568_VP_DSP_CTRL__OUT_MODE			GENMASK(3, 0)
 
+#define RK3588_VP_CLK_CTRL__DCLK_OUT_DIV		GENMASK(3, 2)
+#define RK3588_VP_CLK_CTRL__DCLK_CORE_DIV		GENMASK(1, 0)
+
 #define RK3568_VP_POST_SCL_CTRL__VSCALEDOWN		BIT(1)
 #define RK3568_VP_POST_SCL_CTRL__HSCALEDOWN		BIT(0)
 
@@ -382,11 +414,37 @@ enum dst_factor_mode {
 #define RK3568_SYS_DSP_INFACE_EN_HDMI			BIT(1)
 #define RK3568_SYS_DSP_INFACE_EN_RGB			BIT(0)
 
+#define RK3588_SYS_DSP_INFACE_EN_MIPI1_MUX		GENMASK(22, 21)
+#define RK3588_SYS_DSP_INFACE_EN_MIPI0_MUX		GENMASK(20, 20)
+#define RK3588_SYS_DSP_INFACE_EN_EDP_HDMI1_MUX		GENMASK(19, 18)
+#define RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX		GENMASK(17, 16)
+#define RK3588_SYS_DSP_INFACE_EN_DP1_MUX		GENMASK(15, 14)
+#define RK3588_SYS_DSP_INFACE_EN_DP0_MUX		GENMASK(13, 12)
+#define RK3588_SYS_DSP_INFACE_EN_DPI			GENMASK(9, 8)
+#define RK3588_SYS_DSP_INFACE_EN_MIPI1			BIT(7)
+#define RK3588_SYS_DSP_INFACE_EN_MIPI0			BIT(6)
+#define RK3588_SYS_DSP_INFACE_EN_HDMI1			BIT(5)
+#define RK3588_SYS_DSP_INFACE_EN_EDP1			BIT(4)
+#define RK3588_SYS_DSP_INFACE_EN_HDMI0			BIT(3)
+#define RK3588_SYS_DSP_INFACE_EN_EDP0			BIT(2)
+#define RK3588_SYS_DSP_INFACE_EN_DP1			BIT(1)
+#define RK3588_SYS_DSP_INFACE_EN_DP0			BIT(0)
+
+#define RK3588_DSP_IF_MIPI1_PCLK_DIV			GENMASK(27, 26)
+#define RK3588_DSP_IF_MIPI0_PCLK_DIV			GENMASK(25, 24)
+#define RK3588_DSP_IF_EDP_HDMI1_PCLK_DIV		GENMASK(22, 22)
+#define RK3588_DSP_IF_EDP_HDMI1_DCLK_DIV		GENMASK(21, 20)
+#define RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV		GENMASK(18, 18)
+#define RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV		GENMASK(17, 16)
+
 #define RK3568_DSP_IF_POL__MIPI_PIN_POL			GENMASK(19, 16)
 #define RK3568_DSP_IF_POL__EDP_PIN_POL			GENMASK(15, 12)
 #define RK3568_DSP_IF_POL__HDMI_PIN_POL			GENMASK(7, 4)
 #define RK3568_DSP_IF_POL__RGB_LVDS_PIN_POL		GENMASK(3, 0)
 
+#define RK3588_DSP_IF_POL__DP1_PIN_POL			GENMASK(14, 12)
+#define RK3588_DSP_IF_POL__DP0_PIN_POL			GENMASK(10, 8)
+
 #define RK3568_VP0_MIPI_CTRL__DCLK_DIV2_PHASE_LOCK	BIT(5)
 #define RK3568_VP0_MIPI_CTRL__DCLK_DIV2			BIT(4)
 
@@ -407,8 +465,12 @@ enum dst_factor_mode {
 #define RK3568_OVL_PORT_SEL__SEL_PORT			GENMASK(31, 16)
 #define RK3568_OVL_PORT_SEL__SMART1			GENMASK(31, 30)
 #define RK3568_OVL_PORT_SEL__SMART0			GENMASK(29, 28)
+#define RK3588_OVL_PORT_SEL__ESMART3			GENMASK(31, 30)
+#define RK3588_OVL_PORT_SEL__ESMART2			GENMASK(29, 28)
 #define RK3568_OVL_PORT_SEL__ESMART1			GENMASK(27, 26)
 #define RK3568_OVL_PORT_SEL__ESMART0			GENMASK(25, 24)
+#define RK3588_OVL_PORT_SEL__CLUSTER3			GENMASK(23, 22)
+#define RK3588_OVL_PORT_SEL__CLUSTER2			GENMASK(21, 20)
 #define RK3568_OVL_PORT_SEL__CLUSTER1			GENMASK(19, 18)
 #define RK3568_OVL_PORT_SEL__CLUSTER0			GENMASK(17, 16)
 #define RK3568_OVL_PORT_SET__PORT2_MUX			GENMASK(11, 8)
@@ -421,6 +483,10 @@ enum dst_factor_mode {
 #define RK3568_CLUSTER_DLY_NUM__CLUSTER0_1		GENMASK(15, 8)
 #define RK3568_CLUSTER_DLY_NUM__CLUSTER0_0		GENMASK(7, 0)
 
+#define RK3568_CLUSTER_WIN_CTRL0__WIN0_EN		BIT(0)
+
+#define RK3568_SMART_REGION0_CTRL__WIN0_EN		BIT(0)
+
 #define RK3568_SMART_DLY_NUM__SMART1			GENMASK(31, 24)
 #define RK3568_SMART_DLY_NUM__SMART0			GENMASK(23, 16)
 #define RK3568_SMART_DLY_NUM__ESMART1			GENMASK(15, 8)
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
index 2c45d81983a5..275d265891db 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
@@ -34,6 +34,30 @@ static const uint32_t formats_cluster[] = {
 	DRM_FORMAT_Y210, /* yuv422_10bit non-Linear mode only */
 };
 
+static const uint32_t formats_esmart[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_NV12, /* yuv420_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV21, /* yvu420_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV16, /* yuv422_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV61, /* yvu422_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV20, /* yuv422_10bit linear mode, 2 plane, no padding */
+	DRM_FORMAT_NV24, /* yuv444_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV42, /* yvu444_8bit linear mode, 2 plane */
+	DRM_FORMAT_NV30, /* yuv444_10bit linear mode, 2 plane, no padding */
+	DRM_FORMAT_NV15, /* yuv420_10bit linear mode, 2 plane, no padding */
+	DRM_FORMAT_YVYU, /* yuv422_8bit[YVYU] linear mode */
+	DRM_FORMAT_VYUY, /* yuv422_8bit[VYUY] linear mode */
+	DRM_FORMAT_YUYV, /* yuv422_8bit[YUYV] linear mode */
+	DRM_FORMAT_UYVY, /* yuv422_8bit[UYVY] linear mode */
+};
+
 static const uint32_t formats_rk356x_esmart[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
@@ -236,7 +260,188 @@ static const struct vop2_win_data rk3568_vop_win_data[] = {
 	},
 };
 
+static const struct vop2_video_port_data rk3588_vop_video_ports[] = {
+	{
+		.id = 0,
+		.feature = VOP_FEATURE_OUTPUT_10BIT,
+		.gamma_lut_len = 1024,
+		.cubic_lut_len = 9 * 9 * 9, /* 9x9x9 */
+		.max_output = { 4096, 2304 },
+		/* hdr2sdr sdr2hdr hdr2hdr sdr2sdr */
+		.pre_scan_max_dly = { 76, 65, 65, 54 },
+		.offset = 0xc00,
+	}, {
+		.id = 1,
+		.feature = VOP_FEATURE_OUTPUT_10BIT,
+		.gamma_lut_len = 1024,
+		.cubic_lut_len = 729, /* 9x9x9 */
+		.max_output = { 4096, 2304 },
+		.pre_scan_max_dly = { 76, 65, 65, 54 },
+		.offset = 0xd00,
+	}, {
+		.id = 2,
+		.feature = VOP_FEATURE_OUTPUT_10BIT,
+		.gamma_lut_len = 1024,
+		.cubic_lut_len = 17 * 17 * 17, /* 17x17x17 */
+		.max_output = { 4096, 2304 },
+		.pre_scan_max_dly = { 52, 52, 52, 52 },
+		.offset = 0xe00,
+	}, {
+		.id = 3,
+		.gamma_lut_len = 1024,
+		.max_output = { 2048, 1536 },
+		.pre_scan_max_dly = { 52, 52, 52, 52 },
+		.offset = 0xf00,
+	},
+};
+
+/*
+ * rk3588 vop with 4 cluster, 4 esmart win.
+ * Every cluster can work as 4K win or split into two win.
+ * All win in cluster support AFBCD.
+ *
+ * Every esmart win and smart win support 4 Multi-region.
+ *
+ * Scale filter mode:
+ *
+ * * Cluster:  bicubic for horizontal scale up, others use bilinear
+ * * ESmart:
+ *    * nearest-neighbor/bilinear/bicubic for scale up
+ *    * nearest-neighbor/bilinear/average for scale down
+ *
+ * AXI Read ID assignment:
+ * Two AXI bus:
+ * AXI0 is a read/write bus with a higher performance.
+ * AXI1 is a read only bus.
+ *
+ * Every window on a AXI bus must assigned two unique
+ * read id(yrgb_id/uv_id, valid id are 0x1~0xe).
+ *
+ * AXI0:
+ * Cluster0/1, Esmart0/1, WriteBack
+ *
+ * AXI 1:
+ * Cluster2/3, Esmart2/3
+ *
+ */
+static const struct vop2_win_data rk3588_vop_win_data[] = {
+	{
+		.name = "Cluster0-win0",
+		.phys_id = ROCKCHIP_VOP2_CLUSTER0,
+		.base = 0x1000,
+		.formats = formats_cluster,
+		.nformats = ARRAY_SIZE(formats_cluster),
+		.format_modifiers = format_modifiers_afbc,
+		.layer_sel_id = 0,
+		.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270 |
+				       DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y,
+		.max_upscale_factor = 4,
+		.max_downscale_factor = 4,
+		.dly = { 4, 26, 29 },
+		.type = DRM_PLANE_TYPE_PRIMARY,
+		.feature = WIN_FEATURE_AFBDC | WIN_FEATURE_CLUSTER,
+	}, {
+		.name = "Cluster1-win0",
+		.phys_id = ROCKCHIP_VOP2_CLUSTER1,
+		.base = 0x1200,
+		.formats = formats_cluster,
+		.nformats = ARRAY_SIZE(formats_cluster),
+		.format_modifiers = format_modifiers_afbc,
+		.layer_sel_id = 1,
+		.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270 |
+				       DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_PRIMARY,
+		.max_upscale_factor = 4,
+		.max_downscale_factor = 4,
+		.dly = { 4, 26, 29 },
+		.feature = WIN_FEATURE_AFBDC | WIN_FEATURE_CLUSTER,
+	}, {
+		.name = "Cluster2-win0",
+		.phys_id = ROCKCHIP_VOP2_CLUSTER2,
+		.base = 0x1400,
+		.formats = formats_cluster,
+		.nformats = ARRAY_SIZE(formats_cluster),
+		.format_modifiers = format_modifiers_afbc,
+		.layer_sel_id = 4,
+		.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270 |
+				       DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_PRIMARY,
+		.max_upscale_factor = 4,
+		.max_downscale_factor = 4,
+		.dly = { 4, 26, 29 },
+		.feature = WIN_FEATURE_AFBDC | WIN_FEATURE_CLUSTER,
+	}, {
+		.name = "Cluster3-win0",
+		.phys_id = ROCKCHIP_VOP2_CLUSTER3,
+		.base = 0x1600,
+		.formats = formats_cluster,
+		.nformats = ARRAY_SIZE(formats_cluster),
+		.format_modifiers = format_modifiers_afbc,
+		.layer_sel_id = 5,
+		.supported_rotations = DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270 |
+				       DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_PRIMARY,
+		.max_upscale_factor = 4,
+		.max_downscale_factor = 4,
+		.dly = { 4, 26, 29 },
+		.feature = WIN_FEATURE_AFBDC | WIN_FEATURE_CLUSTER,
+	}, {
+		.name = "Esmart0-win0",
+		.phys_id = ROCKCHIP_VOP2_ESMART0,
+		.formats = formats_esmart,
+		.nformats = ARRAY_SIZE(formats_esmart),
+		.format_modifiers = format_modifiers,
+		.base = 0x1800,
+		.layer_sel_id = 2,
+		.supported_rotations = DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_OVERLAY,
+		.max_upscale_factor = 8,
+		.max_downscale_factor = 8,
+		.dly = { 23, 45, 48 },
+	}, {
+		.name = "Esmart1-win0",
+		.phys_id = ROCKCHIP_VOP2_ESMART1,
+		.formats = formats_esmart,
+		.nformats = ARRAY_SIZE(formats_esmart),
+		.format_modifiers = format_modifiers,
+		.base = 0x1a00,
+		.layer_sel_id = 3,
+		.supported_rotations = DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_OVERLAY,
+		.max_upscale_factor = 8,
+		.max_downscale_factor = 8,
+		.dly = { 23, 45, 48 },
+	}, {
+		.name = "Esmart2-win0",
+		.phys_id = ROCKCHIP_VOP2_ESMART2,
+		.base = 0x1c00,
+		.formats = formats_esmart,
+		.nformats = ARRAY_SIZE(formats_esmart),
+		.format_modifiers = format_modifiers,
+		.layer_sel_id = 6,
+		.supported_rotations = DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_OVERLAY,
+		.max_upscale_factor = 8,
+		.max_downscale_factor = 8,
+		.dly = { 23, 45, 48 },
+	}, {
+		.name = "Esmart3-win0",
+		.phys_id = ROCKCHIP_VOP2_ESMART3,
+		.formats = formats_esmart,
+		.nformats = ARRAY_SIZE(formats_esmart),
+		.format_modifiers = format_modifiers,
+		.base = 0x1e00,
+		.layer_sel_id = 7,
+		.supported_rotations = DRM_MODE_REFLECT_Y,
+		.type = DRM_PLANE_TYPE_OVERLAY,
+		.max_upscale_factor = 8,
+		.max_downscale_factor = 8,
+		.dly = { 23, 45, 48 },
+	},
+};
+
 static const struct vop2_data rk3566_vop = {
+	.feature = VOP2_FEATURE_HAS_SYS_GRF,
 	.nr_vps = 3,
 	.max_input = { 4096, 2304 },
 	.max_output = { 4096, 2304 },
@@ -247,6 +452,7 @@ static const struct vop2_data rk3566_vop = {
 };
 
 static const struct vop2_data rk3568_vop = {
+	.feature = VOP2_FEATURE_HAS_SYS_GRF,
 	.nr_vps = 3,
 	.max_input = { 4096, 2304 },
 	.max_output = { 4096, 2304 },
@@ -256,6 +462,18 @@ static const struct vop2_data rk3568_vop = {
 	.soc_id = 3568,
 };
 
+static const struct vop2_data rk3588_vop = {
+	.feature = VOP2_FEATURE_HAS_SYS_GRF | VOP2_FEATURE_HAS_VO1_GRF |
+		   VOP2_FEATURE_HAS_VOP_GRF | VOP2_FEATURE_HAS_SYS_PMU,
+	.nr_vps = 4,
+	.max_input = { 4096, 4320 },
+	.max_output = { 4096, 4320 },
+	.vp = rk3588_vop_video_ports,
+	.win = rk3588_vop_win_data,
+	.win_size = ARRAY_SIZE(rk3588_vop_win_data),
+	.soc_id = 3588,
+};
+
 static const struct of_device_id vop2_dt_match[] = {
 	{
 		.compatible = "rockchip,rk3566-vop",
@@ -263,6 +481,9 @@ static const struct of_device_id vop2_dt_match[] = {
 	}, {
 		.compatible = "rockchip,rk3568-vop",
 		.data = &rk3568_vop,
+	}, {
+		.compatible = "rockchip,rk3588-vop",
+		.data = &rk3588_vop
 	}, {
 	},
 };
-- 
2.34.1

Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Sascha Hauer]

Hi Andy,

Looks good overall, two small things inside.

On Wed, Nov 22, 2023 at 08:55:44PM +0800, Andy Yan wrote:
>  
> +#define vop2_output_if_is_hdmi(x)	(x == ROCKCHIP_VOP2_EP_HDMI0 || x == ROCKCHIP_VOP2_EP_HDMI1)
> +#define vop2_output_if_is_dp(x)		(x == ROCKCHIP_VOP2_EP_DP0 || x == ROCKCHIP_VOP2_EP_DP1)
> +#define vop2_output_if_is_edp(x)	(x == ROCKCHIP_VOP2_EP_EDP0 || x == ROCKCHIP_VOP2_EP_EDP1)
> +#define vop2_output_if_is_mipi(x)	(x == ROCKCHIP_VOP2_EP_MIPI0 || x == ROCKCHIP_VOP2_EP_MIPI1)
> +#define vop2_output_if_is_lvds(x)	(x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
> +#define vop2_output_if_is_dpi(x)	(x == ROCKCHIP_VOP2_EP_RGB0)

Not that it matters in practice here, but you should add braces around
the x argument in the macros usage, i.e. ((x) == ROCKCHIP_VOP2_EP_RGB0)

> +static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32 polflags)
> +{
> +	struct vop2 *vop2 = vp->vop2;
> +	int dclk_core_div, dclk_out_div, if_pixclk_div, if_dclk_div;
> +	unsigned long clock;
> +	u32 die, dip, div, vp_clk_div, val;
> +
> +	clock = rk3588_calc_cru_cfg(vp, id, &dclk_core_div, &dclk_out_div,
> +				    &if_pixclk_div, &if_dclk_div);
> +	if (!clock)
> +		return 0;
> +
> +	vp_clk_div = FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_CORE_DIV, dclk_core_div);
> +	vp_clk_div |= FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_OUT_DIV, dclk_out_div);
> +
> +	die = vop2_readl(vop2, RK3568_DSP_IF_EN);
> +	dip = vop2_readl(vop2, RK3568_DSP_IF_POL);
> +	div = vop2_readl(vop2, RK3568_DSP_IF_CTRL);
> +
> +	switch (id) {
> +	case ROCKCHIP_VOP2_EP_HDMI0:
> +		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV, if_dclk_div);

you should clear the bits of a mask before setting them again. The same
goes for several other bits modified in this switch/case.

> +		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV, if_pixclk_div);
> +		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX;
> +		die |= RK3588_SYS_DSP_INFACE_EN_HDMI0 |
> +			    FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX, vp->id);
> +		val = rk3588_get_hdmi_pol(polflags);
> +		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 1, 1));
> +		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0, HIWORD_UPDATE(val, 6, 5));
> +		break;

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 v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Andy Yan]

Hi Sascha,

On 11/27/23 19:19, Sascha Hauer wrote:
> Hi Andy,
>
> Looks good overall, two small things inside.
>
> On Wed, Nov 22, 2023 at 08:55:44PM +0800, Andy Yan wrote:
>>   
>> +#define vop2_output_if_is_hdmi(x)	(x == ROCKCHIP_VOP2_EP_HDMI0 || x == ROCKCHIP_VOP2_EP_HDMI1)
>> +#define vop2_output_if_is_dp(x)		(x == ROCKCHIP_VOP2_EP_DP0 || x == ROCKCHIP_VOP2_EP_DP1)
>> +#define vop2_output_if_is_edp(x)	(x == ROCKCHIP_VOP2_EP_EDP0 || x == ROCKCHIP_VOP2_EP_EDP1)
>> +#define vop2_output_if_is_mipi(x)	(x == ROCKCHIP_VOP2_EP_MIPI0 || x == ROCKCHIP_VOP2_EP_MIPI1)
>> +#define vop2_output_if_is_lvds(x)	(x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
>> +#define vop2_output_if_is_dpi(x)	(x == ROCKCHIP_VOP2_EP_RGB0)
> Not that it matters in practice here, but you should add braces around
> the x argument in the macros usage, i.e. ((x) == ROCKCHIP_VOP2_EP_RGB0)
Okay , will do.
>> +static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id, u32 polflags)
>> +{
>> +	struct vop2 *vop2 = vp->vop2;
>> +	int dclk_core_div, dclk_out_div, if_pixclk_div, if_dclk_div;
>> +	unsigned long clock;
>> +	u32 die, dip, div, vp_clk_div, val;
>> +
>> +	clock = rk3588_calc_cru_cfg(vp, id, &dclk_core_div, &dclk_out_div,
>> +				    &if_pixclk_div, &if_dclk_div);
>> +	if (!clock)
>> +		return 0;
>> +
>> +	vp_clk_div = FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_CORE_DIV, dclk_core_div);
>> +	vp_clk_div |= FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_OUT_DIV, dclk_out_div);
>> +
>> +	die = vop2_readl(vop2, RK3568_DSP_IF_EN);
>> +	dip = vop2_readl(vop2, RK3568_DSP_IF_POL);
>> +	div = vop2_readl(vop2, RK3568_DSP_IF_CTRL);
>> +
>> +	switch (id) {
>> +	case ROCKCHIP_VOP2_EP_HDMI0:
>> +		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV, if_dclk_div);
> you should clear the bits of a mask before setting them again. The same
> goes for several other bits modified in this switch/case.


Thanks for catching this, will fixed in next version.

>
>> +		div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV, if_pixclk_div);
>> +		die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX;
>> +		die |= RK3588_SYS_DSP_INFACE_EN_HDMI0 |
>> +			    FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX, vp->id);
>> +		val = rk3588_get_hdmi_pol(polflags);
>> +		regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2, HIWORD_UPDATE(1, 1, 1));
>> +		regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0, HIWORD_UPDATE(val, 6, 5));
>> +		break;
> Sascha
>

Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Heiko Stübner]

Hi Andy,

Am Mittwoch, 22. November 2023, 13:55:44 CET schrieb Andy Yan:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> VOP2 on rk3588:
> 
> Four video ports:
> VP0 Max 4096x2160
> VP1 Max 4096x2160
> VP2 Max 4096x2160
> VP3 Max 2048x1080
> 
> 4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
> 4 4K Esmart windows with line RGB/YUV support
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - add rk3588_ prefix for functions which are rk3588 only
> - make some calculation as fixed value and keep calculation formula as
>   comment
> - check return value for some cru calculation functions.
> - check return value for syscon_regmap_lookup_by_phandle
> - add NV20/NV30 for esmart plane
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  66 ++++
>  drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
>  3 files changed, 660 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 4bcc405bcf11..9eecbe1f71f9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

> @@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
>  static void vop2_cfg_done(struct vop2_video_port *vp)
>  {
>  	struct vop2 *vop2 = vp->vop2;
> +	u32 val;
> +
> +	val = BIT(vp->id) | (BIT(vp->id) << 16) |
> +		RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
>  
> -	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
> -			BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
> +	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);

I don't fully understand that code:
(1) the write mask is also present on the rk3568, so should this change
    be a separate patch with a fixes tag?
(2) RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN does not contain the part for
    the write-mask

	#define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN     BIT(15)

    why is this working then?

>  }
>  
>  static void vop2_win_disable(struct vop2_win *win)

[...]

> @@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
>  		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
>  		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
> -		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
> +		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
> +		else
> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
> +

I think this at least warrants a comment, what is happening here. Also,
can you already see how future vop2-users are behaving - aka are all new
socs in the "else" part of the conditional, or would a switch-case better
represent future socs?


>  		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
>  		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
>  		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);


> @@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>  		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>  
> -		rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
> +		/*
> +		 * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
> +		 * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
> +		 * system cru may be the 1/2 or 1/4 of mode->clock.
> +		 */
> +		clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
>  	}
>  
> +	if (!clock)
> +		return;
> +

hmm, shouldn't the check for the validity of a mode happen before
atomic_enable is run? So this shouldn't error out in the middle of the
function?


>  	if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
>  	    !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
>  		out_mode = ROCKCHIP_OUT_MODE_P888;


Thanks
Heiko

Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Andy Yan]

Hi Heiko:

Thanks for you review.

On 11/27/23 23:29, Heiko Stübner wrote:
> Hi Andy,
>
> Am Mittwoch, 22. November 2023, 13:55:44 CET schrieb Andy Yan:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> VOP2 on rk3588:
>>
>> Four video ports:
>> VP0 Max 4096x2160
>> VP1 Max 4096x2160
>> VP2 Max 4096x2160
>> VP3 Max 2048x1080
>>
>> 4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
>> 4 4K Esmart windows with line RGB/YUV support
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - add rk3588_ prefix for functions which are rk3588 only
>> - make some calculation as fixed value and keep calculation formula as
>>    comment
>> - check return value for some cru calculation functions.
>> - check return value for syscon_regmap_lookup_by_phandle
>> - add NV20/NV30 for esmart plane
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  66 ++++
>>   drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
>>   3 files changed, 660 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 4bcc405bcf11..9eecbe1f71f9 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
>>   static void vop2_cfg_done(struct vop2_video_port *vp)
>>   {
>>   	struct vop2 *vop2 = vp->vop2;
>> +	u32 val;
>> +
>> +	val = BIT(vp->id) | (BIT(vp->id) << 16) |
>> +		RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
>>   
>> -	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
>> -			BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
>> +	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);
> I don't fully understand that code:
> (1) the write mask is also present on the rk3568, so should this change
>      be a separate patch with a fixes tag?

The write mask of VP config done on rk356x is missing, that means

you can write the corresponding mask bit, but it has no effect.

I once considered making it a separate patch,  I can split it as a separate patch if

you like.

> (2) RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN does not contain the part for
>      the write-mask
>
> 	#define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN     BIT(15)
>
>      why is this working then?


Actually this bit has no write-mask bit. 🙂

>
>>   }
>>   
>>   static void vop2_win_disable(struct vop2_win *win)
> [...]
>
>> @@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>>   			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
>>   		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
>>   		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
>> -		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
>> +		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
>> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
>> +		else
>> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
>> +
> I think this at least warrants a comment, what is happening here. Also,
> can you already see how future vop2-users are behaving - aka are all new
> socs in the "else" part of the conditional, or would a switch-case better
> represent future socs?


On rk356x, this bit is auto gating enable, but this function is not work well so

we need to disable this function.

On rk3588, and the following new soc(rk3528/rk3576), this bit is gating disable,

we should write 1 to disable gating when enable a cluster window.


Maybe i add some comments in next version ?



>
>>   		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
>>   		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
>>   		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
>
>> @@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>   	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>   		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>   
>> -		rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
>> +		/*
>> +		 * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
>> +		 * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
>> +		 * system cru may be the 1/2 or 1/4 of mode->clock.
>> +		 */
>> +		clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
>>   	}
>>   
>> +	if (!clock)
>> +		return;
>> +
> hmm, shouldn't the check for the validity of a mode happen before
> atomic_enable is run? So this shouldn't error out in the middle of the
> function?
>
>
>>   	if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
>>   	    !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
>>   		out_mode = ROCKCHIP_OUT_MODE_P888;
>
> Thanks
> Heiko
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Heiko Stübner]

Hi Andy,

Am Dienstag, 28. November 2023, 10:32:55 CET schrieb Andy Yan:
> On 11/27/23 23:29, Heiko Stübner wrote:
> > Am Mittwoch, 22. November 2023, 13:55:44 CET schrieb Andy Yan:
> >> From: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> VOP2 on rk3588:
> >>
> >> Four video ports:
> >> VP0 Max 4096x2160
> >> VP1 Max 4096x2160
> >> VP2 Max 4096x2160
> >> VP3 Max 2048x1080
> >>
> >> 4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
> >> 4 4K Esmart windows with line RGB/YUV support
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >>
> >> ---
> >>
> >> Changes in v2:
> >> - add rk3588_ prefix for functions which are rk3588 only
> >> - make some calculation as fixed value and keep calculation formula as
> >>    comment
> >> - check return value for some cru calculation functions.
> >> - check return value for syscon_regmap_lookup_by_phandle
> >> - add NV20/NV30 for esmart plane
> >>
> >>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
> >>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  66 ++++
> >>   drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
> >>   3 files changed, 660 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> index 4bcc405bcf11..9eecbe1f71f9 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> @@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
> >>   static void vop2_cfg_done(struct vop2_video_port *vp)
> >>   {
> >>   	struct vop2 *vop2 = vp->vop2;
> >> +	u32 val;
> >> +
> >> +	val = BIT(vp->id) | (BIT(vp->id) << 16) |
> >> +		RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
> >>   
> >> -	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
> >> -			BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
> >> +	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);
> > I don't fully understand that code:
> > (1) the write mask is also present on the rk3568, so should this change
> >      be a separate patch with a fixes tag?
> 
> The write mask of VP config done on rk356x is missing, that means
> you can write the corresponding mask bit, but it has no effect.
>
> I once considered making it a separate patch,  I can split it as a separate patch if
> you like.

I think I'd like it to be a separate patch please.


> > (2) RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN does not contain the part for
> >      the write-mask
> >
> > 	#define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN     BIT(15)
> >
> >      why is this working then?
> 
> 
> Actually this bit has no write-mask bit. 🙂

when doing that separate patch mentioned above, could you also add a
comment to the code stating that RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN
doesn't have a write mask bit please?

Because the TRM is not clear and ideally I'd not forget this fact for
the future :-) .


> >>   }
> >>   
> >>   static void vop2_win_disable(struct vop2_win *win)
> > [...]
> >
> >> @@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
> >>   			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
> >>   		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
> >>   		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
> >> -		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
> >> +		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
> >> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
> >> +		else
> >> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
> >> +
> > I think this at least warrants a comment, what is happening here. Also,
> > can you already see how future vop2-users are behaving - aka are all new
> > socs in the "else" part of the conditional, or would a switch-case better
> > represent future socs?
> 
> 
> On rk356x, this bit is auto gating enable, but this function is not work well so
> we need to disable this function.
> On rk3588, and the following new soc(rk3528/rk3576), this bit is gating disable,
> we should write 1 to disable gating when enable a cluster window.
> 
> 
> Maybe i add some comments in next version ?

Yep that comment would be helpful. And with your explanation the code
itself can stay as it is :-)

Thanks
Heiko


> >>   		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
> >>   		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
> >>   		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
> >
> >> @@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> >>   	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> >>   		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> >>   
> >> -		rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
> >> +		/*
> >> +		 * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
> >> +		 * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
> >> +		 * system cru may be the 1/2 or 1/4 of mode->clock.
> >> +		 */
> >> +		clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
> >>   	}
> >>   
> >> +	if (!clock)
> >> +		return;
> >> +
> > hmm, shouldn't the check for the validity of a mode happen before
> > atomic_enable is run? So this shouldn't error out in the middle of the
> > function?
> >
> >
> >>   	if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
> >>   	    !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
> >>   		out_mode = ROCKCHIP_OUT_MODE_P888;
> >
> > Thanks
> > Heiko
> >
> >
> >
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588

[Andy Yan]

Hi Heiko:

On 11/28/23 17:44, Heiko Stübner wrote:
> Hi Andy,
>
> Am Dienstag, 28. November 2023, 10:32:55 CET schrieb Andy Yan:
>> On 11/27/23 23:29, Heiko Stübner wrote:
>>> Am Mittwoch, 22. November 2023, 13:55:44 CET schrieb Andy Yan:
>>>> From: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> VOP2 on rk3588:
>>>>
>>>> Four video ports:
>>>> VP0 Max 4096x2160
>>>> VP1 Max 4096x2160
>>>> VP2 Max 4096x2160
>>>> VP3 Max 2048x1080
>>>>
>>>> 4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
>>>> 4 4K Esmart windows with line RGB/YUV support
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - add rk3588_ prefix for functions which are rk3588 only
>>>> - make some calculation as fixed value and keep calculation formula as
>>>>     comment
>>>> - check return value for some cru calculation functions.
>>>> - check return value for syscon_regmap_lookup_by_phandle
>>>> - add NV20/NV30 for esmart plane
>>>>
>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  66 ++++
>>>>    drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
>>>>    3 files changed, 660 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> index 4bcc405bcf11..9eecbe1f71f9 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> @@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
>>>>    static void vop2_cfg_done(struct vop2_video_port *vp)
>>>>    {
>>>>    	struct vop2 *vop2 = vp->vop2;
>>>> +	u32 val;
>>>> +
>>>> +	val = BIT(vp->id) | (BIT(vp->id) << 16) |
>>>> +		RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
>>>>    
>>>> -	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
>>>> -			BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
>>>> +	regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);
>>> I don't fully understand that code:
>>> (1) the write mask is also present on the rk3568, so should this change
>>>       be a separate patch with a fixes tag?
>> The write mask of VP config done on rk356x is missing, that means
>> you can write the corresponding mask bit, but it has no effect.
>>
>> I once considered making it a separate patch,  I can split it as a separate patch if
>> you like.
> I think I'd like it to be a separate patch please.
>
>
>>> (2) RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN does not contain the part for
>>>       the write-mask
>>>
>>> 	#define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN     BIT(15)
>>>
>>>       why is this working then?
>>
>> Actually this bit has no write-mask bit. 🙂
> when doing that separate patch mentioned above, could you also add a
> comment to the code stating that RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN
> doesn't have a write mask bit please?
>
> Because the TRM is not clear and ideally I'd not forget this fact for
> the future :-) .
>

Okay, will do both above.

>>>>    }
>>>>    
>>>>    static void vop2_win_disable(struct vop2_win *win)
>>> [...]
>>>
>>>> @@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>>>>    			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
>>>>    		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
>>>>    		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
>>>> -		vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
>>>> +		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
>>>> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
>>>> +		else
>>>> +			vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
>>>> +
>>> I think this at least warrants a comment, what is happening here. Also,
>>> can you already see how future vop2-users are behaving - aka are all new
>>> socs in the "else" part of the conditional, or would a switch-case better
>>> represent future socs?
>>
>> On rk356x, this bit is auto gating enable, but this function is not work well so
>> we need to disable this function.
>> On rk3588, and the following new soc(rk3528/rk3576), this bit is gating disable,
>> we should write 1 to disable gating when enable a cluster window.
>>
>>
>> Maybe i add some comments in next version ?
> Yep that comment would be helpful. And with your explanation the code
> itself can stay as it is :-)


will do.

> Thanks
> Heiko
>
>
>>>>    		vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
>>>>    		transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
>>>>    		vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
>>>> @@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>>>>    	drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>>>>    		struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>>>>    
>>>> -		rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
>>>> +		/*
>>>> +		 * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
>>>> +		 * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
>>>> +		 * system cru may be the 1/2 or 1/4 of mode->clock.
>>>> +		 */
>>>> +		clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
>>>>    	}
>>>>    
>>>> +	if (!clock)
>>>> +		return;
>>>> +
>>> hmm, shouldn't the check for the validity of a mode happen before
>>> atomic_enable is run? So this shouldn't error out in the middle of the
>>> function?


Actually it is a check like the check of clk_prepares_enable at the beginning,

maybe one place can do this is at crtc_atomic_check ? But we really don't need to

do the calculate and enable the related interface at every frame commit.

with a grep i can find many platforms do this kind of check in crtc_atomic_enable(ade/meson/vc4/omap/malidp/tidss_crtc_atomic_enable, etc...)

so maybe just let it as it is now?

>>>
>>>
>>>>    	if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
>>>>    	    !(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
>>>>    		out_mode = ROCKCHIP_OUT_MODE_P888;
>>> Thanks
>>> Heiko
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

[PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

/sys/kernel/debug/dri/vop2/summary:  dump vop display state
/sys/kernel/debug/dri/vop2/regs: dump whole vop registers
/sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
activated modules

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

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

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9eecbe1f71f9..4a2342209c15 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -27,6 +27,7 @@
 #include <drm/drm_debugfs.h>
 #include <drm/drm_flip_work.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
@@ -187,6 +188,7 @@ struct vop2 {
 	 */
 	u32 registered_num_wins;
 
+	struct resource *res;
 	void __iomem *regs;
 	struct regmap *map;
 
@@ -228,6 +230,44 @@ struct vop2 {
 #define vop2_output_if_is_lvds(x)	(x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
 #define vop2_output_if_is_dpi(x)	(x == ROCKCHIP_VOP2_EP_RGB0)
 
+struct vop2_regs_dump {
+	const char *name;
+	u32 base;
+	u32 en_reg;
+	u32 en_val;
+	u32 en_mask;
+};
+
+/*
+ * bus-format types.
+ */
+struct drm_bus_format_enum_list {
+	int type;
+	const char *name;
+};
+
+static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
+	{ DRM_MODE_CONNECTOR_Unknown, "Unknown" },
+	{ MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
+	{ MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
+	{ MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
+	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
+	{ MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
+	{ MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
+	{ MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
+	{ MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
+	{ MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
+	{ MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
+	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
+	{ MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
+	{ MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
+	{ MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
+	{ MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
+	{ MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
+};
+static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
+
 static const struct regmap_config vop2_regmap_config;
 
 static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
@@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
 	.atomic_disable = vop2_crtc_atomic_disable,
 };
 
+static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
+{
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+
+	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (crtc->state->connector_mask & drm_connector_mask(connector))
+			seq_printf(s, "    Connector: %s\n", connector->name);
+
+	}
+	drm_connector_list_iter_end(&conn_iter);
+}
+
+static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
+{
+	struct vop2_win *win = to_vop2_win(plane);
+	struct drm_plane_state *pstate = plane->state;
+	struct drm_rect *src, *dst;
+	struct drm_framebuffer *fb;
+	struct drm_gem_object *obj;
+	struct rockchip_gem_object *rk_obj;
+	bool xmirror;
+	bool ymirror;
+	bool rotate_270;
+	bool rotate_90;
+	dma_addr_t fb_addr;
+	int i;
+
+	seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
+	if (!pstate || !pstate->fb)
+		return 0;
+
+	fb = pstate->fb;
+	src = &pstate->src;
+	dst = &pstate->dst;
+	xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
+	ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
+	rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
+	rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
+
+	seq_printf(s, "\twin_id: %d\n", win->win_id);
+
+	seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
+		   &fb->format->format,
+		   drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
+		   pstate->alpha >> 8);
+	seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
+		   xmirror, ymirror, rotate_90, rotate_270);
+	seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
+	seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
+		   src->y1 >> 16, drm_rect_width(src) >> 16,
+		   drm_rect_height(src) >> 16);
+	seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
+		   drm_rect_width(dst), drm_rect_height(dst));
+
+	for (i = 0; i < fb->format->num_planes; i++) {
+		obj = fb->obj[0];
+		rk_obj = to_rockchip_obj(obj);
+		fb_addr = rk_obj->dma_addr + fb->offsets[0];
+
+		seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
+			   i, &fb_addr, fb->pitches[i], fb->offsets[i]);
+	}
+
+	return 0;
+}
+
+static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
+{
+	struct vop2_video_port *vp = to_vop2_video_port(crtc);
+	struct drm_crtc_state *cstate = crtc->state;
+	struct rockchip_crtc_state *vcstate;
+	struct drm_display_mode *mode;
+	struct drm_plane *plane;
+	bool interlaced;
+
+	seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
+		   "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
+
+	if (!cstate || !cstate->active)
+		return 0;
+
+	mode = &crtc->state->adjusted_mode;
+	vcstate = to_rockchip_crtc_state(cstate);
+	interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
+
+	vop2_dump_connector_on_crtc(crtc, s);
+	seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
+		    drm_get_bus_format_name(vcstate->bus_format));
+	seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
+	seq_printf(s, " color_space[%d]\n", vcstate->color_space);
+	seq_printf(s, "    Display mode: %dx%d%s%d\n",
+		    mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
+		    drm_mode_vrefresh(mode));
+	seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
+		    mode->clock, mode->crtc_clock, mode->type, mode->flags);
+	seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
+		    mode->hsync_end, mode->htotal);
+	seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
+		    mode->vsync_end, mode->vtotal);
+
+	drm_atomic_crtc_for_each_plane(plane, crtc) {
+		vop2_plane_state_dump(s, plane);
+	}
+
+	return 0;
+}
+
+static int vop2_summary_show(struct seq_file *s, void *data)
+{
+	struct drm_info_node *node = s->private;
+	struct drm_minor *minor = node->minor;
+	struct drm_device *drm_dev = minor->dev;
+	struct drm_crtc *crtc;
+
+	drm_modeset_lock_all(drm_dev);
+	drm_for_each_crtc(crtc, drm_dev) {
+		vop2_crtc_state_dump(crtc, s);
+	}
+	drm_modeset_unlock_all(drm_dev);
+
+	return 0;
+}
+
+static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
+{
+	resource_size_t start;
+	const int reg_num = 0x110 / 4;
+	u32 val;
+	int i;
+
+	if (dump->en_mask) {
+		val = vop2_readl(vop2, dump->base + dump->en_reg);
+		if ((val & dump->en_mask) != dump->en_val)
+			return;
+	}
+	seq_printf(s, "\n%s:\n", dump->name);
+
+	start = vop2->res->start + dump->base;
+	for (i = 0; i < reg_num;) {
+		seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
+			   vop2_readl(vop2, dump->base + (4 * i)),
+			   vop2_readl(vop2, dump->base + (4 * (i + 1))),
+			   vop2_readl(vop2, dump->base + (4 * (i + 2))),
+			   vop2_readl(vop2, dump->base + (4 * (i + 3))));
+		i += 4;
+	}
+
+}
+
+static int vop2_regs_show(struct seq_file *s, void *arg)
+{
+	struct drm_info_node *node = s->private;
+	struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
+	struct drm_minor *minor = node->minor;
+	struct drm_device *drm_dev = minor->dev;
+
+	struct vop2_regs_dump dump;
+
+	drm_modeset_lock_all(drm_dev);
+
+	if (vop2->enable_count) {
+		dump.en_mask = 0;
+
+		dump.name = "SYS";
+		dump.base = RK3568_REG_CFG_DONE;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "OVL";
+		dump.base = RK3568_OVL_CTRL;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP0";
+		dump.base = 0x0c00;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP1";
+		dump.base = 0x0c00 + 0x100;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP2";
+		dump.base = 0x0c00 + 0x200;
+		vop2_regs_print(vop2, s, &dump);
+		if (vop2->data->soc_id == 3588) {
+			dump.name = "VP3";
+			dump.base = 0x0c00 + 0x300;
+			vop2_regs_print(vop2, s, &dump);
+		}
+		dump.name = "Cluster0";
+		dump.base = 0x1000;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Cluster1";
+		dump.base = 0x1000 + 0x200;
+		vop2_regs_print(vop2, s, &dump);
+		if (vop2->data->soc_id == 3588) {
+			dump.name = "Cluster2";
+			dump.base = 0x1000 + 0x400;
+			vop2_regs_print(vop2, s, &dump);
+
+			dump.name = "Cluster3";
+			dump.base = 0x1000 + 0x600;
+			vop2_regs_print(vop2, s, &dump);
+		}
+		dump.name = "Esmart0";
+		dump.base = 0x1000 + 0x800;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Esmart1";
+		dump.base = 0x1000 + 0xa00;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Esmart2";
+		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+			dump.name = "Smart0";
+		dump.base = 0x1000 + 0xc00;
+		vop2_regs_print(vop2, s,  &dump);
+
+		dump.name = "Esmart3";
+		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+			dump.name = "Smart1";
+		dump.base = 0x1000 + 0xe00;
+		vop2_regs_print(vop2, s,  &dump);
+	} else {
+		seq_printf(s, "VOP disabled:\n");
+	}
+	drm_modeset_unlock_all(drm_dev);
+
+	return 0;
+}
+
+static int vop2_active_regs_show(struct seq_file *s, void *data)
+{
+	struct drm_info_node *node = s->private;
+	struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
+	struct drm_minor *minor = node->minor;
+	struct drm_device *drm_dev = minor->dev;
+
+	struct vop2_regs_dump dump;
+
+	drm_modeset_lock_all(drm_dev);
+	if (vop2->enable_count) {
+		dump.en_mask = 0;
+
+		dump.name = "SYS";
+		dump.base = RK3568_REG_CFG_DONE;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "OVL";
+		dump.base = RK3568_OVL_CTRL;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP0";
+		dump.base = 0x0c00;
+		dump.en_mask = RK3568_VP_DSP_CTRL__STANDBY;
+		dump.en_reg = RK3568_VP_DSP_CTRL;
+		dump.en_val = 0;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP1";
+		dump.base = 0x0c00 + 0x100;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "VP2";
+		dump.base = 0x0c00 + 0x200;
+		vop2_regs_print(vop2, s, &dump);
+		if (vop2->data->soc_id == 3588) {
+			dump.name = "VP3";
+			dump.base = 0x0c00 + 0x300;
+			vop2_regs_print(vop2, s, &dump);
+		}
+
+		dump.en_mask = RK3568_CLUSTER_WIN_CTRL0__WIN0_EN;
+		dump.en_reg = RK3568_CLUSTER_WIN_CTRL0;
+		dump.en_val = 1;
+
+		dump.name = "Cluster0";
+		dump.base = 0x1000;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Cluster1";
+		dump.base = 0x1000 + 0x200;
+		vop2_regs_print(vop2, s, &dump);
+		if (vop2->data->soc_id == 3588) {
+			dump.name = "Cluster2";
+			dump.base = 0x1000 + 0x400;
+			vop2_regs_print(vop2, s, &dump);
+
+			dump.name = "Cluster3";
+			dump.base = 0x1000 + 0x600;
+			vop2_regs_print(vop2, s, &dump);
+		}
+
+		dump.name = "Esmart0";
+		dump.base = 0x1000 + 0x800;
+		dump.en_mask = RK3568_SMART_REGION0_CTRL__WIN0_EN;
+		dump.en_reg = RK3568_SMART_REGION0_CTRL;
+		dump.en_val = 1;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Esmart1";
+		dump.base = 0x1000 + 0xa00;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Esmart2";
+		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+			dump.name = "Smart0";
+		dump.base = 0x1000 + 0xc00;
+		vop2_regs_print(vop2, s, &dump);
+
+		dump.name = "Esmart3";
+		if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+			dump.name = "Smart1";
+		dump.base = 0x1000 + 0xe00;
+		vop2_regs_print(vop2, s, &dump);
+	} else {
+		seq_printf(s, "VOP disabled:\n");
+	}
+	drm_modeset_unlock_all(drm_dev);
+
+	return 0;
+}
+
+static struct drm_info_list vop2_debugfs_list[] = {
+	{ "summary", vop2_summary_show, 0, NULL },
+	{ "active_regs", vop2_active_regs_show,   0, NULL },
+	{ "regs", vop2_regs_show,   0, NULL },
+};
+
+static void vop2_debugfs_init(struct vop2 *vop2, struct drm_minor *minor)
+{
+	struct dentry *root;
+	unsigned int i;
+
+	root = debugfs_create_dir("vop2", minor->debugfs_root);
+	if (!IS_ERR(root)) {
+		for (i = 0; i < ARRAY_SIZE(vop2_debugfs_list); i++)
+			vop2_debugfs_list[i].data = vop2;
+
+		drm_debugfs_create_files(vop2_debugfs_list,
+					 ARRAY_SIZE(vop2_debugfs_list),
+					 root, minor);
+	}
+}
+
+static int vop2_crtc_late_register(struct drm_crtc *crtc)
+{
+	struct vop2_video_port *vp = to_vop2_video_port(crtc);
+	struct vop2 *vop2 = vp->vop2;
+
+	if (drm_crtc_index(crtc) == 0)
+		vop2_debugfs_init(vop2, crtc->dev->primary);
+
+	return 0;
+}
+
 static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
 {
 	struct rockchip_crtc_state *vcstate;
@@ -2536,6 +2933,7 @@ static const struct drm_crtc_funcs vop2_crtc_funcs = {
 	.atomic_destroy_state = vop2_crtc_destroy_state,
 	.enable_vblank = vop2_crtc_enable_vblank,
 	.disable_vblank = vop2_crtc_disable_vblank,
+	.late_register = vop2_crtc_late_register,
 };
 
 static irqreturn_t vop2_isr(int irq, void *data)
@@ -3080,6 +3478,7 @@ static int vop2_bind(struct device *dev, struct device *master, void *data)
 		return -EINVAL;
 	}
 
+	vop2->res = res;
 	vop2->regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(vop2->regs))
 		return PTR_ERR(vop2->regs);
-- 
2.34.1

Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support

[Sascha Hauer]

On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> /sys/kernel/debug/dri/vop2/summary:  dump vop display state
> /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
> /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
> activated modules
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
>  1 file changed, 399 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 9eecbe1f71f9..4a2342209c15 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -27,6 +27,7 @@
>  #include <drm/drm_debugfs.h>
>  #include <drm/drm_flip_work.h>
>  #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -187,6 +188,7 @@ struct vop2 {
>  	 */
>  	u32 registered_num_wins;
>  
> +	struct resource *res;
>  	void __iomem *regs;
>  	struct regmap *map;
>  
> @@ -228,6 +230,44 @@ struct vop2 {
>  #define vop2_output_if_is_lvds(x)	(x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
>  #define vop2_output_if_is_dpi(x)	(x == ROCKCHIP_VOP2_EP_RGB0)
>  
> +struct vop2_regs_dump {
> +	const char *name;
> +	u32 base;
> +	u32 en_reg;
> +	u32 en_val;
> +	u32 en_mask;
> +};
> +
> +/*
> + * bus-format types.
> + */
> +struct drm_bus_format_enum_list {
> +	int type;
> +	const char *name;
> +};
> +
> +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
> +	{ DRM_MODE_CONNECTOR_Unknown, "Unknown" },
> +	{ MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
> +	{ MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
> +	{ MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
> +	{ MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
> +	{ MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
> +	{ MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
> +	{ MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
> +	{ MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
> +	{ MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
> +	{ MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
> +	{ MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
> +	{ MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
> +	{ MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
> +	{ MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
> +	{ MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
> +	{ MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
> +};
> +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
> +
>  static const struct regmap_config vop2_regmap_config;
>  
>  static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
> @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
>  	.atomic_disable = vop2_crtc_atomic_disable,
>  };
>  
> +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
> +{
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +
> +	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (crtc->state->connector_mask & drm_connector_mask(connector))
> +			seq_printf(s, "    Connector: %s\n", connector->name);
> +
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +}
> +
> +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
> +{
> +	struct vop2_win *win = to_vop2_win(plane);
> +	struct drm_plane_state *pstate = plane->state;
> +	struct drm_rect *src, *dst;
> +	struct drm_framebuffer *fb;
> +	struct drm_gem_object *obj;
> +	struct rockchip_gem_object *rk_obj;
> +	bool xmirror;
> +	bool ymirror;
> +	bool rotate_270;
> +	bool rotate_90;
> +	dma_addr_t fb_addr;
> +	int i;
> +
> +	seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
> +	if (!pstate || !pstate->fb)
> +		return 0;
> +
> +	fb = pstate->fb;
> +	src = &pstate->src;
> +	dst = &pstate->dst;
> +	xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
> +	ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
> +	rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
> +	rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
> +
> +	seq_printf(s, "\twin_id: %d\n", win->win_id);
> +
> +	seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
> +		   &fb->format->format,
> +		   drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
> +		   pstate->alpha >> 8);
> +	seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
> +		   xmirror, ymirror, rotate_90, rotate_270);
> +	seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
> +	seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
> +		   src->y1 >> 16, drm_rect_width(src) >> 16,
> +		   drm_rect_height(src) >> 16);
> +	seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
> +		   drm_rect_width(dst), drm_rect_height(dst));
> +
> +	for (i = 0; i < fb->format->num_planes; i++) {
> +		obj = fb->obj[0];
> +		rk_obj = to_rockchip_obj(obj);
> +		fb_addr = rk_obj->dma_addr + fb->offsets[0];
> +
> +		seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
> +			   i, &fb_addr, fb->pitches[i], fb->offsets[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
> +{
> +	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	struct drm_crtc_state *cstate = crtc->state;
> +	struct rockchip_crtc_state *vcstate;
> +	struct drm_display_mode *mode;
> +	struct drm_plane *plane;
> +	bool interlaced;
> +
> +	seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
> +		   "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
> +
> +	if (!cstate || !cstate->active)
> +		return 0;
> +
> +	mode = &crtc->state->adjusted_mode;
> +	vcstate = to_rockchip_crtc_state(cstate);
> +	interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> +
> +	vop2_dump_connector_on_crtc(crtc, s);
> +	seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
> +		    drm_get_bus_format_name(vcstate->bus_format));
> +	seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
> +	seq_printf(s, " color_space[%d]\n", vcstate->color_space);
> +	seq_printf(s, "    Display mode: %dx%d%s%d\n",
> +		    mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
> +		    drm_mode_vrefresh(mode));
> +	seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
> +		    mode->clock, mode->crtc_clock, mode->type, mode->flags);
> +	seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
> +		    mode->hsync_end, mode->htotal);
> +	seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
> +		    mode->vsync_end, mode->vtotal);
> +
> +	drm_atomic_crtc_for_each_plane(plane, crtc) {
> +		vop2_plane_state_dump(s, plane);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vop2_summary_show(struct seq_file *s, void *data)
> +{
> +	struct drm_info_node *node = s->private;
> +	struct drm_minor *minor = node->minor;
> +	struct drm_device *drm_dev = minor->dev;
> +	struct drm_crtc *crtc;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	drm_for_each_crtc(crtc, drm_dev) {
> +		vop2_crtc_state_dump(crtc, s);
> +	}
> +	drm_modeset_unlock_all(drm_dev);
> +
> +	return 0;
> +}
> +
> +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
> +{
> +	resource_size_t start;
> +	const int reg_num = 0x110 / 4;

If I'm not mistaken this prints a register space of 0x110 bytes.
Shouldn't it be 0x100 bytes instead?

Also, are all these register spaces really have the same size? Does it
make sense to add the size to struct vop2_regs_dump?

> +	u32 val;
> +	int i;
> +
> +	if (dump->en_mask) {
> +		val = vop2_readl(vop2, dump->base + dump->en_reg);
> +		if ((val & dump->en_mask) != dump->en_val)
> +			return;
> +	}
> +	seq_printf(s, "\n%s:\n", dump->name);
> +
> +	start = vop2->res->start + dump->base;
> +	for (i = 0; i < reg_num;) {
> +		seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
> +			   vop2_readl(vop2, dump->base + (4 * i)),
> +			   vop2_readl(vop2, dump->base + (4 * (i + 1))),
> +			   vop2_readl(vop2, dump->base + (4 * (i + 2))),
> +			   vop2_readl(vop2, dump->base + (4 * (i + 3))));
> +		i += 4;
> +	}
> +
> +}
> +
> +static int vop2_regs_show(struct seq_file *s, void *arg)
> +{
> +	struct drm_info_node *node = s->private;
> +	struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
> +	struct drm_minor *minor = node->minor;
> +	struct drm_device *drm_dev = minor->dev;
> +
> +	struct vop2_regs_dump dump;
> +
> +	drm_modeset_lock_all(drm_dev);
> +
> +	if (vop2->enable_count) {
> +		dump.en_mask = 0;
> +
> +		dump.name = "SYS";
> +		dump.base = RK3568_REG_CFG_DONE;
> +		vop2_regs_print(vop2, s, &dump);

Can you create a statically initialized array of struct vop2_regs_dump
and iterate over it?
You would need an additional present_in_soc_xy flag in struct
vop2_regs_dump, but other than that I don't see a problem and the result
might look better.

For the windows it might also be an option to iterate over
vop2->data->win instead. This array already contains the register base
addresses and window names.

> +static int vop2_active_regs_show(struct seq_file *s, void *data)
> +{
> +	struct drm_info_node *node = s->private;
> +	struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
> +	struct drm_minor *minor = node->minor;
> +	struct drm_device *drm_dev = minor->dev;
> +
> +	struct vop2_regs_dump dump;
> +
> +	drm_modeset_lock_all(drm_dev);
> +	if (vop2->enable_count) {
> +		dump.en_mask = 0;
> +
> +		dump.name = "SYS";
> +		dump.base = RK3568_REG_CFG_DONE;
> +		vop2_regs_print(vop2, s, &dump);

Not sure if we really need an additional debugfs entry to print only the
active entities, but if we do then we could avoid a bit of code
duplication by adding creating a common register dump function called
from vop2_regs_show() and vop2_active_regs_show() which takes an
additional ignore_disabled argument.

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 v2 11/12] drm/rockchip: vop2: Add debugfs support

[Sascha Hauer]

On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
>    Hi Sascha:
> 
>    thanks for you review.
> 
>    On 11/27/23 18:13, Sascha Hauer wrote:
> 
>  On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
> 
>  From: Andy Yan [1]<andy.yan@rock-chips.com>
> 
>  /sys/kernel/debug/dri/vop2/summary:  dump vop display state
>  /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
>  /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
>  activated modules
> 
>  Signed-off-by: Andy Yan [2]<andy.yan@rock-chips.com>
>  ---
> 
>  (no changes since v1)
> 
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
>   1 file changed, 399 insertions(+)
> 
>  diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>  index 9eecbe1f71f9..4a2342209c15 100644
>  --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>  +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>  @@ -27,6 +27,7 @@
>   #include <drm/drm_debugfs.h>
>   #include <drm/drm_flip_work.h>
>   #include <drm/drm_framebuffer.h>
>  +#include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_probe_helper.h>
>   #include <drm/drm_vblank.h>
> 
>  @@ -187,6 +188,7 @@ struct vop2 {
>           */
>          u32 registered_num_wins;
> 
>  +       struct resource *res;
>          void __iomem *regs;
>          struct regmap *map;
> 
>  @@ -228,6 +230,44 @@ struct vop2 {
>   #define vop2_output_if_is_lvds(x)      (x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
>   #define vop2_output_if_is_dpi(x)       (x == ROCKCHIP_VOP2_EP_RGB0)
> 
>  +struct vop2_regs_dump {
>  +       const char *name;
>  +       u32 base;
>  +       u32 en_reg;
>  +       u32 en_val;
>  +       u32 en_mask;
>  +};
>  +
>  +/*
>  + * bus-format types.
>  + */
>  +struct drm_bus_format_enum_list {
>  +       int type;
>  +       const char *name;
>  +};
>  +
>  +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
>  +       { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
>  +       { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
>  +       { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
>  +       { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
>  +       { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
>  +       { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
>  +       { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
>  +       { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
>  +       { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
>  +       { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
>  +       { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
>  +       { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
>  +       { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
>  +       { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
>  +       { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
>  +       { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
>  +       { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
>  +       { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
>  +};
>  +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
>  +
>   static const struct regmap_config vop2_regmap_config;
> 
>   static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
>  @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
>          .atomic_disable = vop2_crtc_atomic_disable,
>   };
> 
>  +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
>  +{
>  +       struct drm_connector_list_iter conn_iter;
>  +       struct drm_connector *connector;
>  +
>  +       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
>  +       drm_for_each_connector_iter(connector, &conn_iter) {
>  +               if (crtc->state->connector_mask & drm_connector_mask(connector))
>  +                       seq_printf(s, "    Connector: %s\n", connector->name);
>  +
>  +       }
>  +       drm_connector_list_iter_end(&conn_iter);
>  +}
>  +
>  +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
>  +{
>  +       struct vop2_win *win = to_vop2_win(plane);
>  +       struct drm_plane_state *pstate = plane->state;
>  +       struct drm_rect *src, *dst;
>  +       struct drm_framebuffer *fb;
>  +       struct drm_gem_object *obj;
>  +       struct rockchip_gem_object *rk_obj;
>  +       bool xmirror;
>  +       bool ymirror;
>  +       bool rotate_270;
>  +       bool rotate_90;
>  +       dma_addr_t fb_addr;
>  +       int i;
>  +
>  +       seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
>  +       if (!pstate || !pstate->fb)
>  +               return 0;
>  +
>  +       fb = pstate->fb;
>  +       src = &pstate->src;
>  +       dst = &pstate->dst;
>  +       xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
>  +       ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
>  +       rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
>  +       rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
>  +
>  +       seq_printf(s, "\twin_id: %d\n", win->win_id);
>  +
>  +       seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
>  +                  &fb->format->format,
>  +                  drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
>  +                  pstate->alpha >> 8);
>  +       seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
>  +                  xmirror, ymirror, rotate_90, rotate_270);
>  +       seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
>  +       seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
>  +                  src->y1 >> 16, drm_rect_width(src) >> 16,
>  +                  drm_rect_height(src) >> 16);
>  +       seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
>  +                  drm_rect_width(dst), drm_rect_height(dst));
>  +
>  +       for (i = 0; i < fb->format->num_planes; i++) {
>  +               obj = fb->obj[0];
>  +               rk_obj = to_rockchip_obj(obj);
>  +               fb_addr = rk_obj->dma_addr + fb->offsets[0];
>  +
>  +               seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
>  +                          i, &fb_addr, fb->pitches[i], fb->offsets[i]);
>  +       }
>  +
>  +       return 0;
>  +}
>  +
>  +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
>  +{
>  +       struct vop2_video_port *vp = to_vop2_video_port(crtc);
>  +       struct drm_crtc_state *cstate = crtc->state;
>  +       struct rockchip_crtc_state *vcstate;
>  +       struct drm_display_mode *mode;
>  +       struct drm_plane *plane;
>  +       bool interlaced;
>  +
>  +       seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
>  +                  "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
>  +
>  +       if (!cstate || !cstate->active)
>  +               return 0;
>  +
>  +       mode = &crtc->state->adjusted_mode;
>  +       vcstate = to_rockchip_crtc_state(cstate);
>  +       interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>  +
>  +       vop2_dump_connector_on_crtc(crtc, s);
>  +       seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
>  +                   drm_get_bus_format_name(vcstate->bus_format));
>  +       seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
>  +       seq_printf(s, " color_space[%d]\n", vcstate->color_space);
>  +       seq_printf(s, "    Display mode: %dx%d%s%d\n",
>  +                   mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
>  +                   drm_mode_vrefresh(mode));
>  +       seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
>  +                   mode->clock, mode->crtc_clock, mode->type, mode->flags);
>  +       seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
>  +                   mode->hsync_end, mode->htotal);
>  +       seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
>  +                   mode->vsync_end, mode->vtotal);
>  +
>  +       drm_atomic_crtc_for_each_plane(plane, crtc) {
>  +               vop2_plane_state_dump(s, plane);
>  +       }
>  +
>  +       return 0;
>  +}
>  +
>  +static int vop2_summary_show(struct seq_file *s, void *data)
>  +{
>  +       struct drm_info_node *node = s->private;
>  +       struct drm_minor *minor = node->minor;
>  +       struct drm_device *drm_dev = minor->dev;
>  +       struct drm_crtc *crtc;
>  +
>  +       drm_modeset_lock_all(drm_dev);
>  +       drm_for_each_crtc(crtc, drm_dev) {
>  +               vop2_crtc_state_dump(crtc, s);
>  +       }
>  +       drm_modeset_unlock_all(drm_dev);
>  +
>  +       return 0;
>  +}
>  +
>  +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
>  +{
>  +       resource_size_t start;
>  +       const int reg_num = 0x110 / 4;
> 
>  If I'm not mistaken this prints a register space of 0x110 bytes.
>  Shouldn't it be 0x100 bytes instead?
> 
>  Also, are all these register spaces really have the same size? Does it
>  make sense to add the size to struct vop2_regs_dump?
> 
>    In fact, most used registers of the most blocks are not more than 100, but
>    for Cluster windows,
> 
>    there is a CLUSTER_CTRL register sting at 0x100.
> 
>    I think i should add the size to struct vop2_regs_dump.
> 
> 
>  +       u32 val;
>  +       int i;
>  +
>  +       if (dump->en_mask) {
>  +               val = vop2_readl(vop2, dump->base + dump->en_reg);
>  +               if ((val & dump->en_mask) != dump->en_val)
>  +                       return;
>  +       }
>  +       seq_printf(s, "\n%s:\n", dump->name);
>  +
>  +       start = vop2->res->start + dump->base;
>  +       for (i = 0; i < reg_num;) {
>  +               seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
>  +                          vop2_readl(vop2, dump->base + (4 * i)),
>  +                          vop2_readl(vop2, dump->base + (4 * (i + 1))),
>  +                          vop2_readl(vop2, dump->base + (4 * (i + 2))),
>  +                          vop2_readl(vop2, dump->base + (4 * (i + 3))));
>  +               i += 4;
>  +       }
>  +
>  +}
>  +
>  +static int vop2_regs_show(struct seq_file *s, void *arg)
>  +{
>  +       struct drm_info_node *node = s->private;
>  +       struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
>  +       struct drm_minor *minor = node->minor;
>  +       struct drm_device *drm_dev = minor->dev;
>  +
>  +       struct vop2_regs_dump dump;
>  +
>  +       drm_modeset_lock_all(drm_dev);
>  +
>  +       if (vop2->enable_count) {
>  +               dump.en_mask = 0;
>  +
>  +               dump.name = "SYS";
>  +               dump.base = RK3568_REG_CFG_DONE;
>  +               vop2_regs_print(vop2, s, &dump);
> 
>  Can you create a statically initialized array of struct vop2_regs_dump
>  and iterate over it?
>  You would need an additional present_in_soc_xy flag in struct
>  vop2_regs_dump, but other than that I don't see a problem and the result
>  might look better.
> 
>  For the windows it might also be an option to iterate over
>  vop2->data->win instead. This array already contains the register base
>  addresses and window names.
> 
>    In fact, we have a dump_regs  arrar in vop2_data per soc in our bsp
>    kernel[0],
> 
>    do you like something like that?
> 
>    [0]
>    [3]https://github.com/armbian/linux-rockchip/blob/rk-5.10-rkr6/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c#L3684

This looks good from a first glance. I would suggest using C99
initializers though.

>  Not sure if we really need an additional debugfs entry to print only the
>  active entities, but if we do then we could avoid a bit of code
>  duplication by adding creating a common register dump function called
>  from vop2_regs_show() and vop2_active_regs_show() which takes an
>  additional ignore_disabled argument.
> 
>    As the whole vop2 registers block is very large, so some times only dump
> 
>    active modules make we dig bugs easier.
> 
>    It seems that if we  "initialized array of struct vop2_regs_dump" as you
>    said befor, we can avoid
> 
>    some duplication code here?

Yes.

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 v2 11/12] drm/rockchip: vop2: Add debugfs support

[Andy Yan]

Hi Sascha:



On 11/29/23 16:52, Sascha Hauer wrote:
> On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
>>     Hi Sascha:
>>
>>     thanks for you review.
>>
>>     On 11/27/23 18:13, Sascha Hauer wrote:
>>
>>   On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
>>
>>   From: Andy Yan [1]<andy.yan@rock-chips.com>
>>
>>   /sys/kernel/debug/dri/vop2/summary:  dump vop display state
>>   /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
>>   /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
>>   activated modules
>>
>>   Signed-off-by: Andy Yan [2]<andy.yan@rock-chips.com>
>>   ---
>>
>>   (no changes since v1)
>>
>>    drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
>>    1 file changed, 399 insertions(+)
>>
>>   diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>   index 9eecbe1f71f9..4a2342209c15 100644
>>   --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>   +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>   @@ -27,6 +27,7 @@
>>    #include <drm/drm_debugfs.h>
>>    #include <drm/drm_flip_work.h>
>>    #include <drm/drm_framebuffer.h>
>>   +#include <drm/drm_gem_framebuffer_helper.h>
>>    #include <drm/drm_probe_helper.h>
>>    #include <drm/drm_vblank.h>
>>
>>   @@ -187,6 +188,7 @@ struct vop2 {
>>            */
>>           u32 registered_num_wins;
>>
>>   +       struct resource *res;
>>           void __iomem *regs;
>>           struct regmap *map;
>>
>>   @@ -228,6 +230,44 @@ struct vop2 {
>>    #define vop2_output_if_is_lvds(x)      (x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
>>    #define vop2_output_if_is_dpi(x)       (x == ROCKCHIP_VOP2_EP_RGB0)
>>
>>   +struct vop2_regs_dump {
>>   +       const char *name;
>>   +       u32 base;
>>   +       u32 en_reg;
>>   +       u32 en_val;
>>   +       u32 en_mask;
>>   +};
>>   +
>>   +/*
>>   + * bus-format types.
>>   + */
>>   +struct drm_bus_format_enum_list {
>>   +       int type;
>>   +       const char *name;
>>   +};
>>   +
>>   +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
>>   +       { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
>>   +       { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
>>   +       { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
>>   +       { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
>>   +       { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
>>   +       { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
>>   +       { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
>>   +       { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
>>   +       { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
>>   +       { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
>>   +       { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
>>   +       { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
>>   +       { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
>>   +       { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
>>   +       { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
>>   +       { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
>>   +       { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
>>   +       { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
>>   +};
>>   +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
>>   +
>>    static const struct regmap_config vop2_regmap_config;
>>
>>    static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
>>   @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
>>           .atomic_disable = vop2_crtc_atomic_disable,
>>    };
>>
>>   +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
>>   +{
>>   +       struct drm_connector_list_iter conn_iter;
>>   +       struct drm_connector *connector;
>>   +
>>   +       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
>>   +       drm_for_each_connector_iter(connector, &conn_iter) {
>>   +               if (crtc->state->connector_mask & drm_connector_mask(connector))
>>   +                       seq_printf(s, "    Connector: %s\n", connector->name);
>>   +
>>   +       }
>>   +       drm_connector_list_iter_end(&conn_iter);
>>   +}
>>   +
>>   +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
>>   +{
>>   +       struct vop2_win *win = to_vop2_win(plane);
>>   +       struct drm_plane_state *pstate = plane->state;
>>   +       struct drm_rect *src, *dst;
>>   +       struct drm_framebuffer *fb;
>>   +       struct drm_gem_object *obj;
>>   +       struct rockchip_gem_object *rk_obj;
>>   +       bool xmirror;
>>   +       bool ymirror;
>>   +       bool rotate_270;
>>   +       bool rotate_90;
>>   +       dma_addr_t fb_addr;
>>   +       int i;
>>   +
>>   +       seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
>>   +       if (!pstate || !pstate->fb)
>>   +               return 0;
>>   +
>>   +       fb = pstate->fb;
>>   +       src = &pstate->src;
>>   +       dst = &pstate->dst;
>>   +       xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
>>   +       ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
>>   +       rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
>>   +       rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
>>   +
>>   +       seq_printf(s, "\twin_id: %d\n", win->win_id);
>>   +
>>   +       seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
>>   +                  &fb->format->format,
>>   +                  drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
>>   +                  pstate->alpha >> 8);
>>   +       seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
>>   +                  xmirror, ymirror, rotate_90, rotate_270);
>>   +       seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
>>   +       seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
>>   +                  src->y1 >> 16, drm_rect_width(src) >> 16,
>>   +                  drm_rect_height(src) >> 16);
>>   +       seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
>>   +                  drm_rect_width(dst), drm_rect_height(dst));
>>   +
>>   +       for (i = 0; i < fb->format->num_planes; i++) {
>>   +               obj = fb->obj[0];
>>   +               rk_obj = to_rockchip_obj(obj);
>>   +               fb_addr = rk_obj->dma_addr + fb->offsets[0];
>>   +
>>   +               seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
>>   +                          i, &fb_addr, fb->pitches[i], fb->offsets[i]);
>>   +       }
>>   +
>>   +       return 0;
>>   +}
>>   +
>>   +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
>>   +{
>>   +       struct vop2_video_port *vp = to_vop2_video_port(crtc);
>>   +       struct drm_crtc_state *cstate = crtc->state;
>>   +       struct rockchip_crtc_state *vcstate;
>>   +       struct drm_display_mode *mode;
>>   +       struct drm_plane *plane;
>>   +       bool interlaced;
>>   +
>>   +       seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
>>   +                  "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
>>   +
>>   +       if (!cstate || !cstate->active)
>>   +               return 0;
>>   +
>>   +       mode = &crtc->state->adjusted_mode;
>>   +       vcstate = to_rockchip_crtc_state(cstate);
>>   +       interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>>   +
>>   +       vop2_dump_connector_on_crtc(crtc, s);
>>   +       seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
>>   +                   drm_get_bus_format_name(vcstate->bus_format));
>>   +       seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
>>   +       seq_printf(s, " color_space[%d]\n", vcstate->color_space);
>>   +       seq_printf(s, "    Display mode: %dx%d%s%d\n",
>>   +                   mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
>>   +                   drm_mode_vrefresh(mode));
>>   +       seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
>>   +                   mode->clock, mode->crtc_clock, mode->type, mode->flags);
>>   +       seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
>>   +                   mode->hsync_end, mode->htotal);
>>   +       seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
>>   +                   mode->vsync_end, mode->vtotal);
>>   +
>>   +       drm_atomic_crtc_for_each_plane(plane, crtc) {
>>   +               vop2_plane_state_dump(s, plane);
>>   +       }
>>   +
>>   +       return 0;
>>   +}
>>   +
>>   +static int vop2_summary_show(struct seq_file *s, void *data)
>>   +{
>>   +       struct drm_info_node *node = s->private;
>>   +       struct drm_minor *minor = node->minor;
>>   +       struct drm_device *drm_dev = minor->dev;
>>   +       struct drm_crtc *crtc;
>>   +
>>   +       drm_modeset_lock_all(drm_dev);
>>   +       drm_for_each_crtc(crtc, drm_dev) {
>>   +               vop2_crtc_state_dump(crtc, s);
>>   +       }
>>   +       drm_modeset_unlock_all(drm_dev);
>>   +
>>   +       return 0;
>>   +}
>>   +
>>   +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
>>   +{
>>   +       resource_size_t start;
>>   +       const int reg_num = 0x110 / 4;
>>
>>   If I'm not mistaken this prints a register space of 0x110 bytes.
>>   Shouldn't it be 0x100 bytes instead?
>>
>>   Also, are all these register spaces really have the same size? Does it
>>   make sense to add the size to struct vop2_regs_dump?
>>
>>     In fact, most used registers of the most blocks are not more than 100, but
>>     for Cluster windows,
>>
>>     there is a CLUSTER_CTRL register sting at 0x100.
>>
>>     I think i should add the size to struct vop2_regs_dump.
>>
>>
>>   +       u32 val;
>>   +       int i;
>>   +
>>   +       if (dump->en_mask) {
>>   +               val = vop2_readl(vop2, dump->base + dump->en_reg);
>>   +               if ((val & dump->en_mask) != dump->en_val)
>>   +                       return;
>>   +       }
>>   +       seq_printf(s, "\n%s:\n", dump->name);
>>   +
>>   +       start = vop2->res->start + dump->base;
>>   +       for (i = 0; i < reg_num;) {
>>   +               seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
>>   +                          vop2_readl(vop2, dump->base + (4 * i)),
>>   +                          vop2_readl(vop2, dump->base + (4 * (i + 1))),
>>   +                          vop2_readl(vop2, dump->base + (4 * (i + 2))),
>>   +                          vop2_readl(vop2, dump->base + (4 * (i + 3))));
>>   +               i += 4;
>>   +       }
>>   +
>>   +}
>>   +
>>   +static int vop2_regs_show(struct seq_file *s, void *arg)
>>   +{
>>   +       struct drm_info_node *node = s->private;
>>   +       struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
>>   +       struct drm_minor *minor = node->minor;
>>   +       struct drm_device *drm_dev = minor->dev;
>>   +
>>   +       struct vop2_regs_dump dump;
>>   +
>>   +       drm_modeset_lock_all(drm_dev);
>>   +
>>   +       if (vop2->enable_count) {
>>   +               dump.en_mask = 0;
>>   +
>>   +               dump.name = "SYS";
>>   +               dump.base = RK3568_REG_CFG_DONE;
>>   +               vop2_regs_print(vop2, s, &dump);
>>
>>   Can you create a statically initialized array of struct vop2_regs_dump
>>   and iterate over it?
>>   You would need an additional present_in_soc_xy flag in struct
>>   vop2_regs_dump, but other than that I don't see a problem and the result
>>   might look better.
>>
>>   For the windows it might also be an option to iterate over
>>   vop2->data->win instead. This array already contains the register base
>>   addresses and window names.
>>
>>     In fact, we have a dump_regs  arrar in vop2_data per soc in our bsp
>>     kernel[0],
>>
>>     do you like something like that?
>>
>>     [0]
>>     [3]https://github.com/armbian/linux-rockchip/blob/rk-5.10-rkr6/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c#L3684
> 
> This looks good from a first glance. I would suggest using C99
> initializers though.


Thanks for your reply, but am not quiet claer about the C99 initializers, would you plase make it more specific,
or give some example ?


> 
>>   Not sure if we really need an additional debugfs entry to print only the
>>   active entities, but if we do then we could avoid a bit of code
>>   duplication by adding creating a common register dump function called
>>   from vop2_regs_show() and vop2_active_regs_show() which takes an
>>   additional ignore_disabled argument.
>>
>>     As the whole vop2 registers block is very large, so some times only dump
>>
>>     active modules make we dig bugs easier.
>>
>>     It seems that if we  "initialized array of struct vop2_regs_dump" as you
>>     said befor, we can avoid
>>
>>     some duplication code here?
> 
> Yes.
> 
> Sascha
> 

Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support

[Sascha Hauer]

On Wed, Nov 29, 2023 at 07:01:37PM +0800, Andy Yan wrote:
> Hi Sascha:
> 
> 
> 
> On 11/29/23 16:52, Sascha Hauer wrote:
> > On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
> > >     Hi Sascha:
> > > 
> > >     thanks for you review.
> > > 
> > >     On 11/27/23 18:13, Sascha Hauer wrote:
> > > 
> > >   On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
> > > 
> > >   From: Andy Yan [1]<andy.yan@rock-chips.com>
> > > 
> > >   /sys/kernel/debug/dri/vop2/summary:  dump vop display state
> > >   /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
> > >   /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
> > >   activated modules
> > > 
> > >   Signed-off-by: Andy Yan [2]<andy.yan@rock-chips.com>
> > >   ---
> > > 
> > >   (no changes since v1)
> > > 
> > >    drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
> > >    1 file changed, 399 insertions(+)
> > > 
> > >   diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > >   index 9eecbe1f71f9..4a2342209c15 100644
> > >   --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > >   +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > >   @@ -27,6 +27,7 @@
> > >    #include <drm/drm_debugfs.h>
> > >    #include <drm/drm_flip_work.h>
> > >    #include <drm/drm_framebuffer.h>
> > >   +#include <drm/drm_gem_framebuffer_helper.h>
> > >    #include <drm/drm_probe_helper.h>
> > >    #include <drm/drm_vblank.h>
> > > 
> > >   @@ -187,6 +188,7 @@ struct vop2 {
> > >            */
> > >           u32 registered_num_wins;
> > > 
> > >   +       struct resource *res;
> > >           void __iomem *regs;
> > >           struct regmap *map;
> > > 
> > >   @@ -228,6 +230,44 @@ struct vop2 {
> > >    #define vop2_output_if_is_lvds(x)      (x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
> > >    #define vop2_output_if_is_dpi(x)       (x == ROCKCHIP_VOP2_EP_RGB0)
> > > 
> > >   +struct vop2_regs_dump {
> > >   +       const char *name;
> > >   +       u32 base;
> > >   +       u32 en_reg;
> > >   +       u32 en_val;
> > >   +       u32 en_mask;
> > >   +};
> > >   +
> > >   +/*
> > >   + * bus-format types.
> > >   + */
> > >   +struct drm_bus_format_enum_list {
> > >   +       int type;
> > >   +       const char *name;
> > >   +};
> > >   +
> > >   +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
> > >   +       { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
> > >   +       { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
> > >   +       { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
> > >   +       { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
> > >   +       { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
> > >   +       { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
> > >   +       { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
> > >   +       { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
> > >   +       { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
> > >   +       { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
> > >   +       { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
> > >   +       { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
> > >   +       { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
> > >   +       { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
> > >   +       { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
> > >   +       { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
> > >   +       { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
> > >   +       { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
> > >   +};
> > >   +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
> > >   +
> > >    static const struct regmap_config vop2_regmap_config;
> > > 
> > >    static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
> > >   @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
> > >           .atomic_disable = vop2_crtc_atomic_disable,
> > >    };
> > > 
> > >   +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
> > >   +{
> > >   +       struct drm_connector_list_iter conn_iter;
> > >   +       struct drm_connector *connector;
> > >   +
> > >   +       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> > >   +       drm_for_each_connector_iter(connector, &conn_iter) {
> > >   +               if (crtc->state->connector_mask & drm_connector_mask(connector))
> > >   +                       seq_printf(s, "    Connector: %s\n", connector->name);
> > >   +
> > >   +       }
> > >   +       drm_connector_list_iter_end(&conn_iter);
> > >   +}
> > >   +
> > >   +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
> > >   +{
> > >   +       struct vop2_win *win = to_vop2_win(plane);
> > >   +       struct drm_plane_state *pstate = plane->state;
> > >   +       struct drm_rect *src, *dst;
> > >   +       struct drm_framebuffer *fb;
> > >   +       struct drm_gem_object *obj;
> > >   +       struct rockchip_gem_object *rk_obj;
> > >   +       bool xmirror;
> > >   +       bool ymirror;
> > >   +       bool rotate_270;
> > >   +       bool rotate_90;
> > >   +       dma_addr_t fb_addr;
> > >   +       int i;
> > >   +
> > >   +       seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
> > >   +       if (!pstate || !pstate->fb)
> > >   +               return 0;
> > >   +
> > >   +       fb = pstate->fb;
> > >   +       src = &pstate->src;
> > >   +       dst = &pstate->dst;
> > >   +       xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
> > >   +       ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
> > >   +       rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
> > >   +       rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
> > >   +
> > >   +       seq_printf(s, "\twin_id: %d\n", win->win_id);
> > >   +
> > >   +       seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
> > >   +                  &fb->format->format,
> > >   +                  drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
> > >   +                  pstate->alpha >> 8);
> > >   +       seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
> > >   +                  xmirror, ymirror, rotate_90, rotate_270);
> > >   +       seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
> > >   +       seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
> > >   +                  src->y1 >> 16, drm_rect_width(src) >> 16,
> > >   +                  drm_rect_height(src) >> 16);
> > >   +       seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
> > >   +                  drm_rect_width(dst), drm_rect_height(dst));
> > >   +
> > >   +       for (i = 0; i < fb->format->num_planes; i++) {
> > >   +               obj = fb->obj[0];
> > >   +               rk_obj = to_rockchip_obj(obj);
> > >   +               fb_addr = rk_obj->dma_addr + fb->offsets[0];
> > >   +
> > >   +               seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
> > >   +                          i, &fb_addr, fb->pitches[i], fb->offsets[i]);
> > >   +       }
> > >   +
> > >   +       return 0;
> > >   +}
> > >   +
> > >   +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
> > >   +{
> > >   +       struct vop2_video_port *vp = to_vop2_video_port(crtc);
> > >   +       struct drm_crtc_state *cstate = crtc->state;
> > >   +       struct rockchip_crtc_state *vcstate;
> > >   +       struct drm_display_mode *mode;
> > >   +       struct drm_plane *plane;
> > >   +       bool interlaced;
> > >   +
> > >   +       seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
> > >   +                  "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
> > >   +
> > >   +       if (!cstate || !cstate->active)
> > >   +               return 0;
> > >   +
> > >   +       mode = &crtc->state->adjusted_mode;
> > >   +       vcstate = to_rockchip_crtc_state(cstate);
> > >   +       interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> > >   +
> > >   +       vop2_dump_connector_on_crtc(crtc, s);
> > >   +       seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
> > >   +                   drm_get_bus_format_name(vcstate->bus_format));
> > >   +       seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
> > >   +       seq_printf(s, " color_space[%d]\n", vcstate->color_space);
> > >   +       seq_printf(s, "    Display mode: %dx%d%s%d\n",
> > >   +                   mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
> > >   +                   drm_mode_vrefresh(mode));
> > >   +       seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
> > >   +                   mode->clock, mode->crtc_clock, mode->type, mode->flags);
> > >   +       seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
> > >   +                   mode->hsync_end, mode->htotal);
> > >   +       seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
> > >   +                   mode->vsync_end, mode->vtotal);
> > >   +
> > >   +       drm_atomic_crtc_for_each_plane(plane, crtc) {
> > >   +               vop2_plane_state_dump(s, plane);
> > >   +       }
> > >   +
> > >   +       return 0;
> > >   +}
> > >   +
> > >   +static int vop2_summary_show(struct seq_file *s, void *data)
> > >   +{
> > >   +       struct drm_info_node *node = s->private;
> > >   +       struct drm_minor *minor = node->minor;
> > >   +       struct drm_device *drm_dev = minor->dev;
> > >   +       struct drm_crtc *crtc;
> > >   +
> > >   +       drm_modeset_lock_all(drm_dev);
> > >   +       drm_for_each_crtc(crtc, drm_dev) {
> > >   +               vop2_crtc_state_dump(crtc, s);
> > >   +       }
> > >   +       drm_modeset_unlock_all(drm_dev);
> > >   +
> > >   +       return 0;
> > >   +}
> > >   +
> > >   +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
> > >   +{
> > >   +       resource_size_t start;
> > >   +       const int reg_num = 0x110 / 4;
> > > 
> > >   If I'm not mistaken this prints a register space of 0x110 bytes.
> > >   Shouldn't it be 0x100 bytes instead?
> > > 
> > >   Also, are all these register spaces really have the same size? Does it
> > >   make sense to add the size to struct vop2_regs_dump?
> > > 
> > >     In fact, most used registers of the most blocks are not more than 100, but
> > >     for Cluster windows,
> > > 
> > >     there is a CLUSTER_CTRL register sting at 0x100.
> > > 
> > >     I think i should add the size to struct vop2_regs_dump.
> > > 
> > > 
> > >   +       u32 val;
> > >   +       int i;
> > >   +
> > >   +       if (dump->en_mask) {
> > >   +               val = vop2_readl(vop2, dump->base + dump->en_reg);
> > >   +               if ((val & dump->en_mask) != dump->en_val)
> > >   +                       return;
> > >   +       }
> > >   +       seq_printf(s, "\n%s:\n", dump->name);
> > >   +
> > >   +       start = vop2->res->start + dump->base;
> > >   +       for (i = 0; i < reg_num;) {
> > >   +               seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
> > >   +                          vop2_readl(vop2, dump->base + (4 * i)),
> > >   +                          vop2_readl(vop2, dump->base + (4 * (i + 1))),
> > >   +                          vop2_readl(vop2, dump->base + (4 * (i + 2))),
> > >   +                          vop2_readl(vop2, dump->base + (4 * (i + 3))));
> > >   +               i += 4;
> > >   +       }
> > >   +
> > >   +}
> > >   +
> > >   +static int vop2_regs_show(struct seq_file *s, void *arg)
> > >   +{
> > >   +       struct drm_info_node *node = s->private;
> > >   +       struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
> > >   +       struct drm_minor *minor = node->minor;
> > >   +       struct drm_device *drm_dev = minor->dev;
> > >   +
> > >   +       struct vop2_regs_dump dump;
> > >   +
> > >   +       drm_modeset_lock_all(drm_dev);
> > >   +
> > >   +       if (vop2->enable_count) {
> > >   +               dump.en_mask = 0;
> > >   +
> > >   +               dump.name = "SYS";
> > >   +               dump.base = RK3568_REG_CFG_DONE;
> > >   +               vop2_regs_print(vop2, s, &dump);
> > > 
> > >   Can you create a statically initialized array of struct vop2_regs_dump
> > >   and iterate over it?
> > >   You would need an additional present_in_soc_xy flag in struct
> > >   vop2_regs_dump, but other than that I don't see a problem and the result
> > >   might look better.
> > > 
> > >   For the windows it might also be an option to iterate over
> > >   vop2->data->win instead. This array already contains the register base
> > >   addresses and window names.
> > > 
> > >     In fact, we have a dump_regs  arrar in vop2_data per soc in our bsp
> > >     kernel[0],
> > > 
> > >     do you like something like that?
> > > 
> > >     [0]
> > >     [3]https://github.com/armbian/linux-rockchip/blob/rk-5.10-rkr6/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c#L3684
> > 
> > This looks good from a first glance. I would suggest using C99
> > initializers though.
> 
> 
> Thanks for your reply, but am not quiet claer about the C99 initializers, would you plase make it more specific,
> or give some example ?

Instead of:

static const struct vop_dump_regs rk3588_dump_regs[] = {
	{ RK3568_REG_CFG_DONE, "SYS", {0}, 0 },

do:

static const struct vop_dump_regs rk3588_dump_regs[] = {
	{
		.offset = RK3568_REG_CFG_DONE.
		.name = "SYS"
	},
	...

It's a bit more verbose but better readable for people who don't know
the order of the struct members.

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 v2 11/12] drm/rockchip: vop2: Add debugfs support

[Andy Yan]

Hi Sascha:

On 11/29/23 20:59, Sascha Hauer wrote:
> On Wed, Nov 29, 2023 at 07:01:37PM +0800, Andy Yan wrote:
>> Hi Sascha:
>>
>>
>>
>> On 11/29/23 16:52, Sascha Hauer wrote:
>>> On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
>>>>      Hi Sascha:
>>>>
>>>>      thanks for you review.
>>>>
>>>>      On 11/27/23 18:13, Sascha Hauer wrote:
>>>>
>>>>    On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
>>>>
>>>>    From: Andy Yan [1]<andy.yan@rock-chips.com>
>>>>
>>>>    /sys/kernel/debug/dri/vop2/summary:  dump vop display state
>>>>    /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
>>>>    /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
>>>>    activated modules
>>>>
>>>>    Signed-off-by: Andy Yan [2]<andy.yan@rock-chips.com>
>>>>    ---
>>>>
>>>>    (no changes since v1)
>>>>
>>>>     drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++++++++++++++++++
>>>>     1 file changed, 399 insertions(+)
>>>>
>>>>    diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>    index 9eecbe1f71f9..4a2342209c15 100644
>>>>    --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>    +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>>    @@ -27,6 +27,7 @@
>>>>     #include <drm/drm_debugfs.h>
>>>>     #include <drm/drm_flip_work.h>
>>>>     #include <drm/drm_framebuffer.h>
>>>>    +#include <drm/drm_gem_framebuffer_helper.h>
>>>>     #include <drm/drm_probe_helper.h>
>>>>     #include <drm/drm_vblank.h>
>>>>
>>>>    @@ -187,6 +188,7 @@ struct vop2 {
>>>>             */
>>>>            u32 registered_num_wins;
>>>>
>>>>    +       struct resource *res;
>>>>            void __iomem *regs;
>>>>            struct regmap *map;
>>>>
>>>>    @@ -228,6 +230,44 @@ struct vop2 {
>>>>     #define vop2_output_if_is_lvds(x)      (x == ROCKCHIP_VOP2_EP_LVDS0 || x == ROCKCHIP_VOP2_EP_LVDS1)
>>>>     #define vop2_output_if_is_dpi(x)       (x == ROCKCHIP_VOP2_EP_RGB0)
>>>>
>>>>    +struct vop2_regs_dump {
>>>>    +       const char *name;
>>>>    +       u32 base;
>>>>    +       u32 en_reg;
>>>>    +       u32 en_val;
>>>>    +       u32 en_mask;
>>>>    +};
>>>>    +
>>>>    +/*
>>>>    + * bus-format types.
>>>>    + */
>>>>    +struct drm_bus_format_enum_list {
>>>>    +       int type;
>>>>    +       const char *name;
>>>>    +};
>>>>    +
>>>>    +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
>>>>    +       { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
>>>>    +       { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
>>>>    +       { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
>>>>    +       { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
>>>>    +       { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
>>>>    +       { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
>>>>    +       { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
>>>>    +       { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
>>>>    +       { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
>>>>    +       { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
>>>>    +       { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
>>>>    +       { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
>>>>    +       { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
>>>>    +       { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
>>>>    +       { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
>>>>    +       { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
>>>>    +       { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
>>>>    +       { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
>>>>    +};
>>>>    +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
>>>>    +
>>>>     static const struct regmap_config vop2_regmap_config;
>>>>
>>>>     static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
>>>>    @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
>>>>            .atomic_disable = vop2_crtc_atomic_disable,
>>>>     };
>>>>
>>>>    +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct seq_file *s)
>>>>    +{
>>>>    +       struct drm_connector_list_iter conn_iter;
>>>>    +       struct drm_connector *connector;
>>>>    +
>>>>    +       drm_connector_list_iter_begin(crtc->dev, &conn_iter);
>>>>    +       drm_for_each_connector_iter(connector, &conn_iter) {
>>>>    +               if (crtc->state->connector_mask & drm_connector_mask(connector))
>>>>    +                       seq_printf(s, "    Connector: %s\n", connector->name);
>>>>    +
>>>>    +       }
>>>>    +       drm_connector_list_iter_end(&conn_iter);
>>>>    +}
>>>>    +
>>>>    +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
>>>>    +{
>>>>    +       struct vop2_win *win = to_vop2_win(plane);
>>>>    +       struct drm_plane_state *pstate = plane->state;
>>>>    +       struct drm_rect *src, *dst;
>>>>    +       struct drm_framebuffer *fb;
>>>>    +       struct drm_gem_object *obj;
>>>>    +       struct rockchip_gem_object *rk_obj;
>>>>    +       bool xmirror;
>>>>    +       bool ymirror;
>>>>    +       bool rotate_270;
>>>>    +       bool rotate_90;
>>>>    +       dma_addr_t fb_addr;
>>>>    +       int i;
>>>>    +
>>>>    +       seq_printf(s, "    %s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" : "DISABLED");
>>>>    +       if (!pstate || !pstate->fb)
>>>>    +               return 0;
>>>>    +
>>>>    +       fb = pstate->fb;
>>>>    +       src = &pstate->src;
>>>>    +       dst = &pstate->dst;
>>>>    +       xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
>>>>    +       ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
>>>>    +       rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
>>>>    +       rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
>>>>    +
>>>>    +       seq_printf(s, "\twin_id: %d\n", win->win_id);
>>>>    +
>>>>    +       seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
>>>>    +                  &fb->format->format,
>>>>    +                  drm_is_afbc(fb->modifier) ? "[AFBC]" : "",
>>>>    +                  pstate->alpha >> 8);
>>>>    +       seq_printf(s, "\trotate: xmirror: %d ymirror: %d rotate_90: %d rotate_270: %d\n",
>>>>    +                  xmirror, ymirror, rotate_90, rotate_270);
>>>>    +       seq_printf(s, "\tzpos: %d\n", pstate->normalized_zpos);
>>>>    +       seq_printf(s, "\tsrc: pos[%d, %d] rect[%d x %d]\n", src->x1 >> 16,
>>>>    +                  src->y1 >> 16, drm_rect_width(src) >> 16,
>>>>    +                  drm_rect_height(src) >> 16);
>>>>    +       seq_printf(s, "\tdst: pos[%d, %d] rect[%d x %d]\n", dst->x1, dst->y1,
>>>>    +                  drm_rect_width(dst), drm_rect_height(dst));
>>>>    +
>>>>    +       for (i = 0; i < fb->format->num_planes; i++) {
>>>>    +               obj = fb->obj[0];
>>>>    +               rk_obj = to_rockchip_obj(obj);
>>>>    +               fb_addr = rk_obj->dma_addr + fb->offsets[0];
>>>>    +
>>>>    +               seq_printf(s, "\tbuf[%d]: addr: %pad pitch: %d offset: %d\n",
>>>>    +                          i, &fb_addr, fb->pitches[i], fb->offsets[i]);
>>>>    +       }
>>>>    +
>>>>    +       return 0;
>>>>    +}
>>>>    +
>>>>    +static int vop2_crtc_state_dump(struct drm_crtc *crtc, struct seq_file *s)
>>>>    +{
>>>>    +       struct vop2_video_port *vp = to_vop2_video_port(crtc);
>>>>    +       struct drm_crtc_state *cstate = crtc->state;
>>>>    +       struct rockchip_crtc_state *vcstate;
>>>>    +       struct drm_display_mode *mode;
>>>>    +       struct drm_plane *plane;
>>>>    +       bool interlaced;
>>>>    +
>>>>    +       seq_printf(s, "Video Port%d: %s\n", vp->id, !cstate ?
>>>>    +                  "DISABLED" : cstate->active ? "ACTIVE" : "DISABLED");
>>>>    +
>>>>    +       if (!cstate || !cstate->active)
>>>>    +               return 0;
>>>>    +
>>>>    +       mode = &crtc->state->adjusted_mode;
>>>>    +       vcstate = to_rockchip_crtc_state(cstate);
>>>>    +       interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
>>>>    +
>>>>    +       vop2_dump_connector_on_crtc(crtc, s);
>>>>    +       seq_printf(s, "\tbus_format[%x]: %s\n", vcstate->bus_format,
>>>>    +                   drm_get_bus_format_name(vcstate->bus_format));
>>>>    +       seq_printf(s, "\toutput_mode[%x]", vcstate->output_mode);
>>>>    +       seq_printf(s, " color_space[%d]\n", vcstate->color_space);
>>>>    +       seq_printf(s, "    Display mode: %dx%d%s%d\n",
>>>>    +                   mode->hdisplay, mode->vdisplay, interlaced ? "i" : "p",
>>>>    +                   drm_mode_vrefresh(mode));
>>>>    +       seq_printf(s, "\tclk[%d] real_clk[%d] type[%x] flag[%x]\n",
>>>>    +                   mode->clock, mode->crtc_clock, mode->type, mode->flags);
>>>>    +       seq_printf(s, "\tH: %d %d %d %d\n", mode->hdisplay, mode->hsync_start,
>>>>    +                   mode->hsync_end, mode->htotal);
>>>>    +       seq_printf(s, "\tV: %d %d %d %d\n", mode->vdisplay, mode->vsync_start,
>>>>    +                   mode->vsync_end, mode->vtotal);
>>>>    +
>>>>    +       drm_atomic_crtc_for_each_plane(plane, crtc) {
>>>>    +               vop2_plane_state_dump(s, plane);
>>>>    +       }
>>>>    +
>>>>    +       return 0;
>>>>    +}
>>>>    +
>>>>    +static int vop2_summary_show(struct seq_file *s, void *data)
>>>>    +{
>>>>    +       struct drm_info_node *node = s->private;
>>>>    +       struct drm_minor *minor = node->minor;
>>>>    +       struct drm_device *drm_dev = minor->dev;
>>>>    +       struct drm_crtc *crtc;
>>>>    +
>>>>    +       drm_modeset_lock_all(drm_dev);
>>>>    +       drm_for_each_crtc(crtc, drm_dev) {
>>>>    +               vop2_crtc_state_dump(crtc, s);
>>>>    +       }
>>>>    +       drm_modeset_unlock_all(drm_dev);
>>>>    +
>>>>    +       return 0;
>>>>    +}
>>>>    +
>>>>    +static void vop2_regs_print(struct vop2 *vop2, struct seq_file *s, struct vop2_regs_dump *dump)
>>>>    +{
>>>>    +       resource_size_t start;
>>>>    +       const int reg_num = 0x110 / 4;
>>>>
>>>>    If I'm not mistaken this prints a register space of 0x110 bytes.
>>>>    Shouldn't it be 0x100 bytes instead?
>>>>
>>>>    Also, are all these register spaces really have the same size? Does it
>>>>    make sense to add the size to struct vop2_regs_dump?
>>>>
>>>>      In fact, most used registers of the most blocks are not more than 100, but
>>>>      for Cluster windows,
>>>>
>>>>      there is a CLUSTER_CTRL register sting at 0x100.
>>>>
>>>>      I think i should add the size to struct vop2_regs_dump.
>>>>
>>>>
>>>>    +       u32 val;
>>>>    +       int i;
>>>>    +
>>>>    +       if (dump->en_mask) {
>>>>    +               val = vop2_readl(vop2, dump->base + dump->en_reg);
>>>>    +               if ((val & dump->en_mask) != dump->en_val)
>>>>    +                       return;
>>>>    +       }
>>>>    +       seq_printf(s, "\n%s:\n", dump->name);
>>>>    +
>>>>    +       start = vop2->res->start + dump->base;
>>>>    +       for (i = 0; i < reg_num;) {
>>>>    +               seq_printf(s, "%08x:  %08x %08x %08x %08x\n", (u32)start + i * 4,
>>>>    +                          vop2_readl(vop2, dump->base + (4 * i)),
>>>>    +                          vop2_readl(vop2, dump->base + (4 * (i + 1))),
>>>>    +                          vop2_readl(vop2, dump->base + (4 * (i + 2))),
>>>>    +                          vop2_readl(vop2, dump->base + (4 * (i + 3))));
>>>>    +               i += 4;
>>>>    +       }
>>>>    +
>>>>    +}
>>>>    +
>>>>    +static int vop2_regs_show(struct seq_file *s, void *arg)
>>>>    +{
>>>>    +       struct drm_info_node *node = s->private;
>>>>    +       struct vop2 *vop2 = (struct vop2 *)node->info_ent->data;
>>>>    +       struct drm_minor *minor = node->minor;
>>>>    +       struct drm_device *drm_dev = minor->dev;
>>>>    +
>>>>    +       struct vop2_regs_dump dump;
>>>>    +
>>>>    +       drm_modeset_lock_all(drm_dev);
>>>>    +
>>>>    +       if (vop2->enable_count) {
>>>>    +               dump.en_mask = 0;
>>>>    +
>>>>    +               dump.name = "SYS";
>>>>    +               dump.base = RK3568_REG_CFG_DONE;
>>>>    +               vop2_regs_print(vop2, s, &dump);
>>>>
>>>>    Can you create a statically initialized array of struct vop2_regs_dump
>>>>    and iterate over it?
>>>>    You would need an additional present_in_soc_xy flag in struct
>>>>    vop2_regs_dump, but other than that I don't see a problem and the result
>>>>    might look better.
>>>>
>>>>    For the windows it might also be an option to iterate over
>>>>    vop2->data->win instead. This array already contains the register base
>>>>    addresses and window names.
>>>>
>>>>      In fact, we have a dump_regs  arrar in vop2_data per soc in our bsp
>>>>      kernel[0],
>>>>
>>>>      do you like something like that?
>>>>
>>>>      [0]
>>>>      [3]https://github.com/armbian/linux-rockchip/blob/rk-5.10-rkr6/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c#L3684
>>>
>>> This looks good from a first glance. I would suggest using C99
>>> initializers though.
>>
>>
>> Thanks for your reply, but am not quiet claer about the C99 initializers, would you plase make it more specific,
>> or give some example ?
> 
> Instead of:
> 
> static const struct vop_dump_regs rk3588_dump_regs[] = {
> 	{ RK3568_REG_CFG_DONE, "SYS", {0}, 0 },
> 
> do:
> 
> static const struct vop_dump_regs rk3588_dump_regs[] = {
> 	{
> 		.offset = RK3568_REG_CFG_DONE.
> 		.name = "SYS"
> 	},
> 	...
> 
> It's a bit more verbose but better readable for people who don't know
> the order of the struct members.


Got it, will do.
> 
> Sascha
> 

[PATCH v2 12/12] arm64: dts: rockchip: Add vop on rk3588

[Andy Yan]

From: Andy Yan <andy.yan@rock-chips.com>

Add vop dt node for rk3588.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

(no changes since v1)

 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 96 +++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 7064c0e9179f..c1007b1cbead 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -362,6 +362,11 @@ spll: clock-0 {
 		#clock-cells = <0>;
 	};
 
+	display_subsystem: display-subsystem {
+		compatible = "rockchip,display-subsystem";
+		ports = <&vop_out>;
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH 0>,
@@ -474,6 +479,16 @@ sys_grf: syscon@fd58c000 {
 		reg = <0x0 0xfd58c000 0x0 0x1000>;
 	};
 
+	vop_grf: syscon@fd5a4000 {
+		compatible = "rockchip,rk3588-vop-grf", "syscon";
+		reg = <0x0 0xfd5a4000 0x0 0x2000>;
+	};
+
+	vo1_grf: syscon@fd5a8000 {
+		compatible = "rockchip,rk3588-vo-grf", "syscon";
+		reg = <0x0 0xfd5a8000 0x0 0x100>;
+	};
+
 	php_grf: syscon@fd5b0000 {
 		compatible = "rockchip,rk3588-php-grf", "syscon";
 		reg = <0x0 0xfd5b0000 0x0 0x1000>;
@@ -593,6 +608,87 @@ i2c0: i2c@fd880000 {
 		status = "disabled";
 	};
 
+	vop: vop@fdd90000 {
+		compatible = "rockchip,rk3588-vop";
+		reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
+		reg-names = "vop", "gamma_lut";
+		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_VOP>,
+			 <&cru HCLK_VOP>,
+			 <&cru DCLK_VOP0>,
+			 <&cru DCLK_VOP1>,
+			 <&cru DCLK_VOP2>,
+			 <&cru DCLK_VOP3>,
+			 <&cru PCLK_VOP_ROOT>;
+		clock-names = "aclk",
+			      "hclk",
+			      "dclk_vp0",
+			      "dclk_vp1",
+			      "dclk_vp2",
+			      "dclk_vp3",
+			      "pclk_vop";
+		resets = <&cru SRST_A_VOP>,
+			 <&cru SRST_H_VOP>,
+			 <&cru SRST_D_VOP0>,
+			 <&cru SRST_D_VOP1>,
+			 <&cru SRST_D_VOP2>,
+			 <&cru SRST_D_VOP3>;
+		reset-names = "axi",
+			      "ahb",
+			      "dclk_vp0",
+			      "dclk_vp1",
+			      "dclk_vp2",
+			      "dclk_vp3";
+		iommus = <&vop_mmu>;
+		power-domains = <&power RK3588_PD_VOP>;
+		rockchip,grf = <&sys_grf>;
+		rockchip,vop-grf = <&vop_grf>;
+		rockchip,vo1-grf = <&vo1_grf>;
+		rockchip,pmu = <&pmu>;
+
+		status = "disabled";
+		vop_out: ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			vp0: port@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+			};
+
+			vp1: port@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+			};
+
+			vp2: port@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+			};
+
+			vp3: port@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+			};
+		};
+	};
+
+	vop_mmu: iommu@fdd97e00 {
+		compatible = "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
+		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "vop_mmu";
+		clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
+		clock-names = "aclk", "iface";
+		#iommu-cells = <0>;
+		power-domains = <&power RK3588_PD_VOP>;
+		status = "disabled";
+	};
+
 	uart0: serial@fd890000 {
 		compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
 		reg = <0x0 0xfd890000 0x0 0x100>;
-- 
2.34.1