The inherent defect of the AMDGPU driver about hotplug

[Aaron Chou]

Yes, I agree.

On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher <alexdeucher at gmail.com> wrote:
>
> 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