Skip to content

Commit

Permalink
drm/amdgpu/dm/mst: Use the correct topology mgr pointer in amdgpu_dm_…
Browse files Browse the repository at this point in the history
…connector

This bug hurt me. Basically, it appears that we've been grabbing the
entirely wrong mutex in the MST DSC computation code for amdgpu! While
we've been grabbing:

  amdgpu_dm_connector->mst_mgr

That's zero-initialized memory, because the only connectors we'll ever
actually be doing DSC computations for are MST ports. Which have mst_mgr
zero-initialized, and instead have the correct topology mgr pointer located
at:

  amdgpu_dm_connector->mst_port->mgr;

I'm a bit impressed that until now, this code has managed not to crash
anyone's systems! It does seem to cause a warning in LOCKDEP though:

  [   66.637670] DEBUG_LOCKS_WARN_ON(lock->magic != lock)

This was causing the problems that appeared to have been introduced by:

  commit 4d07b0b ("drm/display/dp_mst: Move all payload info into the atomic state")

This wasn't actually where they came from though. Presumably, before the
only thing we were doing with the topology mgr pointer was attempting to
grab mst_mgr->lock. Since the above commit however, we grab much more
information from mst_mgr including the atomic MST state and respective
modesetting locks.

This patch also implies that up until now, it's quite likely we could be
susceptible to race conditions when going through the MST topology state
for DSC computations since we technically will not have grabbed any lock
when going through it.

So, let's fix this by adjusting all the respective code paths to look at
the right pointer and skip things that aren't actual MST connectors from a
topology.

Gitlab issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171

Signed-off-by: Lyude Paul <[email protected]>
Fixes: 8c20a1e ("drm/amd/display: MST DSC compute fair share")
Cc: <[email protected]> # v5.6+
Reviewed-by: Wayne Lin <[email protected]>
Signed-off-by: Alex Deucher <[email protected]>
  • Loading branch information
Lyude authored and alexdeucher committed Nov 21, 2022
1 parent 2f3a127 commit dfbc004
Showing 1 changed file with 18 additions and 19 deletions.
37 changes: 18 additions & 19 deletions drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
Original file line number Diff line number Diff line change
Expand Up @@ -1117,6 +1117,7 @@ int compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,
struct dc_stream_state *stream;
bool computed_streams[MAX_PIPES];
struct amdgpu_dm_connector *aconnector;
struct drm_dp_mst_topology_mgr *mst_mgr;
int link_vars_start_index = 0;
int ret = 0;

Expand All @@ -1131,7 +1132,7 @@ int compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,

aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;

if (!aconnector || !aconnector->dc_sink)
if (!aconnector || !aconnector->dc_sink || !aconnector->port)
continue;

if (!aconnector->dc_sink->dsc_caps.dsc_dec_caps.is_dsc_supported)
Expand All @@ -1146,16 +1147,13 @@ int compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,
if (!is_dsc_need_re_compute(state, dc_state, stream->link))
continue;

mutex_lock(&aconnector->mst_mgr.lock);

ret = compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars,
&aconnector->mst_mgr,
mst_mgr = aconnector->port->mgr;
mutex_lock(&mst_mgr->lock);
ret = compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars, mst_mgr,
&link_vars_start_index);
if (ret != 0) {
mutex_unlock(&aconnector->mst_mgr.lock);
mutex_unlock(&mst_mgr->lock);
if (ret != 0)
return ret;
}
mutex_unlock(&aconnector->mst_mgr.lock);

for (j = 0; j < dc_state->stream_count; j++) {
if (dc_state->streams[j]->link == stream->link)
Expand All @@ -1182,6 +1180,7 @@ static int pre_compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,
struct dc_stream_state *stream;
bool computed_streams[MAX_PIPES];
struct amdgpu_dm_connector *aconnector;
struct drm_dp_mst_topology_mgr *mst_mgr;
int link_vars_start_index = 0;
int ret;

Expand All @@ -1196,7 +1195,7 @@ static int pre_compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,

aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;

if (!aconnector || !aconnector->dc_sink)
if (!aconnector || !aconnector->dc_sink || !aconnector->port)
continue;

if (!aconnector->dc_sink->dsc_caps.dsc_dec_caps.is_dsc_supported)
Expand All @@ -1208,15 +1207,13 @@ static int pre_compute_mst_dsc_configs_for_state(struct drm_atomic_state *state,
if (!is_dsc_need_re_compute(state, dc_state, stream->link))
continue;

mutex_lock(&aconnector->mst_mgr.lock);
ret = compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars,
&aconnector->mst_mgr,
mst_mgr = aconnector->port->mgr;
mutex_lock(&mst_mgr->lock);
ret = compute_mst_dsc_configs_for_link(state, dc_state, stream->link, vars, mst_mgr,
&link_vars_start_index);
if (ret != 0) {
mutex_unlock(&aconnector->mst_mgr.lock);
mutex_unlock(&mst_mgr->lock);
if (ret != 0)
return ret;
}
mutex_unlock(&aconnector->mst_mgr.lock);

for (j = 0; j < dc_state->stream_count; j++) {
if (dc_state->streams[j]->link == stream->link)
Expand Down Expand Up @@ -1419,6 +1416,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
unsigned int upper_link_bw_in_kbps = 0, down_link_bw_in_kbps = 0;
unsigned int max_compressed_bw_in_kbps = 0;
struct dc_dsc_bw_range bw_range = {0};
struct drm_dp_mst_topology_mgr *mst_mgr;

/*
* check if the mode could be supported if DSC pass-through is supported
Expand All @@ -1427,7 +1425,8 @@ enum dc_status dm_dp_mst_is_port_support_mode(
*/
if (is_dsc_common_config_possible(stream, &bw_range) &&
aconnector->port->passthrough_aux) {
mutex_lock(&aconnector->mst_mgr.lock);
mst_mgr = aconnector->port->mgr;
mutex_lock(&mst_mgr->lock);

cur_link_settings = stream->link->verified_link_cap;

Expand All @@ -1440,7 +1439,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
end_to_end_bw_in_kbps = min(upper_link_bw_in_kbps,
down_link_bw_in_kbps);

mutex_unlock(&aconnector->mst_mgr.lock);
mutex_unlock(&mst_mgr->lock);

/*
* use the maximum dsc compression bandwidth as the required
Expand Down

0 comments on commit dfbc004

Please sign in to comment.