Skip to content

Commit

Permalink
drm/amd/display: Fix dynamic link encoder access.
Browse files Browse the repository at this point in the history
[Why]
Assuming DIG link encoders are statically mapped to links can cause
system instability due to null pointer accesses.

[How]
- Add checks for non-null link encoder pointers before trying to access
them.
- When a hardware platform uses dynamic DIG assignment (i.e. resource
function 'link_encs_assign' defined) and a link supports flexible
mapping to DIGs, use the link_enc_cfg API to access the DIG assigned to
a link or stream.

Reviewed-by: Meenakshikumar Somasundaram <meenakshikumar.somasundaram@amd.com>
Acked-by: Mikita Lipski <mikita.lipski@amd.com>
Signed-off-by: Jimmy Kizito <Jimmy.Kizito@amd.com>
Tested-by: Daniel Wheeler <daniel.wheeler@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
  • Loading branch information
Jimmy Kizito authored and Alex Deucher committed Sep 14, 2021
1 parent 035f549 commit 64d283c
Show file tree
Hide file tree
Showing 12 changed files with 147 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ int dcn31_get_active_display_cnt_wa(
const struct dc_link *link = dc->links[i];

/* abusing the fact that the dig and phy are coupled to see if the phy is enabled */
if (link->link_enc->funcs->is_dig_enabled &&
if (link->link_enc && link->link_enc->funcs->is_dig_enabled &&
link->link_enc->funcs->is_dig_enabled(link->link_enc))
display_count++;
}
Expand Down
42 changes: 34 additions & 8 deletions drivers/gpu/drm/amd/display/dc/core/dc_link.c
Original file line number Diff line number Diff line change
Expand Up @@ -3457,6 +3457,10 @@ static enum dc_status deallocate_mst_payload(struct pipe_ctx *pipe_ctx)
static void update_psp_stream_config(struct pipe_ctx *pipe_ctx, bool dpms_off)
{
struct cp_psp *cp_psp = &pipe_ctx->stream->ctx->cp_psp;
#if defined(CONFIG_DRM_AMD_DC_DCN)
struct link_encoder *link_enc = NULL;
#endif

if (cp_psp && cp_psp->funcs.update_stream_config) {
struct cp_psp_stream_config config = {0};
enum dp_panel_mode panel_mode =
Expand All @@ -3468,8 +3472,21 @@ static void update_psp_stream_config(struct pipe_ctx *pipe_ctx, bool dpms_off)
config.dig_be = pipe_ctx->stream->link->link_enc_hw_inst;
#if defined(CONFIG_DRM_AMD_DC_DCN)
config.stream_enc_idx = pipe_ctx->stream_res.stream_enc->id - ENGINE_ID_DIGA;
config.link_enc_idx = pipe_ctx->stream->link->link_enc->transmitter - TRANSMITTER_UNIPHY_A;
config.phy_idx = pipe_ctx->stream->link->link_enc->transmitter - TRANSMITTER_UNIPHY_A;
if (pipe_ctx->stream->link->ep_type == DISPLAY_ENDPOINT_PHY) {
link_enc = pipe_ctx->stream->link->link_enc;
config.phy_idx = link_enc->transmitter - TRANSMITTER_UNIPHY_A;
} else if (pipe_ctx->stream->link->dc->res_pool->funcs->link_encs_assign) {
/* Use link encoder assignment from current DC state - which may differ from the DC state to be
* committed - when updating PSP config.
*/
link_enc = link_enc_cfg_get_link_enc_used_by_stream(
pipe_ctx->stream->link->dc->current_state,
pipe_ctx->stream);
config.phy_idx = 0; /* Clear phy_idx for non-physical display endpoints. */
}
ASSERT(link_enc);
if (link_enc)
config.link_enc_idx = link_enc->transmitter - TRANSMITTER_UNIPHY_A;
if (is_dp_128b_132b_signal(pipe_ctx)) {
config.stream_enc_idx = pipe_ctx->stream_res.hpo_dp_stream_enc->id - ENGINE_ID_HPO_DP_0;
config.link_enc_idx = pipe_ctx->stream->link->hpo_dp_link_enc->inst;
Expand Down Expand Up @@ -3576,6 +3593,7 @@ void core_link_enable_stream(
struct dc_stream_state *stream = pipe_ctx->stream;
struct dc_link *link = stream->sink->link;
enum dc_status status;
struct link_encoder *link_enc;
#if defined(CONFIG_DRM_AMD_DC_DCN)
enum otg_out_mux_dest otg_out_dest = OUT_MUX_DIO;
#endif
Expand All @@ -3585,15 +3603,22 @@ void core_link_enable_stream(
dc_is_virtual_signal(pipe_ctx->stream->signal))
return;

if (dc->res_pool->funcs->link_encs_assign && stream->link->ep_type != DISPLAY_ENDPOINT_PHY)
link_enc = stream->link_enc;
else
link_enc = stream->link->link_enc;
ASSERT(link_enc);

#if defined(CONFIG_DRM_AMD_DC_DCN)
if (!dc_is_virtual_signal(pipe_ctx->stream->signal)
&& !is_dp_128b_132b_signal(pipe_ctx)) {
#else
if (!dc_is_virtual_signal(pipe_ctx->stream->signal)) {
#endif
stream->link->link_enc->funcs->setup(
stream->link->link_enc,
pipe_ctx->stream->signal);
if (link_enc)
link_enc->funcs->setup(
link_enc,
pipe_ctx->stream->signal);
pipe_ctx->stream_res.stream_enc->funcs->setup_stereo_sync(
pipe_ctx->stream_res.stream_enc,
pipe_ctx->stream_res.tg->inst,
Expand Down Expand Up @@ -3748,9 +3773,10 @@ void core_link_enable_stream(
#else
if (!dc_is_virtual_signal(pipe_ctx->stream->signal))
#endif
stream->link->link_enc->funcs->setup(
stream->link->link_enc,
pipe_ctx->stream->signal);
if (link_enc)
link_enc->funcs->setup(
link_enc,
pipe_ctx->stream->signal);

dc->hwss.enable_stream(pipe_ctx);

Expand Down
36 changes: 32 additions & 4 deletions drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
Original file line number Diff line number Diff line change
Expand Up @@ -2624,13 +2624,27 @@ static enum dc_link_rate get_lttpr_max_link_rate(struct dc_link *link)

bool dc_link_dp_get_max_link_enc_cap(const struct dc_link *link, struct dc_link_settings *max_link_enc_cap)
{
struct link_encoder *link_enc = NULL;

if (!max_link_enc_cap) {
DC_LOG_ERROR("%s: Could not return max link encoder caps", __func__);
return false;
}

if (link->link_enc->funcs->get_max_link_cap) {
link->link_enc->funcs->get_max_link_cap(link->link_enc, max_link_enc_cap);
/* Links supporting dynamically assigned link encoder will be assigned next
* available encoder if one not already assigned.
*/
if (link->is_dig_mapping_flexible &&
link->dc->res_pool->funcs->link_encs_assign) {
link_enc = link_enc_cfg_get_link_enc_used_by_link(link->dc->current_state, link);
if (link_enc == NULL)
link_enc = link_enc_cfg_get_next_avail_link_enc(link->dc, link->dc->current_state);
} else
link_enc = link->link_enc;
ASSERT(link_enc);

if (link_enc && link_enc->funcs->get_max_link_cap) {
link_enc->funcs->get_max_link_cap(link_enc, max_link_enc_cap);
return true;
}

Expand All @@ -2646,9 +2660,23 @@ static struct dc_link_settings get_max_link_cap(struct dc_link *link)
#if defined(CONFIG_DRM_AMD_DC_DCN)
enum dc_link_rate lttpr_max_link_rate;
#endif
struct link_encoder *link_enc = NULL;

/* Links supporting dynamically assigned link encoder will be assigned next
* available encoder if one not already assigned.
*/
if (link->is_dig_mapping_flexible &&
link->dc->res_pool->funcs->link_encs_assign) {
link_enc = link_enc_cfg_get_link_enc_used_by_link(link->dc->current_state, link);
if (link_enc == NULL)
link_enc = link_enc_cfg_get_next_avail_link_enc(link->dc, link->dc->current_state);
} else
link_enc = link->link_enc;
ASSERT(link_enc);

/* get max link encoder capability */
link->link_enc->funcs->get_max_link_cap(link->link_enc, &max_link_cap);
if (link_enc)
link_enc->funcs->get_max_link_cap(link_enc, &max_link_cap);
#if defined(CONFIG_DRM_AMD_DC_DCN)
if (max_link_cap.link_rate >= LINK_RATE_UHBR10 &&
!link->hpo_dp_link_enc)
Expand Down Expand Up @@ -2867,7 +2895,7 @@ bool dp_verify_link_cap(
* PHY will sometimes be in bad state on hotplugging display from certain USB-C dongle,
* so add extra cycle of enabling and disabling the PHY before first link training.
*/
if (link->link_enc->features.flags.bits.DP_IS_USB_C &&
if (link->link_enc && link->link_enc->features.flags.bits.DP_IS_USB_C &&
link->dc->debug.usbc_combo_phy_reset_wa) {
dp_enable_link_phy(link, link->connector_signal, dp_cs_id, cur);
dp_disable_link_phy(link, link->connector_signal);
Expand Down
25 changes: 15 additions & 10 deletions drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ static struct dc_stream_state *get_stream_using_link_enc(
for (i = 0; i < state->stream_count; i++) {
struct link_enc_assignment assignment = state->res_ctx.link_enc_assignments[i];

if (assignment.valid && (assignment.eng_id == eng_id)) {
if ((assignment.valid == true) && (assignment.eng_id == eng_id)) {
stream_idx = i;
break;
}
Expand Down Expand Up @@ -254,7 +254,7 @@ struct dc_link *link_enc_cfg_get_link_using_link_enc(
for (i = 0; i < state->stream_count; i++) {
struct link_enc_assignment assignment = state->res_ctx.link_enc_assignments[i];

if (assignment.valid && (assignment.eng_id == eng_id)) {
if ((assignment.valid == true) && (assignment.eng_id == eng_id)) {
stream_idx = i;
break;
}
Expand All @@ -274,7 +274,6 @@ struct link_encoder *link_enc_cfg_get_link_enc_used_by_link(
{
struct link_encoder *link_enc = NULL;
struct display_endpoint_id ep_id;
int stream_idx = -1;
int i;

ep_id = (struct display_endpoint_id) {
Expand All @@ -283,20 +282,15 @@ struct link_encoder *link_enc_cfg_get_link_enc_used_by_link(

for (i = 0; i < state->stream_count; i++) {
struct link_enc_assignment assignment = state->res_ctx.link_enc_assignments[i];

if (assignment.valid &&
if (assignment.valid == true &&
assignment.ep_id.link_id.id == ep_id.link_id.id &&
assignment.ep_id.link_id.enum_id == ep_id.link_id.enum_id &&
assignment.ep_id.link_id.type == ep_id.link_id.type &&
assignment.ep_id.ep_type == ep_id.ep_type) {
stream_idx = i;
break;
link_enc = link->dc->res_pool->link_encoders[assignment.eng_id - ENGINE_ID_DIGA];
}
}

if (stream_idx != -1)
link_enc = state->streams[stream_idx]->link_enc;

return link_enc;
}

Expand All @@ -313,3 +307,14 @@ struct link_encoder *link_enc_cfg_get_next_avail_link_enc(

return link_enc;
}

struct link_encoder *link_enc_cfg_get_link_enc_used_by_stream(
struct dc_state *state,
const struct dc_stream_state *stream)
{
struct link_encoder *link_enc;

link_enc = link_enc_cfg_get_link_enc_used_by_link(state, stream->link);

return link_enc;
}
7 changes: 4 additions & 3 deletions drivers/gpu/drm/amd/display/dc/core/dc_link_hwss.c
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,10 @@ void dp_retrain_link_dp_test(struct dc_link *link,
if ((&pipes[i])->stream_res.audio && !link->dc->debug.az_endpoint_mute_only)
(&pipes[i])->stream_res.audio->funcs->az_disable((&pipes[i])->stream_res.audio);

link->link_enc->funcs->disable_output(
link->link_enc,
SIGNAL_TYPE_DISPLAY_PORT);
if (link->link_enc)
link->link_enc->funcs->disable_output(
link->link_enc,
SIGNAL_TYPE_DISPLAY_PORT);

/* Clear current link setting. */
memset(&link->cur_link_settings, 0,
Expand Down
3 changes: 2 additions & 1 deletion drivers/gpu/drm/amd/display/dc/core/dc_resource.c
Original file line number Diff line number Diff line change
Expand Up @@ -2970,7 +2970,8 @@ enum dc_status dc_validate_stream(struct dc *dc, struct dc_stream_state *stream)
res = DC_FAIL_CONTROLLER_VALIDATE;

if (res == DC_OK) {
if (!link->link_enc->funcs->validate_output_with_stream(
if (link->ep_type == DISPLAY_ENDPOINT_PHY &&
!link->link_enc->funcs->validate_output_with_stream(
link->link_enc, stream))
res = DC_FAIL_ENC_VALIDATE;
}
Expand Down
22 changes: 17 additions & 5 deletions drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include "transform.h"
#include "stream_encoder.h"
#include "link_encoder.h"
#include "link_enc_cfg.h"
#include "link_hwss.h"
#include "dc_link_dp.h"
#if defined(CONFIG_DRM_AMD_DC_DCN)
Expand Down Expand Up @@ -1192,6 +1193,7 @@ void dce110_disable_stream(struct pipe_ctx *pipe_ctx)
struct dc_stream_state *stream = pipe_ctx->stream;
struct dc_link *link = stream->link;
struct dc *dc = pipe_ctx->stream->ctx->dc;
struct link_encoder *link_enc = NULL;

if (dc_is_hdmi_tmds_signal(pipe_ctx->stream->signal)) {
pipe_ctx->stream_res.stream_enc->funcs->stop_hdmi_info_packets(
Expand All @@ -1213,20 +1215,29 @@ void dce110_disable_stream(struct pipe_ctx *pipe_ctx)

dc->hwss.disable_audio_stream(pipe_ctx);

/* Link encoder may have been dynamically assigned to non-physical display endpoint. */
if (link->ep_type == DISPLAY_ENDPOINT_PHY)
link_enc = link->link_enc;
else if (dc->res_pool->funcs->link_encs_assign)
link_enc = link_enc_cfg_get_link_enc_used_by_link(link->dc->current_state, link);
ASSERT(link_enc);

#if defined(CONFIG_DRM_AMD_DC_DCN)
if (is_dp_128b_132b_signal(pipe_ctx)) {
pipe_ctx->stream_res.hpo_dp_stream_enc->funcs->disable(
pipe_ctx->stream_res.hpo_dp_stream_enc);
setup_dp_hpo_stream(pipe_ctx, false);
/* TODO - DP2.0 HW: unmap stream from link encoder here */
} else {
link->link_enc->funcs->connect_dig_be_to_fe(
link->link_enc,
if (link_enc)
link_enc->funcs->connect_dig_be_to_fe(
link_enc,
pipe_ctx->stream_res.stream_enc->id,
false);
}
#else
link->link_enc->funcs->connect_dig_be_to_fe(
if (link_enc)
link_enc->funcs->connect_dig_be_to_fe(
link->link_enc,
pipe_ctx->stream_res.stream_enc->id,
false);
Expand Down Expand Up @@ -1666,8 +1677,9 @@ static void power_down_encoders(struct dc *dc)
if (signal != SIGNAL_TYPE_EDP)
signal = SIGNAL_TYPE_NONE;

dc->links[i]->link_enc->funcs->disable_output(
dc->links[i]->link_enc, signal);
if (dc->links[i]->ep_type == DISPLAY_ENDPOINT_PHY)
dc->links[i]->link_enc->funcs->disable_output(
dc->links[i]->link_enc, signal);

dc->links[i]->link_status.link_active = false;
memset(&dc->links[i]->cur_link_settings, 0,
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
Original file line number Diff line number Diff line change
Expand Up @@ -1296,7 +1296,7 @@ struct stream_encoder *dcn10_find_first_free_match_stream_enc_for_link(
* in daisy chain use case
*/
j = i;
if (pool->stream_enc[i]->id ==
if (link->ep_type == DISPLAY_ENDPOINT_PHY && pool->stream_enc[i]->id ==
link->link_enc->preferred_engine)
return pool->stream_enc[i];
}
Expand Down
13 changes: 10 additions & 3 deletions drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
Original file line number Diff line number Diff line change
Expand Up @@ -2381,6 +2381,13 @@ void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
uint32_t active_total_with_borders;
uint32_t early_control = 0;
struct timing_generator *tg = pipe_ctx->stream_res.tg;
struct link_encoder *link_enc;

if (link->is_dig_mapping_flexible &&
link->dc->res_pool->funcs->link_encs_assign)
link_enc = pipe_ctx->stream->link_enc;
else
link_enc = link->link_enc;

/* For MST, there are multiply stream go to only one link.
* connect DIG back_end to front_end while enable_stream and
Expand All @@ -2397,9 +2404,9 @@ void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
link->hpo_dp_link_enc->inst);
}

if (!is_dp_128b_132b_signal(pipe_ctx))
link->link_enc->funcs->connect_dig_be_to_fe(
link->link_enc, pipe_ctx->stream_res.stream_enc->id, true);
if (!is_dp_128b_132b_signal(pipe_ctx) && link_enc)
link_enc->funcs->connect_dig_be_to_fe(
link_enc, pipe_ctx->stream_res.stream_enc->id, true);

if (dc_is_dp_signal(pipe_ctx->stream->signal))
dp_source_sequence_trace(link, DPCD_SOURCE_SEQ_AFTER_CONNECT_DIG_FE_BE);
Expand Down
20 changes: 19 additions & 1 deletion drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
#include "dce/dce_aux.h"
#include "dce/dce_i2c.h"
#include "vm_helper.h"
#include "link_enc_cfg.h"

#include "amdgpu_socbb.h"

Expand Down Expand Up @@ -1596,12 +1597,29 @@ static void get_pixel_clock_parameters(
const struct dc_stream_state *stream = pipe_ctx->stream;
struct pipe_ctx *odm_pipe;
int opp_cnt = 1;
struct dc_link *link = stream->link;
struct link_encoder *link_enc = NULL;

for (odm_pipe = pipe_ctx->next_odm_pipe; odm_pipe; odm_pipe = odm_pipe->next_odm_pipe)
opp_cnt++;

pixel_clk_params->requested_pix_clk_100hz = stream->timing.pix_clk_100hz;
pixel_clk_params->encoder_object_id = stream->link->link_enc->id;

/* Links supporting dynamically assigned link encoder will be assigned next
* available encoder if one not already assigned.
*/
if (link->is_dig_mapping_flexible &&
link->dc->res_pool->funcs->link_encs_assign) {
link_enc = link_enc_cfg_get_link_enc_used_by_stream(stream->link->dc->current_state, stream);
if (link_enc == NULL)
link_enc = link_enc_cfg_get_next_avail_link_enc(stream->link->dc,
stream->link->dc->current_state);
} else
link_enc = stream->link->link_enc;
ASSERT(link_enc);

if (link_enc)
pixel_clk_params->encoder_object_id = link_enc->id;
pixel_clk_params->signal_type = pipe_ctx->stream->signal;
pixel_clk_params->controller_id = pipe_ctx->stream_res.tg->inst + 1;
/* TODO: un-hardcode*/
Expand Down
Loading

0 comments on commit 64d283c

Please sign in to comment.