Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/tegra: manual next_bridge calls in tegra_dbi_atomic_enable() break disable - [High] drm/tegra: double drm_bridge_remove() and leak in tegra_dbi_host_attach() - [High] drm/tegra: driver conflict and deferral deadlock in devm_tegra_dc_dbi_init() - [Medium] drm/tegra: misconfigured pin select and mux in tegra_dbi_atomic_enable() - [Low] drm/tegra: missing select DRM_MIPI_DBI causes link error in tegra_dbi_probe() --
commit 38b0aba6a8c5c45d84b4bce85bd75f2063d5eedc Author: Svyatoslav Ryhel <[email protected]> drm/tegra: Add support for 8-bit CPU interface This commit adds a MIPI DBI Type B host and DRM bridge driver to support the 8-bit CPU interface on Tegra20/30 display controllers. > diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile > index e399b40d64a1d..ae7fccb3d6421 100644 > --- a/drivers/gpu/drm/tegra/Makefile > +++ b/drivers/gpu/drm/tegra/Makefile > @@ -18,6 +18,7 @@ tegra-drm-y := \ > hdmi.o \ > mipi-phy.o \ > dsi.o \ > + dbi.o \ > sor.o \ > dpaux.o \ > gr2d.o \ [Severity: Low] Can building tegra-drm fail with an undefined reference to devm_mipi_dbi_device_register() when CONFIG_DRM_MIPI_DBI is disabled or built as a module while CONFIG_DRM_TEGRA is built-in? Here dbi.o is unconditionally added to tegra-drm-y whenever CONFIG_DRM_TEGRA is enabled, and tegra_dbi_probe() in drivers/gpu/drm/tegra/dbi.c calls devm_mipi_dbi_device_register(): dbidev = devm_mipi_dbi_device_register(dev, &priv->dbi_host, np); However, devm_mipi_dbi_device_register() is defined in drivers/gpu/drm/drm_mipi_dbi_bus.c, which is only compiled when CONFIG_DRM_MIPI_DBI is enabled: obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o drm_mipi_dbi_bus.o and include/drm/drm_mipi_dbi_bus.h does not provide fallback stubs. Should config DRM_TEGRA in drivers/gpu/drm/tegra/Kconfig select DRM_MIPI_DBI alongside DRM_MIPI_DSI and DRM_PANEL? > diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c > new file mode 100644 > index 0000000000000..e129be0f21da5 > --- /dev/null > +++ b/drivers/gpu/drm/tegra/dbi.c [ ... ] > +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 int tegra_dbi_host_attach(struct mipi_dbi_host *host, struct > mipi_dbi_device *dbidev) > +{ > + struct tegra_dbi_priv *priv = dbi_host_to_tegra_dbi(host); > + struct device *dev = priv->dev; > + struct drm_bridge *bridge; > + struct device_node *ep; > + int ret; > + > + bridge = of_drm_get_bridge_by_endpoint(dev_of_node(dev), 1, -1); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + > + bridge->type = DRM_MODE_CONNECTOR_DPI; > + priv->next_bridge = bridge; > + > + /* get input ep (port0/endpoint0) */ > + ret = -EINVAL; > + ep = of_graph_get_endpoint_by_regs(dev_of_node(dev), 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; > + > + return devm_drm_bridge_add(dev, &priv->bridge); > +} > + > +static void tegra_dbi_host_detach(struct mipi_dbi_host *host, struct > mipi_dbi_device *dbidev) > +{ > + struct tegra_dbi_priv *priv = dbi_host_to_tegra_dbi(host); > + > + drm_bridge_remove(&priv->bridge); > + drm_bridge_put(priv->next_bridge); > +} [Severity: High] Won't using devm_drm_bridge_add() in tegra_dbi_host_attach() alongside an explicit drm_bridge_remove() in tegra_dbi_host_detach() cause drm_bridge_remove() to run twice on priv->bridge during unbind, as well as leak priv->next_bridge if devm_drm_bridge_add() fails? In tegra_dbi_probe(), priv is allocated with devm_drm_bridge_alloc(), which initializes priv->bridge.refcount to 1 and registers a devres action on priv->dev to call drm_bridge_put_void(&priv->bridge). When tegra_dbi_host_attach() calls devm_drm_bridge_add(dev, &priv->bridge), it increments priv->bridge.refcount to 2 and registers a devres action on priv->dev to call drm_bridge_remove_void(&priv->bridge). When the DBI device or tegra-dbi unbinds, mipi_dbi_device_unregister() invokes: mipi_dbi_device_unregister() mipi_dbi_detach() tegra_dbi_host_detach() drm_bridge_remove(&priv->bridge) which destroys bridge->hpd_mutex and bridge->hpd_state_mutex and drops priv->bridge.refcount from 2 to 1. When priv->dev's devres then runs drm_bridge_remove_void(&priv->bridge), it calls drm_bridge_remove(&priv->bridge) a second time, destroying both mutexes again and dropping priv->bridge.refcount from 1 to 0, which frees priv via __drm_bridge_free(). Immediately afterward, the drm_bridge_put_void() devres action from devm_drm_bridge_alloc() runs on the already-freed priv->bridge. Also, if devm_drm_bridge_add() fails in tegra_dbi_host_attach(), the reference on priv->next_bridge acquired by of_drm_get_bridge_by_endpoint() is never released with drm_bridge_put(). Should tegra_dbi_host_attach() use drm_bridge_add() instead of devm_drm_bridge_add()? [ ... ] > +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 drm_bridge *next_bridge = priv->next_bridge; > + struct tegra_dc *dc = priv->dc; > + u32 value; [ ... ] > + /* Unshadow to set up command and data pins */ > + tegra_dc_writel(dc, WRITE_MUX | READ_MUX, DC_CMD_STATE_ACCESS); > + > + if (priv->reg) > + tegra_dbi_init_secondary_interface(priv); > + else > + tegra_dbi_init_primary_interface(priv); > + > + priv->active = true; > + > + /* > + * DBI requires that the next bridge be enabled here in order to > + * send DCS commands, since the interface pins will subsequently be > + * reconfigured for video data. > + */ > + if (next_bridge->funcs && next_bridge->funcs->atomic_enable) > + next_bridge->funcs->atomic_enable(next_bridge, state); > + > + tegra_dbi_pin_set(dc, &priv->index->cs, 0); > + > + priv->active = false; > + > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS); > + > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++) > + tegra_dc_writel(dc, priv->com_pin_backup[i].value, > + priv->com_pin_backup[i].offset); > + > + tegra_dc_commit(dc); > + > + /* Set LDC pin to V Pulse 1 */ > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6)); > + value |= PIN_OUTPUT_SELECT_F4(PIN_OUTPUT_SELECT_LDC); > + 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); > +} > + > +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 drm_bridge *next_bridge = priv->next_bridge; > + struct tegra_dc *dc = priv->dc; > + > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++) > + priv->com_pin_backup[i].value = tegra_dc_readl(dc, > + priv->com_pin_backup[i].offset); > + > + tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS); > + > + if (priv->reg) > + tegra_dbi_init_secondary_interface(priv); > + else > + tegra_dbi_init_primary_interface(priv); > + > + priv->active = true; > + > + /* > + * DBI requires that the next bridge be disabled here in order to > + * send DCS commands, since the interface pins will subsequently be > + * reconfigured for video data. > + */ > + if (next_bridge->funcs && next_bridge->funcs->atomic_disable) > + next_bridge->funcs->atomic_disable(next_bridge, state); > + > + priv->active = false; > + > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS); > + > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++) > + tegra_dc_writel(dc, priv->com_pin_backup[i].value, > + priv->com_pin_backup[i].offset); > + > + tegra_dc_commit(dc); > +} [Severity: High] Because tegra_dbi_attach() attaches priv->next_bridge to the encoder's bridge_chain via drm_bridge_attach(), won't the DRM bridge chain helpers already invoke next_bridge->funcs->atomic_enable() and atomic_disable(), causing panel DCS commands during disable to fail with -ENODEV? When disabling the display, drm_atomic_bridge_chain_disable() iterates encoder->bridge_chain in reverse order, so it calls priv->next_bridge->funcs->atomic_disable() (for example, panel_bridge_atomic_disable() -> drm_panel_disable()) before tegra_dbi_atomic_disable() runs. At that point priv->active is still false and the DC pins are still in video mode, so every MIPI DCS command issued by the panel's disable callback is rejected by tegra_dbi_dbi_command() with -ENODEV, while drm_panel_disable() still marks panel->enabled = false. When drm_atomic_bridge_chain_disable() subsequently calls tegra_dbi_atomic_disable(), it sets priv->active = true and manually calls next_bridge->funcs->atomic_disable(next_bridge, state) a second time, which sees panel->enabled == false, logs "Skipping disable of already disabled panel", and returns without sending the panel's DCS disable sequence. Similarly, during enable, drm_atomic_bridge_chain_enable() calls tegra_dbi_atomic_enable(), which manually calls next_bridge->funcs->atomic_enable(next_bridge, state), and then drm_atomic_bridge_chain_enable() advances to priv->next_bridge in encoder->bridge_chain and calls next_bridge->funcs->atomic_enable() a second time while priv->active is false. [Severity: Medium] In tegra_dbi_atomic_enable() and tegra_dbi_atomic_disable(), there also appear to be a couple of register configuration issues: First, at the end of tegra_dbi_atomic_enable(): /* Set LDC pin to V Pulse 1 */ value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6)); value |= PIN_OUTPUT_SELECT_F4(PIN_OUTPUT_SELECT_LDC); tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(6)); This unconditionally modifies PIN_LDC in DC_COM_PIN_OUTPUT_SELECT(6) without clearing PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LDC) first, and without checking priv->reg. When the secondary interface (priv->reg == 1) is used, tegra_dbi_idx[1] uses PIN_LVP0 in DC_COM_PIN_OUTPUT_SELECT(4) as the D/C pin rather than PIN_LDC. Does the secondary interface need V_PULSE1 routed to its own D/C pin instead of PIN_LDC? Second, tegra_dbi_atomic_disable() writes only WRITE_MUX to DC_CMD_STATE_ACCESS: tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS); whereas tegra_dbi_atomic_enable() writes WRITE_MUX | READ_MUX before calling tegra_dbi_init_primary_interface() or tegra_dbi_init_secondary_interface(). Without READ_MUX set, won't the tegra_dc_readl() calls in those init functions read from the shadowed assembly registers while tegra_dc_writel() writes to the active registers? [ ... ] > +static struct platform_driver tegra_dbi_driver = { > + .driver.name = "tegra-dbi", > + .probe = tegra_dbi_probe, > +}; > + > +static void tegra_dbi_release(void *pdev) > +{ > + platform_device_unregister(pdev); > + platform_driver_unregister(&tegra_dbi_driver); > +} > + > +int devm_tegra_dc_dbi_init(struct device *dev) > +{ > + struct device_node *dbi_np __free(device_node) = > + of_get_available_child_by_name(dev_of_node(dev), "dbi"); > + struct platform_device_info dbi_info = { 0 }; > + struct platform_device *dbi; > + int ret; > + > + /* Missing node means that DBI is not used, continue */ > + if (!dbi_np) > + return 0; > + > + ret = platform_driver_register(&tegra_dbi_driver); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to register DBI driver\n"); > + > + dbi_info.parent = dev; > + dbi_info.name = "tegra-dbi"; > + dbi_info.id = PLATFORM_DEVID_AUTO; > + dbi_info.fwnode = of_fwnode_handle(dbi_np); > + dbi_info.of_node_reused = true; > + > + dbi = platform_device_register_full(&dbi_info); > + if (IS_ERR(dbi)) { > + platform_driver_unregister(&tegra_dbi_driver); > + return dev_err_probe(dev, PTR_ERR(dbi), > + "Failed to create DBI device\n"); > + } > + > + ret = devm_add_action_or_reset(dev, tegra_dbi_release, dbi); > + if (ret) > + return ret; > + > + return 0; > +} > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index 5bbf2c028edd8..768cf82776632 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > @@ -3382,6 +3382,10 @@ static int tegra_dc_probe(struct platform_device *pdev) > } > } > > + err = devm_tegra_dc_dbi_init(&pdev->dev); > + if (err) > + return dev_err_probe(&pdev->dev, err, "failed to init DBI\n"); > + > err = tegra_dc_rgb_probe(dc); > if (err < 0 && err != -ENODEV) > return dev_err_probe(&pdev->dev, err, [Severity: High] Can devm_tegra_dc_dbi_init() deadlock on probe deferral if the child MIPI DBI panel driver is not yet bound when tegra_dc_rgb_probe() runs, and also fail with -EBUSY if both display controllers have an available "dbi" child node? In tegra_dc_probe(), devm_tegra_dc_dbi_init() is called immediately before tegra_dc_rgb_probe(dc). In tegra_dbi_probe(), devm_mipi_dbi_device_register() creates the child mipi_dbi_device, but priv->bridge is only registered later in tegra_dbi_host_attach() when the panel driver binds to that DBI device and calls devm_mipi_dbi_attach(). If the panel driver is built as a module, has not registered yet, or defers its own probe (for example waiting on a regulator, GPIO, or backlight), devm_tegra_dc_dbi_init() still returns 0, and then tegra_dc_rgb_probe(dc) -> tegra_output_probe() -> drm_of_find_panel_or_bridge() returns -EPROBE_DEFER because priv->bridge has not been added yet. When tegra_dc_probe() fails with -EPROBE_DEFER, devres on &pdev->dev runs tegra_dbi_release(), which unregisters the tegra-dbi platform device and destroys the child mipi_dbi_device. With the mipi_dbi_device removed, how can the panel driver ever bind to it and call tegra_dbi_host_attach() to resolve the deferral? Additionally, tegra_dbi_driver is a single static platform_driver registered inside devm_tegra_dc_dbi_init(). If both display controllers on Tegra20/30 have an available "dbi" child node, won't the second DC's call to platform_driver_register(&tegra_dbi_driver) fail with -EBUSY, and unbinding one DC unregister tegra_dbi_driver while the other DC is still using it? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
