Re: [PATCH 21/24] drm/amd/display: Retry link detection on hotplug

["Timur Kristóf" <timur.kristof@gmail.com>]

On Friday, June 5, 2026 6:10:28 PM Central European Summer Time Zuo, Jerry 
wrote:
> AMD General
> 
> Hi Timur:
> 
>      Please let me know whether you have validated the sequence by any SST
> and HDMI monitor. Basically it is to confirm the link retry workqueue is
> getting executed with hotplug and dpms use cases without introducing side
> effect. We've tested locally, but we don't see any link retry workqueue is
> getting executed by either SST or HDMI monitors. That makes us hard to
> validate the new error handling logic. It would be perfect that if you have
> done that at your local setup to confirm the new logic is regression-free
> for SST and HDMI.

Hello Jerry,

Yes, it worked for me last I tried, but I can double-check to make sure.
How did you try to test it?
 
>      Apart from that, we find a regression in MST. MST has its own detection
> logic and should be separated from SST and HDMI.

Can you say what the regression is and how to reproduce it?
I'd like to look into it and fix it.

By the way, how do you test MST in general? On my machine, MST doesn't work at 
with the DC display driver and is basically broken on every GPU generation 
that I tried.

Thanks,
Timur


> 
> 
> > -----Original Message-----
> > From: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > Sent: Thursday, June 4, 2026 10:52
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Wentland, Harry <Harry.Wentland@amd.com>; Li, Sun peng (Leo)
> > <Sunpeng.Li@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Li,
> > Roman <Roman.Li@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Chung,
> > ChiaHsuan (Tom) <ChiaHsuan.Chung@amd.com>; Zuo, Jerry
> > <Jerry.Zuo@amd.com>; Wheeler, Daniel <Daniel.Wheeler@amd.com>; Wu,
> > Ray <Ray.Wu@amd.com>; LIPSKI, IVAN <IVAN.LIPSKI@amd.com>; Hung, Alex
> > <Alex.Hung@amd.com>; Lin, Ping Lei <PingLei.Lin@amd.com>; Chen, Chen-
> > Yu <Chen-Yu.Chen@amd.com>; Timur Kristóf <timur.kristof@gmail.com>
> > Subject: [PATCH 21/24] drm/amd/display: Retry link detection on hotplug
> >
> >
> >
> > From: Timur Kristóf <timur.kristof@gmail.com>
> >
> >
> >
> > When dc_link_detect_connection_type thinks that a display is connected,
> > but
 dc_link_detect failed, enqueue delayed work to retry the link
> > detection again.>
> >
> >
> > Useful when eg. HPD pin is high but the display isn't ready and didn't
> > respond
 to DDC.
> >
> >
> >
> > - The display is "slow to wake up", ie. DDC isn't ready,
> > 
> >   for example we couldn't read EDID. Can happen with any
> >   connector type with certain "slow" displays.
> >   Some displays may take up to 15~20 sec or more to wake up.
> >
> >
> >
> > - On hotplug, the HPD pin may make contact before the DDC pins,
> > 
> >   so we couldn't read the EDID. This most often happens with
> >   DVI connectors, rarely with HDMI. It is not impossible but
> >   extremely rare with other connector types.
> >
> >
> >
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > Reviewed-by: Alex Hung <alex.hung@amd.com>
> > ---
> > 
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 138
> > 
> > ++++++++++++++++++  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > 
> > |  16 ++
> >  
> >  2 files changed, 154 insertions(+)
> >
> >
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index d1b1eb67d937..40295a5edbec 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -161,6 +161,17 @@ MODULE_FIRMWARE(FIRMWARE_DCN_42_DMUB);
> > 
> >  #define FIRMWARE_DCN_42B_DMUB "amdgpu/dcn_4_2_1_dmcub.bin"
> >  MODULE_FIRMWARE(FIRMWARE_DCN_42B_DMUB);
> >
> >
> >
> > +/**
> > + * define AMDGPU_DM_HPD_MAX_NUM_RETRIES - maximum amount of
> > retries for
> > +hotplug detection  */ #define AMDGPU_DM_HPD_MAX_NUM_RETRIES 5
> > +
> > +/**
> > + * define AMDGPU_DM_HPD_RETRY_DELAY_MSEC - millisecond delay
> > between
> > +hotplug detection retries  */ #define
> > AMDGPU_DM_HPD_RETRY_DELAY_MSEC
> > +1500
> > +
> > +
> > 
> >  /**
> >  
> >   * DOC: overview
> >   *
> > 
> > @@ -959,6 +970,125 @@ static void dm_handle_hpd_work(struct
> > work_struct *work)
> >
> >
> >
> >  }
> >
> >
> >
> > +/**
> > + * dm_handle_delayed_hpd_work() - Handle delayed HPD (hotplug
> > +detection)
> > + *
> > + * @w: Base work item structure
> > + *
> > + * Used for retrying HPD after a delay. Just calls the normal HPD
> > helper.
> > + */
> > +static void dm_handle_delayed_hpd_work(struct work_struct *work) {
> > +     struct delayed_work *dw = container_of(work, struct delayed_work,
> > work);
> > +     struct delayed_hpd_work *w = container_of(dw, struct
> > delayed_hpd_work, work);
> > +     struct amdgpu_dm_connector *aconn = w->aconn;
> > +     enum dc_detect_reason reason = w->reason;
> > +
> > +     kfree(w);
> > +     handle_hpd_irq_helper(aconn, reason);
> > +}
> > +
> > +/**
> > + * dm_cancel_delayed_hpd_work() - Cancel pending hotplug detection work
> > +for a connector
> > + *
> > + * @aconnector: Connector on which the HPD event occurred  */ static
> > +void dm_cancel_delayed_hpd_work(struct amdgpu_dm_connector
> > *aconnector)
> > +{
> > +     if (!aconnector || !aconnector->delayed_hpd_work)
> > +             return;
> > +
> > +     cancel_delayed_work(&aconnector->delayed_hpd_work->work);
> > +     aconnector->delayed_hpd_work = NULL;
> > +}
> > +
> > +/**
> > + * dm_cancel_all_delayed_hpd_work() - Cancel all pending hotplug
> > +detection work on the device
> > + *
> > + * @dev: DRM device pointer
> > + */
> > +static void dm_cancel_all_delayed_hpd_work(struct drm_device *dev) {
> > +     struct drm_connector *connector;
> > +     struct drm_connector_list_iter iter;
> > +
> > +     drm_connector_list_iter_begin(dev, &iter);
> > +     drm_for_each_connector_iter(connector, &iter) {
> > +             if (connector->connector_type ==
> > DRM_MODE_CONNECTOR_WRITEBACK)
> > +                     continue;
> > +
> > +
> > 
> >       dm_cancel_delayed_hpd_work(to_amdgpu_dm_connector(connecto
> > 
> > r));
> > +     }
> > +     drm_connector_list_iter_end(&iter);
> > +}
> > +
> > +/**
> > + * dm_queue_delayed_hpd_work() - Enqueue delayed work to handle
> > hotplug
> > +detection
> > + *
> > + * @aconnector: Connector on which the HPD event occurred
> > + * @reason: Reason why we are attempting the HPD
> > + * @msecs: Millisecond delay after which the delayed work is going to
> > +happen
> > + *
> > + * When dc_link_detect_connection_type thinks that a display is
> > +connected,
> > + * but dc_link_detect failed, enqueue delayed work to retry the link
> > + * detection again.
> > + *
> > + * Useful when eg. HPD pin is high but the display isn't ready and
> > + * didn't respond to DDC.
> > + *
> > + * - On boot or suspend/resume, the display is "slow to wake up",
> > + *   ie. DDC isn't ready, for example we couldn't read DP link caps or
> > EDID.
 + *   Can happen to any connector with certain "slow" displays.
> > + *
> > + * - On hotplug, the HPD pin may make contact before the DDC pins,
> > + *   so we couldn't read the EDID. Can happen to any connector but
> > + *   most often to DVI and sometimes to HDMI (rarely to DP).
> > + *
> > + */
> > +static void dm_queue_delayed_hpd_work(struct amdgpu_dm_connector
> > *aconnector,
> > +                                   const enum dc_detect_reason reason,
> > +                                   const unsigned int msecs)
> > +{
> > +     struct drm_device *dev = aconnector->base.dev;
> > +     struct amdgpu_device *adev = drm_to_adev(dev);
> > +     struct delayed_hpd_work *w;
> > +
> > +     if (!aconnector || !aconnector->dc_link ||
> > +         aconnector->dc_link->type == dc_connection_none)
> > +             return;
> > +
> > +     /* Don't retry polled connectors, the polling is going to detect it.
> > */
 +     if (aconnector->base.polled != DRM_CONNECTOR_POLL_HPD)
> > +             return;
> > +
> > +     ++aconnector->num_hpd_retries;
> > +
> > +     drm_dbg(dev, "Can't detect link on %s on try %d\n",
> > +             aconnector->base.name, aconnector->num_hpd_retries);
> > +
> > +     if (aconnector->num_hpd_retries >
> > AMDGPU_DM_HPD_MAX_NUM_RETRIES) {
> > +             drm_warn(dev, "Too many retries on %s: %d, giving up\n",
> > +                      aconnector->base.name, aconnector-
> > 
> > >num_hpd_retries);
> > 
> > +             aconnector->num_hpd_retries = 0;
> > +             return;
> > +     }
> > +
> > +     w = kzalloc(sizeof(*w), GFP_ATOMIC);
> > +
> > +     if (!w)
> > +             return;
> > +
> > +     INIT_DELAYED_WORK(&w->work, dm_handle_delayed_hpd_work);
> > +     w->aconn = aconnector;
> > +     w->reason = reason;
> > +     aconnector->delayed_hpd_work = w;
> > +
> > +     drm_warn(dev, "Enqueueing next retry on %s\n",
> > +              aconnector->base.name);
> > +     queue_delayed_work(adev->dm.delayed_hpd_wq, &w->work,
> > +                        msecs_to_jiffies(msecs));
> > +}
> > +
> > 
> >  static const char *dmub_notification_type_str(enum
> >  dmub_notification_type
> > 
> > e)  {
> > 
> >       switch (e) {
> > 
> > @@ -3249,6 +3379,7 @@ static int dm_hw_fini(struct amdgpu_ip_block
> > *ip_block)  {
> > 
> >       struct amdgpu_device *adev = ip_block->adev;
> >
> >
> >
> > +     dm_cancel_all_delayed_hpd_work(&adev->ddev);
> > 
> >       amdgpu_dm_hpd_fini(adev);
> >
> >
> >
> >       amdgpu_dm_irq_fini(adev);
> > 
> > @@ -3422,6 +3553,8 @@ static int dm_suspend(struct amdgpu_ip_block
> > *ip_block)
> > 
> >       struct amdgpu_device *adev = ip_block->adev;
> >       struct amdgpu_display_manager *dm = &adev->dm;
> >
> >
> >
> > +     dm_cancel_all_delayed_hpd_work(&adev->ddev);
> > +
> > 
> >       if (amdgpu_in_reset(adev)) {
> >       
> >               enum dc_status res;
> >
> >
> >
> > @@ -4451,6 +4584,9 @@ static void handle_hpd_irq_helper(struct
> > amdgpu_dm_connector *aconnector,
> > 
> >                       if (aconnector->base.force ==
> > 
> > DRM_FORCE_UNSPECIFIED ||
> > 
> >                           reason == DETECT_REASON_HPDRX)
> >
> >
> >
> >       drm_kms_helper_connector_hotplug_event(connector);
> > 
> > +             } else {
> 
> 
> 
> We need to return here if aconnector->mst_mgr.mst_state is set.
> 
> 
> 
> > +                     dm_queue_delayed_hpd_work(aconnector, reason,
> > +
> > AMDGPU_DM_HPD_RETRY_DELAY_MSEC);
> > 
> >               }
> >       
> >       }
> >  
> >  }
> > 
> > @@ -4459,6 +4595,8 @@ static void handle_hpd_irq(void *param)  {
> > 
> >       struct amdgpu_dm_connector *aconnector = (struct
> > 
> > amdgpu_dm_connector *)param;
> >
> >
> >
> > +     /* Cancel any pending work */
> > +     dm_cancel_delayed_hpd_work(aconnector);
> > 
> >       handle_hpd_irq_helper(aconnector, DETECT_REASON_HPD);
> >
> >
> >
> >  }
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > index 7d37c1612131..9a66c9e2b78d 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -136,6 +136,18 @@ struct dmub_hpd_work {
> > 
> >       struct amdgpu_device *adev;
> >  
> >  };
> >
> >
> >
> > +/**
> > + * struct delayed_hpd_work - Handle delayed HPD (hot plug detection)
> > +work
> > + *
> > + * @work: Base structure, kernel work data for the work event
> > + * @aconn: Pointer to connector where the HPD event happened  */ struct
> > +delayed_hpd_work {
> > +     struct delayed_work work;
> > +     struct amdgpu_dm_connector *aconn;
> > +     enum dc_detect_reason reason;
> > +};
> > +
> > 
> >  /**
> >  
> >   * struct vblank_control_work - Work data for vblank control
> >   * @work: Kernel work data for the work event @@ -801,6 +813,10 @@
> > 
> > struct amdgpu_dm_connector {
> > 
> >       /* number of modes generated from EDID at 'dc_sink' */
> >       int num_modes;
> >
> >
> >
> > +     /* number of retries on hot plug detection */
> > +     int num_hpd_retries;
> > +     struct delayed_hpd_work *delayed_hpd_work;
> > +
> > 
> >       /* The 'old' sink - before an HPD.
> >       
> >        * The 'current' sink is in dc_link->sink. */
> >       
> >       struct dc_sink *dc_sink;
> > 
> > --
> > 2.54.0
> 
>