Re: [PATCH v2 2/3] drm/bridge: simple-bridge: Support data-enable-active endpoint property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: simple-bridge: missing atomic_check() to propagate DE bus flags
- [Medium] drm/bridge: simple-bridge: timings devm_kzalloc() lifetime mismatch
--

commit 17d03fa8dcd99e5bb3af7da622344c856ca2c887
Author: Leonardo Costa <leonardo.costa@toradex.com>

drm/bridge: simple-bridge: Support data-enable-active endpoint property

This commit reads the data-enable-active property from the input endpoint in
simple_bridge_probe() and allocates a drm_bridge_timings structure to set
DRM_BUS_FLAG_DE_HIGH or DRM_BUS_FLAG_DE_LOW in input_bus_flags when the bridge
does not already define its own timings.

> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index 3fba7002322e9..393228bed6e28 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -171,6 +171,9 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  {
>  	struct simple_bridge *sbridge;
>  	struct device_node *remote;
> +	struct device_node *ep;
> +	bool de_present;
> +	u32 de;
>  
>  	sbridge = devm_drm_bridge_alloc(&pdev->dev, struct simple_bridge,
>  					bridge, &simple_bridge_bridge_funcs);
> @@ -208,10 +211,26 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(sbridge->enable),
>  				     "Unable to retrieve enable GPIO\n");
>  
> -	/* Register the bridge. */
>  	sbridge->bridge.of_node = pdev->dev.of_node;
>  	sbridge->bridge.timings = sbridge->info->timings;
>  
> +	ep = of_graph_get_endpoint_by_regs(pdev->dev.of_node, 0, -1);
> +	de_present = !of_property_read_u32(ep, "data-enable-active", &de);
> +	of_node_put(ep);
> +
> +	if (de_present && !sbridge->bridge.timings) {
> +		struct drm_bridge_timings *timings;
> +
> +		timings = devm_kzalloc(&pdev->dev, sizeof(*timings), GFP_KERNEL);
> +		if (!timings)
> +			return -ENOMEM;

[Severity: Medium]
Can allocating timings with devm_kzalloc() in simple_bridge_probe() cause a
lifetime mismatch with sbridge->bridge?

sbridge is allocated via devm_drm_bridge_alloc(), which manages the container's
lifetime through sbridge->bridge.refcount and only frees it in
__drm_bridge_free() after all drm_bridge references are dropped:

drivers/gpu/drm/drm_bridge.c:__drm_bridge_free()
	if (bridge->funcs->destroy)
		bridge->funcs->destroy(bridge);

	drm_bridge_put(bridge->next_bridge);

	kfree(bridge->container);

If pdev is unbound while an upstream bridge or encoder still holds a reference
to sbridge->bridge, devres_release_all() will free timings immediately, leaving
sbridge->bridge.timings as a dangling pointer that can be dereferenced by
upstream drivers (for example, in imx8mp_hdmi_pvi_bridge_enable()).

Would embedding struct drm_bridge_timings inside struct simple_bridge (as done
in lvds-codec.c and ti-tfp410.c), or pointing to static const
drm_bridge_timings instances for DRM_BUS_FLAG_DE_HIGH and DRM_BUS_FLAG_DE_LOW,
avoid this lifetime mismatch?

> +
> +		timings->input_bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
> +						DRM_BUS_FLAG_DE_LOW;
> +		sbridge->bridge.timings = timings;
> +	}

[Severity: High]
Will setting only sbridge->bridge.timings->input_bus_flags actually propagate
the DE polarity to upstream atomic encoders and bridges?

simple_bridge_bridge_funcs registers atomic state callbacks without an
.atomic_check callback:

drivers/gpu/drm/bridge/simple-bridge.c:
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 an atomic modeset, drm_atomic_bridge_chain_check() calls
drm_atomic_bridge_propagate_bus_flags(), which overwrites
bridge_state->input_bus_cfg.flags with output_flags from the downstream bridge
or connector (0 for a VGA connector) and never checks
bridge->timings->input_bus_flags:

drivers/gpu/drm/drm_bridge.c:drm_atomic_bridge_propagate_bus_flags()
	bridge_state->output_bus_cfg.flags = output_flags;
	...
	bridge_state->input_bus_cfg.flags = output_flags;

Because simple-bridge has a valid drm_bridge_state, upstream bridges and atomic
encoders such as imx_pd_bridge_atomic_check() in
drivers/gpu/drm/imx/ipuv3/parallel-display.c read
next_bridge_state->input_bus_cfg.flags instead of
next_bridge->timings->input_bus_flags:

drivers/gpu/drm/imx/ipuv3/parallel-display.c:imx_pd_bridge_atomic_check()
	if (next_bridge_state)
		bus_flags = next_bridge_state->input_bus_cfg.flags;
	else
		bus_flags = di->bus_flags;

As a result, bus_flags remains 0 instead of DRM_BUS_FLAG_DE_LOW or
DRM_BUS_FLAG_DE_HIGH.

Should simple_bridge_bridge_funcs implement an .atomic_check callback that
copies sbridge->bridge.timings->input_bus_flags into
bridge_state->input_bus_cfg.flags (similar to tfp410_atomic_check() in
drivers/gpu/drm/bridge/ti-tfp410.c)?

> +
> +	/* Register the bridge. */
>  	return devm_drm_bridge_add(&pdev->dev, &sbridge->bridge);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-v2-apalis-imx6-enable-vga-v2-0-ef6a3a780472@toradex.com?part=2