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.
Thanks,
Timur
> > >
> > > Signed-off-by: Timur Kristóf <timur.kris...@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:
> >
