Re: [PATCH v8 13/29] drm/msm/dp: add MST connector creation and topology callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/msm/dp: best_encoder mutation in
  msm_dp_mst_atomic_best_encoder()
- [Medium] drm/msm/dp: bad return and unlocked PBN in
  msm_dp_mst_connector_mode_valid()
- [Medium] drm/msm/dp: ignored display_info parameter in
  msm_dp_display_mode_valid()
- [Low] drm/msm/dp: unchecked allocs and duplicate prop in
  msm_dp_mst_add_connector()
--

commit e6583a4899c42fcd8602aab1688349880400deb9
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: add MST connector creation and topology callbacks

This commit adds the add_connector topology callback and DRM connector
helper operations to create and manage dynamic connectors for DisplayPort
MST ports.

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index 5d22aa34c521c..a2900ff3b0ab3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c

[ ... ]

> +static enum drm_mode_status msm_dp_mst_connector_mode_valid(struct drm_connector *connector,
> +							    const struct drm_display_mode *mode)
> +{
> +	struct msm_dp_mst_connector *mst_conn;
> +	struct drm_dp_mst_port *mst_port;
> +	struct msm_dp *dp_display;
> +	int required_pbn;
> +
> +	if (drm_connector_is_unregistered(connector))
> +		return 0;
> +
> +	mst_conn = to_dp_mst_connector(connector);
> +	mst_port = mst_conn->mst_port;
> +	dp_display = mst_conn->dp_mst->msm_dp;
> +
> +	/* Use minimum possible bpp (6bpc RGB, 8b10b) to avoid rejecting modes
> +	 * that would be feasible at the actual link bpp. mode_valid must not
> +	 * depend on negotiated parameters.
> +	 */
> +	required_pbn = drm_dp_calc_pbn_mode(mode->clock, (6 * 3) << 4);
> +
> +	if (required_pbn > mst_port->full_pbn) {
> +		drm_dbg_dp(dp_display->drm_dev, "mode:%s not supported.\n", mode->name);
> +		return MODE_CLOCK_HIGH;
> +	}

[Severity: Medium]
Does returning 0 from a .mode_valid callback return MODE_OK instead of
MODE_ERROR when the connector is unregistered, accepting all modes without
checking PBN or display limits?

Also, mst_port->full_pbn is documented as protected by
drm_dp_mst_topology_mgr.base.lock, which is not held here (for example,
when connector->force is set in drm_helper_probe_single_connector_modes()
and detect_ctx is skipped).  Implementing .mode_valid_ctx instead of
.mode_valid would allow acquiring mst->mst_mgr.base.lock and setting
*status = MODE_ERROR when unregistered.

> +
> +	return msm_dp_display_mode_valid(dp_display, &connector->display_info, mode);
> +}

[Severity: Medium]
Does msm_dp_display_mode_valid() ignore the connector->display_info passed
here and use the root SST connector's display_info instead?

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_mode_valid() {
    ...
	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
    ...
	mode_bpp = dp->connector->display_info.bpc * num_components;
    ...
}

Because the MST sink's EDID updates the dynamic MST connector rather than
dp->connector, 4:2:0-only modes on MST sinks will not be recognized as
4:2:0 (skipping the vsc_sdp_supported check and failing to halve the pixel
clock, rejecting valid 4:2:0 modes) and mode_bpp will be computed from the
wrong connector's bpc.

[ ... ]

> +static struct drm_encoder *
> +msm_dp_mst_atomic_best_encoder(struct drm_connector *connector, struct drm_atomic_commit *state)
> +{
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *iter;
> +	struct drm_connector_list_iter conn_iter;
> +	u32 stream_mask = 0;
> +	u32 i;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	if (!conn_state)
> +		return NULL;
> +
> +	if (conn_state->best_encoder)
> +		return conn_state->best_encoder;
> +
> +	drm_connector_list_iter_begin(connector->dev, &conn_iter);
> +	drm_for_each_connector_iter(iter, &conn_iter) {
> +		struct drm_connector_state *peer_state;
> +		int stream_id;
> +
> +		if (iter == connector ||
> +		    iter->funcs != &msm_dp_drm_mst_connector_funcs ||
> +		    to_dp_mst_connector(iter)->dp_mst != mst)
> +			continue;
> +
> +		peer_state = drm_atomic_get_new_connector_state(state, iter) ?: iter->state;
> +		if (!peer_state || !peer_state->crtc || !peer_state->best_encoder)
> +			continue;
> +
> +		stream_id = msm_dp_mst_encoder_stream_id(mst, peer_state->best_encoder);
> +		if (stream_id >= 0 && stream_id < mst->max_streams)
> +			stream_mask |= BIT(stream_id);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	for (i = 0; i < mst->max_streams; i++) {
> +		if (!(stream_mask & BIT(i))) {
> +			conn_state->best_encoder = mst->mst_encoders[i].enc;
> +			return mst->mst_encoders[i].enc;
> +		}
> +	}
> +
> +	return NULL;
> +}

[Severity: High]
Does assigning conn_state->best_encoder inside .atomic_best_encoder break
encoder stealing in update_connector_routing()?

drm_atomic_helper_check_modeset() calls .atomic_best_encoder() first in
handle_conflicting_encoders() and then again in update_connector_routing():

drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() {
    ...
	if (new_encoder == new_connector_state->best_encoder) {
		set_best_encoder(state, new_connector_state, new_encoder);

		drm_dbg_atomic(connector->dev,
			       "[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d:%s]\n",
			       connector->base.id, connector->name,
			       new_encoder->base.id, new_encoder->name,
			       new_connector_state->crtc->base.id,
			       new_connector_state->crtc->name);

		return 0;
	}

	steal_encoder(state, new_encoder);
    ...
}

Because handle_conflicting_encoders() already overwrote
conn_state->best_encoder, new_encoder == new_connector_state->best_encoder
is always true in update_connector_routing(), which returns early without
calling steal_encoder().  When one atomic commit disables an MST connector
with a higher connector index and enables another with a lower index on the
same CRTC, the disabled connector retains its stale best_encoder until its
own update_connector_routing() call runs set_best_encoder(..., NULL) and
clears the newly assigned encoder bit from crtc_state->encoder_mask.
Letting the atomic helpers own conn_state->best_encoder (and only returning
the chosen drm_encoder pointer from .atomic_best_encoder) avoids this.

[ ... ]

> +static struct drm_connector *
> +msm_dp_mst_add_connector(struct drm_dp_mst_topology_mgr *mgr,
> +			 struct drm_dp_mst_port *port, const char *pathprop)
> +{
> +	struct msm_dp_mst *mst = container_of(mgr, struct msm_dp_mst, mst_mgr);
> +	struct drm_device *dev = mst->msm_dp->drm_dev;
> +	struct msm_dp_mst_connector *mst_conn;
> +	struct drm_connector *connector;
> +	int rc, i;
> +
> +	mst_conn = kzalloc_obj(*mst_conn);
> +	if (!mst_conn)
> +		return NULL;
> +
> +	connector = &mst_conn->connector;
> +	rc = drm_connector_dynamic_init(dev, connector,
> +					&msm_dp_drm_mst_connector_funcs,
> +					DRM_MODE_CONNECTOR_DisplayPort, NULL);
> +	if (rc)
> +		goto err_free;
> +
> +	mst_conn->dp_mst = mst;
> +
> +	drm_connector_helper_add(connector, &msm_dp_drm_mst_connector_helper_funcs);
> +	connector->funcs->reset(connector);
> +
> +	/* add all encoders as possible encoders */
> +	for (i = 0; i < mst->max_streams; i++) {
> +		rc = drm_connector_attach_encoder(connector, mst->mst_encoders[i].enc);
> +		if (rc) {
> +			drm_err(dev, "[MST] failed to attach encoder:%u to conn:%d rc:%d\n",
> +				mst->mst_encoders[i].enc->base.id,
> +				connector->base.id, rc);
> +			goto err_connector;
> +		}
> +	}
> +
> +	mst_conn->mst_port = port;
> +	drm_dp_mst_get_port_malloc(port);
> +
> +	drm_object_attach_property(&connector->base,
> +				   dev->mode_config.path_property, 0);
> +	drm_object_attach_property(&connector->base,
> +				   dev->mode_config.tile_property, 0);
> +	drm_connector_set_path_property(connector, pathprop);

[Severity: Low]
Does msm_dp_mst_add_connector() attach dev->mode_config.tile_property a
second time?  drm_connector_dynamic_init() -> drm_connector_init_only()
already attaches tile_property, so attaching it again creates a duplicate
property entry on connector->base.properties.

In addition, connector->funcs->reset(connector) allocates connector->state
with kzalloc_obj(), so if that allocation fails, connector->state is NULL
when the connector is registered and later dereferenced by atomic helpers.
The error return from drm_connector_set_path_property() is also unchecked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=13