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 <[email protected]> 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/[email protected]?part=1
