Re: [PATCH v2 3/3] ARM: dts: imx6q-apalis: Add VGA overlay

[sashiko-bot@kernel.org]

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 <leonardo.costa@toradex.com>

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/20261019-v2-apalis-imx6-enable-vga-v2-0-ef6a3a780472@toradex.com?part=3