[PATCH 13/20] drm/amd/display: Add analog link detection
Timur Kristóf
timur.kristof at gmail.com
Wed Aug 20 12:01:09 UTC 2025
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,
I've managed to take a deeper look at this. I think there were actually
two problems in this patch:
1. NULL pointer dereference:
Seems to be caused by the link encoder not being initialized, although
I'm not fully sure how that happens, but I changed the parts that
accessed link_enc, and it no longer crashes now.
2. HPD pin issue:
In link_detect_connection_type() the DC code checks the status of the
HPD pins and returns false when HPD isn't supported. My patch changed
this to take the same code path as if the HPD pins were low. This seems
to have broken something and resulted in a black screen on Rembrandt.
It seems that some other part of the code expects that the link
detection fails when HPD isn't supported. So I fixed this by special
casing VGA connectors to allow link detection just on VGA to succeed
when there are no HPD pins, but left the other code path as-is.
Does that sound reasonable to you?
Thanks & best regards,
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