[PATCH 13/20] drm/amd/display: Add analog link detection
Wheeler, Daniel
Daniel.Wheeler at amd.com
Fri Aug 15 14:28:25 UTC 2025
[Public]
Hi Timur,
Sorry for the delay. It took me a bit to find my Rembrandt system. I tried booting up with your patches, and the system hard hangs after kernel load - I can't get into it via ssh. The error message doesn't seem like it would only be contained to one generation, so I wouldn't be surprised if this is the same issue as the newer systems, just presenting a different way.
Thank you,
Dan Wheeler
Sr. Technologist | AMD
SW Display
------------------------------------------------------------------------------------------------------------------
1 Commerce Valley Dr E, Thornhill, ON L3T 7X6
amd.com
-----Original Message-----
From: Timur Kristóf <timur.kristof at gmail.com>
Sent: Saturday, August 9, 2025 4:17 PM
To: Wheeler, Daniel <Daniel.Wheeler at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 13/20] drm/amd/display: Add analog link detection
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 <Harry.Wentland at amd.com>
> Sent: Friday, August 8, 2025 10:03 AM
> To: Timur Kristóf <timur.kristof at gmail.com>;
> amd-gfx at lists.freedesktop.org; Wheeler, Daniel
> <Daniel.Wheeler at amd.com>
> 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 <timur.kristof at gmail.com>
> > > > > ---
> > > > > .../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:
> > > >
More information about the amd-gfx
mailing list