The inherent defect of the AMDGPU driver about hotplug

[Alex Deucher]

On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou <zhoubb.aaron at gmail.com> wrote:
>
> About one month ago, I have asked a question about HDMI hotplug, the link is:
>
> https://gitlab.freedesktop.org/drm/amd/-/issues/1135#note_492460
>
> And I have put one patch to fix this, as follows:
>
>  39 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  40 index f355d9a752d2..ee657db9a228 100644
>  41 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  42 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  43 @@ -973,7 +973,7 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  44         const struct drm_encoder_helper_funcs *encoder_funcs;
>  45         int r;
>  46         enum drm_connector_status ret =
> connector_status_disconnected;
>  47 -       bool dret = false, broken_edid = false;
>  48 +       bool dret = false, broken_edid = false, undefined_flag =
> false;
>  49
>  50         if (!drm_kms_helper_is_poll_worker()) {
>  51                 r = pm_runtime_get_sync(connector->dev->dev);
>  52 @@ -988,7 +988,12 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  53
>  54         if (amdgpu_connector->ddc_bus)
>  55                 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> false);
>  56 -       if (dret) {
>  57 +
>  58 +       /* Check the HDMI HPD pin status again */
>  59 +       if (!amdgpu_display_hpd_sense(adev,
> amdgpu_connector->hpd.hpd))
>  60 +               undefined_flag = true;
>  61 +
>  62 +       if (dret && !undefined_flag) {
>  63                 amdgpu_connector->detected_by_load = false;
>  64                 amdgpu_connector_free_edid(connector);
>  65                 amdgpu_connector_get_edid(connector);
>
> Maybe the fix is sloppy, so I write the another patch:
>
>  16 diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  17 index c770d73352a7..bb59ebc9a6c8 100644
>  18 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  19 +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>  20 @@ -991,6 +991,7 @@ amdgpu_connector_dvi_detect(struct
> drm_connector *connector, bool force)
>  21         if (amdgpu_connector->ddc_bus)
>  22                 dret = amdgpu_display_ddc_probe(amdgpu_connector,
> false);
>  23         if (dret) {
>  24 +               schedule_work(&adev->hotplug_work);
>  25                 amdgpu_connector->detected_by_load = false;
>  26                 amdgpu_connector_free_edid(connector);
>  27                 amdgpu_connector_get_edid(connector);
>
> Which is better, or neither？

As per the last discussion:

"This is the part I don't understand.  The logic already checks the HPD
status in amdgpu_connector_check_hpd_status_unchanged().  Does it
still report connected at that point?  After that it tries to read the
EDID in amdgpu_display_ddc_probe().  If the monitor is disconnected,
there should be no EDID so dret should be false.  We should try and
figure out why the first HPD check reports connected and the EDID
probe returns true."

There is already an HPD probe in the current logic.  We should try and
understand why we need a second one rather than just adding one.  Does
a delay at the top of that function help?

Alex

>
> --
> Regards,
> Aaron
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx