[PATCH 13/20] drm/amd/display: Add analog link detection
Harry Wentland
harry.wentland at amd.com
Fri Aug 8 14:03:01 UTC 2025
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