Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] drm/bridge: it6162: select DRM_DISPLAY_HDMI_STATE_HELPER in Kconfig - [High] drm/bridge: it6162: fix bridge.ops flags and set supported_hdmi_ver - [High] drm/bridge: it6162: avoid HPD notify self-deadlock in IRQ thread - [High] drm/bridge: it6162: initialize hardware before attaching DSI and bridge - [High] drm/bridge: it6162: fix OF node refcount underflow in it6162_attach_dsi() - [High] drm/bridge: it6162: missing port@3 next_bridge and attach flags handling - [Medium] drm/bridge: it6162: unreachable HDCP code and off-by-one KSV list read - [Medium] drm/bridge: it6162: missing single-DSI rate limit in it6162_hdmi_tmds_char_rate_valid() --
commit bb2197e5059c8f22ed7c430b26d564ec316ac54c Author: Hermes Wu <[email protected]> drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver This commit adds a DRM bridge driver for the ITE IT6162 MIPI DSI to HDMI 2.0 bridge chip, supporting single and dual MIPI DSI inputs, mode setting, EDID retrieval, HDMI audio, and HPD handling. > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index c7da6b02d7b6b..59d4911222747 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -106,6 +106,23 @@ config DRM_INNO_HDMI > select DRM_DISPLAY_HELPER > select DRM_KMS_HELPER > > +config DRM_ITE_IT6162 > + tristate "iTE IT6162 DSI to HDMI bridge" > + depends on OF > + select REGMAP_I2C > + select DRM_MIPI_DSI > + select DRM_PANEL_BRIDGE > + select DRM_KMS_HELPER > + select DRM_HDMI_HELPER > + select DRM_DISPLAY_HDMI_HELPER > + select DRM_DISPLAY_HDCP_HELPER > + select DRM_DISPLAY_HELPER [Severity: High] Should this select DRM_DISPLAY_HDMI_STATE_HELPER instead of DRM_HDMI_HELPER? There is no DRM_HDMI_HELPER Kconfig symbol in the kernel tree. Meanwhile, it6162_bridge_atomic_enable(), it6162_bridge_hdmi_audio_prepare(), and it6162_bridge_hdmi_audio_shutdown() call: drm_atomic_helper_connector_hdmi_update_infoframes() drm_atomic_helper_connector_hdmi_update_audio_infoframe() drm_atomic_helper_connector_hdmi_clear_audio_infoframe() which are compiled only when CONFIG_DRM_DISPLAY_HDMI_STATE_HELPER is enabled. Without selecting DRM_DISPLAY_HDMI_STATE_HELPER, building the driver when no other enabled driver selects that symbol fails at link time with undefined references. [ ... ] > diff --git a/drivers/gpu/drm/bridge/ite-it6162.c > b/drivers/gpu/drm/bridge/ite-it6162.c > new file mode 100644 > index 0000000000000..5f83722ff4e61 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/ite-it6162.c [ ... ] > +static int it6162_hdcp_read_list(struct it6162 *it6162, > + u8 *ksv_list, > + int dev_count) > +{ > + u8 buf[DATA_BUFFER_DEPTH]; > + int i, j, ret; > + > + if (!ksv_list || dev_count <= 0) > + return -EINVAL; > + > + guard(mutex)(&it6162->lock); > + for (i = 0; i < (dev_count / 6 + 1); i++) { > + ret = it6162_infoblock_request_data(it6162, HOST_SET_HDCP_R, > + i + 1, buf); > + if (ret < 0) > + return ret; > + > + for (j = 0; j < 30; j += 5) { > + if ((i * 6 + j / 5) >= dev_count) > + break; > + > + memcpy(&ksv_list[(i * 6 + j / 5) * 5], &buf[j], 5); > + } > + } > + return dev_count; > +} > + > +static void it6162_update_hdcp14(struct it6162 *it6162) > +{ > + struct drm_device *drm = it6162->drm; > + int dev_count; > + u8 bksv[5]; > + u8 bstatus[2]; > + u8 ksvlist[5 * 30]; > + int ret; > + > + ret = it6162_hdcp_read_information(it6162, bstatus, bksv); > + if (ret < 0) { > + drm_err(drm, "fail to read Bstatus"); > + return; > + } > + > + dev_count = DRM_HDCP_NUM_DOWNSTREAM(bstatus[0]); > + if (dev_count == 0) > + return; > + > + if (DRM_HDCP_MAX_DEVICE_EXCEEDED(bstatus[0]) || > + DRM_HDCP_MAX_CASCADE_EXCEEDED(bstatus[1])) { > + drm_err(drm, "HDCP1.4 Max Topology Limit Exceeded"); > + return; > + } > + > + dev_count = dev_count > 30 ? 30 : dev_count; > + ret = it6162_hdcp_read_list(it6162, ksvlist, dev_count); > + if (ret < 0) { > + drm_err(drm, "fail to read KSV list"); > + return; > + } > +} [Severity: Medium] Is the HDCP support in this driver incomplete or unreachable? In it6162_config_default(), it6162->en_hdcp is initialized to false and is never set to true anywhere in the driver, while it6162->bridge.support_hdcp is also never set. In addition, it6162_bridge_atomic_enable() stores conn_state->content_protection into it6162->content_protection, which is never read, and it6162_tx_enable() never calls it6162_tx_hdcp_setup(). Also, in it6162_update_hdcp14() and it6162_update_hdcp23(), the read bstatus/bksv/ksvlist and rxinfo/rxid/rxid_list buffers are unused local arrays, and topology or read failures are not propagated back to it6162_hdcp_handler(), which still sets: cp_status = DRM_MODE_CONTENT_PROTECTION_ENABLED; Finally, in it6162_hdcp_read_list(), the loop bound (dev_count / 6 + 1) instead of DIV_ROUND_UP(dev_count, 6) performs an extra iteration when dev_count is an exact multiple of 6 (such as 6 or the clamped maximum of 30), requesting an out-of-range block index via it6162_infoblock_request_data(). Should the unused HDCP code be removed until it is wired up, or completed and fixed to use DIV_ROUND_UP(dev_count, 6)? [ ... ] > +static void it6162_interrupt_handler(struct it6162 *it6162) > +{ > + unsigned int int_status, tx_status; > + enum drm_connector_status connector_status; > + struct device *dev = it6162->dev; > + struct regmap *regmap = it6162->regmap; > + int err; > + > + err = regmap_read(regmap, OFFSET_EVENT_CHG, &int_status); > + if (err < 0) { > + dev_err(dev, "read failed rx reg[0x%x] err: %d", > + OFFSET_EVENT_CHG, err); > + return; > + } > + > + regmap_write(it6162->regmap, OFFSET_EVENT_CHG, 0xFF); > + > + if (!!GET_BUFFER_STATUS(int_status)) { > + regmap_write(it6162->regmap, OFFSET_DRIVER_DATA, int_status); > + it6162->data_buf_sts = GET_BUFFER_STATUS(int_status); > + wake_up(&it6162->data_buf_wait); > + } > + > + if (!(int_status & EVENT_CHG)) > + return; > + > + err = regmap_read(regmap, OFFSET_TX_STATUS, &tx_status); > + if (err < 0) { > + dev_err(dev, "read failed rx reg[0x%x] err: %d", > + OFFSET_TX_STATUS, err); > + return; > + } > + > + connector_status = GET_TX_HPD_STATUS(tx_status) ? > + connector_status_connected : > + connector_status_disconnected; > + > + if (it6162->connector_status != connector_status) { > + it6162->connector_status = connector_status; > + drm_bridge_hpd_notify(&it6162->bridge, > + it6162->connector_status); > + } [Severity: High] Won't calling drm_bridge_hpd_notify() synchronously inside it6162_interrupt_handler() deadlock when HPD processing reads the EDID? it6162_probe() registers it6162_int_threaded_handler() with IRQF_ONESHOT, so the IRQ line stays masked and the handler cannot re-enter until it6162_int_threaded_handler() returns. However, reading the EDID in it6162_get_edid_block() calls it6162_infoblock_request_data(), which blocks in wait_event_timeout() on it6162->data_buf_wait waiting for it6162_int_threaded_handler() to service the buffer status interrupt and call wake_up(&it6162->data_buf_wait): it6162_int_threaded_handler() it6162_interrupt_handler() drm_bridge_hpd_notify() drm_bridge_connector_hpd_cb() drm_kms_helper_connector_hotplug_event() drm_client_dev_hotplug() drm_helper_probe_single_connector_modes() it6162_bridge_read_edid() it6162_get_edid_block() it6162_infoblock_request_data() wait_event_timeout(it6162->data_buf_wait, ...) Because this runs on the same IRQF_ONESHOT thread that is supposed to wake data_buf_wait, wait_event_timeout() will always stall for TIMEOUT_INFOBLOCK_MS (1800 ms) and fail with -ETIMEDOUT. Similarly, if another thread holds dev->mode_config.mutex and calls it6162_bridge_read_edid() -> it6162_infoblock_request_data() while it6162_interrupt_handler() processes EVENT_CHG and calls drm_bridge_hpd_notify(), the IRQ thread will block waiting for dev->mode_config.mutex and never process the buffer-ready interrupt to wake data_buf_wait. [ ... ] > +static int it6162_attach_dsi(struct it6162 *it6162) > +{ > + struct device *dev = it6162->dev; > + struct device_node *np = dev->of_node; > + const struct mipi_dsi_device_info info = {"it6162-mipi", 0, np}; > + struct mipi_dsi_device *dsi; > + struct mipi_dsi_host *host; > + struct it6162_mipi_cfg *mipi_cfg = &it6162->mipi_cfg; > + int ret = 0; > + > + it6162->dsi = NULL; > + for (int port = 0; port < 2; port++) { > + host = it6162_of_get_dsi_host_by_port(it6162, port); > + if (IS_ERR(host)) > + continue; > + > + mipi_cfg->en_port[port] = true; > + > + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); [Severity: High] Does passing np in mipi_dsi_device_info without taking a reference underflow the OF node refcount of dev->of_node on teardown? devm_mipi_dsi_device_register_full() calls mipi_dsi_device_register_full(), which assigns dsi->dev.of_node = info->node via device_set_node() without calling of_node_get(). When the DSI device is released on probe failure or driver unbind, mipi_dsi_dev_release() unconditionally calls: of_node_put(dev->of_node); Because no matching reference was acquired for each registered DSI device (one in single-DSI mode, two in dual-DSI mode), this drops references owned by the I2C client's device_node. [ ... ] > +static int it6162_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder, > + enum drm_bridge_attach_flags flags) > +{ > + struct it6162 *it6162 = bridge_to_it6162(bridge); > + struct drm_device *drm = bridge->dev; > + > + it6162->drm = drm; > + if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) { > + drm_dbg(drm, "it6162 driver only copes with atomic updates"); > + return -EOPNOTSUPP; > + } > + > + return 0; > +} [Severity: High] Should it6162_bridge_attach() check DRM_BRIDGE_ATTACH_NO_CONNECTOR and attach the downstream HDMI output bridge from port@3? In it6162_bridge_attach(), the flags parameter is ignored: the function neither requires DRM_BRIDGE_ATTACH_NO_CONNECTOR nor creates a connector. In addition, the device tree binding defines port@3 as the HDMI output port (typically connected to an hdmi-connector node), but it6162_probe() only checks ports 0, 1, and 2, and never acquires or attaches the next bridge on port@3. > + > +static enum drm_mode_status > +it6162_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + unsigned long long rate; > + > + rate = drm_hdmi_compute_mode_clock(mode, 8, > DRM_OUTPUT_COLOR_FORMAT_RGB444); > + if (rate == 0) > + return MODE_NOCLOCK; > + > + return bridge->funcs->hdmi_tmds_char_rate_valid(bridge, mode, rate); > +} [ ... ] > +static enum drm_mode_status > +it6162_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + unsigned long long tmds_rate) > +{ > + /*IT6162 hdmi supports HDMI2.0 600Mhz*/ > + if (tmds_rate > 600000000) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} [Severity: Medium] Should it6162_hdmi_tmds_char_rate_valid() or it6162_bridge_mode_valid() also limit the maximum rate when only a single MIPI DSI port is enabled? The commit description notes that single MIPI DSI input supports up to 4K @ 30Hz (~300 MHz) while dual MIPI DSI input is required for 4K @ 60Hz (600 MHz), but it6162_hdmi_tmds_char_rate_valid() allows up to 600 MHz without checking whether both ports in mipi_cfg.en_port[] are enabled. [ ... ] > +static int it6162_probe(struct i2c_client *client) > +{ [ ... ] > + it6162->bridge.of_node = np; > + it6162->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | > + DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_MODES; > + > + it6162->bridge.type = DRM_MODE_CONNECTOR_HDMIA; > + > + it6162->bridge.vendor = "ITE"; > + it6162->bridge.product = "IT6162"; [Severity: High] Are the bridge.ops flags and HDMI version fields configured as intended here? 1. DRM_BRIDGE_OP_MODES is set in it6162->bridge.ops, which tells drm_bridge_connector to call bridge->funcs->get_modes(). Because it6162_bridge_funcs does not implement .get_modes (it only implements .edid_read), drm_bridge_get_modes() will dereference a NULL function pointer: if (!(bridge->ops & DRM_BRIDGE_OP_MODES)) return 0; return bridge->funcs->get_modes(bridge, connector); 2. it6162_interrupt_handler() calls drm_bridge_hpd_notify(), but DRM_BRIDGE_OP_HPD is not set in it6162->bridge.ops, so drm_bridge_connector never registers an HPD callback on this bridge and drm_bridge_hpd_notify() is a no-op. 3. it6162_bridge_funcs implements .hdmi_write_spd_infoframe and .hdmi_clear_spd_infoframe, and bridge.vendor and bridge.product are set, but DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME is omitted from it6162->bridge.ops. 4. DRM_BRIDGE_OP_HDMI is set without setting it6162->bridge.supported_hdmi_ver (for example to HDMI_VERSION_2_0), which triggers a DRM_WARN("HDMI bridge misses supported HDMI version\n") in drm_bridge_add(). > + > + if (it6162_of_get_audio(it6162)) { > + it6162->bridge.ops |= DRM_BRIDGE_OP_HDMI_AUDIO; > + it6162->bridge.hdmi_audio_dev = dev; > + it6162->bridge.hdmi_audio_max_i2s_playback_channels = 8; > + it6162->bridge.hdmi_audio_dai_port = 2; > + } > + > + /* > + * devres releases in reverse order: the IRQ is freed and the HDCP > + * works are cancelled before the bridge is removed, which destroys > + * the HPD notification mutex, and before the supplies and the driver > + * mutex are released. > + */ > + ret = devm_drm_bridge_add(dev, &it6162->bridge); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(dev, it6162_cancel_work_action, it6162); > + if (ret) > + return ret; > + > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + it6162_int_threaded_handler, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT | > + IRQF_NO_AUTOEN, > + "it6162-intp", it6162); > + if (ret) > + return ret; > + > + return it6162_attach_dsi(it6162); > +} [Severity: High] Does registering the bridge and attaching the DSI devices before it6162_hw_start() expose the bridge before the hardware and IRQ are ready? In it6162_probe(), devm_drm_bridge_add() is called before devm_request_threaded_irq() and it6162_attach_dsi(). Then, inside it6162_attach_dsi(), devm_mipi_dsi_attach() is called inside the port loop before it6162_hw_start() runs: it6162_attach_dsi() for (int port = 0; port < 2; port++) { ... devm_mipi_dsi_attach(dev, dsi); } return it6162_hw_start(it6162); When a DSI host's .attach callback binds the DRM pipeline synchronously during devm_mipi_dsi_attach(), the bridge is already active while it6162->hw_ready is still false (causing it6162_bridge_detect() to return connector_status_disconnected) and client->irq is still disabled with IRQF_NO_AUTOEN (causing any EDID read via it6162_infoblock_request_data() to time out). Once it6162_hw_start() subsequently runs, it calls it6162_hw_reset() and resets the chip after the DSI host has attached. Also, in it6162_attach_dsi(), if it6162_load_mipi_pars_from_port() were to return 0, the check: if (ret <= 0) return ret; would return 0 from it6162_probe() without attaching DSI or starting the hardware. Should the MIPI DT parameters be parsed and it6162_hw_start() be completed before publishing the bridge and attaching the DSI host(s)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
