* The inherent defect of the AMDGPU driver about hotplug
@ 2020-06-15 9:29 Aaron Chou
2020-06-15 19:08 ` Alex Deucher
0 siblings, 1 reply; 4+ messages in thread
From: Aaron Chou @ 2020-06-15 9:29 UTC (permalink / raw)
To: Deucher, Alexander, amd-gfx list, Christian König
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？
--
Regards,
Aaron
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: The inherent defect of the AMDGPU driver about hotplug
2020-06-15 9:29 The inherent defect of the AMDGPU driver about hotplug Aaron Chou
@ 2020-06-15 19:08 ` Alex Deucher
2020-06-16 0:50 ` Aaron Chou
0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2020-06-15 19:08 UTC (permalink / raw)
To: Aaron Chou; +Cc: Deucher, Alexander, Christian König, amd-gfx list
On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou <zhoubb.aaron@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: The inherent defect of the AMDGPU driver about hotplug
2020-06-15 19:08 ` Alex Deucher
@ 2020-06-16 0:50 ` Aaron Chou
2020-06-16 7:09 ` Ernst Sjöstrand
0 siblings, 1 reply; 4+ messages in thread
From: Aaron Chou @ 2020-06-16 0:50 UTC (permalink / raw)
To: Alex Deucher; +Cc: Deucher, Alexander, Christian König, amd-gfx list
Yes, I agree.
On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou <zhoubb.aaron@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: The inherent defect of the AMDGPU driver about hotplug
2020-06-16 0:50 ` Aaron Chou
@ 2020-06-16 7:09 ` Ernst Sjöstrand
0 siblings, 0 replies; 4+ messages in thread
From: Ernst Sjöstrand @ 2020-06-16 7:09 UTC (permalink / raw)
To: Aaron Chou
Cc: Alex Deucher, Deucher, Alexander, Christian König, amd-gfx list
[-- Attachment #1.1: Type: text/plain, Size: 4360 bytes --]
Have you tried a much later kernel btw? I saw you mentioned 4.19.
Den tis 16 juni 2020 kl 02:50 skrev Aaron Chou <zhoubb.aaron@gmail.com>:
> Yes, I agree.
>
> On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher <alexdeucher@gmail.com>
> wrote:
> >
> > On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou <zhoubb.aaron@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
[-- Attachment #1.2: Type: text/html, Size: 6205 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-16 7:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 9:29 The inherent defect of the AMDGPU driver about hotplug Aaron Chou
2020-06-15 19:08 ` Alex Deucher
2020-06-16 0:50 ` Aaron Chou
2020-06-16 7:09 ` Ernst Sjöstrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.