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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The workaround for erratum i2097 incorrectly moves disabled layers to (-1, -1) on-screen instead of a non-visible off-screen area, causing a visual glitch.
--

commit 80e2da1fc464e6c124d0362e7f424161a94f05ab
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 # i2097

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 58d5eb033bdbb..02a3cf50e0ceb 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,

[ ... ]

> @@ -1503,6 +1565,53 @@ 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;
> +		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);

[Severity: Medium]
Will this move the disabled layer to position (-1, -1) rather than a
non-visible off-screen area?

If the hardware interprets the POSX and POSY register bitfields as signed
integers (two's complement), setting the coordinates using OVR_LAYER_MAX_POS
which evaluates to FIELD_MAX(mask) (an all-ones bit pattern) will result
in -1.

This could cause a visual glitch where the disabled layer flashes at the
top-left corner of the screen instead of being hidden.

Does OVR_LAYER_MAX_POS in dispc_ovr_enable_layer() need to avoid setting the
sign bit to properly move the layer off-screen during normal KMS operations?

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

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