Re: [PATCH v4 7/7] drm/tegra: Add support for 8-bit CPU interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/tegra: duplicate panel enable and broken disable in tegra_dbi
- [High] drm/tegra: dbi child devices destroyed on -EPROBE_DEFER in probe()
- [High] drm/tegra: shadowed registers updated with WRITE_MUX set in tegra_dbi_atomic_enable()
- [Medium] drm/tegra: shared mutable com_pin_output array races across DBI instances
- [Medium] drm/tegra: dbi: missing drm_panel_put() in tegra_dbi_probe()
--

commit 7920d1e9b99c981c6e715c2c232ac6f4896a683b
Author: Svyatoslav Ryhel <clamor95@gmail.com>

drm/tegra: Add support for 8-bit CPU interface

This commit adds a DRM bridge driver for the 8-bit CPU (MIPI DBI Type B)
display controller interface on Tegra20/30 SoCs and registers the child
tegra-dbi platform device from tegra_dc_probe().

> diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c
> new file mode 100644
> index 0000000000000..7bd90d32053f2
> --- /dev/null
> +++ b/drivers/gpu/drm/tegra/dbi.c
> @@ -0,0 +1,536 @@

[ ... ]

> +struct reg_entry {
> +	u32 offset;
> +	u32 value;
> +};
> +
> +/*
> + * This is used to backup and restore COM pin state during
> + * tegra_dbi_atomic_enable/disable.
> + */
> +static struct reg_entry com_pin_output[] = {
> +	{ DC_COM_PIN_OUTPUT_ENABLE(0),   0 },
> +	{ DC_COM_PIN_OUTPUT_ENABLE(1),   0 },
> +	{ DC_COM_PIN_OUTPUT_ENABLE(2),   0 },
> +	{ DC_COM_PIN_OUTPUT_ENABLE(3),   0 },
> +	{ DC_COM_PIN_OUTPUT_POLARITY(0), 0 },
> +	{ DC_COM_PIN_OUTPUT_POLARITY(1), 0 },
> +	{ DC_COM_PIN_OUTPUT_POLARITY(2), 0 },
> +	{ DC_COM_PIN_OUTPUT_POLARITY(3), 0 },
> +	{ DC_COM_PIN_OUTPUT_DATA(0),     0 },
> +	{ DC_COM_PIN_OUTPUT_DATA(1),     0 },
> +	{ DC_COM_PIN_OUTPUT_DATA(2),     0 },
> +	{ DC_COM_PIN_OUTPUT_DATA(3),     0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(0),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(1),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(2),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(3),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(4),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(5),   0 },
> +	{ DC_COM_PIN_OUTPUT_SELECT(6),   0 },
> +};

[Severity: Medium]
Can concurrent modesets on two display controllers race on com_pin_output[]?

In drivers/gpu/drm/tegra/dbi.c, com_pin_output[] is a single mutable
file-scope array shared across all tegra-dbi instances without locking.

Both tegra_dbi_atomic_enable() and tegra_dbi_atomic_disable() save per-DC
DC_COM_PIN_OUTPUT_* register values into com_pin_output[i].value, sleep
inside drm_panel_enable() or drm_panel_disable() while bitbanging panel
commands, and then write com_pin_output[i].value back to the DC registers.

If non-blocking atomic commits run concurrently on two CRTCs with DBI
interfaces, won't one instance overwrite com_pin_output[] while the other
is sleeping and restore corrupted register state? Should the saved values
live in struct tegra_dbi_priv or on the stack instead?

[ ... ]

> +static int tegra_dbi_dbi_command(struct mipi_dbi *dbi, u8 *cmd, u8 *param, size_t num)
> +{
> +	struct tegra_dbi_priv *priv = mipi_dbi_to_tegra_dbi(dbi);
> +	u8 command = *cmd;
> +
> +	/* Blocks all commands when interface is not active */
> +	if (!priv->active)
> +		return -ENODEV;
> +
> +	tegra_dbi_write(priv, TEGRA_DBI_COMMAND, command);
> +
> +	for (int i = 0; i < num; i++)
> +		tegra_dbi_write(priv, TEGRA_DBI_DATA, param[i]);
> +
> +	return 0;
> +}

[ ... ]

> +static void tegra_dbi_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_atomic_commit *state)
> +{
> +	struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> +	struct tegra_dc *dc = priv->dc;
> +	u32 value;
> +
> +	value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
> +	value &= ~DISP_CTRL_MODE_MASK;
> +	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
> +
> +	/* Disable all options except cursor if enabled */
> +	value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> +	value &= CURSOR_ENABLE;
> +	tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
> +
> +	tegra_dc_commit(dc);

[ ... ]

> +	/*
> +	 * Backup registers before unshadowing since actual register values
> +	 * may differ and their state backed up now is the correct one.
> +	 */
> +	for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> +		com_pin_output[i].value = tegra_dc_readl(dc, com_pin_output[i].offset);
> +
> +	/* Unshadow to set up command and data pins */
> +	tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS);
> +
> +	if (priv->idx)
> +		tegra_dbi_init_secondary_interface(priv);
> +	else
> +		tegra_dbi_init_primary_interface(priv);
> +
> +	priv->active = true;
> +
> +	if (priv->output.panel)
> +		drm_panel_enable(priv->output.panel);
> +
> +	tegra_dbi_pin_set(dc, &priv->index->cs, 0);
> +
> +	priv->active = false;
> +
> +	for (int i = 0; i <  ARRAY_SIZE(com_pin_output); i++)
> +		tegra_dc_writel(dc, com_pin_output[i].value, com_pin_output[i].offset);
> +
> +	/* Set LDC pin to V Pulse 1 */
> +	value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> +	value |= LDC_OUTPUT_SELECT_V_PULSE1;
> +	tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(6));
> +
> +	value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
> +	value &= ~DISP_CTRL_MODE_MASK;
> +	value |= DISP_CTRL_MODE_C_DISPLAY;
> +	tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
> +
> +	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> +}

[Severity: High]
Won't updating DC_COM_PIN_OUTPUT_SELECT(6) and DC_CMD_DISPLAY_COMMAND while
WRITE_MUX is set leave the assembly registers with stale values that overwrite
the active configuration on the next plane flush?

At the start of tegra_dbi_atomic_enable() in drivers/gpu/drm/tegra/dbi.c,
while DC_CMD_STATE_ACCESS is 0 (targeting the assembly registers), the code
clears DISP_CTRL_MODE_MASK (setting DISP_CTRL_MODE_STOP) in
DC_CMD_DISPLAY_COMMAND and calls tegra_dc_commit(dc). It then writes the
V_PULSE1, init sequence, shift clock, and color control settings to the
assembly registers without committing them.

Next, it sets WRITE_MUX in DC_CMD_STATE_ACCESS so writes go directly to the
active registers during panel bitbanging. However, after restoring
com_pin_output[], it sets LDC_OUTPUT_SELECT_V_PULSE1 in
DC_COM_PIN_OUTPUT_SELECT(6) and DISP_CTRL_MODE_C_DISPLAY in
DC_CMD_DISPLAY_COMMAND before clearing DC_CMD_STATE_ACCESS back to 0, and
does not call tegra_dc_commit(dc).

When tegra_crtc_atomic_flush() runs immediately afterward during
drm_atomic_helper_commit_planes() and writes GENERAL_UPDATE and
GENERAL_ACT_REQ to DC_CMD_STATE_CONTROL, won't the stale assembly registers
(where DC_COM_PIN_OUTPUT_SELECT(6) lacks LDC_OUTPUT_SELECT_V_PULSE1 and
DC_CMD_DISPLAY_COMMAND still holds DISP_CTRL_MODE_STOP) be latched into the
active registers and stop the display?

> +
> +static void tegra_dbi_atomic_disable(struct drm_bridge *bridge,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> +	struct tegra_dc *dc = priv->dc;
> +
> +	for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> +		com_pin_output[i].value = tegra_dc_readl(dc, com_pin_output[i].offset);
> +
> +	tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS);
> +
> +	if (priv->idx)
> +		tegra_dbi_init_secondary_interface(priv);
> +	else
> +		tegra_dbi_init_primary_interface(priv);
> +
> +	priv->active = true;
> +
> +	if (priv->output.panel)
> +		drm_panel_disable(priv->output.panel);
> +
> +	priv->active = false;
> +
> +	for (int i = 0; i <  ARRAY_SIZE(com_pin_output); i++)
> +		tegra_dc_writel(dc, com_pin_output[i].value, com_pin_output[i].offset);
> +
> +	tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> +}

[Severity: High]
How does calling drm_panel_enable() and drm_panel_disable() directly here
interact with the panel_bridge attached in tegra_dbi_attach()?

In tegra_dbi_probe(), the panel is wrapped in a panel_bridge via
devm_drm_panel_bridge_add_typed() and stored in priv->output.bridge, while
the raw panel pointer is also saved in priv->output.panel. tegra_dbi_attach()
then attaches priv->output.bridge to encoder->bridge_chain after tegra_dbi.

During enable, drm_atomic_bridge_chain_enable() runs tegra_dbi_atomic_enable()
first, which sets priv->active = true, calls drm_panel_enable(priv->output.panel)
(setting panel->enabled = true), and resets priv->active = false. Immediately
afterward, panel_bridge_atomic_enable() runs for the attached panel_bridge
and calls drm_panel_enable() a second time, hitting:

    if (panel->enabled) {
        dev_warn(panel->dev, "Skipping enable of already enabled panel\n");
        return;
    }

During disable, drm_atomic_bridge_chain_disable() walks the bridge chain in
reverse order, so panel_bridge_atomic_disable() runs before
tegra_dbi_atomic_disable(). At that point priv->active is still false and the
DC COM pins are not configured for DBI bitbanging, so every MIPI DBI command
sent by panel->funcs->disable() fails in tegra_dbi_dbi_command():

    if (!priv->active)
        return -ENODEV;

drm_panel_disable() then sets panel->enabled = false. When
tegra_dbi_atomic_disable() subsequently runs with priv->active = true and
calls drm_panel_disable(priv->output.panel), it hits:

    if (!panel->enabled) {
        dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
        return;
    }

and returns without calling panel->funcs->disable(). Similarly, any DBI
commands sent from panel .prepare() or .unprepare() callbacks will also see
priv->active == false and fail with -ENODEV.

[ ... ]

> +static int tegra_dbi_attach(struct drm_bridge *bridge,
> +			    struct drm_encoder *encoder,
> +			    enum drm_bridge_attach_flags flags)
> +{
> +	struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> +	struct device *dev = priv->dev;
> +
> +	priv->dc = dev_get_drvdata(dev->parent);
> +	if (!priv->dc)
> +		return -ENODEV;
> +
> +	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> +				 flags);
> +}

[ ... ]

> +static int tegra_dbi_probe(struct platform_device *pdev)
> +{
> +	struct tegra_dbi_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	struct device_node *ep;
> +	int ret;
> +
> +	priv = devm_drm_bridge_alloc(dev, struct tegra_dbi_priv, bridge,
> +				     &tegra_dbi_funcs);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
> +
> +	priv->dev = dev;
> +
> +	/* Default to primaty */
> +	device_property_read_u32(dev, "nvidia,index", &priv->idx);
> +	if (priv->idx)
> +		priv->index = &tegra_dbi_idx[1];
> +	else
> +		priv->index = &tegra_dbi_idx[0];
> +
> +	/* Initialize MIPI DBI interface */
> +	mutex_init(&priv->dbi.cmdlock);
> +	priv->dbi.command = tegra_dbi_dbi_command;
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to probe children\n");
> +
> +	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, &bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> +							 DRM_MODE_CONNECTOR_DPI);
> +		if (IS_ERR(bridge))
> +			return PTR_ERR(bridge);
> +	}

[Severity: Medium]
Does tegra_dbi_probe() leak the struct drm_panel reference acquired by
drm_of_find_panel_or_bridge()?

When drm_of_find_panel_or_bridge() finds a panel via of_drm_find_panel(), it
increments the panel reference count with drm_panel_get(panel).
devm_drm_panel_bridge_add_typed() then takes a second reference via
drm_panel_bridge_add_typed() -> drm_bridge_get(&panel->bridge) and only
releases that second reference on devres teardown.

Because tegra_dbi_probe() never calls drm_panel_put(panel) on error paths or
device unbind, isn't the reference from drm_of_find_panel_or_bridge() leaked?

> +
> +	priv->output.bridge = bridge;
> +	priv->output.panel = panel;
> +
> +	/* get input ep (port0/endpoint0) */
> +	ret = -EINVAL;
> +	ep = of_graph_get_endpoint_by_regs(np, 0, 0);
> +	if (ep) {
> +		ret = of_property_read_u32(ep, "bus-width", &priv->bus_width);
> +		of_node_put(ep);
> +	}
> +
> +	if (ret)
> +		priv->bus_width = 24;
> +
> +	priv->bridge.of_node = np;
> +	priv->bridge.timings = &default_tegra_dbi_timings;
> +
> +	ret = devm_drm_bridge_add(dev, &priv->bridge);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add bridge\n");
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 5c67928bcabfa..8f12d30cd2d8b 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c

[ ... ]

> @@ -3382,6 +3389,28 @@ static int tegra_dc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	dbi_np = of_get_available_child_by_name(pdev->dev.of_node, "dbi");
> +	if (dbi_np) {
> +		struct platform_device_info dbi_info = {
> +			.parent = &pdev->dev,
> +			.name = "tegra-dbi",
> +			.id = PLATFORM_DEVID_AUTO,
> +			.fwnode = of_fwnode_handle(dbi_np),
> +			.of_node_reused = true,
> +		};
> +
> +		dc->dbi = platform_device_register_full(&dbi_info);
> +		of_node_put(dbi_np);
> +		if (IS_ERR(dc->dbi))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(dc->dbi),
> +					     "Failed to create DBI device\n");
> +
> +		err = devm_add_action_or_reset(&pdev->dev, tegra_dc_release_dbi_pdev,
> +					       dc->dbi);
> +		if (err)
> +			return err;
> +	}
> +
>  	err = tegra_dc_rgb_probe(dc);
>  	if (err < 0 && err != -ENODEV)
>  		return dev_err_probe(&pdev->dev, err,

[Severity: High]
What happens here and in tegra_dbi_probe() when CONFIG_DRM_TEGRA_DBI=m, or
when the child panel driver is a module or defers probe?

In tegra_dc_probe(), dc->dbi is registered with a devm cleanup action
(tegra_dc_release_dbi_pdev) right before calling tegra_dc_rgb_probe(dc).
tegra_dc_rgb_probe() -> tegra_output_probe() calls
drm_of_find_panel_or_bridge(), which returns -EPROBE_DEFER unless
tegra_dbi_probe() has already bound and called devm_drm_bridge_add().

Similarly, in tegra_dbi_probe(), devm_of_platform_populate(dev) creates the
child panel platform device and immediately calls
drm_of_find_panel_or_bridge(np, 1, 0, &panel, &bridge), which returns
-EPROBE_DEFER unless the panel driver has already synchronously bound and
registered its panel.

When either drm_of_find_panel_or_bridge() call returns -EPROBE_DEFER,
devres_release_all() runs on probe failure and unregisters the child panel
device and dc->dbi. With those child platform devices destroyed while
tegra-dc is deferred, how can udev autoload their modules or a later driver
registration bind to them and re-trigger deferred probing?

Also, in tegra_dbi_probe(), devm_of_platform_populate(dev) is called before
platform_set_drvdata(pdev, priv). If a child panel driver probes
synchronously during devm_of_platform_populate(dev) and calls
dev_get_drvdata(dev->parent) to obtain struct mipi_dbi, won't it get NULL?

Should platform_set_drvdata(pdev, priv) be called before populating children?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022132259.240717-1-clamor95@gmail.com?part=7