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