Re: [PATCH v5 2/2] drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver

[sashiko-bot@kernel.org]

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 <Hermes.wu@ite.com.tw>

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/20261029-upstream-6162-v5-0-8369f9e5afe6@ite.com.tw?part=2