[PATCH] drm/tidss: Fix synclost issue while disabling layer due to erratum i2097

[PATCH] drm/tidss: Fix synclost issue while disabling layer due to erratum i2097

[Yashas D]

A gap beyond a frame duration for disabling the layer in overlay manager
and go bit setting on the corresponding video port may lead to SYNC_LOST.
Fix by moving the layer to the non-visible area and deferring the ENABLE=0
write, immediately before the GO bit, guaranteeing both occur within the
same frame window.

Link: https://www.ti.com/lit/pdf/sprz530

Signed-off-by: Yashas D <y-d@ti.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 108 ++++++++++++++++++++++++++++
 drivers/gpu/drm/tidss/tidss_dispc.h |   1 +
 2 files changed, 109 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 58d5eb033bdb..6cbf9dfc38b8 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -38,6 +38,8 @@
 #include "tidss_dispc_regs.h"
 #include "tidss_scale_coefs.h"
 
+#define OVR_LAYER_MAX_POS(mask)		FIELD_MAX(mask)
+
 static const u16 tidss_k2g_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
 	[DSS_REVISION_OFF] =                    0x00,
 	[DSS_SYSCONFIG_OFF] =                   0x04,
@@ -467,6 +469,8 @@ struct dispc_device {
 
 	struct clk *fclk;
 
+	unsigned long fclk_rate;
+
 	bool is_enabled;
 
 	struct dss_vp_data vp_data[TIDSS_MAX_PORTS];
@@ -477,6 +481,9 @@ struct dispc_device {
 	u32 memory_bandwidth_limit;
 
 	struct dispc_errata errata;
+
+	// WA for erratum i2097: OVR Layer Disable May Cause Sync Lost.
+	u32 pending_disable_layers[TIDSS_MAX_PORTS];
 };
 
 static void dispc_write(struct dispc_device *dispc, u16 reg, u32 val)
@@ -1128,6 +1135,12 @@ static void dispc_enable_am65x_oldi(struct dispc_device *dispc, u32 hw_videoport
 void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
 		      const struct drm_crtc_state *state)
 {
+	/*
+	 * WA for erratum i2097: clear any stale layer disable tracking
+	 * state left over from the previous VP enable/disable cycle.
+	 */
+	dispc->pending_disable_layers[hw_videoport] = 0;
+
 	const struct tidss_crtc_state *tstate = to_tidss_crtc_state(state);
 	const struct dispc_bus_format *fmt;
 	const struct drm_display_mode *mode = &state->adjusted_mode;
@@ -1233,6 +1246,38 @@ void dispc_vp_go(struct dispc_device *dispc, u32 hw_videoport)
 {
 	WARN_ON(VP_REG_GET(dispc, hw_videoport, DISPC_VP_CONTROL,
 			   DISPC_VP_CONTROL_GOBIT_MASK));
+
+	if (dispc->errata.i2097 &&
+	    dispc->pending_disable_layers[hw_videoport]) {
+		u32 layer;
+		u32 delay_ns;
+
+		/* WA for erratum i2097: set GO bit #1 to latch position
+		 * changes into the DSS pipeline, wait 10 DSS functional clock
+		 * cycles, then write ENABLE=0.
+		 */
+		VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
+			       DISPC_VP_CONTROL_GOBIT_MASK);
+
+		if (dispc->fclk_rate)
+			delay_ns = DIV_ROUND_UP_ULL((u64)10 * NSEC_PER_SEC,
+						    dispc->fclk_rate);
+		else
+			delay_ns = 500;
+
+		ndelay(delay_ns);
+
+		for (layer = 0; layer < dispc->feat->num_vids; layer++) {
+			if (dispc->pending_disable_layers[hw_videoport] &
+			    BIT(layer))
+				OVR_REG_FLD_MOD(dispc, hw_videoport,
+						DISPC_OVR_ATTRIBUTES(layer),
+						0,
+						DISPC_OVR_ATTRIBUTES_ENABLE_MASK);
+		}
+		dispc->pending_disable_layers[hw_videoport] = 0;
+	}
+
 	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
 		       DISPC_VP_CONTROL_GOBIT_MASK);
 }
@@ -1503,6 +1548,63 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
 	if (dispc->feat->subrev == DISPC_K2G)
 		return;
 
+	if (dispc->errata.i2097 && !enable) {
+		/*
+		 * WA for erratum i2097:
+		 *
+		 * Do not write ENABLE=0 directly. Instead move the layer to
+		 * the non-visible area so it contributes no pixels.
+		 *
+		 * Position register layout differs per SoC:
+		 *   J721E : DISPC_OVR_ATTRIBUTES2, X[13:0],  Y[29:16] (14-bit)
+		 *   Others: DISPC_OVR_ATTRIBUTES,  X[17:6],  Y[30:19] (12-bit)
+		 */
+		switch (dispc->feat->subrev) {
+		case DISPC_J721E:
+			OVR_REG_FLD_MOD(dispc, hw_videoport,
+					DISPC_OVR_ATTRIBUTES2(layer),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES2_POSX_MASK),
+					DISPC_OVR_ATTRIBUTES2_POSX_MASK);
+			OVR_REG_FLD_MOD(dispc, hw_videoport,
+					DISPC_OVR_ATTRIBUTES2(layer),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES2_POSY_MASK),
+					DISPC_OVR_ATTRIBUTES2_POSY_MASK);
+			break;
+		case DISPC_AM62L:
+			OVR_REG_FLD_MOD(dispc, 0,
+					DISPC_OVR_ATTRIBUTES(0),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSX_MASK),
+					DISPC_OVR_ATTRIBUTES_POSX_MASK);
+			OVR_REG_FLD_MOD(dispc, 0,
+					DISPC_OVR_ATTRIBUTES(0),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSY_MASK),
+					DISPC_OVR_ATTRIBUTES_POSY_MASK);
+			break;
+		default:
+			OVR_REG_FLD_MOD(dispc, hw_videoport,
+					DISPC_OVR_ATTRIBUTES(layer),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSX_MASK),
+					DISPC_OVR_ATTRIBUTES_POSX_MASK);
+			OVR_REG_FLD_MOD(dispc, hw_videoport,
+					DISPC_OVR_ATTRIBUTES(layer),
+					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSY_MASK),
+					DISPC_OVR_ATTRIBUTES_POSY_MASK);
+			break;
+		}
+
+		dispc->pending_disable_layers[hw_videoport] |= BIT(layer);
+		return;
+	}
+
+	if (dispc->errata.i2097 && enable) {
+		/*
+		 * Layer being re-enabled: cancel any pending disable so
+		 * dispc_vp_go() does not write ENABLE=0 after we have
+		 * just written ENABLE=1 here.
+		 */
+		dispc->pending_disable_layers[hw_videoport] &= ~BIT(layer);
+	}
+
 	OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer),
 			!!enable, DISPC_OVR_ATTRIBUTES_ENABLE_MASK);
 }
@@ -2871,6 +2973,12 @@ static void dispc_init_errata(struct dispc_device *dispc)
 		dispc->errata.i2000 = true;
 		dev_info(dispc->dev, "WA for erratum i2000: YUV formats disabled\n");
 	}
+
+	if (dispc->feat->subrev != DISPC_K2G) {
+		dispc->errata.i2097 = true;
+		dev_info(dispc->dev,
+			 "WA for erratum i2097: OVR layer disable uses non-visible area\n");
+	}
 }
 
 /*
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 739d211d0018..e2ce9fdf1279 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -57,6 +57,7 @@ struct dispc_vid_info {
 
 struct dispc_errata {
 	bool i2000; /* DSS Does Not Support YUV Pixel Data Formats */
+	bool i2097; /* OVR Layer Disable May Cause Sync Lost */
 };
 
 enum dispc_vp_bus_type {
-- 
2.34.1

Re: [PATCH] drm/tidss: Fix synclost issue while disabling layer due to erratum i2097

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The newly added `fclk_rate` field is never populated, causing the hardware delay computation to unconditionally fall back to a hardcoded default.
- [High] Deferring layer disable writes to `dispc_vp_go()` breaks CRTC teardown and modesets, leaving layers persistently enabled in hardware.
- [Low] The AM62L specific workaround in `dispc_ovr_enable_layer()` incorrectly hardcodes both the videoport and layer indices to `0`.
--

commit 6082951d3123b46199528652dff5e8564f311738
Author: Yashas D <y-d@ti.com>
drm/tidss: Fix synclost issue while disabling layer due to erratum i2097

A gap beyond a frame duration for disabling the layer in overlay manager
and go bit setting on the corresponding video port may lead to SYNC_LOST.
Fix by moving the layer to the non-visible area and deferring the ENABLE=0
write, immediately before the GO bit, guaranteeing both occur within the
same frame window.

Link: https://www.ti.com/lit/pdf/sprz530

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 58d5eb033bdbb..6cbf9dfc38b87 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -467,6 +469,8 @@ struct dispc_device {
>  
>  	struct clk *fclk;
>  
> +	unsigned long fclk_rate;
> +

[ ... ]

> @@ -1233,6 +1246,38 @@ void dispc_vp_go(struct dispc_device *dispc, u32 hw_videoport)
>  {
>  	WARN_ON(VP_REG_GET(dispc, hw_videoport, DISPC_VP_CONTROL,
>  			   DISPC_VP_CONTROL_GOBIT_MASK));
> +
> +	if (dispc->errata.i2097 &&
> +	    dispc->pending_disable_layers[hw_videoport]) {
> +		u32 layer;
> +		u32 delay_ns;
> +
> +		/* WA for erratum i2097: set GO bit #1 to latch position
> +		 * changes into the DSS pipeline, wait 10 DSS functional clock
> +		 * cycles, then write ENABLE=0.
> +		 */
> +		VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1,
> +			       DISPC_VP_CONTROL_GOBIT_MASK);
> +
> +		if (dispc->fclk_rate)
> +			delay_ns = DIV_ROUND_UP_ULL((u64)10 * NSEC_PER_SEC,
> +						    dispc->fclk_rate);
> +		else
> +			delay_ns = 500;

[Severity: High]
Does fclk_rate ever get initialized?

The newly added fclk_rate field in struct dispc_device does not appear to be
populated anywhere in the driver.

This causes the hardware delay computation to unconditionally fall back to
the hardcoded 500ns default. If the DSS functional clock is slower than 20MHz,
500ns is less than the 10 clock cycles strictly required by the hardware erratum
workaround.

Will this cause the driver to write ENABLE=0 too early and potentially trigger
the SYNC_LOST bug it intends to fix?

[ ... ]

> @@ -1503,6 +1548,63 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
>  	if (dispc->feat->subrev == DISPC_K2G)
>  		return;
>  
> +	if (dispc->errata.i2097 && !enable) {
> +		/*
> +		 * WA for erratum i2097:
> +		 *
> +		 * Do not write ENABLE=0 directly. Instead move the layer to
> +		 * the non-visible area so it contributes no pixels.
> +		 *
> +		 * Position register layout differs per SoC:
> +		 *   J721E : DISPC_OVR_ATTRIBUTES2, X[13:0],  Y[29:16] (14-bit)
> +		 *   Others: DISPC_OVR_ATTRIBUTES,  X[17:6],  Y[30:19] (12-bit)
> +		 */
> +		switch (dispc->feat->subrev) {
> +		case DISPC_J721E:
> +			OVR_REG_FLD_MOD(dispc, hw_videoport,
> +					DISPC_OVR_ATTRIBUTES2(layer),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES2_POSX_MASK),
> +					DISPC_OVR_ATTRIBUTES2_POSX_MASK);
> +			OVR_REG_FLD_MOD(dispc, hw_videoport,
> +					DISPC_OVR_ATTRIBUTES2(layer),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES2_POSY_MASK),
> +					DISPC_OVR_ATTRIBUTES2_POSY_MASK);
> +			break;
> +		case DISPC_AM62L:
> +			OVR_REG_FLD_MOD(dispc, 0,
> +					DISPC_OVR_ATTRIBUTES(0),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSX_MASK),
> +					DISPC_OVR_ATTRIBUTES_POSX_MASK);
> +			OVR_REG_FLD_MOD(dispc, 0,
> +					DISPC_OVR_ATTRIBUTES(0),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSY_MASK),
> +					DISPC_OVR_ATTRIBUTES_POSY_MASK);
> +			break;

[Severity: Low]
Is it intentional to hardcode 0 for the videoport and layer indices here
instead of using the function arguments hw_videoport and layer?

While AM62L hardware currently only defines one videoport and one layer, this
hardcoding creates a disconnect where the tracking update below uses the
function arguments (dispc->pending_disable_layers[hw_videoport] |= BIT(layer))
while the hardware writes to VP 0.

Does this specific switch case need to exist, given that the default case
handles the exact same registers but correctly utilizes the function arguments?

> +		default:
> +			OVR_REG_FLD_MOD(dispc, hw_videoport,
> +					DISPC_OVR_ATTRIBUTES(layer),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSX_MASK),
> +					DISPC_OVR_ATTRIBUTES_POSX_MASK);
> +			OVR_REG_FLD_MOD(dispc, hw_videoport,
> +					DISPC_OVR_ATTRIBUTES(layer),
> +					OVR_LAYER_MAX_POS(DISPC_OVR_ATTRIBUTES_POSY_MASK),
> +					DISPC_OVR_ATTRIBUTES_POSY_MASK);
> +			break;
> +		}
> +
> +		dispc->pending_disable_layers[hw_videoport] |= BIT(layer);
> +		return;
> +	}

[Severity: High]
Does deferring this layer disable write break CRTC teardown?

During CRTC disable, tidss_crtc_atomic_disable() attempts to disable all
layers, which now sets pending_disable_layers and returns early. The deferred
write is supposedly handled in dispc_vp_go().

However, during a CRTC disable, tidss_crtc_atomic_flush() explicitly skips
calling dispc_vp_go():

drivers/gpu/drm/tidss/tidss_crtc.c:tidss_crtc_atomic_flush() {
    ...
    if (drm_atomic_crtc_needs_modeset(crtc->state))
        return;
    ...
}

Because of this, the ENABLE=0 write appears to be lost, leaving layers enabled
in the hardware registers when the videoport is disabled. 

Will this break the existing hardware workaround documented in
tidss_crtc_atomic_disable() that warns against leaving a layer enabled while
its videoport is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630103802.1853564-1-y-d@ti.com?part=1