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