On Fri, 2025-08-08 at 14:22 +0000, Wheeler, Daniel wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > The NULL pointer happens right after this error -> "[ 10.046542] > amdgpu 0000:c4:00.0: amdgpu: [drm] *ERROR* KMS: Failed to detect > connector" and only happens with this error. When bisecting the > patchset I was getting just the error without the NULL pointer, and > after removing "drm/amd/display: Add analog link detection" there was > no NULL pointer or error. Probably just need to solve that error and > it'd be good to go. > > > It looks like it's an APU issue as I was able to reproduce this on a > system with an AMD Radeon 8060S and eDP or DP, and yes, the NULL > pointer happens at driver load. > > Thank you, > > Dan Wheeler > Sr. Technologist | AMD > SW Display
Hi Daniel, Thanks for the reply. I appreciate the testing. I'll investigate this and try to fix it in the second version of the series. Do you think it could be reproducible on a Rembrandt APU? Or did you already test that? Upon looking at the patch again, I find it unlikely that pipe_ctx- >stream would be NULL, since the same was already accessed by other branches in the same function. My current best guess is that maybe link->link_enc would be NULL in the link_detect_analog function. In the meantime, can I ask you guys to also take a look at my other series with DCE 6 related fixes? Thanks, Timur > --------------------------------------------------------------------- > --------------------------------------------- > 1 Commerce Valley Dr E, Thornhill, ON L3T 7X6 > amd.com > > > -----Original Message----- > From: Wentland, Harry <[email protected]> > Sent: Friday, August 8, 2025 10:03 AM > To: Timur Kristóf <[email protected]>; > [email protected]; Wheeler, Daniel > <[email protected]> > Subject: Re: [PATCH 13/20] drm/amd/display: Add analog link detection > > > > On 2025-08-07 17:32, Timur Kristóf wrote: > > On Thu, 2025-08-07 at 16:34 -0400, Harry Wentland wrote: > > > > > > > > > On 2025-08-07 15:12, Harry Wentland wrote: > > > > On 2025-07-23 11:58, Timur Kristóf wrote: > > > > > Analog displays typically have a DDC connection which can be > > > > > used > > > > > by the GPU to read EDID. This commit adds the capability to > > > > > probe > > > > > analog displays using DDC, reading the EDID header and > > > > > deciding > > > > > whether the analog link is connected based on the data that > > > > > was > > > > > read. > > > > > > > > > > As a reference, I used the following functions: > > > > > amdgpu_connector_vga_detect > > > > > amdgpu_display_ddc_probe > > > > > > > > > > DAC load detection will be implemented in a separate commit. > > > > > > > > Another regression in our internal testing with this patch, > > > > unfortunately only on not-yet released HW. > > > > > > > > > > While this shows on unreleased HW I wouldn't be surprised if it > > > repros on other (recent-ish) APUs (integrated GPUs). It's just > > > that this week's test was on currently unreleased HW. > > > > > > Harry > > > > > > > I wonder if pipe-ctx->stream could be NULL in some cases. > > > > > > > > Harry > > > > > > > > Hi Harry, > > > > Can you elaborate when / how it is valid for pipe->ctx->stream to > > be > > NULL when the code gets here? Maybe that would give me a hint how > > to > > resolve it. > > > > I don't know. It was just a guess. > > I should've mentioned... the NULL pointer access happens on driver > load. > > Dan might have more info. > > Harry > > > Thanks, > > Timur > > > > > > > > > > > > > > Signed-off-by: Timur Kristóf <[email protected]> > > > > > --- > > > > > .../amd/display/dc/link/hwss/link_hwss_dio.c | 16 ++-- > > > > > .../drm/amd/display/dc/link/link_detection.c | 80 > > > > > ++++++++++++++++++- > > > > > .../gpu/drm/amd/display/dc/link/link_dpms.c | 3 + > > > > > .../drm/amd/display/dc/link/link_factory.c | 3 + > > > > > 4 files changed, 95 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git > > > > > a/drivers/gpu/drm/amd/display/dc/link/hwss/link_hwss_dio.c > > > > > b/drivers/gpu/drm/amd/display/dc/link/hwss/link_hwss_dio.c > > > > > index f3470716734d..b9ebb992dc98 100644 > > > > > --- > > > > > a/drivers/gpu/drm/amd/display/dc/link/hwss/link_hwss_dio.c > > > > > +++ > > > > > b/drivers/gpu/drm/amd/display/dc/link/hwss/link_hwss_dio.c > > > > > @@ -58,8 +58,9 @@ void setup_dio_stream_encoder(struct > > > > > pipe_ctx > > > > > *pipe_ctx) > > > > > return; > > > > > } > > > > > > > > > > - link_enc->funcs->connect_dig_be_to_fe(link_enc, > > > > > - pipe_ctx->stream_res.stream_enc->id, > > > > > true); > > > > > + if (!dc_is_rgb_signal(pipe_ctx->stream->signal)) > > > > > + 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)) > > > > > pipe_ctx->stream->ctx->dc->link_srv- > > > > > > dp_trace_source_sequence(pipe_ctx->stream->link, > > > > > DPCD_SOURCE_SEQ_AFTER_CONNECT_DI > > > > > G_FE_BE); > > > > > @@ -98,10 +99,13 @@ void reset_dio_stream_encoder(struct > > > > > pipe_ctx > > > > > *pipe_ctx) > > > > > if (stream_enc->funcs->enable_stream) > > > > > stream_enc->funcs->enable_stream(stream_enc, > > > > > pipe_ctx->stream->signal, > > > > > false); > > > > > - link_enc->funcs->connect_dig_be_to_fe( > > > > > - link_enc, > > > > > - pipe_ctx->stream_res.stream_enc->id, > > > > > - false); > > > > > + > > > > > + if (!dc_is_rgb_signal(pipe_ctx->stream->signal)) > > > > > + link_enc->funcs->connect_dig_be_to_fe( > > > > > + link_enc, > > > > > + pipe_ctx->stream_res.stream_enc- > > > > > > id, > > > > > + false); > > > > > + > > > > > if (dc_is_dp_signal(pipe_ctx->stream->signal)) > > > > > pipe_ctx->stream->ctx->dc->link_srv- > > > > > > dp_trace_source_sequence( > > > > > pipe_ctx->stream->link, > > > > > diff --git > > > > > a/drivers/gpu/drm/amd/display/dc/link/link_detection.c > > > > > b/drivers/gpu/drm/amd/display/dc/link/link_detection.c > > > > > index 827b630daf49..fcabc83464af 100644 > > > > > --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c > > > > > +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c > > > > > @@ -942,6 +942,12 @@ static bool > > > > > detect_link_and_local_sink(struct dc_link *link, > > > > > break; > > > > > } > > > > > > > > > > + case SIGNAL_TYPE_RGB: { > > > > > + sink_caps.transaction_type = > > > > > DDC_TRANSACTION_TYPE_I2C; > > > > > + sink_caps.signal = SIGNAL_TYPE_RGB; > > > > > + break; > > > > > + } > > > > > + > > > > > case SIGNAL_TYPE_LVDS: { > > > > > sink_caps.transaction_type = > > > > > DDC_TRANSACTION_TYPE_I2C; > > > > > sink_caps.signal = SIGNAL_TYPE_LVDS; > > > > > @@ -1133,9 +1139,17 @@ static bool > > > > > detect_link_and_local_sink(struct dc_link *link, > > > > > sink = prev_sink; > > > > > prev_sink = NULL; > > > > > } > > > > > - query_hdcp_capability(sink->sink_signal, > > > > > link); > > > > > + > > > > > + if (!sink->edid_caps.analog) > > > > > + query_hdcp_capability(sink- > > > > > > sink_signal, link); > > > > > } > > > > > > > > > > + /* DVI-I connector connected to analog display. > > > > > */ > > > > > + if ((link->link_enc->connector.id == > > > > > CONNECTOR_ID_DUAL_LINK_DVII || > > > > > + link->link_enc->connector.id == > > > > > CONNECTOR_ID_SINGLE_LINK_DVII) && > > > > > + sink->edid_caps.analog) > > > > > + sink->sink_signal = SIGNAL_TYPE_RGB; > > > > > + > > > > > /* HDMI-DVI Dongle */ > > > > > if (sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A > > > > > && > > > > > !sink->edid_caps.edid_hdmi) > > > > > @@ -1228,6 +1242,64 @@ static bool > > > > > detect_link_and_local_sink(struct dc_link *link, > > > > > return true; > > > > > } > > > > > > > > > > +/** > > > > > + * Evaluates whether an EDID header is acceptable, > > > > > + * for the purpose of determining a connection with a > > > > > display. > > > > > + */ > > > > > +static bool link_detect_evaluate_edid_header(uint8_t > > > > > edid_header[8]) > > > > > +{ > > > > > + int edid_header_score = 0; > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < 8; ++i) > > > > > + edid_header_score += edid_header[i] == ((i == 0 > > > > > > > i == 7) ? 0x00 : 0xff); > > > > > + > > > > > + return edid_header_score >= 6; > > > > > +} > > > > > + > > > > > +/** > > > > > + * Tries to detect a connected display by probing the DDC > > > > > + * and reading the EDID header. > > > > > + * The probing is considered successful if we receive a > > > > > + * reply from the DDC over I2C and the EDID header matches. > > > > > + */ > > > > > +static bool link_detect_ddc_probe(struct dc_link *link) > > > > > +{ > > > > > + if (!link->ddc) > > > > > + return false; > > > > > + > > > > > + uint8_t edid_header[8] = {0}; > > > > > + bool ddc_probed = i2c_read(link->ddc, 0x50, edid_header, > > > > > sizeof(edid_header)); > > > > > + > > > > > + if (!ddc_probed) > > > > > + return false; > > > > > + > > > > > + if (!link_detect_evaluate_edid_header(edid_header)) > > > > > + return false; > > > > > + > > > > > + return true; > > > > > +} > > > > > + > > > > > +/** > > > > > + * Determines if there is an analog sink connected. > > > > > + */ > > > > > +static bool link_detect_analog(struct dc_link *link, enum > > > > > dc_connection_type *type) > > > > > +{ > > > > > + /* Don't care about connectors that don't support an > > > > > analog signal. */ > > > > > + if (link->link_enc->connector.id != CONNECTOR_ID_VGA && > > > > > + link->link_enc->connector.id != > > > > > CONNECTOR_ID_SINGLE_LINK_DVII && > > > > > + link->link_enc->connector.id != > > > > > CONNECTOR_ID_DUAL_LINK_DVII) > > > > > + return false; > > > > > + > > > > > + if (link_detect_ddc_probe(link)) { > > > > > + *type = dc_connection_single; > > > > > + return true; > > > > > + } > > > > > + > > > > > + *type = dc_connection_none; > > > > > + return true; > > > > > +} > > > > > + > > > > > /* > > > > > * link_detect_connection_type() - Determine if there is a > > > > > sink > > > > > connected > > > > > * > > > > > @@ -1238,6 +1310,7 @@ static bool > > > > > detect_link_and_local_sink(struct dc_link *link, > > > > > bool link_detect_connection_type(struct dc_link *link, enum > > > > > dc_connection_type *type) > > > > > { > > > > > uint32_t is_hpd_high = 0; > > > > > + bool supports_hpd = link->irq_source_hpd != > > > > > DC_IRQ_SOURCE_INVALID; > > > > > > > > > > if (link->connector_signal == SIGNAL_TYPE_LVDS) { > > > > > *type = dc_connection_single; > > > > > @@ -1261,6 +1334,8 @@ bool link_detect_connection_type(struct > > > > > dc_link *link, enum dc_connection_type * > > > > > return true; > > > > > } > > > > > > > > > > + if (!supports_hpd) > > > > > + return link_detect_analog(link, type); > > > > > > > > > > if (!query_hpd_status(link, &is_hpd_high)) > > > > > goto hpd_gpio_failure; > > > > > @@ -1269,6 +1344,9 @@ bool link_detect_connection_type(struct > > > > > dc_link *link, enum dc_connection_type * > > > > > *type = dc_connection_single; > > > > > /* TODO: need to do the actual detection */ > > > > > } else { > > > > > + if (link_detect_analog(link, type)) > > > > > + return true; > > > > > + > > > > > *type = dc_connection_none; > > > > > if (link->connector_signal == SIGNAL_TYPE_EDP) { > > > > > /* eDP is not connected, power down it > > > > > */ > > > > > diff --git a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c > > > > > b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c > > > > > index d6b7347c6c11..ac25d89a4148 100644 > > > > > --- a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c > > > > > +++ b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c > > > > > @@ -2256,6 +2256,9 @@ static enum dc_status enable_link( > > > > > enable_link_lvds(pipe_ctx); > > > > > status = DC_OK; > > > > > break; > > > > > + case SIGNAL_TYPE_RGB: > > > > > + status = DC_OK; > > > > > + break; > > > > > case SIGNAL_TYPE_VIRTUAL: > > > > > status = enable_link_virtual(pipe_ctx); > > > > > break; > > > > > diff --git > > > > > a/drivers/gpu/drm/amd/display/dc/link/link_factory.c > > > > > b/drivers/gpu/drm/amd/display/dc/link/link_factory.c > > > > > index 71c10a1261b9..c9725fd316f6 100644 > > > > > --- a/drivers/gpu/drm/amd/display/dc/link/link_factory.c > > > > > +++ b/drivers/gpu/drm/amd/display/dc/link/link_factory.c > > > > > @@ -555,6 +555,9 @@ static bool construct_phy(struct dc_link > > > > > *link, > > > > > case CONNECTOR_ID_DUAL_LINK_DVII: > > > > > link->connector_signal = > > > > > SIGNAL_TYPE_DVI_DUAL_LINK; > > > > > break; > > > > > + case CONNECTOR_ID_VGA: > > > > > + link->connector_signal = SIGNAL_TYPE_RGB; > > > > > + break; > > > > > case CONNECTOR_ID_DISPLAY_PORT: > > > > > case CONNECTOR_ID_MXM: > > > > > case CONNECTOR_ID_USBC: > > > >
