Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ARM: dts: imx6q-apalis: VGA data-enable-active ignored by simple-bridge --
commit a66c8dd73182954ba43ed81cada90ae73edf93a0 Author: Leonardo Costa <[email protected]> ARM: dts: imx6q-apalis: Add VGA overlay This commit adds a device tree overlay for the VGA output on the Apalis Evaluation Board, connecting the parallel RGB display interface to a non-programmable video DAC and VGA connector with an active-low data-enable signal, and wires up composite DTB targets in the Makefile. > diff --git a/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso > b/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso > new file mode 100644 > index 0000000000000..991c2b7c27773 > --- /dev/null > +++ b/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso > @@ -0,0 +1,80 @@ [ ... ] > + vga-encoder { > + compatible = "dumb-vga-dac"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + > + vga_enc_in: endpoint { > + data-enable-active = <0>; > + remote-endpoint = <&vga_display_out>; > + }; > + }; [Severity: High] Does data-enable-active = <0> on vga_enc_in actually reach the IPUv3 CRTC? In simple_bridge_probe() in drivers/gpu/drm/bridge/simple-bridge.c, the data-enable-active property is parsed into timings->input_bus_flags: if (de_present && !sbridge->bridge.timings) { struct drm_bridge_timings *timings; timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL); if (!timings) return -ENOMEM; timings->input_bus_flags = de ? DRM_BUS_FLAG_DE_HIGH : DRM_BUS_FLAG_DE_LOW; sbridge->bridge.timings = timings; } However, simple_bridge_bridge_funcs registers atomic bridge state callbacks without an .atomic_check callback: static const struct drm_bridge_funcs simple_bridge_bridge_funcs = { .atomic_create_state = drm_atomic_helper_bridge_create_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .attach = simple_bridge_attach, .atomic_enable = simple_bridge_enable, .atomic_disable = simple_bridge_disable, }; During drm_atomic_bridge_chain_check() -> drm_atomic_bridge_propagate_bus_flags(), sbridge's bridge_state->input_bus_cfg.flags is overwritten with the downstream display-connector bridge's input_bus_cfg.flags (which is 0), and sbridge->bridge.timings->input_bus_flags is never copied into bridge_state->input_bus_cfg.flags. Then, upstream in imx_pd_bridge_atomic_check() in drivers/gpu/drm/imx/ipuv3/parallel-display.c: if (next_bridge) next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, next_bridge); if (next_bridge_state) bus_flags = next_bridge_state->input_bus_cfg.flags; else bus_flags = di->bus_flags; Because next_bridge_state is non-NULL, bus_flags is read as 0 from next_bridge_state->input_bus_cfg.flags without consulting next_bridge->timings->input_bus_flags. When ipu_crtc_mode_set_nofb() -> ipu_di_init_sync_panel() runs, sig_cfg.enable_pol = !(imx_crtc_state->bus_flags & DRM_BUS_FLAG_DE_LOW) evaluates to 1, driving the data-enable pin active-high instead of active-low and gating off the VGA DAC output during active video. Should simple-bridge implement .atomic_check to populate bridge_state->input_bus_cfg.flags from bridge->timings->input_bus_flags so the active-low polarity is propagated to imx-parallel-display? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
