Re: The inherent defect of the AMDGPU driver about hotplug
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 :
> Yes, I agree.
>
> On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher
> wrote:
> >
> > On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou
> 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(>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
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: The inherent defect of the AMDGPU driver about hotplug
Yes, I agree.
On Tue, Jun 16, 2020 at 3:08 AM Alex Deucher wrote:
>
> On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou 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(>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
Re: The inherent defect of the AMDGPU driver about hotplug
On Mon, Jun 15, 2020 at 5:30 AM Aaron Chou 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(>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
The inherent defect of the AMDGPU driver about hotplug
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(>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
