[PATCH v5 0/3] Add support for AM62L DSS

[PATCH v5 0/3] Add support for AM62L DSS

[Devarsh Thakkar]

This adds support for DSS subsystem present in TI's AM62L SoC
which supports single display pipeline with DPI output which
is also routed to DSI Tx controller within the SoC.

Change Log:
V5:
- Use hw_id instead of index for places where it was missed
  so that we pick correct base address for vid region

V4:
- Update vid_info struct to keep hw_id and instantiate
  only for actually existing pipes

V3:
- Make generic infra to support truncated K3 DSS IP's
- Remove AM62A updates from AM62L DT binding updates

V2:
- Fix incorrect format of compatible string (comma instead of
  hyphen) for AM62L SoC
- Use separate register space and helper functions for AM62L
  due to minor differences in register offset/bit position differences
  for first plane

Rangediff:
V4->V5:
- https://gist.github.com/devarsht/a0e6aa7b1c19f47facd0058962e3c3c2

V3->V4:
- https://gist.github.com/devarsht/1e75c9e1ac0cdfc01703a0776e31e782

V2->V3:
- https://gist.github.com/devarsht/24fa8dd2986861efa431352d19ebbb41

V1->V2
- https://gist.github.com/devarsht/11d47f25ca9fea6976e6284330ddf443

Links to previous versions:
V4: https://lore.kernel.org/all/20250326145736.3659670-1-devarsht@ti.com/
V3: https://lore.kernel.org/all/20250306132914.1469387-1-devarsht@ti.com/
V2: https://lore.kernel.org/all/20250204061552.3720261-1-devarsht@ti.com/
V1: https://lore.kernel.org/all/20241231090432.3649158-1-devarsht@ti.com/

Test logs:
https://gist.github.com/devarsht/82505ca69f0bd5d9788bfc240d2e83d4

Devarsh Thakkar (3):
  dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS
  drm/tidss: Update infrastructure to support K3 DSS cut-down versions
  drm/tidss: Add support for AM62L display subsystem

 .../bindings/display/ti/ti,am65x-dss.yaml     |  21 +-
 drivers/gpu/drm/tidss/tidss_crtc.c            |  11 +-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 193 ++++++++++++++----
 drivers/gpu/drm/tidss/tidss_dispc.h           |  13 +-
 drivers/gpu/drm/tidss/tidss_drv.c             |   1 +
 drivers/gpu/drm/tidss/tidss_kms.c             |   2 +-
 drivers/gpu/drm/tidss/tidss_plane.c           |   2 +-
 7 files changed, 195 insertions(+), 48 deletions(-)

-- 
2.39.1

[PATCH v5 1/3] dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS

[Devarsh Thakkar]

The DSS controller on TI's AM62L SoC is an update from that on TI's
AM625/AM65x/AM62A7 SoC. The AM62L DSS [1] only supports a single display
pipeline using a single overlay manager, single video port and a single
video lite pipeline which does not support scaling.

The output of video port is routed to SoC boundary via DPI interface and
the DPI signals from the video port are also routed to DSI Tx controller
present within the SoC.

[1]: Section 11.7 (Display Subsystem and Peripherals)
Link : https://www.ti.com/lit/pdf/sprujb4

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V5:
- No change

V4:
- No change

V3:
- Remove AM62A references as suggested
- Add Reviewed-by

V2: 
- Add Reviewed-by
- s/ti,am62l,dss/ti,am62l-dss
 
 .../bindings/display/ti/ti,am65x-dss.yaml     | 21 ++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 31c4ffcb599c..a5b13cb7bc73 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -12,18 +12,25 @@ maintainers:
   - Tomi Valkeinen <tomi.valkeinen@ti.com>
 
 description: |
-  The AM625 and AM65x TI Keystone Display SubSystem with two output
+  The AM625 and AM65x TI Keystone Display SubSystem has two output
   ports and two video planes. In AM65x DSS, the first video port
   supports 1 OLDI TX and in AM625 DSS, the first video port output is
   internally routed to 2 OLDI TXes. The second video port supports DPI
   format. The first plane is full video plane with all features and the
   second is a "lite plane" without scaling support.
+  The AM62L display subsystem has a single output port which supports DPI
+  format but it only supports single video "lite plane" which does not support
+  scaling. The output port is routed to SoC boundary via DPI interface and same
+  DPI signals are also routed internally to DSI Tx controller present within the
+  SoC. Due to clocking limitations only one of the interface i.e. either DSI or
+  DPI can be used at once.
 
 properties:
   compatible:
     enum:
       - ti,am625-dss
       - ti,am62a7-dss
+      - ti,am62l-dss
       - ti,am65x-dss
 
   reg:
@@ -91,6 +98,8 @@ properties:
           For AM625 DSS, the internal DPI output port node from video
           port 1.
           For AM62A7 DSS, the port is tied off inside the SoC.
+          For AM62L DSS, the DSS DPI output port node from video port 1
+          or DSI Tx controller node connected to video port 1.
 
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
@@ -123,6 +132,16 @@ allOf:
         ports:
           properties:
             port@0: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am62l-dss
+    then:
+      properties:
+        ports:
+          properties:
+            port@1: false
 
 required:
   - compatible
-- 
2.39.1

[PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

SoCs like AM62Lx support cut-down version of K3 DSS where although same
register space is supported as in other K3 DSS supported SoCs such as
AM65x, AM62x, AM62Ax but some of the resources such as planes and
corresponding register spaces are truncated.

For e.g. AM62Lx has only single VIDL pipeline supported, so corresponding
register spaces for other video pipelines need to be skipped.

To add a generic support for future SoCs where one or more video pipelines
can get truncated from the parent register space, move the video plane
related information to vid_info struct which will also have a field to
indicate hardware index of each of the available video planes, so that
driver only maps and programs those video pipes and skips the unavailable
ones.

While at it, also change the num_planes field in the features structure to
num_vid so that all places in code which use vid_info structure are
highlighted in the code.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V5:
- Use separate variable for hw_id and add it in missing places to access
  correct VID pipeline bits in common registers

V4:
- Create vid_info struct only for instantiated planes
- s/num_planes/num_vids
- s/vid_lite/is_lite
- Add hw_id member in vid_info struct and remove is_present

V2->V3:
- No change (patch introduced in V3)
 
 drivers/gpu/drm/tidss/tidss_crtc.c  |  11 +-
 drivers/gpu/drm/tidss/tidss_dispc.c | 152 +++++++++++++++++++++-------
 drivers/gpu/drm/tidss/tidss_dispc.h |  11 +-
 drivers/gpu/drm/tidss/tidss_kms.c   |   2 +-
 drivers/gpu/drm/tidss/tidss_plane.c |   2 +-
 5 files changed, 131 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 94f8e3178df5..c555f6717e7d 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -130,7 +130,7 @@ static void tidss_crtc_position_planes(struct tidss_device *tidss,
 	    !to_tidss_crtc_state(cstate)->plane_pos_changed)
 		return;
 
-	for (layer = 0; layer < tidss->feat->num_planes; layer++) {
+	for (layer = 0; layer < tidss->feat->num_vids ; layer++) {
 		struct drm_plane_state *pstate;
 		struct drm_plane *plane;
 		bool layer_active = false;
@@ -271,9 +271,12 @@ static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
 	 * another videoport, the DSS will report sync lost issues. Disable all
 	 * the layers here as a work-around.
 	 */
-	for (u32 layer = 0; layer < tidss->feat->num_planes; layer++)
-		dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, layer,
-				       false);
+	for (u32 layer = 0; layer < tidss->feat->num_vids; layer++) {
+		u32 hw_id = tidss->feat->vid_info[layer].hw_id;
+
+		dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport,
+				       hw_id, false);
+	}
 
 	dispc_vp_disable(tidss->dispc, tcrtc->hw_videoport);
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index cacb5f3d8085..da6fe4e3ca85 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -103,9 +103,16 @@ const struct dispc_features dispc_k2g_feats = {
 		},
 	},
 
-	.num_planes = 1,
-	.vid_name = { "vid1" },
-	.vid_lite = { false },
+	.num_vids = 1,
+
+	.vid_info = {
+		{
+			.name = "vid1",
+			.is_lite = false,
+			.hw_id = 0,
+		},
+	},
+
 	.vid_order = { 0 },
 };
 
@@ -178,11 +185,22 @@ const struct dispc_features dispc_am65x_feats = {
 		},
 	},
 
-	.num_planes = 2,
+	.num_vids = 2,
 	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
-	.vid_name = { "vid", "vidl1" },
-	.vid_lite = { false, true, },
-	.vid_order = { 1, 0 },
+	.vid_info = {
+		{
+			.name = "vid",
+			.is_lite = false,
+			.hw_id = 0,
+		},
+		{
+			.name = "vidl1",
+			.is_lite = true,
+			.hw_id = 1,
+		},
+	},
+
+	.vid_order = {1, 0},
 };
 
 static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
@@ -267,9 +285,32 @@ const struct dispc_features dispc_j721e_feats = {
 			.gamma_type = TIDSS_GAMMA_10BIT,
 		},
 	},
-	.num_planes = 4,
-	.vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
-	.vid_lite = { 0, 1, 0, 1, },
+
+	.num_vids = 4,
+
+	.vid_info = {
+		{
+			.name = "vid1",
+			.is_lite = false,
+			.hw_id = 0,
+		},
+		{
+			.name = "vidl1",
+			.is_lite = true,
+			.hw_id = 1,
+		},
+		{
+			.name = "vid2",
+			.is_lite = false,
+			.hw_id = 2,
+		},
+		{
+			.name = "vidl2",
+			.is_lite = true,
+			.hw_id = 3,
+		},
+	},
+
 	.vid_order = { 1, 3, 0, 2 },
 };
 
@@ -315,11 +356,23 @@ const struct dispc_features dispc_am625_feats = {
 		},
 	},
 
-	.num_planes = 2,
+	.num_vids = 2,
+
 	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
-	.vid_name = { "vid", "vidl1" },
-	.vid_lite = { false, true, },
-	.vid_order = { 1, 0 },
+	.vid_info = {
+		{
+			.name = "vid",
+			.is_lite = false,
+			.hw_id = 0,
+		},
+		{
+			.name = "vidl1",
+			.is_lite = true,
+			.hw_id = 1,
+		}
+	},
+
+	.vid_order = {1, 0},
 };
 
 const struct dispc_features dispc_am62a7_feats = {
@@ -369,11 +422,22 @@ const struct dispc_features dispc_am62a7_feats = {
 		},
 	},
 
-	.num_planes = 2,
-	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
-	.vid_name = { "vid", "vidl1" },
-	.vid_lite = { false, true, },
-	.vid_order = { 1, 0 },
+	.num_vids = 2,
+
+	.vid_info = {
+		{
+			.name = "vid",
+			.is_lite = false,
+			.hw_id = 0,
+		},
+		{
+			.name = "vidl1",
+			.is_lite = true,
+			.hw_id = 1,
+		}
+	},
+
+	.vid_order = {1, 0},
 };
 
 static const u16 *dispc_common_regmap;
@@ -788,9 +852,12 @@ void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
 		if (clearmask & DSS_IRQ_VP_MASK(i))
 			dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
 	}
-	for (i = 0; i < dispc->feat->num_planes; ++i) {
+
+	for (i = 0; i < dispc->feat->num_vids; ++i) {
+		u32 hw_id = dispc->feat->vid_info[i].hw_id;
+
 		if (clearmask & DSS_IRQ_PLANE_MASK(i))
-			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
+			dispc_k3_vid_write_irqstatus(dispc, hw_id, clearmask);
 	}
 
 	/* always clear the top level irqstatus */
@@ -809,8 +876,11 @@ dispc_irq_t dispc_k3_read_and_clear_irqstatus(struct dispc_device *dispc)
 	for (i = 0; i < dispc->feat->num_vps; ++i)
 		status |= dispc_k3_vp_read_irqstatus(dispc, i);
 
-	for (i = 0; i < dispc->feat->num_planes; ++i)
-		status |= dispc_k3_vid_read_irqstatus(dispc, i);
+	for (i = 0; i < dispc->feat->num_vids; ++i) {
+		u32 hw_id = dispc->feat->vid_info[i].hw_id;
+
+		status |= dispc_k3_vid_read_irqstatus(dispc, hw_id);
+	}
 
 	dispc_k3_clear_irqstatus(dispc, status);
 
@@ -825,8 +895,11 @@ static dispc_irq_t dispc_k3_read_irqenable(struct dispc_device *dispc)
 	for (i = 0; i < dispc->feat->num_vps; ++i)
 		enable |= dispc_k3_vp_read_irqenable(dispc, i);
 
-	for (i = 0; i < dispc->feat->num_planes; ++i)
-		enable |= dispc_k3_vid_read_irqenable(dispc, i);
+	for (i = 0; i < dispc->feat->num_vids; ++i) {
+		u32 hw_id = dispc->feat->vid_info[i].hw_id;
+
+		enable |= dispc_k3_vid_read_irqenable(dispc, hw_id);
+	}
 
 	return enable;
 }
@@ -849,19 +922,22 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
 			main_enable |= BIT(i);		/* VP IRQ */
 		else
 			main_disable |= BIT(i);		/* VP IRQ */
+
 	}
 
-	for (i = 0; i < dispc->feat->num_planes; ++i) {
-		dispc_k3_vid_set_irqenable(dispc, i, mask);
+	for (i = 0; i < dispc->feat->num_vids; ++i) {
+		u32 hw_id = dispc->feat->vid_info[i].hw_id;
+
+		dispc_k3_vid_set_irqenable(dispc, hw_id, mask);
+
 		if (mask & DSS_IRQ_PLANE_MASK(i))
-			main_enable |= BIT(i + 4);	/* VID IRQ */
+			main_enable |= BIT(hw_id + 4);	/* VID IRQ */
 		else
-			main_disable |= BIT(i + 4);	/* VID IRQ */
+			main_disable |= BIT(hw_id + 4);	/* VID IRQ */
 	}
 
 	if (main_enable)
 		dispc_write(dispc, DISPC_IRQENABLE_SET, main_enable);
-
 	if (main_disable)
 		dispc_write(dispc, DISPC_IRQENABLE_CLR, main_disable);
 
@@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device *dispc, u32 hw_plane,
 		      const struct drm_plane_state *state,
 		      u32 hw_videoport)
 {
-	bool lite = dispc->feat->vid_lite[hw_plane];
+	bool lite = dispc->feat->vid_info[hw_plane].is_lite;
 	u32 fourcc = state->fb->format->format;
 	bool need_scaling = state->src_w >> 16 != state->crtc_w ||
 		state->src_h >> 16 != state->crtc_h;
@@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
 		       const struct drm_plane_state *state,
 		       u32 hw_videoport)
 {
-	bool lite = dispc->feat->vid_lite[hw_plane];
+	bool lite = dispc->feat->vid_info[hw_plane].is_lite;
 	u32 fourcc = state->fb->format->format;
 	u16 cpp = state->fb->format->cpp[0];
 	u32 fb_width = state->fb->pitches[0] / cpp;
@@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct dispc_device *dispc)
 	/* MFLAG_START = MFLAGNORMALSTARTMODE */
 	REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
 
-	for (hw_plane = 0; hw_plane < dispc->feat->num_planes; hw_plane++) {
+	for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
 		u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
 		u32 thr_low, thr_high;
 		u32 mflag_low, mflag_high;
@@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct dispc_device *dispc)
 
 		dev_dbg(dispc->dev,
 			"%s: bufsize %u, buf_threshold %u/%u, mflag threshold %u/%u preload %u\n",
-			dispc->feat->vid_name[hw_plane],
+			dispc->feat->vid_info[hw_plane].name,
 			size,
 			thr_high, thr_low,
 			mflag_high, mflag_low,
@@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct dispc_device *dispc)
 	/* MFLAG_START = MFLAGNORMALSTARTMODE */
 	REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
 
-	for (hw_plane = 0; hw_plane < dispc->feat->num_planes; hw_plane++) {
+	for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
 		u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
 		u32 thr_low, thr_high;
 		u32 mflag_low, mflag_high;
@@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct dispc_device *dispc)
 
 		dev_dbg(dispc->dev,
 			"%s: bufsize %u, buf_threshold %u/%u, mflag threshold %u/%u preload %u\n",
-			dispc->feat->vid_name[hw_plane],
+			dispc->feat->vid_info[hw_plane].name,
 			size,
 			thr_high, thr_low,
 			mflag_high, mflag_low,
@@ -2898,8 +2974,8 @@ int dispc_init(struct tidss_device *tidss)
 	if (r)
 		return r;
 
-	for (i = 0; i < dispc->feat->num_planes; i++) {
-		r = dispc_iomap_resource(pdev, dispc->feat->vid_name[i],
+	for (i = 0; i < dispc->feat->num_vids; i++) {
+		r = dispc_iomap_resource(pdev, dispc->feat->vid_info[i].name,
 					 &dispc->base_vid[i]);
 		if (r)
 			return r;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..72a0146e57d5 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -46,6 +46,12 @@ struct dispc_features_scaling {
 	u32 xinc_max;
 };
 
+struct dispc_vid_info {
+	const char *name; /* Should match dt reg names */
+	u32 hw_id;
+	bool is_lite;
+};
+
 struct dispc_errata {
 	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
 };
@@ -82,9 +88,8 @@ struct dispc_features {
 	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
 	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
 	struct tidss_vp_feat vp_feat;
-	u32 num_planes;
-	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
-	bool vid_lite[TIDSS_MAX_PLANES];
+	u32 num_vids;
+	struct dispc_vid_info vid_info[TIDSS_MAX_PLANES];
 	u32 vid_order[TIDSS_MAX_PLANES];
 };
 
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index f371518f8697..19432c08ec6b 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -115,7 +115,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 
 	const struct dispc_features *feat = tidss->feat;
 	u32 max_vps = feat->num_vps;
-	u32 max_planes = feat->num_planes;
+	u32 max_planes = feat->num_vids;
 
 	struct pipe pipes[TIDSS_MAX_PORTS];
 	u32 num_pipes = 0;
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
index 719412e6c346..142ae81951a0 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -200,7 +200,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
 	struct tidss_plane *tplane;
 	enum drm_plane_type type;
 	u32 possible_crtcs;
-	u32 num_planes = tidss->feat->num_planes;
+	u32 num_planes = tidss->feat->num_vids;
 	u32 color_encodings = (BIT(DRM_COLOR_YCBCR_BT601) |
 			       BIT(DRM_COLOR_YCBCR_BT709));
 	u32 color_ranges = (BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
-- 
2.39.1

[PATCH v5 3/3] drm/tidss: Add support for AM62L display subsystem

[Devarsh Thakkar]

Enable display for AM62L DSS [1] which supports only a single display
pipeline using a single overlay manager, single video port and a single
video lite pipeline which does not support scaling.

The output of video port is routed to SoC boundary via DPI interface and
the DPI signals from the video port are also routed to DSI Tx controller
present within the SoC.

[1]: Section 11.7 (Display Subsystem and Peripherals)
Link : https://www.ti.com/lit/pdf/sprujb4

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V5:
- No change

V4:
- Rebase on top of previous patch to use vid_info structure

V3: 
- Rebase on top of
  0002-drm-tidss-Update-infra-to-support-DSS7-cut-down-vers.patch
- Use the generic "tidss_am65x_common_regs" as common reg space
  instead of creating a new one

V2: 
- Add separate common reg space for AM62L
- Add separate irq enable/disable/read/clear helpers for AM62L
- Use separate helper function for setting overlay attributes
- Drop Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
  due to additional changes made in V2.

 drivers/gpu/drm/tidss/tidss_dispc.c | 41 +++++++++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
 drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
 3 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index da6fe4e3ca85..6ec7aff95cc6 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -440,6 +440,42 @@ const struct dispc_features dispc_am62a7_feats = {
 	.vid_order = {1, 0},
 };
 
+const struct dispc_features dispc_am62l_feats = {
+	.max_pclk_khz = {
+		[DISPC_VP_DPI] = 165000,
+	},
+
+	.subrev = DISPC_AM62L,
+
+	.common = "common",
+	.common_regs = tidss_am65x_common_regs,
+
+	.num_vps = 1,
+	.vp_name = { "vp1" },
+	.ovr_name = { "ovr1" },
+	.vpclk_name =  { "vp1" },
+	.vp_bus_type = { DISPC_VP_DPI },
+
+	.vp_feat = { .color = {
+			.has_ctm = true,
+			.gamma_size = 256,
+			.gamma_type = TIDSS_GAMMA_8BIT,
+		},
+	},
+
+	.num_vids = 1,
+
+	.vid_info = {
+		{
+			.name = "vidl1",
+			.is_lite = true,
+			.hw_id = 1,
+		}
+	},
+
+	.vid_order = {0},
+};
+
 static const u16 *dispc_common_regmap;
 
 struct dss_vp_data {
@@ -955,6 +991,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
 		return dispc_k2g_read_and_clear_irqstatus(dispc);
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		return dispc_k3_read_and_clear_irqstatus(dispc);
@@ -972,6 +1009,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_set_irqenable(dispc, mask);
@@ -1464,6 +1502,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 		dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
 					  x, y, layer);
@@ -2384,6 +2423,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 	case DISPC_J721E:
 		dispc_k3_plane_init(dispc);
@@ -2492,6 +2532,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
 		break;
 	case DISPC_AM625:
 	case DISPC_AM62A7:
+	case DISPC_AM62L:
 	case DISPC_AM65X:
 		dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
 		break;
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 72a0146e57d5..28958514b8f5 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -67,6 +67,7 @@ enum dispc_vp_bus_type {
 enum dispc_dss_subrevision {
 	DISPC_K2G,
 	DISPC_AM625,
+	DISPC_AM62L,
 	DISPC_AM62A7,
 	DISPC_AM65X,
 	DISPC_J721E,
@@ -96,6 +97,7 @@ struct dispc_features {
 extern const struct dispc_features dispc_k2g_feats;
 extern const struct dispc_features dispc_am625_feats;
 extern const struct dispc_features dispc_am62a7_feats;
+extern const struct dispc_features dispc_am62l_feats;
 extern const struct dispc_features dispc_am65x_feats;
 extern const struct dispc_features dispc_j721e_feats;
 
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d4652e8cc28c..f2a4f659f574 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -242,6 +242,7 @@ static const struct of_device_id tidss_of_table[] = {
 	{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
 	{ .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
 	{ .compatible = "ti,am62a7-dss", .data = &dispc_am62a7_feats, },
+	{ .compatible = "ti,am62l-dss", .data = &dispc_am62l_feats, },
 	{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
 	{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
 	{ }
-- 
2.39.1

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

Hi,

On 29/04/2025 17:36, Devarsh Thakkar wrote:
> SoCs like AM62Lx support cut-down version of K3 DSS where although same
> register space is supported as in other K3 DSS supported SoCs such as
> AM65x, AM62x, AM62Ax but some of the resources such as planes and
> corresponding register spaces are truncated.
> 
> For e.g. AM62Lx has only single VIDL pipeline supported, so corresponding
> register spaces for other video pipelines need to be skipped.
> 
> To add a generic support for future SoCs where one or more video pipelines
> can get truncated from the parent register space, move the video plane
> related information to vid_info struct which will also have a field to
> indicate hardware index of each of the available video planes, so that
> driver only maps and programs those video pipes and skips the unavailable
> ones.
> 
> While at it, also change the num_planes field in the features structure to
> num_vid so that all places in code which use vid_info structure are
> highlighted in the code.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V5:
> - Use separate variable for hw_id and add it in missing places to access
>    correct VID pipeline bits in common registers
> 
> V4:
> - Create vid_info struct only for instantiated planes
> - s/num_planes/num_vids
> - s/vid_lite/is_lite
> - Add hw_id member in vid_info struct and remove is_present
> 
> V2->V3:
> - No change (patch introduced in V3)
>   
>   drivers/gpu/drm/tidss/tidss_crtc.c  |  11 +-
>   drivers/gpu/drm/tidss/tidss_dispc.c | 152 +++++++++++++++++++++-------
>   drivers/gpu/drm/tidss/tidss_dispc.h |  11 +-
>   drivers/gpu/drm/tidss/tidss_kms.c   |   2 +-
>   drivers/gpu/drm/tidss/tidss_plane.c |   2 +-
>   5 files changed, 131 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 94f8e3178df5..c555f6717e7d 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -130,7 +130,7 @@ static void tidss_crtc_position_planes(struct tidss_device *tidss,
>   	    !to_tidss_crtc_state(cstate)->plane_pos_changed)
>   		return;
>   
> -	for (layer = 0; layer < tidss->feat->num_planes; layer++) {
> +	for (layer = 0; layer < tidss->feat->num_vids ; layer++) {
>   		struct drm_plane_state *pstate;
>   		struct drm_plane *plane;
>   		bool layer_active = false;
> @@ -271,9 +271,12 @@ static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
>   	 * another videoport, the DSS will report sync lost issues. Disable all
>   	 * the layers here as a work-around.
>   	 */
> -	for (u32 layer = 0; layer < tidss->feat->num_planes; layer++)
> -		dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, layer,
> -				       false);
> +	for (u32 layer = 0; layer < tidss->feat->num_vids; layer++) {
> +		u32 hw_id = tidss->feat->vid_info[layer].hw_id;
> +
> +		dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport,
> +				       hw_id, false);
> +	}
>   
>   	dispc_vp_disable(tidss->dispc, tcrtc->hw_videoport);
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cacb5f3d8085..da6fe4e3ca85 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -103,9 +103,16 @@ const struct dispc_features dispc_k2g_feats = {
>   		},
>   	},
>   
> -	.num_planes = 1,
> -	.vid_name = { "vid1" },
> -	.vid_lite = { false },
> +	.num_vids = 1,
> +
> +	.vid_info = {
> +		{
> +			.name = "vid1",
> +			.is_lite = false,
> +			.hw_id = 0,
> +		},
> +	},
> +
>   	.vid_order = { 0 },
>   };
>   
> @@ -178,11 +185,22 @@ const struct dispc_features dispc_am65x_feats = {
>   		},
>   	},
>   
> -	.num_planes = 2,
> +	.num_vids = 2,
>   	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
> -	.vid_name = { "vid", "vidl1" },
> -	.vid_lite = { false, true, },
> -	.vid_order = { 1, 0 },
> +	.vid_info = {
> +		{
> +			.name = "vid",
> +			.is_lite = false,
> +			.hw_id = 0,
> +		},
> +		{
> +			.name = "vidl1",
> +			.is_lite = true,
> +			.hw_id = 1,
> +		},
> +	},
> +
> +	.vid_order = {1, 0},
>   };
>   
>   static const u16 tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
> @@ -267,9 +285,32 @@ const struct dispc_features dispc_j721e_feats = {
>   			.gamma_type = TIDSS_GAMMA_10BIT,
>   		},
>   	},
> -	.num_planes = 4,
> -	.vid_name = { "vid1", "vidl1", "vid2", "vidl2" },
> -	.vid_lite = { 0, 1, 0, 1, },
> +
> +	.num_vids = 4,
> +
> +	.vid_info = {
> +		{
> +			.name = "vid1",
> +			.is_lite = false,
> +			.hw_id = 0,
> +		},
> +		{
> +			.name = "vidl1",
> +			.is_lite = true,
> +			.hw_id = 1,
> +		},
> +		{
> +			.name = "vid2",
> +			.is_lite = false,
> +			.hw_id = 2,
> +		},
> +		{
> +			.name = "vidl2",
> +			.is_lite = true,
> +			.hw_id = 3,
> +		},
> +	},
> +
>   	.vid_order = { 1, 3, 0, 2 },
>   };
>   
> @@ -315,11 +356,23 @@ const struct dispc_features dispc_am625_feats = {
>   		},
>   	},
>   
> -	.num_planes = 2,
> +	.num_vids = 2,
> +
>   	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
> -	.vid_name = { "vid", "vidl1" },
> -	.vid_lite = { false, true, },
> -	.vid_order = { 1, 0 },
> +	.vid_info = {
> +		{
> +			.name = "vid",
> +			.is_lite = false,
> +			.hw_id = 0,
> +		},
> +		{
> +			.name = "vidl1",
> +			.is_lite = true,
> +			.hw_id = 1,
> +		}
> +	},
> +
> +	.vid_order = {1, 0},
>   };
>   
>   const struct dispc_features dispc_am62a7_feats = {
> @@ -369,11 +422,22 @@ const struct dispc_features dispc_am62a7_feats = {
>   		},
>   	},
>   
> -	.num_planes = 2,
> -	/* note: vid is plane_id 0 and vidl1 is plane_id 1 */
> -	.vid_name = { "vid", "vidl1" },
> -	.vid_lite = { false, true, },
> -	.vid_order = { 1, 0 },
> +	.num_vids = 2,
> +
> +	.vid_info = {
> +		{
> +			.name = "vid",
> +			.is_lite = false,
> +			.hw_id = 0,
> +		},
> +		{
> +			.name = "vidl1",
> +			.is_lite = true,
> +			.hw_id = 1,
> +		}
> +	},
> +
> +	.vid_order = {1, 0},
>   };
>   
>   static const u16 *dispc_common_regmap;
> @@ -788,9 +852,12 @@ void dispc_k3_clear_irqstatus(struct dispc_device *dispc, dispc_irq_t clearmask)
>   		if (clearmask & DSS_IRQ_VP_MASK(i))
>   			dispc_k3_vp_write_irqstatus(dispc, i, clearmask);
>   	}
> -	for (i = 0; i < dispc->feat->num_planes; ++i) {
> +
> +	for (i = 0; i < dispc->feat->num_vids; ++i) {
> +		u32 hw_id = dispc->feat->vid_info[i].hw_id;
> +
>   		if (clearmask & DSS_IRQ_PLANE_MASK(i))
> -			dispc_k3_vid_write_irqstatus(dispc, i, clearmask);
> +			dispc_k3_vid_write_irqstatus(dispc, hw_id, clearmask);
>   	}
>   
>   	/* always clear the top level irqstatus */
> @@ -809,8 +876,11 @@ dispc_irq_t dispc_k3_read_and_clear_irqstatus(struct dispc_device *dispc)
>   	for (i = 0; i < dispc->feat->num_vps; ++i)
>   		status |= dispc_k3_vp_read_irqstatus(dispc, i);
>   
> -	for (i = 0; i < dispc->feat->num_planes; ++i)
> -		status |= dispc_k3_vid_read_irqstatus(dispc, i);
> +	for (i = 0; i < dispc->feat->num_vids; ++i) {
> +		u32 hw_id = dispc->feat->vid_info[i].hw_id;
> +
> +		status |= dispc_k3_vid_read_irqstatus(dispc, hw_id);
> +	}
>   
>   	dispc_k3_clear_irqstatus(dispc, status);
>   
> @@ -825,8 +895,11 @@ static dispc_irq_t dispc_k3_read_irqenable(struct dispc_device *dispc)
>   	for (i = 0; i < dispc->feat->num_vps; ++i)
>   		enable |= dispc_k3_vp_read_irqenable(dispc, i);
>   
> -	for (i = 0; i < dispc->feat->num_planes; ++i)
> -		enable |= dispc_k3_vid_read_irqenable(dispc, i);
> +	for (i = 0; i < dispc->feat->num_vids; ++i) {
> +		u32 hw_id = dispc->feat->vid_info[i].hw_id;
> +
> +		enable |= dispc_k3_vid_read_irqenable(dispc, hw_id);
> +	}
>   
>   	return enable;
>   }
> @@ -849,19 +922,22 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
>   			main_enable |= BIT(i);		/* VP IRQ */
>   		else
>   			main_disable |= BIT(i);		/* VP IRQ */
> +
>   	}
>   
> -	for (i = 0; i < dispc->feat->num_planes; ++i) {
> -		dispc_k3_vid_set_irqenable(dispc, i, mask);
> +	for (i = 0; i < dispc->feat->num_vids; ++i) {
> +		u32 hw_id = dispc->feat->vid_info[i].hw_id;
> +
> +		dispc_k3_vid_set_irqenable(dispc, hw_id, mask);
> +
>   		if (mask & DSS_IRQ_PLANE_MASK(i))
> -			main_enable |= BIT(i + 4);	/* VID IRQ */
> +			main_enable |= BIT(hw_id + 4);	/* VID IRQ */
>   		else
> -			main_disable |= BIT(i + 4);	/* VID IRQ */
> +			main_disable |= BIT(hw_id + 4);	/* VID IRQ */
>   	}
>   
>   	if (main_enable)
>   		dispc_write(dispc, DISPC_IRQENABLE_SET, main_enable);
> -
>   	if (main_disable)
>   		dispc_write(dispc, DISPC_IRQENABLE_CLR, main_disable);
>   
> @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device *dispc, u32 hw_plane,
>   		      const struct drm_plane_state *state,
>   		      u32 hw_videoport)
>   {
> -	bool lite = dispc->feat->vid_lite[hw_plane];
> +	bool lite = dispc->feat->vid_info[hw_plane].is_lite;

I don't think this is correct. You can't access the vid_info[] with the 
hw-id.

>   	u32 fourcc = state->fb->format->format;
>   	bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>   		state->src_h >> 16 != state->crtc_h;
> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>   		       const struct drm_plane_state *state,
>   		       u32 hw_videoport)
>   {
> -	bool lite = dispc->feat->vid_lite[hw_plane];
> +	bool lite = dispc->feat->vid_info[hw_plane].is_lite;

Here too.

>   	u32 fourcc = state->fb->format->format;
>   	u16 cpp = state->fb->format->cpp[0];
>   	u32 fb_width = state->fb->pitches[0] / cpp;
> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct dispc_device *dispc)
>   	/* MFLAG_START = MFLAGNORMALSTARTMODE */
>   	REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>   
> -	for (hw_plane = 0; hw_plane < dispc->feat->num_planes; hw_plane++) {
> +	for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>   		u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>   		u32 thr_low, thr_high;
>   		u32 mflag_low, mflag_high;
> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct dispc_device *dispc)
>   
>   		dev_dbg(dispc->dev,
>   			"%s: bufsize %u, buf_threshold %u/%u, mflag threshold %u/%u preload %u\n",
> -			dispc->feat->vid_name[hw_plane],
> +			dispc->feat->vid_info[hw_plane].name,

Here hw_plane is not actually the hw-id (anymore), but elsewhere in this 
function it is used as a hw-id, which is no longer correct.

>   			size,
>   			thr_high, thr_low,
>   			mflag_high, mflag_low,
> @@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct dispc_device *dispc)
>   	/* MFLAG_START = MFLAGNORMALSTARTMODE */
>   	REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>   
> -	for (hw_plane = 0; hw_plane < dispc->feat->num_planes; hw_plane++) {
> +	for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>   		u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>   		u32 thr_low, thr_high;
>   		u32 mflag_low, mflag_high;
> @@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct dispc_device *dispc)
>   
>   		dev_dbg(dispc->dev,
>   			"%s: bufsize %u, buf_threshold %u/%u, mflag threshold %u/%u preload %u\n",
> -			dispc->feat->vid_name[hw_plane],
> +			dispc->feat->vid_info[hw_plane].name,

And here.

All these issues make me wonder whether we have the right model. It's 
just too easy to get the usage wrong.

I'm not sure which way to go here.

Fix the current issues? It's a bit cumbersome to go from hw-id to the 
index (needs a search), just to get some hw properties.

Or go back to the earlier one, with a vid array containing unused slots? 
That makes the for loops a bit harder.

I need to think about it...

  Tomi

>   			size,
>   			thr_high, thr_low,
>   			mflag_high, mflag_low,
> @@ -2898,8 +2974,8 @@ int dispc_init(struct tidss_device *tidss)
>   	if (r)
>   		return r;
>   
> -	for (i = 0; i < dispc->feat->num_planes; i++) {
> -		r = dispc_iomap_resource(pdev, dispc->feat->vid_name[i],
> +	for (i = 0; i < dispc->feat->num_vids; i++) {
> +		r = dispc_iomap_resource(pdev, dispc->feat->vid_info[i].name,
>   					 &dispc->base_vid[i]);
>   		if (r)
>   			return r;
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 086327d51a90..72a0146e57d5 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -46,6 +46,12 @@ struct dispc_features_scaling {
>   	u32 xinc_max;
>   };
>   
> +struct dispc_vid_info {
> +	const char *name; /* Should match dt reg names */
> +	u32 hw_id;
> +	bool is_lite;
> +};
> +
>   struct dispc_errata {
>   	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
>   };
> @@ -82,9 +88,8 @@ struct dispc_features {
>   	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
>   	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
>   	struct tidss_vp_feat vp_feat;
> -	u32 num_planes;
> -	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
> -	bool vid_lite[TIDSS_MAX_PLANES];
> +	u32 num_vids;
> +	struct dispc_vid_info vid_info[TIDSS_MAX_PLANES];
>   	u32 vid_order[TIDSS_MAX_PLANES];
>   };
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index f371518f8697..19432c08ec6b 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -115,7 +115,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>   
>   	const struct dispc_features *feat = tidss->feat;
>   	u32 max_vps = feat->num_vps;
> -	u32 max_planes = feat->num_planes;
> +	u32 max_planes = feat->num_vids;
>   
>   	struct pipe pipes[TIDSS_MAX_PORTS];
>   	u32 num_pipes = 0;
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index 719412e6c346..142ae81951a0 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -200,7 +200,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
>   	struct tidss_plane *tplane;
>   	enum drm_plane_type type;
>   	u32 possible_crtcs;
> -	u32 num_planes = tidss->feat->num_planes;
> +	u32 num_planes = tidss->feat->num_vids;
>   	u32 color_encodings = (BIT(DRM_COLOR_YCBCR_BT601) |
>   			       BIT(DRM_COLOR_YCBCR_BT709));
>   	u32 color_ranges = (BIT(DRM_COLOR_YCBCR_FULL_RANGE) |

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

Hi Tomi

Thanks for the review.

<snip>
>>   @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>> *dispc, u32 hw_plane,
>>                 const struct drm_plane_state *state,
>>                 u32 hw_videoport)
>>   {
>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
> 
> I don't think this is correct. You can't access the vid_info[] with the
> hw-id.

I don't think hw_id is getting passed to hw_plane here. The
dispc_plane_check is called from tidss_plane_atomic_check which passes
hw_plane as tplane->hw_plane_id and this index starts from actually
instantiated planes i.e. from 0 and are contiguous as these are
populated from vid_order array (hw_plane_id =
feat->vid_order[tidss->num_planes];) and not the hw_id index.

So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
getting passed as 0 and that's how it is able to access the first and
only member of vid_info struct and read the properties correctly and
function properly as seen in test logs [1].

> 
>>       u32 fourcc = state->fb->format->format;
>>       bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>           state->src_h >> 16 != state->crtc_h;
>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>> *dispc, u32 hw_plane,
>>                  const struct drm_plane_state *state,
>>                  u32 hw_videoport)
>>   {
>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
> 
> Here too.

Here also hw_plane is getting passed as 0 and not the hw_id which is 1
for AM62L.

> 
>>       u32 fourcc = state->fb->format->format;
>>       u16 cpp = state->fb->format->cpp[0];
>>       u32 fb_width = state->fb->pitches[0] / cpp;
>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>> dispc_device *dispc)
>>       /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>       REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>   -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>> hw_plane++) {
>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>           u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>           u32 thr_low, thr_high;
>>           u32 mflag_low, mflag_high;
>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>> dispc_device *dispc)
>>             dev_dbg(dispc->dev,
>>               "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>> %u/%u preload %u\n",
>> -            dispc->feat->vid_name[hw_plane],
>> +            dispc->feat->vid_info[hw_plane].name,
> 
> Here hw_plane is not actually the hw-id (anymore), but elsewhere in this
> function it is used as a hw-id, which is no longer correct.

For accessing vid_info hw_plane needs to be used which is the index of
actually instantiated planes and I see it as correctly being passed for
AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
skip the not-instantiated vid regions by adding the offset per the hw_id
index.

> 
>>               size,
>>               thr_high, thr_low,
>>               mflag_high, mflag_low,
>> @@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct
>> dispc_device *dispc)
>>       /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>       REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>   -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>> hw_plane++) {
>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>           u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>           u32 thr_low, thr_high;
>>           u32 mflag_low, mflag_high;
>> @@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct
>> dispc_device *dispc)
>>             dev_dbg(dispc->dev,
>>               "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>> %u/%u preload %u\n",
>> -            dispc->feat->vid_name[hw_plane],
>> +            dispc->feat->vid_info[hw_plane].name,
> 
> And here.
> 
> All these issues make me wonder whether we have the right model. It's
> just too easy to get the usage wrong.
> 
> I'm not sure which way to go here.
> 
> Fix the current issues? It's a bit cumbersome to go from hw-id to the
> index (needs a search), just to get some hw properties.
> 
> Or go back to the earlier one, with a vid array containing unused slots?
> That makes the for loops a bit harder.
> 
> I need to think about it...
> 

Hmm, I don't think so, it seems to look fine to me and work fine too. I
have tested thoroughly for AM62L (which has uninstantiated vid region)
along with AM62x and AM62A with all planes displayed simultaneously. If
you want I can put on some test logs, create some dummy holes for VID
regions in AM62 and AM62A to put this on to some further negative tests.

Also if naming convention is confusing (hw_id vs hw_plane) then maybe we
can use something else like vid_idx ??

[1]: https://gist.github.com/devarsht/82505ca69f0bd5d9788bfc240d2e83d4

Regards
Devarsh

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

On 30/04/2025 19:37, Devarsh Thakkar wrote:
> Hi Tomi
> 
> Thanks for the review.
> 
> <snip>
>>>    @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>> *dispc, u32 hw_plane,
>>>                  const struct drm_plane_state *state,
>>>                  u32 hw_videoport)
>>>    {
>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>
>> I don't think this is correct. You can't access the vid_info[] with the
>> hw-id.
> 
> I don't think hw_id is getting passed to hw_plane here. The
> dispc_plane_check is called from tidss_plane_atomic_check which passes
> hw_plane as tplane->hw_plane_id and this index starts from actually
> instantiated planes i.e. from 0 and are contiguous as these are

Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed 
now), and tidss_plane.c calls dispc_plane_enable() with 
tplane->hw_plane_id as the hw_plane parameter, which is used as a HW 
plane ID... Then... One of these is wrong, no?

> populated from vid_order array (hw_plane_id =
> feat->vid_order[tidss->num_planes];) and not the hw_id index.
> 
> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
> getting passed as 0 and that's how it is able to access the first and
> only member of vid_info struct and read the properties correctly and
> function properly as seen in test logs [1].

If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable() 
call would enable the wrong plane, wouldn't it?

But even if it all works, I think this highlights how confusing it is...

> 
>>
>>>        u32 fourcc = state->fb->format->format;
>>>        bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>            state->src_h >> 16 != state->crtc_h;
>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>> *dispc, u32 hw_plane,
>>>                   const struct drm_plane_state *state,
>>>                   u32 hw_videoport)
>>>    {
>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>
>> Here too.
> 
> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
> for AM62L.
> 
>>
>>>        u32 fourcc = state->fb->format->format;
>>>        u16 cpp = state->fb->format->cpp[0];
>>>        u32 fb_width = state->fb->pitches[0] / cpp;
>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>> dispc_device *dispc)
>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>> hw_plane++) {
>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>            u32 thr_low, thr_high;
>>>            u32 mflag_low, mflag_high;
>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>> dispc_device *dispc)
>>>              dev_dbg(dispc->dev,
>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>> %u/%u preload %u\n",
>>> -            dispc->feat->vid_name[hw_plane],
>>> +            dispc->feat->vid_info[hw_plane].name,
>>
>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in this
>> function it is used as a hw-id, which is no longer correct.
> 
> For accessing vid_info hw_plane needs to be used which is the index of
> actually instantiated planes and I see it as correctly being passed for
> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
> skip the not-instantiated vid regions by adding the offset per the hw_id
> index.

Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to 
access the vid_info[], and as a parameter to functions that take 
hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses 
wrong?

Oh, wait... I think I see it now. For some functions using the hw_id as 
the hw_plane parameter is fine, as they access the VID's registers by 
just using, e.g. dispc_vid_write(), which gets the address correctly 
from dispc->base_vid[hw_plane], as that one is indexed from 0 to num_vids.

But some functions use registers that have bits based on the hw_id (like 
dispc_k3_vid_write_irqstatus), and then we use the hw_id for the 
hw_plane parameter. If that function were to also write a vid register, 
using the passed hw_plane, it wouldn't work, but I guess we don't do that.

It feels broken... We can't have 'hw_plane' that's sometimes the HW id 
(i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).

>>
>>>                size,
>>>                thr_high, thr_low,
>>>                mflag_high, mflag_low,
>>> @@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct
>>> dispc_device *dispc)
>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>> hw_plane++) {
>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>            u32 thr_low, thr_high;
>>>            u32 mflag_low, mflag_high;
>>> @@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct
>>> dispc_device *dispc)
>>>              dev_dbg(dispc->dev,
>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>> %u/%u preload %u\n",
>>> -            dispc->feat->vid_name[hw_plane],
>>> +            dispc->feat->vid_info[hw_plane].name,
>>
>> And here.
>>
>> All these issues make me wonder whether we have the right model. It's
>> just too easy to get the usage wrong.
>>
>> I'm not sure which way to go here.
>>
>> Fix the current issues? It's a bit cumbersome to go from hw-id to the
>> index (needs a search), just to get some hw properties.
>>
>> Or go back to the earlier one, with a vid array containing unused slots?
>> That makes the for loops a bit harder.
>>
>> I need to think about it...
>>
> 
> Hmm, I don't think so, it seems to look fine to me and work fine too. I
> have tested thoroughly for AM62L (which has uninstantiated vid region)
> along with AM62x and AM62A with all planes displayed simultaneously. If
> you want I can put on some test logs, create some dummy holes for VID
> regions in AM62 and AM62A to put this on to some further negative tests.
 >
> Also if naming convention is confusing (hw_id vs hw_plane) then maybe we
> can use something else like vid_idx ??

It is confusing. But I think it's also broken, in the sense that e.g. 
dispc_k3_vid_write_irqstatus() has hw_plane parameter. But it's actually 
hw_id.

I'm not sure if naming them differently helps here. It's super 
confusing. What indices do we have?

- The lowest level HW IDs, e.g. for DISPC_VID_IRQSTATUS()
- The index for the dispc->vid_info[]
- The index to tidss->planes[]
- drm_plane->index

Originally I kept the drm_plane and the HW index separate, so that the 
dispc.c doesn't really deal with the drm_plane at all. But I wonder if 
we need to change that, as drm_plane pointer can't really be 
"understood" wrong, whereas an two indices are easy to mix.

  Tomi

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

Hi Tomi

Thanks for quick comments.

On 30/04/25 23:12, Tomi Valkeinen wrote:
> On 30/04/2025 19:37, Devarsh Thakkar wrote:
>> Hi Tomi
>>
>> Thanks for the review.
>>
>> <snip>
>>>>    @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>>> *dispc, u32 hw_plane,
>>>>                  const struct drm_plane_state *state,
>>>>                  u32 hw_videoport)
>>>>    {
>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>
>>> I don't think this is correct. You can't access the vid_info[] with the
>>> hw-id.
>>
>> I don't think hw_id is getting passed to hw_plane here. The
>> dispc_plane_check is called from tidss_plane_atomic_check which passes
>> hw_plane as tplane->hw_plane_id and this index starts from actually
>> instantiated planes i.e. from 0 and are contiguous as these are
> 
> Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed
> now), and tidss_plane.c calls dispc_plane_enable() with tplane-
>>hw_plane_id as the hw_plane parameter, which is used as a HW plane
> ID... Then... One of these is wrong, no?
> 

As mentioned here [1], dispc_plane_enable acts on VID_* registers which
are only mapped per the instantiated/actual pipes present in the SoC, so
the indexing always starts from 0 and we need not worry about skipping
un-instantiated planes.

So hw_plane_id -> Index of only instantiated planes starting from 0
hw_id -> Hardware Index taking into account instantiated +
un-instantiated/skipped planes main used for common0/1 region registers
dealing with VID planes.


For e.g. for AM62L which includes VIDL pipe
hw_plane_id -> 0
hw_id -> 1


>> populated from vid_order array (hw_plane_id =
>> feat->vid_order[tidss->num_planes];) and not the hw_id index.
>>
>> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
>> getting passed as 0 and that's how it is able to access the first and
>> only member of vid_info struct and read the properties correctly and
>> function properly as seen in test logs [1].
> 
> If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable()
> call would enable the wrong plane, wouldn't it?
> 
> But even if it all works, I think this highlights how confusing it is...
> 
>>
>>>
>>>>        u32 fourcc = state->fb->format->format;
>>>>        bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>>            state->src_h >> 16 != state->crtc_h;
>>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>>> *dispc, u32 hw_plane,
>>>>                   const struct drm_plane_state *state,
>>>>                   u32 hw_videoport)
>>>>    {
>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>
>>> Here too.
>>
>> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
>> for AM62L.
>>
>>>
>>>>        u32 fourcc = state->fb->format->format;
>>>>        u16 cpp = state->fb->format->cpp[0];
>>>>        u32 fb_width = state->fb->pitches[0] / cpp;
>>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>>> dispc_device *dispc)
>>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>> hw_plane++) {
>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>            u32 thr_low, thr_high;
>>>>            u32 mflag_low, mflag_high;
>>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>>> dispc_device *dispc)
>>>>              dev_dbg(dispc->dev,
>>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>> %u/%u preload %u\n",
>>>> -            dispc->feat->vid_name[hw_plane],
>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>
>>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in this
>>> function it is used as a hw-id, which is no longer correct.
>>
>> For accessing vid_info hw_plane needs to be used which is the index of
>> actually instantiated planes and I see it as correctly being passed for
>> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
>> skip the not-instantiated vid regions by adding the offset per the hw_id
>> index.
> 
> Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to
> access the vid_info[], and as a parameter to functions that take
> hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses
> wrong?
> 
> Oh, wait... I think I see it now. For some functions using the hw_id as
> the hw_plane parameter is fine, as they access the VID's registers by
> just using, e.g. dispc_vid_write(), which gets the address correctly
> from dispc->base_vid[hw_plane], as that one is indexed from 0 to num_vids.
> 

Yes exactly.

> But some functions use registers that have bits based on the hw_id (like
> dispc_k3_vid_write_irqstatus), and then we use the hw_id for the
> hw_plane parameter. If that function were to also write a vid register,
> using the passed hw_plane, it wouldn't work, but I guess we don't do that.
> 

Yes, hw_id is only for dispc_k3_vid* functions dealing with common
region registers that play with VID pipes.

> It feels broken... We can't have 'hw_plane' that's sometimes the HW id
> (i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).
> 

Sorry I don't follow, what exactly is broken here. hw_plane is for
instantiated planes present in SoC used in context of VID* register
space while doing reg writess and hw_id is the plane hardware index
w.r.t larger K3 family i.e used in context for common register space.

>>>
>>>>                size,
>>>>                thr_high, thr_low,
>>>>                mflag_high, mflag_low,
>>>> @@ -2265,7 +2341,7 @@ static void dispc_k3_plane_init(struct
>>>> dispc_device *dispc)
>>>>        /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>        REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>    -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>> hw_plane++) {
>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>>            u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>            u32 thr_low, thr_high;
>>>>            u32 mflag_low, mflag_high;
>>>> @@ -2281,7 +2357,7 @@ static void dispc_k3_plane_init(struct
>>>> dispc_device *dispc)
>>>>              dev_dbg(dispc->dev,
>>>>                "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>> %u/%u preload %u\n",
>>>> -            dispc->feat->vid_name[hw_plane],
>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>
>>> And here.
>>>
>>> All these issues make me wonder whether we have the right model. It's
>>> just too easy to get the usage wrong.
>>>
>>> I'm not sure which way to go here.
>>>
>>> Fix the current issues? It's a bit cumbersome to go from hw-id to the
>>> index (needs a search), just to get some hw properties.
>>>
>>> Or go back to the earlier one, with a vid array containing unused slots?
>>> That makes the for loops a bit harder.
>>>
>>> I need to think about it...
>>>
>>
>> Hmm, I don't think so, it seems to look fine to me and work fine too. I
>> have tested thoroughly for AM62L (which has uninstantiated vid region)
>> along with AM62x and AM62A with all planes displayed simultaneously. If
>> you want I can put on some test logs, create some dummy holes for VID
>> regions in AM62 and AM62A to put this on to some further negative tests.
>>
>> Also if naming convention is confusing (hw_id vs hw_plane) then maybe we
>> can use something else like vid_idx ??
> 
> It is confusing. But I think it's also broken, in the sense that e.g.
> dispc_k3_vid_write_irqstatus() has hw_plane parameter. But it's actually
> hw_id.
> 
> I'm not sure if naming them differently helps here. It's super
> confusing. What indices do we have?
> 
> - The lowest level HW IDs, e.g. for DISPC_VID_IRQSTATUS()
> - The index for the dispc->vid_info[]
> - The index to tidss->planes[]
> - drm_plane->index
> 

All above are for the instantiated ones so they start from 0.
Since we decide to use "common region" of AM65x family as you suggested
in V1 review comments [2] we need to handle the caveats associated with
VID and VIDL register bits for this common region only w.r.t SoCs which
don't include both of them.

So for e.g. am65x common region include both VID (idx0) and VIDL (idx1)
but am62L does not have VID, but the offset and bit indexing for VIDL in
both  AM65X and AM62L is still the same i.e. idx VIDL.


> Originally I kept the drm_plane and the HW index separate, so that the
> dispc.c doesn't really deal with the drm_plane at all. But I wonder if
> we need to change that, as drm_plane pointer can't really be
> "understood" wrong, whereas an two indices are easy to mix.
> 

Sorry I don't follow exactly on concern above and I am not able to catch
any bugs either in the testing done so far in multiple SoCs. If you see
any potential issue or a negative scenario I can help put debug logs or
provide a register dump.


For the naming if this is confusing, is cmn_vid_idx a better name to
highlight that it only deals with vid/vidl specific indexing and
register/bit writes for common region ?

[1] https://lore.kernel.org/all/096ff788-7a25-47a3-ad13-caff971cf0bc@ti.com/
[2]
https://lore.kernel.org/all/f6b20a29-1205-4f5e-87b6-fec58bd43545@ideasonboard.com/

Regards
Devarsh

>  Tomi
> 

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

Hi,

On 02/05/2025 10:06, Devarsh Thakkar wrote:
> Hi Tomi
> 
> Thanks for quick comments.
> 
> On 30/04/25 23:12, Tomi Valkeinen wrote:
>> On 30/04/2025 19:37, Devarsh Thakkar wrote:
>>> Hi Tomi
>>>
>>> Thanks for the review.
>>>
>>> <snip>
>>>>>     @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>>>> *dispc, u32 hw_plane,
>>>>>                   const struct drm_plane_state *state,
>>>>>                   u32 hw_videoport)
>>>>>     {
>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>
>>>> I don't think this is correct. You can't access the vid_info[] with the
>>>> hw-id.
>>>
>>> I don't think hw_id is getting passed to hw_plane here. The
>>> dispc_plane_check is called from tidss_plane_atomic_check which passes
>>> hw_plane as tplane->hw_plane_id and this index starts from actually
>>> instantiated planes i.e. from 0 and are contiguous as these are
>>
>> Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed
>> now), and tidss_plane.c calls dispc_plane_enable() with tplane-
>>> hw_plane_id as the hw_plane parameter, which is used as a HW plane
>> ID... Then... One of these is wrong, no?
>>
> 
> As mentioned here [1], dispc_plane_enable acts on VID_* registers which
> are only mapped per the instantiated/actual pipes present in the SoC, so
> the indexing always starts from 0 and we need not worry about skipping
> un-instantiated planes.
> 
> So hw_plane_id -> Index of only instantiated planes starting from 0
> hw_id -> Hardware Index taking into account instantiated +
> un-instantiated/skipped planes main used for common0/1 region registers
> dealing with VID planes.
> 
> 
> For e.g. for AM62L which includes VIDL pipe
> hw_plane_id -> 0
> hw_id -> 1
> 
> 
>>> populated from vid_order array (hw_plane_id =
>>> feat->vid_order[tidss->num_planes];) and not the hw_id index.
>>>
>>> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
>>> getting passed as 0 and that's how it is able to access the first and
>>> only member of vid_info struct and read the properties correctly and
>>> function properly as seen in test logs [1].
>>
>> If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable()
>> call would enable the wrong plane, wouldn't it?
>>
>> But even if it all works, I think this highlights how confusing it is...
>>
>>>
>>>>
>>>>>         u32 fourcc = state->fb->format->format;
>>>>>         bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>>>             state->src_h >> 16 != state->crtc_h;
>>>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>>>> *dispc, u32 hw_plane,
>>>>>                    const struct drm_plane_state *state,
>>>>>                    u32 hw_videoport)
>>>>>     {
>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>
>>>> Here too.
>>>
>>> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
>>> for AM62L.
>>>
>>>>
>>>>>         u32 fourcc = state->fb->format->format;
>>>>>         u16 cpp = state->fb->format->cpp[0];
>>>>>         u32 fb_width = state->fb->pitches[0] / cpp;
>>>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>>>> dispc_device *dispc)
>>>>>         /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>>         REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>>     -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>>> hw_plane++) {
>>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids; hw_plane++) {
>>>>>             u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>>             u32 thr_low, thr_high;
>>>>>             u32 mflag_low, mflag_high;
>>>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>>>> dispc_device *dispc)
>>>>>               dev_dbg(dispc->dev,
>>>>>                 "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>>> %u/%u preload %u\n",
>>>>> -            dispc->feat->vid_name[hw_plane],
>>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>>
>>>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in this
>>>> function it is used as a hw-id, which is no longer correct.
>>>
>>> For accessing vid_info hw_plane needs to be used which is the index of
>>> actually instantiated planes and I see it as correctly being passed for
>>> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
>>> skip the not-instantiated vid regions by adding the offset per the hw_id
>>> index.
>>
>> Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to
>> access the vid_info[], and as a parameter to functions that take
>> hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses
>> wrong?
>>
>> Oh, wait... I think I see it now. For some functions using the hw_id as
>> the hw_plane parameter is fine, as they access the VID's registers by
>> just using, e.g. dispc_vid_write(), which gets the address correctly
>> from dispc->base_vid[hw_plane], as that one is indexed from 0 to num_vids.
>>
> 
> Yes exactly.
> 
>> But some functions use registers that have bits based on the hw_id (like
>> dispc_k3_vid_write_irqstatus), and then we use the hw_id for the
>> hw_plane parameter. If that function were to also write a vid register,
>> using the passed hw_plane, it wouldn't work, but I guess we don't do that.
>>
> 
> Yes, hw_id is only for dispc_k3_vid* functions dealing with common
> region registers that play with VID pipes.
> 
>> It feels broken... We can't have 'hw_plane' that's sometimes the HW id
>> (i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).
>>
> 
> Sorry I don't follow, what exactly is broken here. hw_plane is for
> instantiated planes present in SoC used in context of VID* register
> space while doing reg writess and hw_id is the plane hardware index
> w.r.t larger K3 family i.e used in context for common register space.

We have, as an example, these two functions:

void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool 
enable)

static void dispc_k3_vid_write_irqstatus(struct dispc_device *dispc, u32 
hw_plane, dispc_irq_t vidstat)

When the caller calls these functions on AM62L, what does it provide in 
'hw_plane' when it wants to enable the first plane of write the 
irqstatus for the first plane? For dispc_plane_enable(), the caller uses 
0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...

With a quick look at the code, changing the callers to pass the "old 
style" hw_plane as the parameter to those irq functions, and the 
functions internally get the hw_id, would solve most of the problems. 
There's still dispc_k3_set_irqenable() which manages 'main_disable' and 
needs the hw_id, but maybe that's fine, even if a bit confusing.

  Tomi

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

Hi Tomi,

Thanks for the quick revert.

On 02/05/25 13:37, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/05/2025 10:06, Devarsh Thakkar wrote:
>> Hi Tomi
>>
>> Thanks for quick comments.
>>
>> On 30/04/25 23:12, Tomi Valkeinen wrote:
>>> On 30/04/2025 19:37, Devarsh Thakkar wrote:
>>>> Hi Tomi
>>>>
>>>> Thanks for the review.
>>>>
>>>> <snip>
>>>>>>     @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>>>>> *dispc, u32 hw_plane,
>>>>>>                   const struct drm_plane_state *state,
>>>>>>                   u32 hw_videoport)
>>>>>>     {
>>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>>
>>>>> I don't think this is correct. You can't access the vid_info[] with
>>>>> the
>>>>> hw-id.
>>>>
>>>> I don't think hw_id is getting passed to hw_plane here. The
>>>> dispc_plane_check is called from tidss_plane_atomic_check which passes
>>>> hw_plane as tplane->hw_plane_id and this index starts from actually
>>>> instantiated planes i.e. from 0 and are contiguous as these are
>>>
>>> Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed
>>> now), and tidss_plane.c calls dispc_plane_enable() with tplane-
>>>> hw_plane_id as the hw_plane parameter, which is used as a HW plane
>>> ID... Then... One of these is wrong, no?
>>>
>>
>> As mentioned here [1], dispc_plane_enable acts on VID_* registers which
>> are only mapped per the instantiated/actual pipes present in the SoC, so
>> the indexing always starts from 0 and we need not worry about skipping
>> un-instantiated planes.
>>
>> So hw_plane_id -> Index of only instantiated planes starting from 0
>> hw_id -> Hardware Index taking into account instantiated +
>> un-instantiated/skipped planes main used for common0/1 region registers
>> dealing with VID planes.
>>
>>
>> For e.g. for AM62L which includes VIDL pipe
>> hw_plane_id -> 0
>> hw_id -> 1
>>
>>
>>>> populated from vid_order array (hw_plane_id =
>>>> feat->vid_order[tidss->num_planes];) and not the hw_id index.
>>>>
>>>> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
>>>> getting passed as 0 and that's how it is able to access the first and
>>>> only member of vid_info struct and read the properties correctly and
>>>> function properly as seen in test logs [1].
>>>
>>> If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable()
>>> call would enable the wrong plane, wouldn't it?
>>>
>>> But even if it all works, I think this highlights how confusing it is...
>>>
>>>>
>>>>>
>>>>>>         u32 fourcc = state->fb->format->format;
>>>>>>         bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>>>>             state->src_h >> 16 != state->crtc_h;
>>>>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>>>>> *dispc, u32 hw_plane,
>>>>>>                    const struct drm_plane_state *state,
>>>>>>                    u32 hw_videoport)
>>>>>>     {
>>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>>
>>>>> Here too.
>>>>
>>>> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
>>>> for AM62L.
>>>>
>>>>>
>>>>>>         u32 fourcc = state->fb->format->format;
>>>>>>         u16 cpp = state->fb->format->cpp[0];
>>>>>>         u32 fb_width = state->fb->pitches[0] / cpp;
>>>>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>>>>> dispc_device *dispc)
>>>>>>         /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>>>         REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>>>     -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>>>> hw_plane++) {
>>>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids;
>>>>>> hw_plane++) {
>>>>>>             u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>>>             u32 thr_low, thr_high;
>>>>>>             u32 mflag_low, mflag_high;
>>>>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>>>>> dispc_device *dispc)
>>>>>>               dev_dbg(dispc->dev,
>>>>>>                 "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>>>> %u/%u preload %u\n",
>>>>>> -            dispc->feat->vid_name[hw_plane],
>>>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>>>
>>>>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in
>>>>> this
>>>>> function it is used as a hw-id, which is no longer correct.
>>>>
>>>> For accessing vid_info hw_plane needs to be used which is the index of
>>>> actually instantiated planes and I see it as correctly being passed for
>>>> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
>>>> skip the not-instantiated vid regions by adding the offset per the
>>>> hw_id
>>>> index.
>>>
>>> Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to
>>> access the vid_info[], and as a parameter to functions that take
>>> hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses
>>> wrong?
>>>
>>> Oh, wait... I think I see it now. For some functions using the hw_id as
>>> the hw_plane parameter is fine, as they access the VID's registers by
>>> just using, e.g. dispc_vid_write(), which gets the address correctly
>>> from dispc->base_vid[hw_plane], as that one is indexed from 0 to
>>> num_vids.
>>>
>>
>> Yes exactly.
>>
>>> But some functions use registers that have bits based on the hw_id (like
>>> dispc_k3_vid_write_irqstatus), and then we use the hw_id for the
>>> hw_plane parameter. If that function were to also write a vid register,
>>> using the passed hw_plane, it wouldn't work, but I guess we don't do
>>> that.
>>>
>>
>> Yes, hw_id is only for dispc_k3_vid* functions dealing with common
>> region registers that play with VID pipes.
>>
>>> It feels broken... We can't have 'hw_plane' that's sometimes the HW id
>>> (i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).
>>>
>>
>> Sorry I don't follow, what exactly is broken here. hw_plane is for
>> instantiated planes present in SoC used in context of VID* register
>> space while doing reg writess and hw_id is the plane hardware index
>> w.r.t larger K3 family i.e used in context for common register space.
> 
> We have, as an example, these two functions:
> 
> void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool
> enable)
> 
> static void dispc_k3_vid_write_irqstatus(struct dispc_device *dispc, u32
> hw_plane, dispc_irq_t vidstat)
> 
> When the caller calls these functions on AM62L, what does it provide in
> 'hw_plane' when it wants to enable the first plane of write the
> irqstatus for the first plane? 

It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
accessing am65x common region register space which has vid on idx0 which
we want to skip for am62l.

For dispc_plane_enable(), the caller uses
> 0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...

Yes above is correct, and I think that's how it is supposed to be.

> 
> With a quick look at the code, changing the callers to pass the "old
> style" hw_plane as the parameter to those irq functions, and the
> functions internally get the hw_id, would solve most of the problems.

I don't follow above, hw_plane has 0 so it should not be used for
programming irq related functions which expect idx 1 as explained above.

> There's still dispc_k3_set_irqenable() which manages 'main_disable' and
> needs the hw_id, but maybe that's fine, even if a bit confusing.
> 

I still feel there is no inherent bug here, but let me know if you want
me to put some debug prints or get register dump so that we can double
confirm.

Regards
Devarsh
>  Tomi
> 

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

Hi,

On 02/05/2025 13:47, Devarsh Thakkar wrote:
> Hi Tomi,
> 
> Thanks for the quick revert.
> 
> On 02/05/25 13:37, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 02/05/2025 10:06, Devarsh Thakkar wrote:
>>> Hi Tomi
>>>
>>> Thanks for quick comments.
>>>
>>> On 30/04/25 23:12, Tomi Valkeinen wrote:
>>>> On 30/04/2025 19:37, Devarsh Thakkar wrote:
>>>>> Hi Tomi
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> <snip>
>>>>>>>      @@ -2025,7 +2101,7 @@ int dispc_plane_check(struct dispc_device
>>>>>>> *dispc, u32 hw_plane,
>>>>>>>                    const struct drm_plane_state *state,
>>>>>>>                    u32 hw_videoport)
>>>>>>>      {
>>>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>>>
>>>>>> I don't think this is correct. You can't access the vid_info[] with
>>>>>> the
>>>>>> hw-id.
>>>>>
>>>>> I don't think hw_id is getting passed to hw_plane here. The
>>>>> dispc_plane_check is called from tidss_plane_atomic_check which passes
>>>>> hw_plane as tplane->hw_plane_id and this index starts from actually
>>>>> instantiated planes i.e. from 0 and are contiguous as these are
>>>>
>>>> Well, if tplane->hw_plane_id is not the HW plane id (i.e. it's misnamed
>>>> now), and tidss_plane.c calls dispc_plane_enable() with tplane-
>>>>> hw_plane_id as the hw_plane parameter, which is used as a HW plane
>>>> ID... Then... One of these is wrong, no?
>>>>
>>>
>>> As mentioned here [1], dispc_plane_enable acts on VID_* registers which
>>> are only mapped per the instantiated/actual pipes present in the SoC, so
>>> the indexing always starts from 0 and we need not worry about skipping
>>> un-instantiated planes.
>>>
>>> So hw_plane_id -> Index of only instantiated planes starting from 0
>>> hw_id -> Hardware Index taking into account instantiated +
>>> un-instantiated/skipped planes main used for common0/1 region registers
>>> dealing with VID planes.
>>>
>>>
>>> For e.g. for AM62L which includes VIDL pipe
>>> hw_plane_id -> 0
>>> hw_id -> 1
>>>
>>>
>>>>> populated from vid_order array (hw_plane_id =
>>>>> feat->vid_order[tidss->num_planes];) and not the hw_id index.
>>>>>
>>>>> So for e.g. for AM62L even though hw_id is 1 for VIDL hw_plane is
>>>>> getting passed as 0 and that's how it is able to access the first and
>>>>> only member of vid_info struct and read the properties correctly and
>>>>> function properly as seen in test logs [1].
>>>>
>>>> If for AM62L the tplane->hw_plane_id is 0, the the dispc_plane_enable()
>>>> call would enable the wrong plane, wouldn't it?
>>>>
>>>> But even if it all works, I think this highlights how confusing it is...
>>>>
>>>>>
>>>>>>
>>>>>>>          u32 fourcc = state->fb->format->format;
>>>>>>>          bool need_scaling = state->src_w >> 16 != state->crtc_w ||
>>>>>>>              state->src_h >> 16 != state->crtc_h;
>>>>>>> @@ -2096,7 +2172,7 @@ void dispc_plane_setup(struct dispc_device
>>>>>>> *dispc, u32 hw_plane,
>>>>>>>                     const struct drm_plane_state *state,
>>>>>>>                     u32 hw_videoport)
>>>>>>>      {
>>>>>>> -    bool lite = dispc->feat->vid_lite[hw_plane];
>>>>>>> +    bool lite = dispc->feat->vid_info[hw_plane].is_lite;
>>>>>>
>>>>>> Here too.
>>>>>
>>>>> Here also hw_plane is getting passed as 0 and not the hw_id which is 1
>>>>> for AM62L.
>>>>>
>>>>>>
>>>>>>>          u32 fourcc = state->fb->format->format;
>>>>>>>          u16 cpp = state->fb->format->cpp[0];
>>>>>>>          u32 fb_width = state->fb->pitches[0] / cpp;
>>>>>>> @@ -2210,7 +2286,7 @@ static void dispc_k2g_plane_init(struct
>>>>>>> dispc_device *dispc)
>>>>>>>          /* MFLAG_START = MFLAGNORMALSTARTMODE */
>>>>>>>          REG_FLD_MOD(dispc, DISPC_GLOBAL_MFLAG_ATTRIBUTE, 0, 6, 6);
>>>>>>>      -    for (hw_plane = 0; hw_plane < dispc->feat->num_planes;
>>>>>>> hw_plane++) {
>>>>>>> +    for (hw_plane = 0; hw_plane < dispc->feat->num_vids;
>>>>>>> hw_plane++) {
>>>>>>>              u32 size = dispc_vid_get_fifo_size(dispc, hw_plane);
>>>>>>>              u32 thr_low, thr_high;
>>>>>>>              u32 mflag_low, mflag_high;
>>>>>>> @@ -2226,7 +2302,7 @@ static void dispc_k2g_plane_init(struct
>>>>>>> dispc_device *dispc)
>>>>>>>                dev_dbg(dispc->dev,
>>>>>>>                  "%s: bufsize %u, buf_threshold %u/%u, mflag threshold
>>>>>>> %u/%u preload %u\n",
>>>>>>> -            dispc->feat->vid_name[hw_plane],
>>>>>>> +            dispc->feat->vid_info[hw_plane].name,
>>>>>>
>>>>>> Here hw_plane is not actually the hw-id (anymore), but elsewhere in
>>>>>> this
>>>>>> function it is used as a hw-id, which is no longer correct.
>>>>>
>>>>> For accessing vid_info hw_plane needs to be used which is the index of
>>>>> actually instantiated planes and I see it as correctly being passed for
>>>>> AM62L too. hw_id is only for dispc_k3_vid* functions where we need to
>>>>> skip the not-instantiated vid regions by adding the offset per the
>>>>> hw_id
>>>>> index.
>>>>
>>>> Hmm, sorry, I don't follow. If we use the same variable, hw_plane, to
>>>> access the vid_info[], and as a parameter to functions that take
>>>> hw_plane, e.g., dispc_vid_set_buf_threshold(), isn't one of those uses
>>>> wrong?
>>>>
>>>> Oh, wait... I think I see it now. For some functions using the hw_id as
>>>> the hw_plane parameter is fine, as they access the VID's registers by
>>>> just using, e.g. dispc_vid_write(), which gets the address correctly
>>>> from dispc->base_vid[hw_plane], as that one is indexed from 0 to
>>>> num_vids.
>>>>
>>>
>>> Yes exactly.
>>>
>>>> But some functions use registers that have bits based on the hw_id (like
>>>> dispc_k3_vid_write_irqstatus), and then we use the hw_id for the
>>>> hw_plane parameter. If that function were to also write a vid register,
>>>> using the passed hw_plane, it wouldn't work, but I guess we don't do
>>>> that.
>>>>
>>>
>>> Yes, hw_id is only for dispc_k3_vid* functions dealing with common
>>> region registers that play with VID pipes.
>>>
>>>> It feels broken... We can't have 'hw_plane' that's sometimes the HW id
>>>> (i.e. 1 for AM62L), and sometimes the driver's index (i.e. 0 for AM62L).
>>>>
>>>
>>> Sorry I don't follow, what exactly is broken here. hw_plane is for
>>> instantiated planes present in SoC used in context of VID* register
>>> space while doing reg writess and hw_id is the plane hardware index
>>> w.r.t larger K3 family i.e used in context for common register space.
>>
>> We have, as an example, these two functions:
>>
>> void dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool
>> enable)
>>
>> static void dispc_k3_vid_write_irqstatus(struct dispc_device *dispc, u32
>> hw_plane, dispc_irq_t vidstat)
>>
>> When the caller calls these functions on AM62L, what does it provide in
>> 'hw_plane' when it wants to enable the first plane of write the
>> irqstatus for the first plane?
> 
> It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
> accessing am65x common region register space which has vid on idx0 which
> we want to skip for am62l.
> 
> For dispc_plane_enable(), the caller uses
>> 0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...
> 
> Yes above is correct, and I think that's how it is supposed to be.

No it's not. Both functions have "hw_plane" parameter, yet they require 
a different value to be used even when referring to the same plane.

>> With a quick look at the code, changing the callers to pass the "old
>> style" hw_plane as the parameter to those irq functions, and the
>> functions internally get the hw_id, would solve most of the problems.
> 
> I don't follow above, hw_plane has 0 so it should not be used for
> programming irq related functions which expect idx 1 as explained above.

We have various functions in tidss_dispc.c that have hw_plane as a 
parameter. But the caller is supposed to know that for some functions 
hw_plane is a plane index from 0, and for some it's the hw_id from 
vid_info[].

>> There's still dispc_k3_set_irqenable() which manages 'main_disable' and
>> needs the hw_id, but maybe that's fine, even if a bit confusing.
>>
> 
> I still feel there is no inherent bug here, but let me know if you want
> me to put some debug prints or get register dump so that we can double
> confirm.

I'm not saying there's a bug. I'm saying it's bad code and will cause 
confusion and bugs in the future.

  Tomi

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

Hi,

<snip>
>> It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
>> accessing am65x common region register space which has vid on idx0 which
>> we want to skip for am62l.
>>
>> For dispc_plane_enable(), the caller uses
>>> 0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...
>>
>> Yes above is correct, and I think that's how it is supposed to be.
> 
> No it's not. Both functions have "hw_plane" parameter, yet they require
> a different value to be used even when referring to the same plane.
>
>>> With a quick look at the code, changing the callers to pass the "old
>>> style" hw_plane as the parameter to those irq functions, and the
>>> functions internally get the hw_id, would solve most of the problems.
>>
>> I don't follow above, hw_plane has 0 so it should not be used for
>> programming irq related functions which expect idx 1 as explained above.
> 
> We have various functions in tidss_dispc.c that have hw_plane as a
> parameter. But the caller is supposed to know that for some functions
> hw_plane is a plane index from 0, and for some it's the hw_id from
> vid_info[].
> 
>> There's still dispc_k3_set_irqenable() which manages 'main_disable' and
>>> needs the hw_id, but maybe that's fine, even if a bit confusing.
>>>
>>
>> I still feel there is no inherent bug here, but let me know if you want
>> me to put some debug prints or get register dump so that we can double
>> confirm.
> 
> I'm not saying there's a bug. I'm saying it's bad code and will cause
> confusion and bugs in the future.
> 

Ok I see what you mean to say.....although functionally it is working
fine but from readability point of view it is confusing since both
functions use same argument name i.e hw_plane in two different contexts.
In that case, I would propose to use hw_id as arg name for all
dispc_k3_vid* functions, will that be okay ?

Regards
Devarsh

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

On 02/05/2025 14:52, Devarsh Thakkar wrote:
> Hi,
> 
> <snip>
>>> It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
>>> accessing am65x common region register space which has vid on idx0 which
>>> we want to skip for am62l.
>>>
>>> For dispc_plane_enable(), the caller uses
>>>> 0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...
>>>
>>> Yes above is correct, and I think that's how it is supposed to be.
>>
>> No it's not. Both functions have "hw_plane" parameter, yet they require
>> a different value to be used even when referring to the same plane.
>>
>>>> With a quick look at the code, changing the callers to pass the "old
>>>> style" hw_plane as the parameter to those irq functions, and the
>>>> functions internally get the hw_id, would solve most of the problems.
>>>
>>> I don't follow above, hw_plane has 0 so it should not be used for
>>> programming irq related functions which expect idx 1 as explained above.
>>
>> We have various functions in tidss_dispc.c that have hw_plane as a
>> parameter. But the caller is supposed to know that for some functions
>> hw_plane is a plane index from 0, and for some it's the hw_id from
>> vid_info[].
>>
>>> There's still dispc_k3_set_irqenable() which manages 'main_disable' and
>>>> needs the hw_id, but maybe that's fine, even if a bit confusing.
>>>>
>>>
>>> I still feel there is no inherent bug here, but let me know if you want
>>> me to put some debug prints or get register dump so that we can double
>>> confirm.
>>
>> I'm not saying there's a bug. I'm saying it's bad code and will cause
>> confusion and bugs in the future.
>>
> 
> Ok I see what you mean to say.....although functionally it is working
> fine but from readability point of view it is confusing since both
> functions use same argument name i.e hw_plane in two different contexts.
> In that case, I would propose to use hw_id as arg name for all
> dispc_k3_vid* functions, will that be okay ?

I'd prefer to have all the dispc functions take the same kind of index.

  Tomi

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Devarsh Thakkar]

Hi Tomi

On 03/05/25 14:14, Tomi Valkeinen wrote:
> On 02/05/2025 14:52, Devarsh Thakkar wrote:

>> Hi,
>>
>>
<snip>
>> Ok I see what you mean to say.....although functionally it is working
>> fine but from readability point of view it is confusing since both
>> functions use same argument name i.e hw_plane in two different contexts.
>> In that case, I would propose to use hw_id as arg name for all
>> dispc_k3_vid* functions, will that be okay ?
> 
> I'd prefer to have all the dispc functions take the same kind of index.
> 

Why? Even all dispc functions are not named with same prefix.
1) dispc_vid* functions act on VID* base directly and here plane
indexing would be w.r.t which VID* base we are using e.g VID vs VIDL
2) dispc_k3_vid* functions act on common region bits which are related
to VID pipelines and plane indexing would signify vid base w.r.t common
register space i.e. COMMON_VID_IRQ0 vs COMMON_VID_IRQ1.

As they both act on different register base and refer it in different
contexts (VID* base vs COMMON_VID* base)  and have also been named
differently anyway, I feel it is okay and legitimate to use hw_id for
dispc_k3_vid* functions (which would signify vid* indexing w.r.t common
region) and hw_plane for dispc_vid* functions (which would signify vid*
base w.r.t VID* regions mapped in device-tree).

Regards
Devarsh

>  Tomi
> 

Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

[Tomi Valkeinen]

Hi,

On 05/05/2025 10:49, Devarsh Thakkar wrote:
> Hi Tomi
> 
> On 03/05/25 14:14, Tomi Valkeinen wrote:
>> On 02/05/2025 14:52, Devarsh Thakkar wrote:
> 
>>> Hi,
>>>
>>>
> <snip>
>>> Ok I see what you mean to say.....although functionally it is working
>>> fine but from readability point of view it is confusing since both
>>> functions use same argument name i.e hw_plane in two different contexts.
>>> In that case, I would propose to use hw_id as arg name for all
>>> dispc_k3_vid* functions, will that be okay ?
>>
>> I'd prefer to have all the dispc functions take the same kind of index.
>>
> 
> Why? Even all dispc functions are not named with same prefix.
> 1) dispc_vid* functions act on VID* base directly and here plane
> indexing would be w.r.t which VID* base we are using e.g VID vs VIDL
> 2) dispc_k3_vid* functions act on common region bits which are related
> to VID pipelines and plane indexing would signify vid base w.r.t common
> register space i.e. COMMON_VID_IRQ0 vs COMMON_VID_IRQ1.
> 
> As they both act on different register base and refer it in different
> contexts (VID* base vs COMMON_VID* base)  and have also been named
> differently anyway, I feel it is okay and legitimate to use hw_id for
> dispc_k3_vid* functions (which would signify vid* indexing w.r.t common
> region) and hw_plane for dispc_vid* functions (which would signify vid*
> base w.r.t VID* regions mapped in device-tree).

I'm sorry, I don't understand your argument. Say, if there's code that 
first enables a plane and then wants to read the irqstatus for that 
plane, your argument is that it's better that the plane indices used 
when calling those functions are not the same? Because the called 
functions internally access the data in different ways?

Why does it matter if inside the functions the accessed bits are in 
different register blocks? It's about the same plane. Doesn't it make 
more sense to refer to the plane using the same index number?

  Tomi