Re: [PATCH 2/2] drm/cix: add Sky1 DisplayPort transmitter driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 16 potential
issues to consider:
- [High] drm/cix: unread sink DPCD caps and status race on boot, resume,
  and HPD
- [High] drm/cix: AUX probe in cix_dp_replay_deferred_hotplug() always
  fails with -ENODEV
- [High] drm/cix: broken VSC SDP packing, stream override, and YCbCr420
  validation
- [High] drm/cix: unclocked MMIO register accesses when apb_clk is gated
- [High] drm/cix: NULL pointer dereference when
  DRM_BRIDGE_ATTACH_NO_CONNECTOR is set
- [High] drm/cix: broken YCbCr 4:2:0 mode matching, format selection,
  and VSC SDP
- [Medium] drm/cix: PHY refcount leak and state desync when
  cix_dp_enable() or resume fails
- [Medium] drm/cix: HPD workqueue and IRQ teardown races in deinit,
  detach, and suspend
- [Medium] drm/cix: active streams and PHY power left enabled in
  shutdown and remove
- [Medium] drm/cix: display stays off if cix_dp_prepare() runs before
  cix_dp_pm_complete()
- [Medium] drm/cix: missing stream TU/timing update and unchecked mode
  config on retrain
- [Medium] drm/cix: broken BT.2020 bpc fallback in cix_dp_cal_bpc() and
  6 bpc default
- [Medium] drm/cix: unlocked mode state update in
  cix_dp_bridge_mode_set() before prepare
- [Medium] drm/cix: FIFO underflow read on AUX NACK/DEFER in
  cix_dp_aux_cmd_submit()
- [Medium] drm/cix: plain 64-bit division in cix_dp_adjust_mode() breaks
  32-bit builds
- [Low] drm/cix: cix_dp_aux_cmd_submit() reads reply FIFO on AUX/I2C
  NACK or DEFER
--

commit 002cf156569e15175af86684eb3509dc08da82b1
Author: Luo Liu <Luo.Liu@cixtech.com>

drm/cix: add Sky1 DisplayPort transmitter driver

This commit adds a DRM bridge and connector driver for the CIX Sky1
DisplayPort transmitter, driving the DisplayPort PHY via the generic PHY
API and supporting link training, EDID probing, YCbCr 4:2:0, and hotplug.

> diff --git a/drivers/gpu/drm/cix/dptx/cix_dptx.c b/drivers/gpu/drm/cix/dptx/cix_dptx.c
> new file mode 100644
> index 0000000000000..8915c50bc9db3
> --- /dev/null
> +++ b/drivers/gpu/drm/cix/dptx/cix_dptx.c
[ ... ]
> +static int cix_dp_train_loop(struct cix_dp *dp)
> +{
> +	struct cix_dp_mode *mode = &dp->mode;
> +	u8 bpp = dp->connector.config.bpp;
> +	u8 bw_code = mode->bw_code;
> +	int i;
> +	int j;
> +	int ret;
> +
> +	for (i = 0; i < DP_MAX_TRAINING_LOOP; i++) {
> +		for (j = 0; j <= 3; j++) {
> +			if (!(dp->state & DPTX_STATE_READY))
> +				return 0;
> +
> +			ret = cix_dp_train(dp);
> +			if (!ret)
> +				return 0;
> +
> +			ret = cix_dp_mode_configure(dp, mode->pclock, bw_code,
> +						    bpp, true);
> +			if (ret < 0)
> +				break;
> +
> +			bw_code = dp->mode.bw_code;
> +		}
> +
> +		cix_dp_mode_configure(dp, mode->pclock, 0, bpp, true);
> +		bw_code = dp->mode.bw_code;
> +	}
> +
> +	return -EIO;
> +}

[Severity: Medium]
When cix_dp_hpd_irq_work_func() calls cix_dp_train_loop() while a stream is
enabled and link training downshifts dp->mode.bw_code or changes
dp->mode.lane_cnt, does anything reprogram the active stream's transfer unit
and timing registers?  cix_dp_mode_set_transfer_unit() and
cix_dp_mode_set_timing() depend on bw_code, link_rate, and lane_cnt, so
leaving CIX_DPTX_TU_CONFIG, CIX_DPTX_USER_DATA_COUNT,
CIX_DPTX_SECONDARY_DATA_WINDOW, and CIX_DPTX_DATA_CONTROL at their old values
corrupts the running video stream.  Also, the fallback
cix_dp_mode_configure() call at the end of the outer loop ignores its return
value.

> +
> +static int cix_dp_aux_cmd_submit(struct cix_dp *dp, u32 cmd, u32 addr,
> +				 u8 *buf, u32 bytes, u8 *reply)
> +{
[ ... ]
> +	value = cix_dp_read(dp, CIX_DPTX_AUX_REPLY_CODE);
> +	if (reply)
> +		*reply = value;
> +
> +	if (is_read) {
> +		if (value == CIX_DPTX_AUX_REPLY_CODE_AUX_ACK ||
> +		    value == CIX_DPTX_AUX_REPLY_CODE_I2C_ACK) {
> +			value = cix_dp_read(dp, CIX_DPTX_AUX_REPLY_DATA_COUNT);
> +			if ((value & CIX_DPTX_REPLY_DATA_COUNT_MASK) != bytes)
> +				return -EIO;
> +		}
> +		for (i = 0; i < bytes; i++)
> +			buf[i] = cix_dp_read(dp, CIX_DPTX_AUX_REPLY_DATA);
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Should the CIX_DPTX_AUX_REPLY_DATA read loop be inside the ACK check?  When a
sink responds to a read with NACK or DEFER, the loop still runs and overwrites
buf with stale or empty FIFO reads before drm_dp_aux retries.

> +
> +static ssize_t cix_dp_aux_transfer(struct drm_dp_aux *aux,
> +				   struct drm_dp_aux_msg *msg)
> +{
> +	struct cix_dp *dp = container_of(aux, struct cix_dp, aux);
> +	size_t to_access = 0;
> +	size_t accessed_bytes = 0;
> +	int ret;
> +
> +	if (!cix_dp_get_hpd_state(dp))
> +		return -ENODEV;
> +
> +	do {
> +		to_access = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES,
> +				  msg->size - accessed_bytes);
> +		ret = cix_dp_aux_cmd_submit(dp, msg->request,
> +					    msg->address + accessed_bytes,
> +					    msg->buffer + accessed_bytes,
> +					    to_access, &msg->reply);
> +		if (ret)
> +			return accessed_bytes ? (ssize_t)accessed_bytes : ret;
> +
> +		accessed_bytes += to_access;
> +	} while (accessed_bytes < msg->size);
> +
> +	return msg->size;
> +}

[Severity: Medium]
In addition to cix_dp_aux_cmd_submit() reading from the empty
CIX_DPTX_AUX_REPLY_DATA FIFO when the sink replies with NACK or DEFER, does
cix_dp_aux_transfer() keep submitting later 16-byte chunks after a non-ACK
reply?  cix_dp_aux_cmd_submit() returns 0 on NACK and DEFER, so a multi-chunk
transfer continues looping and overwrites msg->reply instead of stopping and
returning to the caller.

[ ... ]

> +static int cix_dp_set_misc(struct cix_dp *dp,
> +			   struct cix_dp_panel *dp_panel)
> +{
> +	struct cix_dp_config *config;
> +
> +	if (!dp_panel || !dp_panel->connector)
> +		return -EINVAL;
> +
> +	config = &dp_panel->connector->config;
> +
> +	config->misc0 &= ~CIX_DPTX_STREAM_MISC0_SYNC_LOCK;
> +	config->misc1 &= ~CIX_DPTX_VSC_COLORIMETRY_EN;
> +	config->override = 0;
> +
> +	if (dp->caps.vsc_supported &&
> +	    cix_dp_needs_vsc_sdp(config->format, config->colorspace)) {
> +		config->misc1 |= CIX_DPTX_VSC_COLORIMETRY_EN;
> +
> +		switch (config->bpc) {
> +		case 6:
> +			config->override = CIX_DPTX_STREAM_OVERRIDE_BPC_6;
> +			break;
[ ... ]
> +		default:
> +			config->override = CIX_DPTX_STREAM_OVERRIDE_BPC_8;
> +			break;
> +		}
> +
> +		if (config->format == CIX_DP_FORMAT_YCBCR420)
> +			config->override |= CIX_DPTX_STREAM_OVERRIDE_YCbCR_420;
> +
> +		config->override |= CIX_DPTX_STREAM_OVERRIDE_ENABLE;
> +		config->dynamic_range = DP_DYNAMIC_RANGE_CTA;
> +		config->content_type = DP_CONTENT_TYPE_NOT_DEFINED;
> +
> +		goto end;
> +	}
[ ... ]
> +static int dptx_setup_vsc_sdp(struct dp_sdp *sdp,
> +			      struct cix_dp_config *config)
> +{
> +	sdp->sdp_header.HB0 = 0;
> +	sdp->sdp_header.HB1 = 0x7;
> +	sdp->sdp_header.HB2 = 0x5;
> +	sdp->sdp_header.HB3 = 0x13;
> +
> +	switch (config->format) {
> +	case CIX_DP_FORMAT_YCBCR444:
> +		sdp->db[16] = 0x1 << 4;
> +		break;
> +	case CIX_DP_FORMAT_YCBCR422:
> +		sdp->db[16] = 0x1 << 4;
> +		break;
> +	case CIX_DP_FORMAT_YCBCR420:
> +		sdp->db[16] = 0x3 << 4;
> +		break;
> +	default:
> +		break;
> +	}

[Severity: High]
Does dptx_setup_vsc_sdp() program the wrong VSC SDP payload for YCbCr 4:2:2,
RGB, and 6 bpc?

CIX_DP_FORMAT_YCBCR422 sets sdp->db[16] to 0x1 << 4 (YCbCr 4:4:4) instead of
0x2 << 4 (YCbCr 4:2:2).  In the default (RGB) branch, sdp->db[16] is not
cleared before the colorspace switch ORs in the colorimetry bits, leaving
stale format and colorimetry bits from earlier modesets in
conn->sdp[CIX_VSC_SDP].  Further down in dptx_setup_vsc_sdp(), switch
(config->bpc) has no case 6, so 6 bpc RGB falls into default and encodes
0x1 (8 bpc) instead of 0x0 in sdp->db[17].

In cix_dp_set_misc() above, the VSC path only sets the pixel format bits in
config->override for CIX_DP_FORMAT_YCBCR420 (leaving YCbCr 4:2:2 and 4:4:4
as 0/RGB in the override register) and overwrites config->content_type with
DP_CONTENT_TYPE_NOT_DEFINED instead of using conn_state->content_type.  Also,
cix_dp_pick_format_bpc() and cix_dp_connector_mode_valid() select
DRM_OUTPUT_COLOR_FORMAT_YCBCR420 without checking dp->caps.vsc_supported;
when vsc_supported is false, cix_dp_set_misc() falls through and returns
-EINVAL (which cix_dp_panel_hw_cfg() ignores), leaving MISC0, MISC1,
OVERRIDE, and the VSC SDP unconfigured while the RCSU adapter is set to
YCbCr 4:2:0.

[ ... ]

> +static int cix_dp_panel_setup_colorimetry_sdp(struct cix_dp *dp,
> +					      struct cix_dp_panel *dp_panel)
> +{
> +	struct cix_connector *conn = dp_panel->connector;
> +
> +	if (conn->config.override & CIX_DPTX_STREAM_OVERRIDE_ENABLE) {
> +		dptx_setup_vsc_sdp(&conn->sdp[CIX_VSC_SDP], &conn->config);
> +		infoframe_write_packet(dp, dp_panel->source_id, CIX_VSC_SDP,
> +				       &conn->sdp[CIX_VSC_SDP], 1);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int cix_dp_panel_hw_cfg(struct cix_dp *dp, int stream_id)
> +{
> +	struct cix_dp_panel *dp_panel = dp->active_panels[stream_id];
> +	struct drm_display_mode *mode = cix_dp_get_mode(dp, stream_id);
> +	struct cix_connector *conn;
> +
> +	if (!mode)
> +		return -EINVAL;
> +
> +	conn = dp_panel->connector;
> +
> +	cix_dp_cfg_adapter(dp, stream_id);
> +
> +	cix_dp_set_misc(dp, dp_panel);
> +	cix_dp_panel_setup_colorimetry_sdp(dp, dp_panel);
> +
> +	cix_dp_mode_set_transfer_unit(dp, dp_panel->source_id, mode,
> +				      conn->config.bpp);
> +	cix_dp_mode_set_timing(dp, dp_panel->source_id, mode, conn->config.bpp);
> +	return 0;
> +}

[Severity: High]
Does enabling the bridge when attached with DRM_BRIDGE_ATTACH_NO_CONNECTOR
cause a NULL pointer dereference on dp_panel->connector?

When DRM_BRIDGE_ATTACH_NO_CONNECTOR is passed to cix_dp_bridge_attach(), it
returns 0 without calling cix_dp_connector_create(), leaving
dp->dp_panel.connector NULL.  On modeset enable, cix_dp_panel_hw_cfg()
ignores the -EINVAL return from cix_dp_set_misc() and calls
cix_dp_panel_setup_colorimetry_sdp(), which dereferences
conn->config.override, followed by cix_dp_panel_hw_cfg() dereferencing
conn->config.bpp.  In addition, cix_dp_hpd_event_work_func() exits early when
!dp->dp_panel.connector and cix_dp_bridge_funcs does not implement the bridge
detect, EDID, or HPD callbacks needed without an internal connector.

[ ... ]

> +/*
> + * Pure probe: report the current HPD level without touching the session state
> + * or the hardware.  This runs on every DRM probe, poll and hotplug, so
> + * DPTX_STATE_CONNECTED is advanced from the HPD event work and the encoder
> + * prepare path instead.
> + *
> + * The host is initialized by probe, resume and cix_dp_prepare().  Bail out
> + * loudly if we are ever called before that: cix_dp_read() is a plain readl()
> + * and would hang on a gated APB clock.
> + */
> +static int cix_dp_detect(struct cix_dp *dp)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&dp->session_lock);
> +
> +	if (dp->state & DPTX_STATE_SUSPENDED) {
> +		ret = -EBUSY;
> +	} else if (!(dp->state & DPTX_STATE_INITIALIZED)) {
> +		dev_warn_once(dp->dev, "detect with uninitialized host\n");
> +		ret = -ENODEV;
> +	}
> +
> +	mutex_unlock(&dp->session_lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	return cix_dp_get_hpd_state(dp) ? 0 : -ENODEV;
> +}

[Severity: High]
Can MMIO registers be read while dp->apb_clk is gated, hanging the bus?

As noted in the comment above cix_dp_detect(), reading DPTX registers while
the host is uninitialized and apb_clk is gated hangs the bus:

- cix_dp_aux_transfer() calls cix_dp_get_hpd_state() and
  cix_dp_aux_cmd_submit() without acquiring dp->session_lock or checking
  DPTX_STATE_INITIALIZED / DPTX_STATE_SUSPENDED, so /dev/drm_dp_aux* or I2C
  transfers during suspend read unclocked MMIO registers.
- cix_dp_hpd_irq_work_func() and cix_dp_hpd_event_work_func() call
  cix_dp_get_hpd_state() before acquiring dp->session_lock and checking
  DPTX_STATE_INITIALIZED.
- cix_dp_detect() drops dp->session_lock before calling
  cix_dp_get_hpd_state(), and cix_dp_prepare() drops and re-acquires
  dp->session_lock around cix_dp_handle_connect() without rechecking
  DPTX_STATE_INITIALIZED, allowing a concurrent cix_dp_pm_prepare() to gate
  apb_clk in between.

[ ... ]

> +static void cix_dp_replay_deferred_hotplug(struct cix_dp *dp)
> +{
> +	struct phy *phy = dp->phy;
> +	union phy_configure_opts opts = { 0 };
> +	u8 dpcd_rev;
> +
> +	if (!(dp->state & DPTX_STATE_INITIALIZED))
> +		return;
> +	if (dp->force_hpd != connector_status_unknown)
> +		return;
> +	if (cix_dp_get_hpd_state(dp))
> +		return;
> +
> +	phy_validate(phy, PHY_MODE_DP, 0, &opts);
> +	if (phy_get_mode(phy) != PHY_MODE_DP)
> +		return;
> +
> +	/*
> +	 * The combo PHY is already muxed to DP, so the sink may have been
> +	 * plugged in before the connector existed. Probe the AUX channel to
> +	 * make sure one is really there before forcing a hotplug event.
> +	 */
> +	if (drm_dp_dpcd_readb(&dp->aux, DP_DPCD_REV, &dpcd_rev) < 0)
> +		return;
> +
> +	dp->force_hpd = connector_status_connected;
> +	dp->status = connector_status_unknown;
> +	schedule_delayed_work(&dp->hpd_event_work, 0);
> +}

[Severity: High]
Won't drm_dp_dpcd_readb() in cix_dp_replay_deferred_hotplug() always fail
with -ENODEV?

cix_dp_replay_deferred_hotplug() only reaches drm_dp_dpcd_readb() when
cix_dp_get_hpd_state(dp) is false, and it does not set
dp->force_hpd = connector_status_connected until after the read succeeds.
However, drm_dp_dpcd_readb() calls cix_dp_aux_transfer(), which immediately
checks if (!cix_dp_get_hpd_state(dp)) and returns -ENODEV, so a USB-C display
plugged in before bridge attach is never detected.

> +
> +static void cix_dp_hpd_event_work_func(struct work_struct *work)
> +{
> +	struct cix_dp *dp = container_of(work, struct cix_dp,
> +					 hpd_event_work.work);
> +	struct phy *phy = dp->phy;
> +	union phy_configure_opts opts = { 0 };
> +	enum drm_connector_status old_status;
> +	enum phy_mode mode;
> +	int i;
> +
> +	if (!dp->dp_panel.connector)
> +		return;
> +
> +	/*
> +	 * On a USB-C DP alt-mode PHY the hotplug only becomes valid once the
> +	 * PHY has been switched to DP mode, so wait for the mode change.
> +	 * A native DP PHY has no ->set_mode() operation, which leaves the
> +	 * mode at PHY_MODE_INVALID forever; there is nothing to wait for.
> +	 */
> +	if (cix_dp_get_hpd_state(dp) &&
> +	    phy_get_mode(phy) != PHY_MODE_INVALID) {
> +		for (i = 0; i < DP_HPD_MAX_TRIES; i++) {
> +			phy_validate(phy, PHY_MODE_DP, 0, &opts);
> +			mode = phy_get_mode(phy);
> +			if (mode == PHY_MODE_DP)
> +				break;
> +			msleep(100);
> +		}
> +	}
> +
> +	old_status = dp->status;
> +	drm_helper_probe_detect(&dp->connector.base, NULL, false);
> +
> +	if (dp->status == connector_status_connected)
> +		cix_dp_handle_connect(dp);
> +	else
> +		cix_dp_handle_disconnect(dp);
> +
> +	if (old_status != dp->status && dp->drm)
> +		drm_helper_hpd_irq_event(dp->drm);
> +}

[Severity: High]
Can sink DPCD capabilities remain unread across boot, resume, and hotplug,
and can concurrent connector probes suppress hotplug events?

If a native DP/eDP sink is already connected at boot, the HPD interrupt fires
in cix_dp_init() before cix_dp_bridge_attach() sets dp->dp_panel.connector,
so cix_dp_hpd_event_work_func() returns early; later,
cix_dp_replay_deferred_hotplug() also returns early because
cix_dp_get_hpd_state(dp) is true.  Likewise, cix_dp_pm_prepare() clears
DPTX_STATE_CONNECTED on suspend without cix_dp_pm_complete() re-probing DPCD
on resume.  Because cix_dp_connector_detect() only checks HPD level without
calling cix_dp_handle_connect(), dp->dpcd, dp->downstream_ports, dp->caps,
and dp->link_config remain unpopulated during mode validation, atomic_check,
and cix_dp_bridge_mode_set() (which runs before encoder_enable() ->
cix_dp_prepare() calls cix_dp_handle_connect()).

Even when cix_dp_hpd_event_work_func() runs, it calls
drm_helper_probe_detect() before cix_dp_handle_connect(), so
cix_dp_connector_detect() passes stale dp->dpcd and dp->downstream_ports to
drm_dp_set_subconnector_property(), and any concurrent DRM connector probe
updates dp->status before old_status = dp->status is read:

CPU0 (kworker)                        CPU1 (userspace probe)
cix_dp_hpd_event_work_func()
  msleep(100) waiting for PHY
                                      cix_dp_connector_detect()
                                        dp->status = connected
  old_status = dp->status (connected)
  drm_helper_probe_detect()
  if (old_status != dp->status)       <-- false, HPD event dropped

Finally, cix_dp_handle_connect() only clears DPTX_STATE_CONNECTED when
cix_dp_panel_read_sink_caps() returns -ETIMEDOUT or -ENOTCONN; if it fails
with -EIO, -EPROTO, -EBUSY, or -ENODEV, DPTX_STATE_CONNECTED stays set and
prevents later cix_dp_handle_connect() calls from reading the DPCD.

[ ... ]

> +static int cix_dp_prepare(struct cix_dp *dp)
> +{
> +	int ret;
> +
> +	mutex_lock(&dp->session_lock);
> +
> +	if (dp->state & (DPTX_STATE_ENABLED)) {
> +		ret = 0;
> +		goto end;
> +	}
> +
> +	if (!(dp->state & DPTX_STATE_INITIALIZED)) {
> +		ret = cix_dp_host_init(dp);
> +		if (ret)
> +			goto end;
> +	} else if (dp->state & DPTX_STATE_INIT_TRAIN) {
> +		reset_dp_and_reinit(dp);
> +		usleep_range(100, 200);
> +	}
> +
> +	if (cix_dp_get_hpd_state(dp) &&
> +	    !(dp->state & DPTX_STATE_CONNECTED)) {
> +		mutex_unlock(&dp->session_lock);
> +		cix_dp_handle_connect(dp);
> +		mutex_lock(&dp->session_lock);
> +	}
> +
> +	if (!(cix_dp_get_hpd_state(dp) && (dp->state & DPTX_STATE_CONNECTED))) {
> +		ret = 0;
> +		goto end;
> +	}
> +
> +	ret = cix_dp_core_on(dp, true);
> +
> +end:
> +	mutex_unlock(&dp->session_lock);
> +	return ret;
> +}

[Severity: Medium]
If the display controller resumes and calls cix_dp_bridge_atomic_enable() ->
cix_dp_prepare() before cix_dptx_pm_resume() -> cix_dp_pm_complete() clears
DPTX_STATE_SUSPENDED, will the display stay blank?

cix_dp_prepare() handles !(dp->state & DPTX_STATE_INITIALIZED) by calling
cix_dp_host_init(), and then calls cix_dp_handle_connect() to restore
DPTX_STATE_CONNECTED (which cix_dp_pm_prepare() cleared).  However,
cix_dp_handle_connect() bails out immediately when DPTX_STATE_SUSPENDED is
still set, leaving DPTX_STATE_CONNECTED clear so cix_dp_prepare() skips
cix_dp_core_on() and cix_dp_enable() skips enabling the stream.

[ ... ]

> +static int cix_dp_encoder_enable(struct cix_dp *dp, int stream_id, int enable)
> +{
> +	int ret;
> +
> +	if (enable) {
> +		ret = cix_dp_prepare(dp);
> +		if (ret)
> +			return ret;
> +
> +		ret = cix_dp_enable(dp, stream_id);
> +	} else {
> +		if (!(dp->state & DPTX_STATE_INITIALIZED))
> +			return 0;
> +
> +		ret = cix_dp_disable(dp, stream_id);
> +		if (ret)
> +			return ret;
> +
> +		ret = cix_dp_unprepare(dp);
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
Does a failure in cix_dp_enable() leak PHY init/power references and leave
dp->state inconsistent?

cix_dp_prepare() -> cix_dp_core_on() calls phy_init() and phy_power_on() and
sets DPTX_STATE_READY | DPTX_STATE_INIT_TRAIN.  If cix_dp_enable() then fails,
DPTX_STATE_ENABLED is not set, cix_dp_encoder_enable() does not roll back via
cix_dp_core_off(), and a later disable also skips cix_dp_core_off() because
cix_dp_unprepare() returns early when !(dp->state & DPTX_STATE_ENABLED).  On
the next cix_dp_prepare() call, the else if (dp->state &
DPTX_STATE_INIT_TRAIN) branch calls reset_dp_and_reinit(), which resets the
controller and calls phy_init() again without a matching phy_exit(), while
cix_dp_core_on() sees DPTX_STATE_READY still set and returns 0 without
retraining the link.

Similarly, in cix_dp_pm_complete(), if cix_dp_host_init() fails on resume,
enable_irq(dp->irq) is skipped and DPTX_STATE_SUSPENDED stays set, while the
next cix_dp_pm_prepare() or cix_dp_deinit() calls disable_irq(dp->irq)
unconditionally.

[ ... ]

> +static void cix_dp_deinit(struct cix_dp *dp)
> +{
> +	cancel_delayed_work_sync(&dp->sw_hpd_irq_work);
> +	cancel_delayed_work_sync(&dp->hpd_irq_work);
> +	cancel_delayed_work_sync(&dp->hpd_event_work);
> +	disable_irq(dp->irq);
> +
> +	mutex_lock(&dp->session_lock);
> +	cix_dp_host_deinit(dp);
> +	dp->state &= ~(DPTX_STATE_CONFIGURED);
> +	mutex_unlock(&dp->session_lock);
> +
> +	mutex_destroy(&dp->session_lock);
> +}

[Severity: Medium]
Can HPD delayed works be re-queued while cix_dp_deinit(),
cix_dp_bridge_detach(), or cix_dp_pm_prepare() is tearing down the driver?

In cix_dp_deinit(), cancel_delayed_work_sync() runs before
disable_irq(dp->irq):

CPU0                                  CPU1 (irq/kworker)
cix_dp_deinit()
  cancel_delayed_work_sync(...)
                                      cix_dp_irq_handler()
                                        queue_delayed_work(...,
                                          &dp->hpd_event_work, 0)
  disable_irq(dp->irq)
  cix_dp_host_deinit()
  mutex_destroy(&dp->session_lock)
  (devm frees dp)
                                      cix_dp_hpd_event_work_func()
                                        <-- use-after-free / gated APB read

Two related races exist in the other teardown paths:
- cix_dp_bridge_detach() cancels hpd_irq_work before sw_hpd_irq_work (even
  though cix_dp_sw_hpd_irq_work_func() queues hpd_irq_work) and leaves
  dp->irq enabled and dp->dp_panel.connector non-NULL while calling
  drm_connector_cleanup(&dp->connector.base).
- cix_dp_pm_prepare() cancels the delayed works before setting
  DPTX_STATE_SUSPENDED, while cix_dp_oob_hotplug_event() checks
  DPTX_STATE_SUSPENDED unlocked and can schedule hpd_event_work and
  sw_hpd_irq_work after the cancel calls.

[ ... ]

> +static void cix_dptx_remove(struct platform_device *pdev)
> +{
> +	struct cix_dp *dp = dev_get_drvdata(&pdev->dev);
> +
> +	if (!dp)
> +		return;
> +
> +	drm_bridge_remove(&dp->bridge);
> +	cix_dp_deinit(dp);
> +}
[ ... ]
> +static void cix_dptx_shutdown(struct platform_device *pdev)
> +{
> +	struct cix_dp *dp = platform_get_drvdata(pdev);
> +
> +	if (dp) {
> +		mutex_lock(&dp->session_lock);
> +		dp->active_stream_cnt = 0;
> +		mutex_unlock(&dp->session_lock);
> +		cix_dp_pm_prepare(dp);
> +	}
> +}

[Severity: Medium]
What happens to active streams and PHY power if cix_dptx_shutdown() or
cix_dptx_remove() runs while a display stream is enabled?

In cix_dptx_shutdown(), zeroing dp->active_stream_cnt without calling
cix_dp_ctrl_stream_off() or cix_dp_core_off() causes cix_dp_pm_prepare() ->
cix_dp_host_deinit() to call phy_exit(dp->phy) while the PHY is still powered
on (and with the second phy_init() reference from cix_dp_core_on() and
stream_clk still active).  In cix_dptx_remove(), dp->active_stream_cnt is not
cleared and streams are not stopped, so cix_dp_host_deinit() returns early at
if (dp->active_stream_cnt) and leaves the transmitter, clocks, and PHY
enabled.

> diff --git a/drivers/gpu/drm/cix/dptx/cix_dptx_drm.c b/drivers/gpu/drm/cix/dptx/cix_dptx_drm.c
> new file mode 100644
> index 0000000000000..0ddcd76691e03
> --- /dev/null
> +++ b/drivers/gpu/drm/cix/dptx/cix_dptx_drm.c
[ ... ]
> +static int cix_dp_cal_bpc(struct cix_dp *dp,
> +			  struct drm_connector_state *connector_state, int bpc)
> +{
> +	struct drm_display_info *info =
> +		&connector_state->connector->display_info;
> +
> +	if (connector_state->colorspace >= DRM_MODE_COLORIMETRY_BT2020_CYCC &&
> +	    connector_state->colorspace <= DRM_MODE_COLORIMETRY_BT2020_YCC)
> +		bpc = 10;
> +
> +	if (info->bpc && bpc > info->bpc)
> +		bpc = info->bpc;
> +
> +	if (connector_state->max_requested_bpc &&
> +	    bpc > connector_state->max_requested_bpc)
> +		bpc = connector_state->max_requested_bpc;
> +
> +	return bpc;
> +}

[Severity: Medium]
When a BT.2020 colorspace is set, doesn't overwriting bpc = 10 in
cix_dp_cal_bpc() prevent cix_dp_pick_format_bpc() from stepping down to 8 or
6 bpc when link bandwidth cannot fit 10 bpc?  Each fallback call to
compute_available_clock_rate() with curr_bpc = 8 or 6 gets forced back to 10.
Also, in cix_dp_pick_config(), passing max(info->bpc, 6) as suggest_bpc
selects 6 bpc instead of 8 bpc whenever an EDID leaves info->bpc as 0.

[ ... ]

> +static void cix_dp_adjust_mode(struct drm_display_mode *mode,
> +			       struct drm_display_mode *adjusted_mode)
> +{
> +	int diff = mode->htotal - mode->hsync_end;
> +
> +	/*
> +	 * Cix DP requires horizontal backporch to be greater than 12.
> +	 * This limitation may not be compatible with the sink device.
> +	 *
> +	 * Everything below is derived from @mode instead of accumulating on
> +	 * @adjusted_mode, so that repeated atomic checks of the same state
> +	 * produce the same result.
> +	 */
> +	if (diff < CIX_DPTX_MIN_H_BACKPORCH) {
> +		adjusted_mode->htotal = mode->htotal +
> +					CIX_DPTX_MIN_H_BACKPORCH - diff;
> +		adjusted_mode->clock = (int64_t)mode->clock *
> +				       adjusted_mode->htotal / mode->htotal;
> +	}

[Severity: Medium]
Will the plain 64-bit division in (int64_t)mode->clock *
adjusted_mode->htotal / mode->htotal fail to link with an undefined reference
to __divdi3 / __aeabi_ldivmod on 32-bit architectures?  DRM_CIX_DP does not
depend on 64BIT, so this needs div_s64() or mul_u32_u32() with div_u64().

> +
> +	/* The sync polarities are inverted with respect to the source mode. */
> +	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> +		adjusted_mode->flags &= ~DRM_MODE_FLAG_PVSYNC;
> +	else
> +		adjusted_mode->flags |= DRM_MODE_FLAG_PVSYNC;
> +
> +	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> +		adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
> +	else
> +		adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
> +}
[ ... ]
> +static int cix_dp_pick_format_bpc(struct cix_dp *dp,
> +				  struct drm_connector_state *conn_state,
> +				  struct drm_display_mode *adjusted_mode,
> +				  int info_formats, int suggest_bpc,
> +				  int *color_format, int *bpc, int *bpp)
> +{
> +	struct drm_display_info *info =
> +		&conn_state->connector->display_info;
> +	int curr_bpc;
> +	int format;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fmt_prio); i++) {
> +		format = fmt_prio[i].format;
> +		curr_bpc = fmt_prio[i].bpc ? fmt_prio[i].bpc : suggest_bpc;
> +
> +		/* Only use 420_only flag for YUV420 format */
> +		if (!(info_formats & BIT(format)) &&
> +		    !(format == DRM_OUTPUT_COLOR_FORMAT_YCBCR420 &&\n+		      drm_mode_is_420_only(info, adjusted_mode)))
> +			continue;

[Severity: High]
Does inverting the sync polarity flags on adjusted_mode in
cix_dp_adjust_mode() break drm_mode_is_420_only(info, adjusted_mode) and
YCbCr 4:2:0 format selection?

cix_dp_bridge_atomic_check() calls cix_dp_adjust_mode() before
cix_dp_pick_config().  Because cix_dp_adjust_mode() flips
DRM_MODE_FLAG_PVSYNC and DRM_MODE_FLAG_PHSYNC on adjusted_mode (and may alter
htotal and clock), drm_match_cea_mode(adjusted_mode) inside
drm_mode_is_420_only() fails to match the CEA mode table and returns 0.

Even when a mode is 4:2:0-only, cix_dp_pick_format_bpc() iterates fmt_prio[]
from RGB444 and YCBCR422 without checking drm_mode_is_420_only() first, so any
4:2:0-only mode whose clock fits on the link at RGB or YCbCr 4:2:2 picks a
non-4:2:0 format.  Conversely, when info->color_formats has
DRM_OUTPUT_COLOR_FORMAT_YCBCR420 set, fmt_prio[4] can pick YCBCR420 on modes
where the sink does not support 4:2:0, and without checking that
adjusted_mode->clock <= CIX_DP_MAX_FREQ (since YCbCr 4:2:0 forces 1 pixel per
cycle).

[ ... ]

> +static void cix_dp_bridge_mode_set(struct cix_dp *dp,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
> +{
[ ... ]
> +	cix_conn->config.format = cix_dp_drm_format_to_dp(color_format);
> +	cix_conn->config.colorspace = conn_state->colorspace;
> +	cix_conn->config.bpc = bpc;
> +	cix_conn->config.bpp = bpp;
> +	dp->pixel_per_cycle = cix_dp_pick_ppc(adjusted_mode,
> +					      cix_conn->config.format);
> +
> +	link_rate = cix_dp_pick_link_rate(dp, adjusted_mode, bpp);
> +	bw_code = drm_dp_link_rate_to_bw_code(link_rate);
> +
> +	dp->ops->mode_configure(dp, adjusted_mode->clock, bw_code, bpp, false);
[ ... ]
> +}
[ ... ]
> +static void
> +cix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
> +			    struct drm_atomic_commit *state)
> +{
[ ... ]
> +	cix_dp_bridge_mode_set(dp, crtc_state, conn_state);
> +
> +	ret = dp->ops->encoder_enable(dp, dp->dp_panel.source_id, true);
> +	if (ret)
> +		dev_err(dp->dev, "failed to enable DP output: %d\n", ret);
> +}

[Severity: Medium]
Should cix_dp_bridge_mode_set() run under dp->session_lock and after
cix_dp_prepare() has read the sink's DPCD?

cix_dp_bridge_atomic_enable() calls cix_dp_bridge_mode_set() unlocked before
dp->ops->encoder_enable() -> cix_dp_prepare().  This mutates
dp->connector.config, dp->pixel_per_cycle, and dp->mode without holding
dp->session_lock (racing with cix_dp_hpd_irq_work_func() ->
cix_dp_train_loop()), and computes link_rate and lane_cnt from
dp->link_config before cix_dp_prepare() -> cix_dp_handle_connect() /
cix_dp_core_on() updates dp->link_config from the sink's DPCD.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109092623.102291-1-Luo.Liu@cixtech.com?part=2